Skip to content

Commit

Permalink
Do not treat Task.Run methods as 'apparent' for 'use var' (#76229)
Browse files Browse the repository at this point in the history 
Fixes #64902
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi authored Dec 3, 2024
2 parents 98fbf2f + 52776bc commit a4ebe9c
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 122 deletions.
15 changes: 6 additions & 9 deletions ...ers/CSharp/Analyzers/UseImplicitOrExplicitType/CSharpUseImplicitTypeDiagnosticAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@
namespace Microsoft.CodeAnalysis.CSharp.Diagnostics.TypeStyle;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed class CSharpUseImplicitTypeDiagnosticAnalyzer : CSharpTypeStyleDiagnosticAnalyzerBase
internal sealed class CSharpUseImplicitTypeDiagnosticAnalyzer()
: CSharpTypeStyleDiagnosticAnalyzerBase(
diagnosticId: IDEDiagnosticIds.UseImplicitTypeDiagnosticId,
enforceOnBuild: EnforceOnBuildValues.UseImplicitType,
title: s_Title,
message: s_Message)
{
private static readonly LocalizableString s_Title =
new LocalizableResourceString(nameof(CSharpAnalyzersResources.Use_implicit_type), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources));
Expand All @@ -17,12 +22,4 @@ internal sealed class CSharpUseImplicitTypeDiagnosticAnalyzer : CSharpTypeStyleD
new LocalizableResourceString(nameof(CSharpAnalyzersResources.use_var_instead_of_explicit_type), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources));

protected override CSharpTypeStyleHelper Helper => CSharpUseImplicitTypeHelper.Instance;

public CSharpUseImplicitTypeDiagnosticAnalyzer()
: base(diagnosticId: IDEDiagnosticIds.UseImplicitTypeDiagnosticId,
enforceOnBuild: EnforceOnBuildValues.UseImplicitType,
title: s_Title,
message: s_Message)
{
}
}
32 changes: 32 additions & 0 deletions src/Analyzers/CSharp/Tests/UseImplicitOrExplicitType/UseImplicitTypeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3402,4 +3402,36 @@ static void M()
}
""", new(options: ImplicitTypeEverywhere()));
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/64902")]
public async Task TestNotOnAwaitedTask()
{
await TestMissingInRegularAndScriptAsync(
"""
using System.Collections.Generic;
using System.Threading.Tasks;
public class Program
{
public static async Task Main()
{
object test1 = DoSomeWork();
object test2 = await Task.Run(() => DoSomeWork());
IEnumerable<object> test3 = DoSomeWorkGeneric();
[|IEnumerable<object>|] test4 = await Task.Run(() => DoSomeWorkGeneric());
}
public static object DoSomeWork()
{
return new object();
}
public static IEnumerable<object> DoSomeWorkGeneric()
{
return new List<object>();
}
}
""", new(options: ImplicitTypeWhereApparent()));
}
}
91 changes: 34 additions & 57 deletions ...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/TypeStyle/TypeStyleHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand Down Expand Up @@ -113,22 +114,18 @@ public static bool IsTypeApparentInAssignmentExpression(
return false;
}

private static bool IsPossibleCreationOrConversionMethod(IMethodSymbol methodSymbol,
private static bool IsPossibleCreationOrConversionMethod(
IMethodSymbol methodSymbol,
ITypeSymbol? typeInDeclaration,
SemanticModel semanticModel,
ExpressionSyntax containingTypeName,
CancellationToken cancellationToken)
{
if (methodSymbol.ReturnsVoid)
{
return false;
}

var containingType = semanticModel.GetTypeInfo(containingTypeName, cancellationToken).Type;

// The containing type was determined from an expression of the form ContainingType.MemberName, and the
// caller verifies that MemberName resolves to a method symbol.
Contract.ThrowIfNull(containingType);
if (semanticModel.GetTypeInfo(containingTypeName, cancellationToken).Type is not INamedTypeSymbol containingType)
return false;

return IsPossibleCreationMethod(methodSymbol, typeInDeclaration, containingType)
|| IsPossibleConversionMethod(methodSymbol);
Expand All @@ -138,16 +135,34 @@ private static bool IsPossibleCreationOrConversionMethod(IMethodSymbol methodSym
/// Looks for types that have static methods that return the same type as the container.
/// e.g: int.Parse, XElement.Load, Tuple.Create etc.
/// </summary>
private static bool IsPossibleCreationMethod(IMethodSymbol methodSymbol,
private static bool IsPossibleCreationMethod(
IMethodSymbol methodSymbol,
ITypeSymbol? typeInDeclaration,
ITypeSymbol containingType)
INamedTypeSymbol containingType)
{
if (!methodSymbol.IsStatic)
{
return false;

// Check the simple case of the type the method is in returning an instance of that type.
var returnType = UnwrapTupleType(methodSymbol.ReturnType);
if (UnwrapTupleType(containingType).Equals(returnType))
return true;

// Now check for cases like `Tuple.Create(0, true)`. This is a static factory without type arguments
// that returns instances of a generic type.
//
// We explicitly disallow this for Task.X and ValueTask.X as the final type is generally not apparent due to
// complex type inference.
if (UnwrapTupleType(typeInDeclaration)?.GetTypeArguments().Length > 0 &&
containingType.TypeArguments.Length == 0 &&
returnType.Name is not nameof(Task) and not nameof(ValueTask) &&
UnwrapTupleType(containingType).Name.Equals(returnType.Name) &&
containingType.ContainingNamespace.Equals(returnType.ContainingNamespace))
{
return true;
}

return IsContainerTypeEqualToReturnType(methodSymbol, typeInDeclaration, containingType);
return false;
}

/// <summary>
Expand All @@ -164,29 +179,6 @@ private static bool IsPossibleConversionMethod(IMethodSymbol methodSymbol)
return methodSymbol.Name.Equals("To" + returnTypeName, StringComparison.Ordinal);
}

/// <remarks>
/// If there are type arguments on either side of assignment, we match type names instead of type equality
/// to account for inferred generic type arguments.
/// e.g: Tuple.Create(0, true) returns Tuple&lt;X,y&gt; which isn't the same as type Tuple.
/// otherwise, we match for type equivalence
/// </remarks>
private static bool IsContainerTypeEqualToReturnType(IMethodSymbol methodSymbol,
ITypeSymbol? typeInDeclaration,
ITypeSymbol containingType)
{
var returnType = UnwrapTupleType(methodSymbol.ReturnType);

if (UnwrapTupleType(typeInDeclaration)?.GetTypeArguments().Length > 0 ||
containingType.GetTypeArguments().Length > 0)
{
return UnwrapTupleType(containingType).Name.Equals(returnType.Name);
}
else
{
return UnwrapTupleType(containingType).Equals(returnType);
}
}

[return: NotNullIfNotNull(nameof(symbol))]
private static ITypeSymbol? UnwrapTupleType(ITypeSymbol? symbol)
{
Expand All @@ -200,29 +192,14 @@ private static bool IsContainerTypeEqualToReturnType(IMethodSymbol methodSymbol,
}

private static ExpressionSyntax GetRightmostInvocationExpression(ExpressionSyntax node)
{
if (node is AwaitExpressionSyntax awaitExpression && awaitExpression.Expression != null)
{
return GetRightmostInvocationExpression(awaitExpression.Expression);
}

if (node is InvocationExpressionSyntax invocationExpression && invocationExpression.Expression != null)
=> node switch
{
return GetRightmostInvocationExpression(invocationExpression.Expression);
}

if (node is ConditionalAccessExpressionSyntax conditional)
{
return GetRightmostInvocationExpression(conditional.WhenNotNull);
}

return node;
}
AwaitExpressionSyntax { Expression: not null } awaitExpression => GetRightmostInvocationExpression(awaitExpression.Expression),
InvocationExpressionSyntax { Expression: not null } invocationExpression => GetRightmostInvocationExpression(invocationExpression.Expression),
ConditionalAccessExpressionSyntax conditional => GetRightmostInvocationExpression(conditional.WhenNotNull),
_ => node
};

public static bool IsPredefinedType(TypeSyntax type)
{
return type is PredefinedTypeSyntax predefinedType
? SyntaxFacts.IsPredefinedType(predefinedType.Keyword.Kind())
: false;
}
=> type is PredefinedTypeSyntax predefinedType && SyntaxFacts.IsPredefinedType(predefinedType.Keyword.Kind());
}
83 changes: 27 additions & 56 deletions ...UtilitiesAndExtensions/Compiler/CSharp/Utilities/TypeStyle/CSharpUseImplicitTypeHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,30 +46,36 @@ public override TypeStyleResult AnalyzeTypeName(

public override bool ShouldAnalyzeVariableDeclaration(VariableDeclarationSyntax variableDeclaration, CancellationToken cancellationToken)
{
// If the type is already 'var' or 'ref var', this analyzer has no work to do
var type = variableDeclaration.Type.StripRefIfNeeded();
if (type.IsVar)
{
// If the type is already 'var' or 'ref var', this analyzer has no work to do
return false;
}

// The base analyzer may impose further limitations
return base.ShouldAnalyzeVariableDeclaration(variableDeclaration, cancellationToken);
}

protected override bool ShouldAnalyzeForEachStatement(ForEachStatementSyntax forEachStatement, SemanticModel semanticModel, CancellationToken cancellationToken)
{
var type = forEachStatement.Type;
if (type.IsVar || (type.Kind() == SyntaxKind.RefType && ((RefTypeSyntax)type).Type.IsVar))
{
// If the type is already 'var', this analyze has no work to do
// If the type is already 'var' or 'ref var', this analyzer has no work to do
var type = forEachStatement.Type.StripRefIfNeeded();
if (type.IsVar)
return false;
}

// The base analyzer may impose further limitations
return base.ShouldAnalyzeForEachStatement(forEachStatement, semanticModel, cancellationToken);
}

protected override bool ShouldAnalyzeDeclarationExpression(DeclarationExpressionSyntax declaration, SemanticModel semanticModel, CancellationToken cancellationToken)
{
// If the type is already 'var' or 'ref var', this analyzer has no work to do
if (declaration.Type.StripRefIfNeeded().IsVar)
return false;

// The base analyzer may impose further limitations
return base.ShouldAnalyzeDeclarationExpression(declaration, semanticModel, cancellationToken);
}

protected override bool IsStylePreferred(in State state)
{
var stylePreferences = state.TypeStylePreference;
Expand Down Expand Up @@ -110,23 +116,11 @@ SyntaxKind.ForStatement or
SyntaxKind.UsingStatement))
{
// implicitly typed variables cannot be constants.
if ((variableDeclaration.Parent as LocalDeclarationStatementSyntax)?.IsConst == true)
{
if (variableDeclaration.Parent is LocalDeclarationStatementSyntax { IsConst: true })
return false;
}

if (variableDeclaration.Variables.Count != 1)
{
if (variableDeclaration.Variables is not [{ Initializer.Value: var initializer } variable])
return false;
}

var variable = variableDeclaration.Variables[0];
if (variable.Initializer == null)
{
return false;
}

var initializer = variable.Initializer.Value;

// Do not suggest var replacement for stackalloc span expressions.
// This will change the bound type from a span to a pointer.
Expand Down Expand Up @@ -220,9 +214,10 @@ private static bool IsSafeToSwitchToVarWithoutNeedingSpeculation(DeclarationExpr
// If there was only one member in the group, and it was non-generic itself, then this
// change is commonly safe to make without having to actually change to `var` and
// speculatively determine if the change is ok or not.
if (declarationExpression.Parent is not ArgumentSyntax argument ||
argument.Parent is not ArgumentListSyntax argumentList ||
argumentList.Parent is not InvocationExpressionSyntax invocationExpression)
if (declarationExpression.Parent is not ArgumentSyntax
{
Parent: ArgumentListSyntax { Parent: InvocationExpressionSyntax invocationExpression }
} argument)
{
return false;
}
Expand All @@ -231,33 +226,21 @@ argument.Parent is not ArgumentListSyntax argumentList ||
if (memberGroup.Length != 1)
return false;

var method = memberGroup[0] as IMethodSymbol;
if (method == null)
return false;

if (!method.GetTypeParameters().IsEmpty)
if (memberGroup[0] is not IMethodSymbol { TypeParameters.IsEmpty: true } method)
return false;

// Looks pretty good so far. However, this change is not allowed if the user is
// specifying something like `out (int x, int y) t` and the method signature has
// different names for those tuple elements. Check and make sure the types are the
// same before proceeding.
// Looks pretty good so far. However, this change is not allowed if the user is specifying something like `out
// (int x, int y) t` and the method signature has different names for those tuple elements. Check and make sure
// the types are the same before proceeding.

var invocationOp = semanticModel.GetOperation(invocationExpression, cancellationToken) as IInvocationOperation;
if (invocationOp == null)
if (semanticModel.GetOperation(invocationExpression, cancellationToken) is not IInvocationOperation invocationOp)
return false;

var argumentOp = invocationOp.Arguments.FirstOrDefault(a => a.Syntax == argument);
if (argumentOp == null)
if (argumentOp is not { Value.Type: { } valueType, Parameter.Type: { } parameterType })
return false;

if (argumentOp.Value?.Type == null)
return false;

if (argumentOp.Parameter?.Type == null)
return false;

return argumentOp.Value.Type.Equals(argumentOp.Parameter.Type);
return valueType.Equals(parameterType);
}

/// <summary>
Expand Down Expand Up @@ -352,16 +335,4 @@ internal static ExpressionSyntax GetInitializerExpression(ExpressionSyntax initi
current = (current as CheckedExpressionSyntax)?.Expression ?? current;
return current.WalkDownParentheses();
}

protected override bool ShouldAnalyzeDeclarationExpression(DeclarationExpressionSyntax declaration, SemanticModel semanticModel, CancellationToken cancellationToken)
{
if (declaration.Type.IsVar)
{
// If the type is already 'var', this analyze has no work to do
return false;
}

// The base analyzer may impose further limitations
return base.ShouldAnalyzeDeclarationExpression(declaration, semanticModel, cancellationToken);
}
}

0 comments on commit a4ebe9c

Please sign in to comment.