From 52624ec946bc42e19534a6fad6b8263583a257d1 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 12:34:50 -0700 Subject: [PATCH 1/6] Fix issue with 'use explicit cast' and compound assignments --- .../CSharpAddExplicitCastCodeFixProvider.cs | 45 ++++-- .../AddExplicitCast/AddExplicitCastTests.cs | 148 +++++++++++++++++- .../AbstractAddExplicitCastCodeFixProvider.cs | 22 ++- 3 files changed, 193 insertions(+), 22 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs index 74e82c814d2a6..1e2b8b49c659e 100644 --- a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs @@ -13,11 +13,14 @@ using Microsoft.CodeAnalysis.LanguageService; using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; +using Microsoft.CodeAnalysis.Simplification; namespace Microsoft.CodeAnalysis.CSharp.CodeFixes.AddExplicitCast; [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.AddExplicitCast), Shared] -internal sealed partial class CSharpAddExplicitCastCodeFixProvider +[method: ImportingConstructor] +[method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] +internal sealed partial class CSharpAddExplicitCastCodeFixProvider() : AbstractAddExplicitCastCodeFixProvider { /// @@ -30,16 +33,8 @@ internal sealed partial class CSharpAddExplicitCastCodeFixProvider /// private const string CS1503 = nameof(CS1503); - private readonly ArgumentFixer _argumentFixer; - private readonly AttributeArgumentFixer _attributeArgumentFixer; - - [ImportingConstructor] - [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] - public CSharpAddExplicitCastCodeFixProvider() - { - _argumentFixer = new ArgumentFixer(); - _attributeArgumentFixer = new AttributeArgumentFixer(); - } + private readonly ArgumentFixer _argumentFixer = new(); + private readonly AttributeArgumentFixer _attributeArgumentFixer = new(); public override ImmutableArray FixableDiagnosticIds => [CS0266, CS1503]; @@ -98,4 +93,32 @@ protected override bool TryGetTargetTypeInfo( potentialConversionTypes = FilterValidPotentialConversionTypes(document, semanticModel, mutablePotentialConversionTypes); return !potentialConversionTypes.IsEmpty; } + + protected override bool TryLanguageSpecificFix( + SemanticModel semanticModel, SyntaxNode currentRoot, ExpressionSyntax targetNode, CancellationToken cancellationToken, [NotNullWhen(true)] out SyntaxNode? replacement) + { + replacement = null; + + // The compiler is very ambiguous with assignment expressions `(a += b)`. An error on it may be an error on the + // entire expression or on the RHS of the assignment. Have to reverse engineer what it is doing here. + if (targetNode is AssignmentExpressionSyntax assignmentExpression && + assignmentExpression.IsCompoundAssignExpression()) + { + var leftType = semanticModel.GetTypeInfo(assignmentExpression.Left).Type; + var rightType = semanticModel.GetTypeInfo(assignmentExpression.Right).Type; + + if (leftType != null && rightType != null) + { + var conversion = semanticModel.Compilation.ClassifyConversion(rightType, leftType); + if (conversion.Exists && conversion.IsExplicit) + { + replacement = currentRoot.ReplaceNode( + assignmentExpression.Right, + this.Cast(assignmentExpression.Right, leftType)); + } + } + } + + return replacement != null; + } } diff --git a/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs b/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs index ac6aef166fa25..f441b57c1405c 100644 --- a/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs +++ b/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs @@ -16,13 +16,9 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics.AddExplicitCast; [Trait(Traits.Feature, Traits.Features.CodeActionsAddExplicitCast)] -public partial class AddExplicitCastTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor +public partial class AddExplicitCastTests(ITestOutputHelper logger) + : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger) { - public AddExplicitCastTests(ITestOutputHelper logger) - : base(logger) - { - } - internal override (DiagnosticAnalyzer?, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace) => (null, new CSharpAddExplicitCastCodeFixProvider()); @@ -3271,4 +3267,144 @@ void Goo() { } """); } + + [Fact, WorkItem(56141, "https://github.com/dotnet/roslyn/issues/56141")] + public async Task CompoundAssignment1() + { + await TestInRegularAndScriptAsync( + """ + class C + { + void M() + { + int a = 0; + double b = 0; + [||]a += b; + } + } + """, + """ + class C + { + void M() + { + int a = 0; + double b = 0; + a += (int)b; + } + } + """); + } + + [Fact, WorkItem(56141, "https://github.com/dotnet/roslyn/issues/56141")] + public async Task CompoundAssignment2() + { + await TestInRegularAndScriptAsync( + """ + class C + { + void M() + { + int a = 0; + double b = 0; + a = ([||]b += 1); + } + } + """, + """ + class C + { + void M() + { + int a = 0; + double b = 0; + a = ((int)(b += 1)); + } + } + """); + } + + [Fact, WorkItem(56141, "https://github.com/dotnet/roslyn/issues/56141")] + public async Task CompoundAssignment3() + { + await TestInRegularAndScriptAsync( + """ + class C + { + void M() + { + int a = 0; + double b = 0; + a = [||]b += 1; + } + } + """, + """ + class C + { + void M() + { + int a = 0; + double b = 0; + a = (int)(b += 1); + } + } + """); + } + + [Fact, WorkItem(56141, "https://github.com/dotnet/roslyn/issues/56141")] + public async Task CompoundAssignment4() + { + await TestInRegularAndScriptAsync( + """ + class C + { + void M() + { + int a = 0; + double b = 0; + b = ([||]a += b); + } + } + """, + """ + class C + { + void M() + { + int a = 0; + double b = 0; + b = (a += (int)b); + } + } + """); + } + + [Fact, WorkItem(56141, "https://github.com/dotnet/roslyn/issues/56141")] + public async Task CompoundAssignment5() + { + await TestInRegularAndScriptAsync( + """ + class C + { + void M() + { + int a = 0; + double b = 0; + b = [||]a += b; + } + } + """, + """ + class C + { + void M() + { + int a = 0; + double b = 0; + b = a += (int)b; + } + } + """); + } } diff --git a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs index addd607fea7e6..1f0a26a0fd275 100644 --- a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -53,7 +54,7 @@ protected abstract bool TryGetTargetTypeInfo( string diagnosticId, TExpressionSyntax spanNode, CancellationToken cancellationToken, out ImmutableArray<(TExpressionSyntax node, ITypeSymbol type)> potentialConversionTypes); - public override async Task RegisterCodeFixesAsync(CodeFixContext context) + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) { var document = context.Document; var cancellationToken = context.CancellationToken; @@ -128,15 +129,26 @@ private SyntaxNode ApplyFix( return currentRoot.ReplaceNode( targetNode, this.Cast((TExpressionSyntax)castedExpression, conversionType) - .WithTriviaFrom(targetNode) - .WithAdditionalAnnotations(Simplifier.Annotation)); + .WithTriviaFrom(targetNode)); } } } + else if (TryLanguageSpecificFix(semanticModel, currentRoot, targetNode, cancellationToken, out var replacement)) + { + return replacement; + } return currentRoot.ReplaceNode( targetNode, - this.Cast(targetNode, conversionType).WithAdditionalAnnotations(Simplifier.Annotation)); + this.Cast(targetNode, conversionType)); + } + + protected virtual bool TryLanguageSpecificFix( + SemanticModel semanticModel, SyntaxNode currentRoot, TExpressionSyntax targetNode, CancellationToken cancellationToken, + [NotNullWhen(true)] out SyntaxNode? replacement) + { + replacement = null; + return false; } private static string GetSubItemName(SemanticModel semanticModel, int position, ITypeSymbol conversionType) @@ -190,7 +202,7 @@ protected static bool FindCorrespondingParameterByName( return false; } - protected override async Task FixAllAsync( + protected sealed override async Task FixAllAsync( Document document, ImmutableArray diagnostics, SyntaxEditor editor, From 267a19b0c47916afe0b0ceaddc3b76d2850a64ac Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 12:49:43 -0700 Subject: [PATCH 2/6] Simplify --- .../CSharpAddExplicitCastCodeFixProvider.cs | 15 +++----- .../AbstractAddExplicitCastCodeFixProvider.cs | 37 ++++++++----------- ...sualBasicAddExplicitCastCodeFixProvider.vb | 6 ++- 3 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs index 1e2b8b49c659e..0d482aa917069 100644 --- a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs @@ -13,7 +13,6 @@ using Microsoft.CodeAnalysis.LanguageService; using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; -using Microsoft.CodeAnalysis.Simplification; namespace Microsoft.CodeAnalysis.CSharp.CodeFixes.AddExplicitCast; @@ -38,7 +37,7 @@ internal sealed partial class CSharpAddExplicitCastCodeFixProvider() public override ImmutableArray FixableDiagnosticIds => [CS0266, CS1503]; - protected override void GetPartsOfCastOrConversionExpression(ExpressionSyntax expression, out SyntaxNode type, out SyntaxNode castedExpression) + protected override void GetPartsOfCastOrConversionExpression(ExpressionSyntax expression, out SyntaxNode type, out ExpressionSyntax castedExpression) { var castExpression = (CastExpressionSyntax)expression; type = castExpression.Type; @@ -94,11 +93,9 @@ protected override bool TryGetTargetTypeInfo( return !potentialConversionTypes.IsEmpty; } - protected override bool TryLanguageSpecificFix( - SemanticModel semanticModel, SyntaxNode currentRoot, ExpressionSyntax targetNode, CancellationToken cancellationToken, [NotNullWhen(true)] out SyntaxNode? replacement) + protected override (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast( + SemanticModel semanticModel, ExpressionSyntax targetNode, ITypeSymbol conversionType) { - replacement = null; - // The compiler is very ambiguous with assignment expressions `(a += b)`. An error on it may be an error on the // entire expression or on the RHS of the assignment. Have to reverse engineer what it is doing here. if (targetNode is AssignmentExpressionSyntax assignmentExpression && @@ -112,13 +109,11 @@ protected override bool TryLanguageSpecificFix( var conversion = semanticModel.Compilation.ClassifyConversion(rightType, leftType); if (conversion.Exists && conversion.IsExplicit) { - replacement = currentRoot.ReplaceNode( - assignmentExpression.Right, - this.Cast(assignmentExpression.Right, leftType)); + return (assignmentExpression.Right, this.Cast(assignmentExpression.Right, leftType)); } } } - return replacement != null; + return base.Cast(semanticModel, targetNode, conversionType); } } diff --git a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs index 1f0a26a0fd275..5bbca71162e3e 100644 --- a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Immutable; -using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -15,7 +14,6 @@ using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Collections; using Microsoft.CodeAnalysis.Shared.Extensions; -using Microsoft.CodeAnalysis.Simplification; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.CodeFixes.AddExplicitCast; @@ -30,7 +28,7 @@ internal abstract partial class AbstractAddExplicitCastCodeFixProvider /// Output the current type information of the target node and the conversion type(s) that the target node is @@ -91,8 +89,12 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) actions.Add(CodeAction.Create( title, - cancellationToken => Task.FromResult(document.WithSyntaxRoot( - ApplyFix(document, semanticModel, root, targetNode, conversionType, cancellationToken))), + cancellationToken => + { + var (finalTarget, replacement) = ApplyFix(document, semanticModel, targetNode, conversionType, cancellationToken); + + return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(finalTarget, replacement))); + }, title)); } @@ -101,10 +103,9 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) context.Diagnostics); } - private SyntaxNode ApplyFix( + private (SyntaxNode finalTarget, SyntaxNode finalReplacement) ApplyFix( Document document, SemanticModel semanticModel, - SyntaxNode currentRoot, TExpressionSyntax targetNode, ITypeSymbol conversionType, CancellationToken cancellationToken) @@ -126,28 +127,22 @@ private SyntaxNode ApplyFix( if (firstConversion is { IsImplicit: false, IsReference: true } or { IsIdentity: true } && secondConversion is { IsImplicit: false, IsReference: true }) { - return currentRoot.ReplaceNode( - targetNode, - this.Cast((TExpressionSyntax)castedExpression, conversionType) - .WithTriviaFrom(targetNode)); + return (targetNode, this.Cast(castedExpression, conversionType).WithTriviaFrom(targetNode)); } } } - else if (TryLanguageSpecificFix(semanticModel, currentRoot, targetNode, cancellationToken, out var replacement)) - { - return replacement; - } - return currentRoot.ReplaceNode( - targetNode, - this.Cast(targetNode, conversionType)); + return Cast(semanticModel, targetNode, conversionType); } + protected virtual (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast(SemanticModel semanticModel, TExpressionSyntax targetNode, ITypeSymbol conversionType) + => (targetNode, this.Cast(targetNode, conversionType)); + protected virtual bool TryLanguageSpecificFix( SemanticModel semanticModel, SyntaxNode currentRoot, TExpressionSyntax targetNode, CancellationToken cancellationToken, - [NotNullWhen(true)] out SyntaxNode? replacement) + out (SyntaxNode finalTarget, SyntaxNode replacement) result) { - replacement = null; + result = default; return false; } @@ -223,7 +218,7 @@ await editor.ApplyExpressionLevelSemanticEditsAsync( if (TryGetTargetTypeInfo(document, semanticModel, root, diagnostics[0].Id, spanNode, cancellationToken, out var potentialConversionTypes) && potentialConversionTypes.Length == 1) { - return ApplyFix(document, semanticModel, root, potentialConversionTypes[0].node, potentialConversionTypes[0].type, cancellationToken); + var (newTarget, newReplacement) = ApplyFix(document, semanticModel, root, potentialConversionTypes[0].node, potentialConversionTypes[0].type, cancellationToken); } return root; diff --git a/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb b/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb index 7ac527105b272..77f5cabaa303c 100644 --- a/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb +++ b/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb @@ -14,7 +14,6 @@ Imports Microsoft.CodeAnalysis.PooledObjects Imports Microsoft.CodeAnalysis.VisualBasic.Syntax Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast - Partial Friend NotInheritable Class VisualBasicAddExplicitCastCodeFixProvider Inherits AbstractAddExplicitCastCodeFixProvider(Of ExpressionSyntax) @@ -34,7 +33,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast Public Overrides ReadOnly Property FixableDiagnosticIds As ImmutableArray(Of String) = ImmutableArray.Create(BC30512, BC42016, BC30518, BC30519) - Protected Overrides Sub GetPartsOfCastOrConversionExpression(expression As ExpressionSyntax, ByRef type As SyntaxNode, ByRef castedExpression As SyntaxNode) + Protected Overrides Sub GetPartsOfCastOrConversionExpression( + expression As ExpressionSyntax, + ByRef type As SyntaxNode, + ByRef castedExpression As ExpressionSyntax) Dim directCastExpression = TryCast(expression, DirectCastExpressionSyntax) If directCastExpression IsNot Nothing Then type = directCastExpression.Type From 9e2c539b6570582016ba6cd74858a69664545688 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 12:54:45 -0700 Subject: [PATCH 3/6] Fix --- .../AbstractAddExplicitCastCodeFixProvider.cs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs index 5bbca71162e3e..363ebfd92b054 100644 --- a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs @@ -138,14 +138,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) protected virtual (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast(SemanticModel semanticModel, TExpressionSyntax targetNode, ITypeSymbol conversionType) => (targetNode, this.Cast(targetNode, conversionType)); - protected virtual bool TryLanguageSpecificFix( - SemanticModel semanticModel, SyntaxNode currentRoot, TExpressionSyntax targetNode, CancellationToken cancellationToken, - out (SyntaxNode finalTarget, SyntaxNode replacement) result) - { - result = default; - return false; - } - private static string GetSubItemName(SemanticModel semanticModel, int position, ITypeSymbol conversionType) { return string.Format( @@ -179,7 +171,8 @@ private static string GetSubItemName(SemanticModel semanticModel, int position, validPotentialConversionTypes.Add(conversionTuple); } - return validPotentialConversionTypes.Distinct().ToImmutableArray(); + validPotentialConversionTypes.RemoveDuplicates(); + return validPotentialConversionTypes.ToImmutableAndClear(); } protected static bool FindCorrespondingParameterByName( @@ -218,7 +211,8 @@ await editor.ApplyExpressionLevelSemanticEditsAsync( if (TryGetTargetTypeInfo(document, semanticModel, root, diagnostics[0].Id, spanNode, cancellationToken, out var potentialConversionTypes) && potentialConversionTypes.Length == 1) { - var (newTarget, newReplacement) = ApplyFix(document, semanticModel, root, potentialConversionTypes[0].node, potentialConversionTypes[0].type, cancellationToken); + var (newTarget, newReplacement) = ApplyFix(document, semanticModel, potentialConversionTypes[0].node, potentialConversionTypes[0].type, cancellationToken); + return root.ReplaceNode(newTarget, newReplacement); } return root; From 48c225eb70b355a4b8c1f0669fd625842982d0c5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 12:59:21 -0700 Subject: [PATCH 4/6] Simplify --- .../AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs index 0d482aa917069..485d29378f927 100644 --- a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs @@ -108,9 +108,7 @@ protected override (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast( { var conversion = semanticModel.Compilation.ClassifyConversion(rightType, leftType); if (conversion.Exists && conversion.IsExplicit) - { return (assignmentExpression.Right, this.Cast(assignmentExpression.Right, leftType)); - } } } From 5615c2cfe9b160f5e6fb04f40f7a9c1998522016 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 13:16:20 -0700 Subject: [PATCH 5/6] Simplify --- .../CSharpAddExplicitCastCodeFixProvider.cs | 21 ++-- .../AbstractAddExplicitCastCodeFixProvider.cs | 101 ++++++++++-------- ...sualBasicAddExplicitCastCodeFixProvider.vb | 28 ++--- 3 files changed, 72 insertions(+), 78 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs index 485d29378f927..f39b99225d8d6 100644 --- a/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/AddExplicitCast/CSharpAddExplicitCastCodeFixProvider.cs @@ -47,26 +47,23 @@ protected override void GetPartsOfCastOrConversionExpression(ExpressionSyntax ex protected override ExpressionSyntax Cast(ExpressionSyntax expression, ITypeSymbol type) => expression.Cast(type); - protected override bool TryGetTargetTypeInfo( + protected override void AddPotentialTargetTypes( Document document, SemanticModel semanticModel, SyntaxNode root, string diagnosticId, ExpressionSyntax spanNode, - CancellationToken cancellationToken, - out ImmutableArray<(ExpressionSyntax, ITypeSymbol)> potentialConversionTypes) + ArrayBuilder<(ExpressionSyntax node, ITypeSymbol type)> candidates, + CancellationToken cancellationToken) { - potentialConversionTypes = []; - using var _ = ArrayBuilder<(ExpressionSyntax, ITypeSymbol)>.GetInstance(out var mutablePotentialConversionTypes); - if (diagnosticId == CS0266) { var inferenceService = document.GetRequiredLanguageService(); var conversionType = inferenceService.InferType(semanticModel, spanNode, objectAsDefault: false, cancellationToken); if (conversionType is null) - return false; + return; - mutablePotentialConversionTypes.Add((spanNode, conversionType)); + candidates.Add((spanNode, conversionType)); } else if (diagnosticId == CS1503) { @@ -75,7 +72,7 @@ protected override bool TryGetTargetTypeInfo( && argumentList.Parent is SyntaxNode invocationNode) { // invocationNode could be Invocation Expression, Object Creation, Base Constructor...) - mutablePotentialConversionTypes.AddRange(_argumentFixer.GetPotentialConversionTypes( + candidates.AddRange(_argumentFixer.GetPotentialConversionTypes( document, semanticModel, root, targetArgument, argumentList, invocationNode, cancellationToken)); } else if (spanNode.GetAncestorOrThis() is AttributeArgumentSyntax targetAttributeArgument @@ -83,14 +80,10 @@ protected override bool TryGetTargetTypeInfo( && attributeArgumentList.Parent is AttributeSyntax attributeNode) { // attribute node - mutablePotentialConversionTypes.AddRange(_attributeArgumentFixer.GetPotentialConversionTypes( + candidates.AddRange(_attributeArgumentFixer.GetPotentialConversionTypes( document, semanticModel, root, targetAttributeArgument, attributeArgumentList, attributeNode, cancellationToken)); } } - - // clear up duplicate types - potentialConversionTypes = FilterValidPotentialConversionTypes(document, semanticModel, mutablePotentialConversionTypes); - return !potentialConversionTypes.IsEmpty; } protected override (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast( diff --git a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs index 363ebfd92b054..a4676fef25111 100644 --- a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs @@ -22,8 +22,8 @@ internal abstract partial class AbstractAddExplicitCastCodeFixProvider - /// Give a set of least specific types with a limit, and the part exceeding the limit doesn't show any code fix, - /// but logs telemetry + /// Give a set of least specific types with a limit, and the part exceeding the limit doesn't show any code fix, but + /// logs telemetry. /// private const int MaximumConversionOptions = 3; @@ -31,9 +31,9 @@ internal abstract partial class AbstractAddExplicitCastCodeFixProvider - /// Output the current type information of the target node and the conversion type(s) that the target node is - /// going to be cast by. Implicit downcast can appear on Variable Declaration, Return Statement, Function - /// Invocation, Attribute + /// Output the current type information of the target node and the conversion type(s) that the target node is going + /// to be cast by. Implicit downcast can appear on Variable Declaration, Return Statement, Function Invocation, + /// Attribute /// /// For example: /// Base b; Derived d = [||]b; @@ -42,15 +42,26 @@ internal abstract partial class AbstractAddExplicitCastCodeFixProvider /// The Id of diagnostic /// the innermost node that contains the span - /// Output (target expression, potential conversion type) pairs /// - /// True, if there is at least one potential conversion pair, and they are assigned to - /// "potentialConversionTypes" False, if there is no potential conversion pair. + /// Output (target expression, potential conversion type) pairs. /// - protected abstract bool TryGetTargetTypeInfo( + protected ImmutableArray<(TExpressionSyntax node, ITypeSymbol type)> GetPotentialTargetTypes( Document document, SemanticModel semanticModel, SyntaxNode root, - string diagnosticId, TExpressionSyntax spanNode, CancellationToken cancellationToken, - out ImmutableArray<(TExpressionSyntax node, ITypeSymbol type)> potentialConversionTypes); + string diagnosticId, TExpressionSyntax spanNode, CancellationToken cancellationToken) + { + using var _ = ArrayBuilder<(TExpressionSyntax node, ITypeSymbol type)>.GetInstance(out var candidates); + + this.AddPotentialTargetTypes(document, semanticModel, root, diagnosticId, spanNode, candidates, cancellationToken); + candidates.RemoveDuplicates(); + + return FilterValidPotentialConversionTypes(document, semanticModel, candidates); + } + + protected abstract void AddPotentialTargetTypes( + Document document, SemanticModel semanticModel, SyntaxNode root, + string diagnosticId, TExpressionSyntax spanNode, + ArrayBuilder<(TExpressionSyntax node, ITypeSymbol type)> candidates, + CancellationToken cancellationToken); public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) { @@ -66,41 +77,40 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) if (spanNode == null) return; - var hasSolution = TryGetTargetTypeInfo(document, - semanticModel, root, diagnostic.Id, spanNode, cancellationToken, - out var potentialConversionTypes); - if (!hasSolution) - return; + var potentialConversionTypes = GetPotentialTargetTypes( + document, semanticModel, root, diagnostic.Id, spanNode, cancellationToken); if (potentialConversionTypes.Length == 1) { RegisterCodeFix(context, CodeFixesResources.Add_explicit_cast, nameof(CodeFixesResources.Add_explicit_cast)); - return; } + else if (potentialConversionTypes.Length > 1) + { + using var actions = TemporaryArray.Empty; - using var actions = TemporaryArray.Empty; + // MaximumConversionOptions: we show at most [MaximumConversionOptions] options for this code fixer + foreach (var (targetNode, conversionType) in potentialConversionTypes) + { + var title = GetSubItemName(semanticModel, targetNode.SpanStart, conversionType); - // MaximumConversionOptions: we show at most [MaximumConversionOptions] options for this code fixer - for (var i = 0; i < Math.Min(MaximumConversionOptions, potentialConversionTypes.Length); i++) - { - var targetNode = potentialConversionTypes[i].node; - var conversionType = potentialConversionTypes[i].type; - var title = GetSubItemName(semanticModel, targetNode.SpanStart, conversionType); + actions.Add(CodeAction.Create( + title, + cancellationToken => + { + var (finalTarget, replacement) = ApplyFix(document, semanticModel, targetNode, conversionType, cancellationToken); - actions.Add(CodeAction.Create( - title, - cancellationToken => - { - var (finalTarget, replacement) = ApplyFix(document, semanticModel, targetNode, conversionType, cancellationToken); + return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(finalTarget, replacement))); + }, + title)); - return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(finalTarget, replacement))); - }, - title)); - } + if (actions.Count == MaximumConversionOptions) + break; + } - context.RegisterCodeFix( - CodeAction.Create(CodeFixesResources.Add_explicit_cast, actions.ToImmutableAndClear(), isInlinable: false), - context.Diagnostics); + context.RegisterCodeFix( + CodeAction.Create(CodeFixesResources.Add_explicit_cast, actions.ToImmutableAndClear(), isInlinable: false), + context.Diagnostics); + } } private (SyntaxNode finalTarget, SyntaxNode finalReplacement) ApplyFix( @@ -145,20 +155,17 @@ private static string GetSubItemName(SemanticModel semanticModel, int position, conversionType.ToMinimalDisplayString(semanticModel, position)); } - protected static ImmutableArray<(TExpressionSyntax, ITypeSymbol)> FilterValidPotentialConversionTypes( + private static ImmutableArray<(TExpressionSyntax, ITypeSymbol)> FilterValidPotentialConversionTypes( Document document, SemanticModel semanticModel, - ArrayBuilder<(TExpressionSyntax node, ITypeSymbol type)> mutablePotentialConversionTypes) + ArrayBuilder<(TExpressionSyntax node, ITypeSymbol type)> candidates) { var syntaxFacts = document.GetRequiredLanguageService(); var semanticFacts = document.GetRequiredLanguageService(); - using var _ = ArrayBuilder<(TExpressionSyntax, ITypeSymbol)>.GetInstance(out var validPotentialConversionTypes); - foreach (var conversionTuple in mutablePotentialConversionTypes) + using var _ = ArrayBuilder<(TExpressionSyntax, ITypeSymbol)>.GetInstance(candidates.Count, out var validPotentialConversionTypes); + foreach (var (targetNode, targetNodeConversionType) in candidates) { - var targetNode = conversionTuple.node; - var targetNodeConversionType = conversionTuple.type; - // For cases like object creation expression. for example: // Derived d = [||]new Base(); // It is always invalid except the target node has explicit conversion operator or is numeric. @@ -168,10 +175,9 @@ private static string GetSubItemName(SemanticModel semanticModel, int position, continue; } - validPotentialConversionTypes.Add(conversionTuple); + validPotentialConversionTypes.Add((targetNode, targetNodeConversionType)); } - validPotentialConversionTypes.RemoveDuplicates(); return validPotentialConversionTypes.ToImmutableAndClear(); } @@ -208,8 +214,9 @@ await editor.ApplyExpressionLevelSemanticEditsAsync( (semanticModel, root, spanNode) => { // All diagnostics have the same error code - if (TryGetTargetTypeInfo(document, semanticModel, root, diagnostics[0].Id, spanNode, cancellationToken, out var potentialConversionTypes) && - potentialConversionTypes.Length == 1) + var potentialConversionTypes = GetPotentialTargetTypes( + document, semanticModel, root, diagnostics[0].Id, spanNode, cancellationToken); + if (potentialConversionTypes.Length == 1) { var (newTarget, newReplacement) = ApplyFix(document, semanticModel, potentialConversionTypes[0].node, potentialConversionTypes[0].type, cancellationToken); return root.ReplaceNode(newTarget, newReplacement); diff --git a/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb b/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb index 77f5cabaa303c..acda9378bf972 100644 --- a/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb +++ b/src/Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb @@ -53,16 +53,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast Return expression.Cast(type, isResultPredefinedCast:=Nothing) End Function - Protected Overrides Function TryGetTargetTypeInfo( + Protected Overrides Sub AddPotentialTargetTypes( document As Document, semanticModel As SemanticModel, root As SyntaxNode, diagnosticId As String, spanNode As ExpressionSyntax, - cancellationToken As CancellationToken, - ByRef potentialConversionTypes As ImmutableArray(Of (ExpressionSyntax, ITypeSymbol))) As Boolean - potentialConversionTypes = ImmutableArray(Of (ExpressionSyntax, ITypeSymbol)).Empty - Dim mutablePotentialConversionTypes = ArrayBuilder(Of (ExpressionSyntax, ITypeSymbol)).GetInstance() + candidates As ArrayBuilder(Of (node As ExpressionSyntax, type As ITypeSymbol)), + cancellationToken As CancellationToken) Select Case diagnosticId Case BC30512, BC42016 @@ -72,14 +70,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast Dim argumentList = DirectCast(argument.Parent, ArgumentListSyntax) Dim invocationNode = argumentList.Parent - mutablePotentialConversionTypes.AddRange(_fixer.GetPotentialConversionTypes( + candidates.AddRange(_fixer.GetPotentialConversionTypes( document, semanticModel, root, argument, argumentList, invocationNode, cancellationToken)) Else ' spanNode is a right expression in assignment operation Dim inferenceService = document.GetRequiredLanguageService(Of ITypeInferenceService)() Dim conversionType = inferenceService.InferType(semanticModel, spanNode, objectAsDefault:=False, cancellationToken) - mutablePotentialConversionTypes.Add((spanNode, conversionType)) + candidates.Add((spanNode, conversionType)) End If Case BC30518, BC30519 Dim invocationExpressionNode = spanNode.GetAncestors(Of InvocationExpressionSyntax).FirstOrDefault( @@ -90,18 +88,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast ' Collect available cast pairs without target argument If invocationExpressionNode IsNot Nothing Then - mutablePotentialConversionTypes.AddRange( + candidates.AddRange( GetPotentialConversionTypesWithInvocationNode(document, semanticModel, root, invocationExpressionNode, cancellationToken)) ElseIf attributeNode IsNot Nothing Then - mutablePotentialConversionTypes.AddRange( + candidates.AddRange( GetPotentialConversionTypesWithInvocationNode(document, semanticModel, root, attributeNode, cancellationToken)) End If End Select - - ' clear up duplicate types - potentialConversionTypes = FilterValidPotentialConversionTypes(document, semanticModel, mutablePotentialConversionTypes) - Return Not potentialConversionTypes.IsEmpty - End Function + End Sub ''' ''' Find the first argument that need to be cast @@ -113,7 +107,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast ''' Private Shared Function GetTargetArgument( document As Document, - SemanticModel As SemanticModel, + semanticModel As SemanticModel, parameters As ImmutableArray(Of IParameterSymbol), arguments As SeparatedSyntaxList(Of ArgumentSyntax)) As ArgumentSyntax If parameters.Length = 0 Then @@ -144,14 +138,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.AddExplicitCast If parameters(parameterIndex).IsParams Then Dim paramsType = TryCast(parameterType, IArrayTypeSymbol) If paramsType IsNot Nothing Then - Dim conversion = SemanticModel.ClassifyConversion(argumentExpression, paramsType.ElementType) + Dim conversion = semanticModel.ClassifyConversion(argumentExpression, paramsType.ElementType) If conversion.Exists AndAlso Not conversion.IsIdentity Then Return arguments(i) End If End If End If - Dim argumentConversion = SemanticModel.ClassifyConversion(argumentExpression, parameterType) + Dim argumentConversion = semanticModel.ClassifyConversion(argumentExpression, parameterType) If argumentConversion.Exists AndAlso Not argumentConversion.IsIdentity Then Return arguments(i) End If From ab08bdfb9243d96029f886240ed78963a0057c91 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 1 Jul 2024 13:22:57 -0700 Subject: [PATCH 6/6] Improve cases with extra parens --- .../AddExplicitCast/AddExplicitCastTests.cs | 2 +- .../AbstractAddExplicitCastCodeFixProvider.cs | 39 ++++++++++++------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs b/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs index f441b57c1405c..5bb209dbc9423 100644 --- a/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs +++ b/src/Analyzers/CSharp/Tests/AddExplicitCast/AddExplicitCastTests.cs @@ -3318,7 +3318,7 @@ void M() { int a = 0; double b = 0; - a = ((int)(b += 1)); + a = (int)(b += 1); } } """); diff --git a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs index a4676fef25111..4bd41fdbffdf4 100644 --- a/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System; using System.Collections.Immutable; using System.Linq; using System.Threading; @@ -14,6 +13,7 @@ using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Collections; using Microsoft.CodeAnalysis.Shared.Extensions; +using Microsoft.CodeAnalysis.Simplification; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.CodeFixes.AddExplicitCast; @@ -123,26 +123,39 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) var syntaxFacts = document.GetRequiredLanguageService(); var semanticFacts = document.GetRequiredLanguageService(); - // if the node we're about to cast already has a cast, replace that cast if both are reference-identity downcasts. - if (syntaxFacts.IsCastExpression(targetNode) || syntaxFacts.IsConversionExpression(targetNode)) + var (currentTarget, currentReplacement) = ApplyFixWorker(); + + // If the original target was surrounded by parentheses, we can consider trying to remove that as well. + if (syntaxFacts.IsParenthesizedExpression(currentTarget.Parent)) { - GetPartsOfCastOrConversionExpression(targetNode, out var castTypeNode, out var castedExpression); + return (currentTarget.Parent, currentTarget.Parent.ReplaceNode(currentTarget, currentReplacement).WithAdditionalAnnotations(Simplifier.Annotation)); + } - var castType = semanticModel.GetTypeInfo(castTypeNode, cancellationToken).Type; - if (castType != null) + return (currentTarget, currentReplacement); + + (SyntaxNode finalTarget, SyntaxNode finalReplacement) ApplyFixWorker() + { + // if the node we're about to cast already has a cast, replace that cast if both are reference-identity downcasts. + if (syntaxFacts.IsCastExpression(targetNode) || syntaxFacts.IsConversionExpression(targetNode)) { - var firstConversion = semanticFacts.ClassifyConversion(semanticModel, castedExpression, castType); - var secondConversion = semanticModel.Compilation.ClassifyCommonConversion(castType, conversionType); + GetPartsOfCastOrConversionExpression(targetNode, out var castTypeNode, out var castedExpression); - if (firstConversion is { IsImplicit: false, IsReference: true } or { IsIdentity: true } && - secondConversion is { IsImplicit: false, IsReference: true }) + var castType = semanticModel.GetTypeInfo(castTypeNode, cancellationToken).Type; + if (castType != null) { - return (targetNode, this.Cast(castedExpression, conversionType).WithTriviaFrom(targetNode)); + var firstConversion = semanticFacts.ClassifyConversion(semanticModel, castedExpression, castType); + var secondConversion = semanticModel.Compilation.ClassifyCommonConversion(castType, conversionType); + + if (firstConversion is { IsImplicit: false, IsReference: true } or { IsIdentity: true } && + secondConversion is { IsImplicit: false, IsReference: true }) + { + return (targetNode, this.Cast(castedExpression, conversionType).WithTriviaFrom(targetNode)); + } } } - } - return Cast(semanticModel, targetNode, conversionType); + return Cast(semanticModel, targetNode, conversionType); + } } protected virtual (SyntaxNode finalTarget, SyntaxNode finalReplacement) Cast(SemanticModel semanticModel, TExpressionSyntax targetNode, ITypeSymbol conversionType)