Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not treat Task.Run methods as 'apparent' for 'use var' #76229

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Do not treat Task.Run methods as 'apparent' for 'use var' #76229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12d916f
Do not treat Task.Run methods as 'apparent' for 'use var'
CyrusNajmabadi Dec 2, 2024
bb87c6f
Simplify
CyrusNajmabadi Dec 2, 2024
05b4a00
Simplify
CyrusNajmabadi Dec 2, 2024
52776bc
Simplify
CyrusNajmabadi Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions ...ers/CSharp/Analyzers/UseImplicitOrExplicitType/CSharpUseImplicitTypeDiagnosticAnalyzer.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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);
}
}
Loading