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.

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

Implement CodeFix for CA2246 #3694

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
4 changes: 4 additions & 0 deletions src/NetAnalyzers/CSharp/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
; Please do not edit this file manually, it should only be updated through code fix application.
### New Rules
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
CA2246 | Usage | Info | CSharpAssigningSymbolAndItsMemberInSameStatement, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2246)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeQuality.Analyzers;

namespace Microsoft.CodeQuality.CSharp.Analyzers.QualityGuidelines
{
[ExportCodeFixProvider(LanguageNames.CSharp, Name = CSharpAssigningSymbolAndItsMemberInSameStatement.RuleId), Shared]
public sealed class AssigningSymbolAndItsMemberInSameStatementFixer : CodeFixProvider
{
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(CSharpAssigningSymbolAndItsMemberInSameStatement.RuleId);

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
// For something like `a.x = a = b;`, we offer 3 code fixes:
// First:
// a = b;
// a.x = b;
// Second:
// a = b;
// a.x = a;
// Third: (Not currently implemented)
// var temp = a;
// a = b;
// temp.x = b;

var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
if (root.FindNode(context.Span).Parent is not AssignmentExpressionSyntax assignment)
{
return;
}

var members = GetAssignmentMembers(assignment);
if (members.Count < 3)
{
return;
}

var mostRightMember = members.Peek(); // Don't move this near its usage. The result will be different.
var leadingTrivia = assignment.Parent.GetLeadingTrivia();
var trailingTrivia = assignment.Parent.GetTrailingTrivia();
var replacements = new List<SyntaxNode>(members.Count - 1);

while (members.Count > 2)
{
replacements.Add(GetAssignmentExpressionStatement(members, leadingTrivia, trailingTrivia));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure which change to suggest (maybe the method name) but when reviewing it feels weird because we loop on members.Count and it's not really clear there is a change to this variable. At first I thought there was a "silly" bug.

// For the first statement, take all trivia.
// For following statements, take only whitespace trivia.
trailingTrivia = trailingTrivia.Where(t => t.IsKind(SyntaxKind.WhitespaceTrivia)).ToSyntaxTriviaList();
}

var title = MicrosoftCodeQualityAnalyzersResources.AssigningSymbolAndItsMemberInSameStatementTitle;

var replacements1 = replacements.Concat(GetAssignmentExpressionStatement(assignment.Left, mostRightMember, leadingTrivia, trailingTrivia));
var replacements2 = replacements.Concat(GetAssignmentExpressionStatement(members, leadingTrivia, trailingTrivia));
Comment on lines +67 to +68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use a meaningful name instead of XXX1 and XXX2? Maybe something like finalAssignmentToLeft and finalAssignmentToRight.


#pragma warning disable RS1010 // Provide an explicit value for EquivalenceKey - false positve
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having this pragma, could we bump the version of the analyzer we are using to rely on one where I have fixed this issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evangelink Sure. Do you know which version got the fix?
We will also need to remove the suppression in:

#pragma warning disable RS1010 // Provide an explicit value for EquivalenceKey - false positive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just made a test and it seems that the latest available pre-release (i.e. 3.3.1-beta1.20505.3) does not contain the fix so let's move on as-is and we will do a separate PR to get rid of both pragmas.

var nestedCodeAction = CodeAction.Create(title, ImmutableArray.Create<CodeAction>(
new MyCodeAction($"{title} 1", ct => GetDocument(context.Document, root, assignment.Parent, replacements1)),
new MyCodeAction($"{title} 2", ct => GetDocument(context.Document, root, assignment.Parent, replacements2))
Comment on lines +72 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same name suggestion here. We will currently see something like:

title
|-> title 1
|-> title 2

in the lightbulb, that's not really helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evangelink If I used the same wording you suggested in #3694 (comment) as titles (e.g. Split assignments with final assignment to the left variable), do you think it's good to the end-user?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. @mavasani any opinion?

), isInlinable: false);
#pragma warning restore RS1010

context.RegisterCodeFix(nestedCodeAction, context.Diagnostics);
}

/// <summary>
/// If the assignment expression is: a = b = c = d
/// Return a stack containing a, b, c, d with `a` at the bottom and `d` at the top.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
private static Stack<ExpressionSyntax> GetAssignmentMembers(AssignmentExpressionSyntax node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about SplitAssignments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evangelink To me, I'd interpret SplitAssignments such that it returns a collection of the new assignment statements. While it returns a collection of each member in the assignment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I think I am a bit bothered by Members because it's more Expressions but it probably doesn't matter much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with renaming to GetAssignmentExpressions if you think it's clearer.

{
var stack = new Stack<ExpressionSyntax>();
ExpressionSyntax current = node;
while (current is AssignmentExpressionSyntax assignment)
{
stack.Push(assignment.Left);
current = assignment.Right;
}
stack.Push(current);
return stack;
}

private static ExpressionStatementSyntax GetAssignmentExpressionStatement(Stack<ExpressionSyntax> stack, SyntaxTriviaList leadingTrivia, SyntaxTriviaList trailingTrivia)
{
var right = stack.Pop();
var left = stack.Peek();
return GetAssignmentExpressionStatement(left, right, leadingTrivia, trailingTrivia);
}

private static ExpressionStatementSyntax GetAssignmentExpressionStatement(ExpressionSyntax left, ExpressionSyntax right, SyntaxTriviaList leadingTrivia, SyntaxTriviaList trailingTrivia)
=> SyntaxFactory.ExpressionStatement(
SyntaxFactory.AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, left, right))
.WithLeadingTrivia(leadingTrivia).WithTrailingTrivia(trailingTrivia);

private static Task<Document> GetDocument(Document document, SyntaxNode root, SyntaxNode oldNode, IEnumerable<SyntaxNode> replacements)
=> Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(oldNode, replacements)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure but maybe using the DocumentEditor would help with solving the issue you are facing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evangelink Can you elaborate more on how I should use DocumentEditor to fix this?


public override FixAllProvider GetFixAllProvider()
=> WellKnownFixAllProviders.BatchFixer;

private class MyCodeAction : DocumentChangeAction
{
public MyCodeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument)
: base(title, createChangedDocument, title)
{
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeQuality.Analyzers;

namespace Microsoft.CodeQuality.Analyzers.QualityGuidelines
namespace Microsoft.CodeQuality.CSharp.Analyzers.QualityGuidelines
{
/// <summary>
/// CA2246: Prevent objects from being referenced in statements where they are reassigned
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class AssigningSymbolAndItsMemberInSameStatement : DiagnosticAnalyzer
public sealed class CSharpAssigningSymbolAndItsMemberInSameStatement : DiagnosticAnalyzer
{
internal const string RuleId = "CA2246";

Expand Down