Add support for generating the `.. x ? [y] : []` pattern in a collection-expression #69280

CyrusNajmabadi · 2023-07-28T19:43:09Z

Part of: #69132

This change makes it so that if we see:

List<int> list = new List<int>();
list.Add(1);
if (x)
    list.Add(2);

We now generate: [1, .. x ? [2] : []].

This is a form which we view as idiomatic for inserting elements conditionally, and will be optimized by the compiler.

As i've been working in this space, i've been pulling out cleanups that i think help the code. This PR is a collection of them.

THis also adds a ton more tests validating behavior.

Note: next PR works a lot on improving the formatting of the produced collection-expressions.

CyrusNajmabadi · 2023-07-31T23:21:30Z

src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems

@@ -83,6 +83,7 @@
    <Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\UseCollectionExpressionHelpers.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)UseCollectionInitializer\CSharpUseCollectionInitializerAnalyzer.cs" />


there used to be a single UseCollectionInitializerAnalyzer type that was used for both VB and C#. HOwever, it was getting crushed under the weight of having to have special logic (esp. logic that was really only for C#'s collection expressions).

We now use inheritance here, which cleans up a few things (especially around type arguments).

CyrusNajmabadi · 2023-07-31T23:24:38Z

...nalyzers/CSharp/Analyzers/UseCollectionInitializer/CSharpUseCollectionInitializerAnalyzer.cs

+        ForEachStatementSyntax statement,
+        out SyntaxToken identifier,
+        out ExpressionSyntax expression,
+        out IEnumerable<StatementSyntax> statements)


this is a better way than how i was previously doing things iwth ISYntaxFacts. The problem is that there are too many foreach-forms across C# and VB (and in C# itself). So syntaxfacts didn't really know how to deal with things.

Here though we know exactly what form we have, and we can produce a good output result for that for C# no matter what.

CyrusNajmabadi · 2023-07-31T23:25:13Z

...nalyzers/CSharp/Analyzers/UseCollectionInitializer/CSharpUseCollectionInitializerAnalyzer.cs

+        IfStatementSyntax statement,
+        out ExpressionSyntax condition,
+        out IEnumerable<StatementSyntax> whenTrueStatements,
+        out IEnumerable<StatementSyntax>? whenFalseStatements)


same issue. C# has if/else (Where else is optional). VB has if/elif/elif/else. These are really different forms, and trying to shoehorn into ISyntaxFacts was not the right way to do things.

CyrusNajmabadi · 2023-07-31T23:26:32Z

...s/CSharp/CodeFixes/UseCollectionInitializer/CSharpUseCollectionInitializerCodeFixProvider.cs

-                if (!sourceText.AreOnSameLine(expression.GetFirstToken(), expression.GetLastToken()))
-                    return true;
-            }
-
            var totalLength = "{}".Length;
            foreach (var match in matches)


note: while this is getting cleaned up here, it is also getting refactored heavily in the next PR where i redo how we do formatting here entirely. so don't have to examien this too closely :)

CyrusNajmabadi · 2023-07-31T23:29:45Z

src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests.cs

+                            1
+                        };
+                        if (b)
+                            c.Add(2);


these tests validate that for the collection INITIALIZER space we don't do anything special. The new form is for the collection EXPRESSION case.

CyrusNajmabadi · 2023-07-31T23:30:04Z

.../CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests_CollectionExpression.cs

+            {
+                void M(bool b)
+                {
+                    List<int> c = [1, .. {|CS0173:b ? [2] : []|}];


this error is going to be fixed by chuck soon.

CyrusNajmabadi · 2023-07-31T23:30:36Z

.../CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests_CollectionExpression.cs

+                void M()
+                {
+                    List<int> c = [1
+            ];


this formatting is terrible. next PR fixes this up. but it's a lot of manual trivia handling, so i'm pulling it out of this PR to keep things sane.

CyrusNajmabadi · 2023-07-31T23:31:25Z

...nalyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerAnalyzer.cs

+            TForeachStatementSyntax,
+            TIfStatementSyntax,
+            TVariableDeclaratorSyntax,
+            TAnalyzer>, new()


yes. this part is brain melting. but it means the consumption side of this type (which references the derived types) now gets much simpler. WIll point that out when we get tehre.

CyrusNajmabadi · 2023-07-31T23:32:34Z

...nalyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerAnalyzer.cs

+                // if we have an object creation, and it *already* has an initializer in it (like `new T { { x, y } }`)
+                // this can't legally become a collection expression.
+                if (_analyzeForCollectionExpression && this.IsComplexElementInitializer(firstInit))
+                    return false;


needed to make this bool returning as we cannot use the "empty result means failure" approach for 'collection expressions'. For collection expressions an empty result is fine and means "convert to []".

CyrusNajmabadi · 2023-07-31T23:33:27Z

...nalyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerAnalyzer.cs

@@ -107,86 +149,157 @@ protected override void AddMatches(ArrayBuilder<Match<TStatementSyntax>> matches
                    continue;
                }

-                if (extractedChild is TExpressionStatementSyntax expressionStatement)
+                if (extractedChild is TExpressionStatementSyntax expressionStatement &&
+                    TryProcessExpressionStatement(expressionStatement))


this large if/else was extracted into separate nested helper local functions.

One for x.Add(y), one for foreach (var y in z) x.Add(y) and one for if (z) x.Add(y)

CyrusNajmabadi · 2023-07-31T23:34:35Z

...s/Core/CodeFixes/UseCollectionInitializer/AbstractUseCollectionInitializerCodeFixProvider.cs

            while (originalObjectCreationNodes.Count > 0)
            {
                var (originalObjectCreation, useCollectionExpression) = originalObjectCreationNodes.Pop();
                var objectCreation = currentRoot.GetCurrentNodes(originalObjectCreation).Single();

-                var matches = UseCollectionInitializerAnalyzer<TExpressionSyntax, TStatementSyntax, TObjectCreationExpressionSyntax, TMemberAccessExpressionSyntax, TInvocationExpressionSyntax, TExpressionStatementSyntax, TForeachStatementSyntax, TVariableDeclaratorSyntax>.Analyze(
-                    semanticModel, syntaxFacts, objectCreation, useCollectionExpression, cancellationToken);


no need for consumption sides of the helper type ot have to have so much noise. this was getting particularly awful as i needded to add 'IfStatementSyntax' here (and all the other places).

CyrusNajmabadi · 2023-07-31T23:35:20Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

-            statements = commonForeach.Statement is BlockSyntax block
-                ? block.Statements
-                : SpecializedCollections.SingletonEnumerable(commonForeach.Statement);
+            return commonForeach.Expression;


example of how gnarly this was trying to be a syntaxfact (which i added in the previous PR). was happy to remove this here.

CyrusNajmabadi · 2023-07-31T23:35:36Z

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

-        Public Sub GetPartsOfForeachStatement(statement As SyntaxNode, ByRef identifier As SyntaxToken, ByRef expression As SyntaxNode, ByRef statements As IEnumerable(Of SyntaxNode)) Implements ISyntaxFacts.GetPartsOfForeachStatement
-            Dim foreachStatement = DirectCast(statement, ForEachStatementSyntax)
-            Dim declarator = TryCast(foreachStatement.ControlVariable, VariableDeclaratorSyntax)
-            identifier = If(declarator Is Nothing, Nothing, declarator.Names(0).Identifier)


similar gnarlyness for VB.

CyrusNajmabadi · 2023-07-31T23:35:48Z

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

-        Public Sub GetPartsOfForeachStatement(statement As SyntaxNode, ByRef identifier As SyntaxToken, ByRef expression As SyntaxNode, ByRef statements As IEnumerable(Of SyntaxNode)) Implements ISyntaxFacts.GetPartsOfForeachStatement
-            Dim foreachStatement = DirectCast(statement, ForEachStatementSyntax)
-            Dim declarator = TryCast(foreachStatement.ControlVariable, VariableDeclaratorSyntax)
-            identifier = If(declarator Is Nothing, Nothing, declarator.Names(0).Identifier)


similar gnarlyness for VB.

CyrusNajmabadi · 2023-07-31T23:35:55Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

-            statements = commonForeach.Statement is BlockSyntax block
-                ? block.Statements
-                : SpecializedCollections.SingletonEnumerable(commonForeach.Statement);
+            return commonForeach.Expression;


example of how gnarly this was trying to be a syntaxfact (which i added in the previous PR). was happy to remove this here.

CyrusNajmabadi · 2023-07-31T23:36:12Z

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

-        Public Sub GetPartsOfForeachStatement(statement As SyntaxNode, ByRef identifier As SyntaxToken, ByRef expression As SyntaxNode, ByRef statements As IEnumerable(Of SyntaxNode)) Implements ISyntaxFacts.GetPartsOfForeachStatement
-            Dim foreachStatement = DirectCast(statement, ForEachStatementSyntax)
-            Dim declarator = TryCast(foreachStatement.ControlVariable, VariableDeclaratorSyntax)
-            identifier = If(declarator Is Nothing, Nothing, declarator.Names(0).Identifier)


similar gnarlyness for VB.

CyrusNajmabadi · 2023-07-31T23:36:26Z

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

-        Public Sub GetPartsOfForeachStatement(statement As SyntaxNode, ByRef identifier As SyntaxToken, ByRef expression As SyntaxNode, ByRef statements As IEnumerable(Of SyntaxNode)) Implements ISyntaxFacts.GetPartsOfForeachStatement
-            Dim foreachStatement = DirectCast(statement, ForEachStatementSyntax)
-            Dim declarator = TryCast(foreachStatement.ControlVariable, VariableDeclaratorSyntax)
-            identifier = If(declarator Is Nothing, Nothing, declarator.Names(0).Identifier)


similar gnarlyness for VB.

CyrusNajmabadi · 2023-07-31T23:37:55Z

@genlu @Cosifne @akhera99 this is ready for review. I'm sorry for al the churn, but i'm trying to have independent PRs that solve core issues, without something absolutely ginormous that rolls it all in.

However, it does mean that as new things are added, there's often reworking of the core logic to try to keep it clean/rational.

CyrusNajmabadi · 2023-07-31T23:39:36Z

NOte: after the next PR, things should be in a much nicer state. So only 2 more to go with the deep refactorings :)

CyrusNajmabadi · 2023-08-01T19:30:41Z

@akhera99 ptal :)

CyrusNajmabadi · 2023-08-02T05:20:31Z

@akhera99 ptal :)

CyrusNajmabadi added 7 commits July 28, 2023 09:17

Simplify

00eadd6

In progress

51e266a

Merge branch 'useCollectionExpression3' into useCollectionExpression4

a96d399

in progress

7fe6746

Typesafe

e6481fb

testS

a302e5d

Fixes and tests

e9858ea

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 28, 2023

CyrusNajmabadi changed the title ~~Use collection expression4~~ WIP: not ready for review. Jul 28, 2023

CyrusNajmabadi added 4 commits July 28, 2023 12:58

Merge branch 'useCollectionExpression3' into useCollectionExpression4

cde8879

Merge branch 'main' into useCollectionExpression4

bfc5393

Merge branch 'useCollectionExpression3' into useCollectionExpression4

6764a5a

Merge branch 'main' into useCollectionExpression4

d073f13

CyrusNajmabadi changed the title ~~WIP: not ready for review.~~ Cleanup/refactor code in 'use collection initializer/expression'. Jul 31, 2023

CyrusNajmabadi commented Jul 31, 2023

View reviewed changes

formattign

027d00c

CyrusNajmabadi commented Jul 31, 2023

View reviewed changes

CyrusNajmabadi changed the title ~~Cleanup/refactor code in 'use collection initializer/expression'.~~ Add support for generating the .. x ? [y] : [] pattern in a collection-expression Jul 31, 2023

CyrusNajmabadi commented Jul 31, 2023

View reviewed changes

CyrusNajmabadi requested review from genlu, Cosifne and akhera99 July 31, 2023 23:36

CyrusNajmabadi marked this pull request as ready for review July 31, 2023 23:36

CyrusNajmabadi requested a review from a team as a code owner July 31, 2023 23:36

This was referenced Aug 1, 2023

Tracking issue for analyzers/fixers to convert existing code to collection expressions. #69132

Closed

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

Merged

akhera99 approved these changes Aug 2, 2023

View reviewed changes

CyrusNajmabadi merged commit 21c31f9 into dotnet:main Aug 2, 2023

CyrusNajmabadi deleted the useCollectionExpression4 branch August 2, 2023 22:34

ghost added this to the Next milestone Aug 2, 2023

allisonchou mentioned this pull request Aug 3, 2023

[Automated] PRs inserted in VS build main-34003.103 #69363

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generating the `.. x ? [y] : []` pattern in a collection-expression #69280

Add support for generating the `.. x ? [y] : []` pattern in a collection-expression #69280

CyrusNajmabadi commented Jul 28, 2023 •

edited

Loading

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi Jul 31, 2023

CyrusNajmabadi commented Jul 31, 2023

CyrusNajmabadi commented Jul 31, 2023

CyrusNajmabadi commented Aug 1, 2023

CyrusNajmabadi commented Aug 2, 2023

Add support for generating the .. x ? [y] : [] pattern in a collection-expression #69280

Add support for generating the .. x ? [y] : [] pattern in a collection-expression #69280

Conversation

CyrusNajmabadi commented Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 31, 2023

CyrusNajmabadi commented Jul 31, 2023

CyrusNajmabadi commented Aug 1, 2023

CyrusNajmabadi commented Aug 2, 2023

Add support for generating the `.. x ? [y] : []` pattern in a collection-expression #69280

Add support for generating the `.. x ? [y] : []` pattern in a collection-expression #69280

CyrusNajmabadi commented Jul 28, 2023 •

edited

Loading