Collection expression argument parsing (`[with(x, y, z), a, b, c]`). by CyrusNajmabadi · Pull Request #76003 · dotnet/roslyn

CyrusNajmabadi · 2024-11-21T07:00:03Z

An exploration around adding a with(...arguments...) element to collection expressions, to allow arguments to be passed to the final created collection. For example, for: Dictionary<string, int> nameToAge = [with(StringComparer.CaseInsensitive), "mads": 21, .. existingDic].

Relates to test plan #76310

dotnet-policy-service · 2024-11-21T07:17:38Z

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

…s' into withElement

cston · 2025-01-23T18:09:10Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                var element = syntax.Elements[i];
+                if (element is WithElementSyntax withElement)
+                {
+                    Error(diagnostics, ErrorCode.ERR_WithElementMustBeFirst, withElement.WithKeyword.GetLocation());


ERR_WithElementMustBeFirst

Are we testing this? #Closed

likely not. t his was just the parsing PR. binding/testing was intended for you to do :)

If we're adding an error code and binding logic, we should test it. Either way seems fine for this PR (remove logic or test)

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

cston · 2025-01-23T18:10:25Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+            {
+                if (element is KeyValuePairElementSyntax keyValuePairElement)
+                {
+                    MessageID.IDS_FeatureDictionaryExpressions.CheckFeatureAvailability(diagnostics, syntax, keyValuePairElement.ColonToken.GetLocation());


IDS_FeatureDictionaryExpressions.CheckFeatureAvailability

This is checked in BindKeyValuePair(). #Closed

cston · 2025-01-23T18:10:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                }
+                else if (element is WithElementSyntax withElement)
+                {
+                    MessageID.IDS_FeatureCollectionExpressionConstructionArguments.CheckFeatureAvailability(diagnostics, syntax, withElement.WithKeyword.GetLocation());


IDS_FeatureCollectionExpressionConstructionArguments

Are we testing this?

likely not. t his was just the parsing PR. binding/testing was intended for you to do :)

cston · 2025-01-23T18:11:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+
+                    // PROTOTYPE: Error for now.  Flesh this out when we do the binding for `with(...)` arguments.
+                    Error(diagnostics, ErrorCode.ERR_SyntaxError, withElement.WithKeyword, ",");
+                    return new BoundBadExpression(syntax, LookupResultKind.Empty, symbols: [], childBoundNodes: [], CreateErrorType());


return new BoundBadExpression

Is there a reason to fail binding for the collection, or even the element? #Closed

this was just to unblock the PR. i would expect us to want us to be very resilient in practice. but i did no binding work.

cston · 2025-01-23T18:13:49Z

src/Compilers/CSharp/Portable/Errors/MessageID.cs


        IDS_FeatureFirstClassSpan = MessageBase + 12849,
        IDS_FeatureDictionaryExpressions = MessageBase + 12850,
+        IDS_FeatureCollectionExpressionConstructionArguments = MessageBase + 12851,


Consider shortening, perhaps to IDS_FeatureCollectionExpressionArguments. #Closed

cston

LGTM.

Please squash commits when merging, thanks.

CyrusNajmabadi · 2025-01-23T21:45:09Z

...ted/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Main.Generated.cs

+        return (WithElementSyntax)Syntax.InternalSyntax.SyntaxFactory.WithElement((Syntax.InternalSyntax.SyntaxToken)withKeyword.Node!, (Syntax.InternalSyntax.ArgumentListSyntax)argumentList.Green).CreateRed();
+    }
+
+#pragma warning disable RS0027


note: this required changing the generator. but we've done that in the past for this exact issue. fbofw our generator generates a pattern which our RS rules don't particularly like (though we haven't had an actual problem with the syntax apis here, so it just is a conflict in philosophy).

jcouv · 2025-01-23T21:49:38Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                if (element is WithElementSyntax withElement)
+                {
+                    Error(diagnostics, ErrorCode.ERR_WithElementMustBeFirst, withElement.WithKeyword.GetLocation());
+                    break;


Why are we breaking out of the loop? #Closed

because we reported the error on the first vioaltor. I didn't see any value in continuing to report on further violations :)

taht said, binding behavior is entirely in chuck's court. if he wants to change this, i have no issue with that.

...torFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs

jcouv · 2025-01-23T21:54:22Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

            var hasAttributeOrModifiersList = nd.Fields.Any(f => IsAttributeOrModifiersList(f));
-
-            if (hasOptional && hasAttributeOrModifiersList)
+            var writePragma = hasOptional && (hasAttributeOrModifiersList || nd.Name == "WithElementSyntax");


I'm lacking context. What is RS0027 and why does WithElementSyntax need special treatment here? #Closed

i marked the place in teh PR where this has impact. RS2007 says that you should not have APIs that have optional members, if there is an api that has more non optional arguments. Thi sis a suspect, and unmainted roslyn-specific rule that we violate all the time. Basically, it's fine that one can do SyntaxFactory.WithElement() or SyntaxFactory.WithElement(someWithKeyword, someArgList).

this just suppresses that warning in the generated code (which we do with many other nodes).

Thanks
Just to confirm these are the two APIs that would cause that analyzer diagnostic?

static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.WithElement(Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax? argumentList = null) -> Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax! static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.WithElement(Microsoft.CodeAnalysis.SyntaxToken withKeyword, Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax! argumentList) -> Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax!

jcouv · 2025-01-23T21:59:04Z

src/Compilers/CSharp/Test/Syntax/Parsing/CollectionExpressionParsingTests.cs

+    [Fact]
+    public void WithElement2()
+    {
+        UsingExpression("[with()]");


This looks like a breaking change. Let's document it.
Should it be made LangVer-dependent? #Closed

We can doc, but def want to run by ldm first. Our investigations here indicate no usage of this in the wild. So we're going to see if we can get buyoff on this just being a unilateral break .

this is part of hte final syntax/sign-off discussion there.

Should it be made LangVer-dependent?

This is currently an open question.

Since the PR implements a break, let's add a PROTOTYPE marker (to test with LangVer and document break).

open LDM question.

Since the PR implements a break, let's add a PROTOTYPE marker (to test with LangVer and document break).

Done.

Suggested change

UsingExpression("[with()]");

// PROTOTYPE test with LangVer and document break

UsingExpression("[with()]");

My reasoning for pushing on this: If LDM were to approve the break as it is implemented by this PR, so nobody revisits this code, who is going to remember that we didn't document it? Usually, PRs that implement breaks also document them.

Thanks. I see this was addressed elsewhere

jcouv

LGTM Thanks!

CyrusNajmabadi added 14 commits November 20, 2024 18:34

Add WithElement syntax

2325024

Generate code

051fe26

Add parsing

a0dc38a

Add binding bail out.

81d0753

Add specific error

ae6ba2e

Add tests

50957f7

Add tests

fa8324e

Add tests

12ace41

Add tests

5afa1ca

Add tests

3769cc4

Add tests

a2be131

rename

cac7f01

Add tests

aa33541

update switch

329a8a2

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 21, 2024

CyrusNajmabadi added 2 commits November 20, 2024 23:17

add public apis

f658be9

add public apis

29f9858

dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Nov 21, 2024

CyrusNajmabadi added 4 commits November 21, 2024 10:59

success tests

967fef1

failure tests

c8c6c89

Fix

dab5884

Merge remote-tracking branch 'upstream/features/dictionary-expression…

c9e4e35

…s' into withElement

cston reviewed Jan 23, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated Show resolved Hide resolved

cston reviewed Jan 23, 2025

View reviewed changes

cston approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'features/dictionary-expressions' into withElement

5ae0f85

CyrusNajmabadi enabled auto-merge (squash) January 23, 2025 21:23

cston approved these changes Jan 23, 2025

View reviewed changes

CyrusNajmabadi disabled auto-merge January 23, 2025 21:31

jcouv added the Feature - Dictionary Expressions label Jan 23, 2025

Fix generation

268b113

CyrusNajmabadi commented Jan 23, 2025

View reviewed changes

jcouv reviewed Jan 23, 2025

View reviewed changes

...torFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs Show resolved Hide resolved

jcouv reviewed Jan 23, 2025

View reviewed changes

Add prototype

4a33027

jcouv reviewed Jan 23, 2025

View reviewed changes

CyrusNajmabadi added 2 commits January 23, 2025 14:15

Add tests

2e209be

Add docS

95f7fd1

cston approved these changes Jan 23, 2025

View reviewed changes

CyrusNajmabadi added 6 commits January 23, 2025 14:40

Report errors for every with element out of place

a2c65ce

Add tests

5ea87d8

No binding at all

81ecb04

Remove

b052f37

REmove error code

40a6c29

REmove error code

11c65e3

jcouv approved these changes Jan 23, 2025

View reviewed changes

cston approved these changes Jan 23, 2025

View reviewed changes

CyrusNajmabadi enabled auto-merge (squash) January 23, 2025 23:38

Revert

4e2c669

CyrusNajmabadi merged commit f806fd4 into dotnet:features/dictionary-expressions Jan 24, 2025
25 of 28 checks passed

CyrusNajmabadi deleted the withElement branch January 24, 2025 04:24

dotnet-maestro bot mentioned this pull request Jan 22, 2026

[main] Source code updates from dotnet/roslyn dotnet/dotnet#4349

Merged

dotnet-bot mentioned this pull request Jan 24, 2026

[Automated] PRs inserted in VS build main-11423.137 #82138

Closed

	UsingExpression("[with()]");
	// PROTOTYPE test with LangVer and document break
	UsingExpression("[with()]");

Conversation

CyrusNajmabadi commented Nov 21, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Nov 21, 2024

Uh oh!

cston Jan 23, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cston Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcouv Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

CyrusNajmabadi commented Nov 21, 2024 •

edited by jcouv

Loading

cston Jan 23, 2025 •

edited by jcouv

Loading

cston Jan 23, 2025 •

edited

Loading

cston Jan 23, 2025 •

edited

Loading

cston Jan 23, 2025 •

edited

Loading

jcouv Jan 23, 2025 •

edited

Loading

jcouv Jan 23, 2025 •

edited

Loading

jcouv Jan 23, 2025 •

edited

Loading

jcouv Jan 23, 2025 •

edited

Loading