Fix leading comments not being preserved on variable case #74278
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
the
Community
|
@dotnet-policy-service agree |
| @@ -82,6 +83,15 @@ internal partial class CSharpUseCollectionExpressionForBuilderCodeFixProvider() | |||
| foreach (var (statement, _) in analysisResult.Matches) | |||
| subEditor.RemoveNode(statement); | |||
|
|
|||
| //Add the leading trivia from the declaration of the builder to the invocation where we convert the builder to a collection. | |||
| var dummyObjectCreationDeclaration = dummyObjectCreation.Ancestors().Take(3); | |||
There was a problem hiding this comment.
Why Take(3)?At least doc what that number means or turn it into a constant. But I suspect, that this logic is a bit too specific to a single line comment case. What if instead of
// Leading
var builder = ImmutableArray.CreateBuilder<Type>();you have
/* Leading */ var builder = ImmutableArray.CreateBuilder<int>();Yeah, Cyrus said in the issue that this is not a common patterns, but a) it mught crash that logic if there are less than 3 trivia elements and b) I don't think is is complicated to handle this case. Your trivia in single line comment case looks like this:
// Comment
<- white spaces hereWhat you need to do is remove that white space to preserve correct formatting after fix. I think, there is a WithoutTrailingWhiteSpace() extension method for syntax trivia or similar
| //Add the leading trivia from the declaration of the builder to the invocation where we convert the builder to a collection. | ||
| var dummyObjectCreationDeclaration = dummyObjectCreation.Ancestors().Take(3); | ||
|
|
||
| if (dummyObjectCreationDeclaration.Last() is VariableDeclarationSyntax variableDeclaration) |
There was a problem hiding this comment.
| if (dummyObjectCreationDeclaration.Last() is VariableDeclarationSyntax variableDeclaration) | |
| if (dummyObjectCreationDeclaration.LastOrDefault() is VariableDeclarationSyntax variableDeclaration) |
Last() crashes if there are no elements in the sequence. The desired behavior here was to use LastOrDefault()
| { | ||
| //Leading | ||
| {{pattern}} | ||
| //Leading |
There was a problem hiding this comment.
Name leading trivias differently for clarity, e.g. // Leading 1 and // Leading 2
Fixes: #74208
FYI:
This does not fix the return case:
return builder.ToImmutable()and the method case:Goo(builder.ToImmutable())and potential other cases i'm not aware of.