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?
Conversation
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
...Quality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.Fixer.cs
Outdated
Show resolved
Hide resolved
|
||
string title = MicrosoftCodeQualityAnalyzersResources.AssigningSymbolAndItsMemberInSameStatementTitle; | ||
context.RegisterCodeFix(new MyCodeAction(title, | ||
async ct => await SplitAssignment(context.Document, node, ct).ConfigureAwait(false), |
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 believe we should offer 3 code fixes here as suggested in #2527 (comment). Otherwise, we might misdirect user into making a breaking change assuming the tooling is recommending this code fix, hence it should be the appropriate one. CA2246
is certainly one case where we should either have all fixes or none.
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.
@mavasani, Sure. I'd be happy to implement the other code fixes as soon as I get the current codefix working.
282bfb3
to
f7df990
Compare
@Evangelink @mavasani This is ready for another look. |
b1a5d93
to
2db806b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3694 +/- ##
===========================================
- Coverage 95.80% 46.96% -48.84%
===========================================
Files 1168 803 -365
Lines 264635 90387 -174248
Branches 15964 8623 -7341
===========================================
- Hits 253544 42454 -211090
- Misses 9077 46948 +37871
+ Partials 2014 985 -1029 |
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
@Evangelink I wasn't able to handle the case of 4 assignment members ( |
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 will need a second review (tomorrow) but there are some things off.
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
....CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
new MyCodeAction($"{title} 1", ct => GetDocument(context.Document, root, assignment.Parent, replacements1)), | ||
new MyCodeAction($"{title} 2", ct => GetDocument(context.Document, root, assignment.Parent, replacements2)) |
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.
The title
parameter is what get displayed so it feels off to have XXX 1
and XXX 2
.
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.
@Evangelink I have this temporary until I can get with good titles. Do you have any suggestions? Not sure how to describe the code fixes.
I've fixed the trivia handling. However, all tests that contains a comparison of 4 elements (e.g. It looks like that after the first replacement, the second will fail as it tries to replace a node that now isn't part of the syntax tree. I faced such a problem before and the solution was to use ReplaceNode which takes a lambda to compute replacement. But that solution is very hard to apply in this specific case. I'm out of ideas for how to fix these tests. |
var replacements1 = replacements.Concat(GetAssignmentExpressionStatement(assignment.Left, mostRightMember, leadingTrivia, trailingTrivia)); | ||
var replacements2 = replacements.Concat(GetAssignmentExpressionStatement(members, leadingTrivia, trailingTrivia)); | ||
|
||
#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 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?
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.
@Evangelink Sure. Do you know which version got the fix?
We will also need to remove the suppression in:
Line 56 in 91b109f
#pragma warning disable RS1010 // Provide an explicit value for EquivalenceKey - false positive |
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.
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.
|
||
while (members.Count > 2) | ||
{ | ||
replacements.Add(GetAssignmentExpressionStatement(members, leadingTrivia, trailingTrivia)); |
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.
var replacements1 = replacements.Concat(GetAssignmentExpressionStatement(assignment.Left, mostRightMember, leadingTrivia, trailingTrivia)); | ||
var replacements2 = replacements.Concat(GetAssignmentExpressionStatement(members, leadingTrivia, trailingTrivia)); |
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.
Could you please use a meaningful name instead of XXX1 and XXX2? Maybe something like finalAssignmentToLeft
and finalAssignmentToRight
.
new MyCodeAction($"{title} 1", ct => GetDocument(context.Document, root, assignment.Parent, replacements1)), | ||
new MyCodeAction($"{title} 2", ct => GetDocument(context.Document, root, assignment.Parent, replacements2)) |
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.
Same name suggestion here. We will currently see something like:
title
|-> title 1
|-> title 2
in the lightbulb, that's not really helpful.
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.
@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?
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 think so. @mavasani any opinion?
...uality.Analyzers/QualityGuidelines/CSharpAssigningSymbolAndItsMemberInSameStatement.Fixer.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
...t.CodeQuality.Analyzers/QualityGuidelines/AssigningSymbolAndItsMemberInSameStatementTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Amaury Levé <evangelink@gmail.com>
Co-authored-by: Amaury Levé <evangelink@gmail.com>
Fixes #2527