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

Add support for converting existing collection-initializers over to collection expressions. #69321

Merged
merged 72 commits into from
Aug 4, 2023

Conversation

CyrusNajmabadi
Copy link
Member

Part of #69132

Draft until #69280 goes in.

This means code like new List<int> { 1, 2, 3 } will get converted to [1, 2, 3]. We also detect this in the long-statement form like:

List<int> result = new List<int>();
result.Add(1);
result.Add(2);
result.Add(3);

And offer a similar conversion. Additionally we support converting things like:

List<int> result = new List<int>();
result.Add(1);
result.Add(2);
foreach (var v in x)
    result.Add(v);

to: [1, 2, .. v], and so on.

Note: this also works when the initializer already exists. For example:

List<int> result = new List<int> { 1, 2 };
foreach (var v in x)
    result.Add(v);

We try very hard to produce a collection expression that matches the style present, or looks good when no style is present. This involved effectively rewriting the entire core logic taht generates collection expressions to not use the formatting engine at all, but instead manually create everything from scratch. This adds a lot of logic, but makes the end results much higher quality.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review August 2, 2023 23:43
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 2, 2023 23:43
@@ -169,4 +176,101 @@ bool IsInTargetTypedBinaryExpression(BinaryExpressionSyntax binaryExpression, Ex
return binaryExpression.Kind() == SyntaxKind.CoalesceExpression && binaryExpression.Right == expression && HasType(binaryExpression.Left);
}
}

public static CollectionExpressionSyntax ConvertInitializerToCollectionExpression(
Copy link
Member Author

Choose a reason for hiding this comment

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

this helper is used by multiple fixers. SO it moved to this common location.

SourceText sourceText,
InitializerExpressionSyntax originalInitializer,
CollectionExpressionSyntax newCollectionExpression,
bool newCollectionIsSingleLine)
Copy link
Member Author

Choose a reason for hiding this comment

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

same with this helper.

@@ -83,59 +85,9 @@ void RewriteInitializerExpression(InitializerExpressionSyntax initializer)
IsOnSingleLine(sourceText, initializer)));
}

bool ShouldReplaceExistingExpressionEntirely(ExpressionSyntax explicitOrImplicitArray, InitializerExpressionSyntax initializer)
Copy link
Member Author

Choose a reason for hiding this comment

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

this helper moved to a common location.


private static CollectionExpressionSyntax ConvertInitializerToCollectionExpression(
Copy link
Member Author

Choose a reason for hiding this comment

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

this helper moved to a common location.

@@ -25,7 +23,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer
using static SyntaxFactory;

[ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.UseCollectionInitializer), Shared]
internal class CSharpUseCollectionInitializerCodeFixProvider :
internal partial class CSharpUseCollectionInitializerCodeFixProvider :
Copy link
Member Author

Choose a reason for hiding this comment

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

this type changed a ton. It is now 3 files. The main one, with common logic. And then two siblings. One sibling is for creating object-initializers, the other for object-creation expressions.

ImmutableArray<Match<StatementSyntax>> matches)
{
var expressions = CreateCollectionInitializerExpressions();
var withLineBreaks = AddLineBreaks(expressions);
Copy link
Member Author

Choose a reason for hiding this comment

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

the collection initializer case is pretty simple. We create a list of all the expressions, then add lines breaks between the elements (we always do initializers across multiple lines).

Note: in the future we may want to copy the code that properly indents expressions so that when we move expressions into the collection initializer we do a good job with multi-line exprs.

List<int> c =
[
1, 2
];
Copy link
Member Author

Choose a reason for hiding this comment

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

lots of tests got better now that we rewrote the formatting logic for collection-exprs.

2,
];
}
}
""");
}

[Fact]
public async Task TestReplacementLocation_NoElements_ExistingInitializer1()
Copy link
Member Author

Choose a reason for hiding this comment

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

thus begins a huge swath of tests created to validate a ton of whitespace/newline trivia scenarios.

/// Creates the final collection-expression <c>[...]</c> that will replace the given <paramref
/// name="objectCreation"/> expression.
/// </summary>
private static async Task<CollectionExpressionSyntax> CreateCollectionExpressionAsync(
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic is effectively entirely new. The code heavily emphasizes trying to match the new collection-expression style against the current code the user has (or infer what would be best for certain situations). I've tried to doc it heavily as to what it's doing.

@@ -218,28 +231,13 @@ private void AnalyzeNode(SyntaxNodeAnalysisContext context, INamedTypeSymbol ien

return (matches, shouldUseCollectionExpression: true);
}

bool AreCollectionExpressionsSupported()
Copy link
Member Author

Choose a reason for hiding this comment

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

just inlined this method.

@CyrusNajmabadi CyrusNajmabadi requested a review from akhera99 August 3, 2023 17:26
@CyrusNajmabadi
Copy link
Member Author

@akhera99 This is ready for review. THe tests should hopefully help indicate what's going on here and why so much was rewritten :)

@CyrusNajmabadi CyrusNajmabadi requested review from genlu and Cosifne August 4, 2023 15:48
@CyrusNajmabadi
Copy link
Member Author

@akhera99 @genlu @Cosifne ptal. thanks :)

List<int> c = [|new|] List<int>();
// Goo
// Bar
if (b1)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test like so

Suggested change
if (b1)
if (b1) // Comment

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. i'll add in followup pr. right now that comment will be removed.

@CyrusNajmabadi CyrusNajmabadi requested a review from akhera99 August 4, 2023 20:49
@CyrusNajmabadi CyrusNajmabadi merged commit 7c0566e into dotnet:main Aug 4, 2023
@ghost ghost added this to the Next milestone Aug 4, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the useCollectionExpression6 branch August 4, 2023 22:03
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants