-
Notifications
You must be signed in to change notification settings - Fork 457
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
f7df990
2db806b
29f3918
17c641a
3768e90
8e62dcd
f7fc00a
9d14a22
9b01d9d
b97a38c
111abcc
9de938a
ebf37bd
56f180d
f82374a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
## Release 5.0 | ||
|
||
### New Rules | ||
Rule ID | Category | Severity | Notes | ||
--------|----------|----------|------- | ||
CA2246 | Usage | Info | CSharpAssigningSymbolAndItsMemberInSameStatement, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2246) |
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 CSharpAssigningSymbolAndItsMemberInSameStatementFixer : 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)); | ||||
// For the first statement, take all trivia. | ||||
// For following statements, take only whitespace trivia. | ||||
trailingTrivia = trailingTrivia.Where(t => t.IsKind(SyntaxKind.WhitespaceTrivia) || t.IsKind(SyntaxKind.EndOfLineTrivia)).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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||
|
||||
#pragma warning disable RS1010 // Provide an explicit value for EquivalenceKey - false positve | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Evangelink Sure. Do you know which version got the fix? Line 56 in 91b109f
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same name suggestion here. We will currently see something like:
in the lightbulb, that's not really helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Evangelink To me, I'd interpret There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. I think I am a bit bothered by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm okay with renaming to |
||||
{ | ||||
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))); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure but maybe using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Evangelink Can you elaborate more on how I should use |
||||
|
||||
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 |
---|---|---|
|
@@ -153,7 +153,6 @@ CA2242 | Usage | Info | TestForNaNCorrectlyAnalyzer, [Documentation](https://doc | |
CA2243 | Usage | Disabled | AttributeStringLiteralsShouldParseCorrectlyAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2243) | ||
CA2244 | Usage | Info | AvoidDuplicateElementInitialization, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2244) | ||
CA2245 | Usage | Info | AvoidPropertySelfAssignment, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2245) | ||
CA2246 | Usage | Info | AssigningSymbolAndItsMemberInSameStatement, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2246) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
CA2247 | Usage | Warning | DoNotCreateTaskCompletionSourceWithWrongArguments, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2247) | ||
CA2248 | Usage | Info | ProvideCorrectArgumentToEnumHasFlag, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2248) | ||
CA2249 | Usage | Info | PreferStringContainsOverIndexOfAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2249) | ||
|
There was a problem hiding this comment.
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.