IDE0054: Fix compound assignment increment in expression context#54324
Conversation
| return Increment((TExpressionSyntax)leftOfAssign.WithoutTrailingTrivia(), postfix: syntaxFacts.IsStatement(currentAssignment.Parent)); | ||
| } | ||
|
|
||
| if (diagnostic.Properties.ContainsKey(UseCompoundAssignmentUtilities.Decrement)) | ||
| { | ||
| return Decrement((TExpressionSyntax)leftOfAssign); | ||
| return Decrement((TExpressionSyntax)leftOfAssign.WithoutTrailingTrivia(), postfix: syntaxFacts.IsStatement(currentAssignment.Parent)); |
There was a problem hiding this comment.
syntaxFacts.IsStatement seems legit to address #53969 (comment), but maybe there is a better way to detect
if the result of the assignment is used
There is also a lot of repetition, but I don't see a simple way to avoid that.
There was a problem hiding this comment.
What about switch (x = x + 1)? The assignment parent is a statement, but it should be prefix. Am I missing something?
There was a problem hiding this comment.
Yes, you are right. Maybe IsSimpleAssignmentStatement is better.
There was a problem hiding this comment.
I'm not sure. But things I'd make sure they are not broken:
switch(...)return ...yield return ...while (...)var x = ...var x = (...)bool_expr ? ... : ...MethodCall(...)
There was a problem hiding this comment.
Instead of doing syntax analysis here, could we do that by data flow analysis?
| { | ||
| return SyntaxFactory.PostfixUnaryExpression(SyntaxKind.PostIncrementExpression, left); | ||
| } | ||
| protected override ExpressionSyntax Increment(ExpressionSyntax left, bool postfix) |
There was a problem hiding this comment.
nit: I'd call the parameter "operand" instead of left.
| void M() | ||
| { | ||
| for (int i = 0; i < 10; i++) | ||
| for (int i = 0; i < 10; ++i) |
There was a problem hiding this comment.
would prefer this not change.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
tentatively requesting changes.
- if we can make it so we don't change the for loop incrementor, that would be nice.
- insteaf of .WithoutTrivia just do .WithTriviaFrom to preserve leading/trailing trivia.
|
Are you ok with me pushing a couple of changes here? Tahnks! |
| /// <param name="startValue">C#: null | VB: 0</param> | ||
| /// <param name="endCondition">C#: i < 10 | VB: 10</param> | ||
| /// <param name="increments">C#: i++ | VB: Step 1</param> | ||
| void GetPartsOfForStatement(SyntaxNode? node, out SyntaxNode? variableDeclaration, out SyntaxNode? startValue, out SyntaxNode? endCondition, out ImmutableArray<SyntaxNode> increments); |
There was a problem hiding this comment.
I thought the for loops of VB and C# are close enough for a "GetPartsOf" attempt, but they turned out to be quite different. Maybe it's better to create something like IsPartOfForLoopIncrementor in AbstractUseCompoundAssignmentCodeFixProvider or ISyntaxFacts.
Sure. |
|
thanks! |
Fix #53969