Dictionary expressions: remove PROTOTYPE comments #77876

cston · 2025-03-27T18:31:21Z

No description provided.

cston · 2025-03-27T23:46:20Z

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

@@ -907,8 +904,6 @@ private BoundCollectionExpression ConvertCollectionExpression(
                    {
                        var analyzedArguments = AnalyzedArguments.GetInstance();
                        withElement.AddToArguments(analyzedArguments);
-                        // PROTOTYPE: If there are multiple with() elements, should with() elements after


PROTOTYPE

We can bind for error recovery rather than binding a second constructor call if it's difficult to support the latter from the public API, for instance, from IOperation. Otherwise, binding constructor calls for each seems simpler.

As long as the bad constructor calls say they have errors in the operation tree, this seems fine. That's just us giving the full information about what we think the code means.

cston · 2025-03-27T23:47:50Z

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

@@ -1004,8 +997,6 @@ private BoundCollectionExpression ConvertCollectionExpression(
                if (collectionCreation is null)
                {
                    // Bind collection creation with no arguments.
-                    // PROTOTYPE: Should we require a factory method callable with no arguments even if arguments are provided


PROTOTYPE

Tracked with an open question.

cston · 2025-03-27T23:52:04Z

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

@@ -6631,8 +6631,6 @@ static BoundCall bindKeyOrValue(SyntaxNode syntax, BoundExpression expr, MethodS
                return new BoundCall(
                    syntax,
                    receiverOpt: expr,
-                    // PROTOTYPE: What is the correct value for receiver cloning? Test with expression element


PROTOTYPE

These BoundCall nodes are for accessors from a well-known struct type, KeyValuePair.Key<K, V> { get; } and KeyValuePair<K, V>.Value { get; }, so I don't think this applies.

Were the indicated tests added? It wasn't clear to me why these accessors being on well-known type affects what value we should pass here.

It seems like existing places in the binder where False is passed here, are taking care to ensure that the receiver is something we can take a reference to.

Switched to calling ReceiverIsSubjectToCloning(expr, getMethod), to avoid hardcoding the value. However, expr is a BoundValuePlaceholder so ReceiverIsSubjectToCloning() simply returns ThreeState.False.

Should expr be the original expression that the placeholder will be replaced with? cc @AlekseyTs

Should expr be the original expression that the placeholder will be replaced with?

Strictly speaking, the value that we are supposed to pass to the BoundCall constructor for initialBindingReceiverIsSubjectToCloning parameter should reflect what is actually going to happen during code generation. I assume that during lowering, a key value pair will be captured in a temp and then the accessor will be invoked on that temp. Therefore, a cloning will happen, but not as part of IL emit for the node itself.
On the other hand, as far as I understand, the actual initialBindingReceiverIsSubjectToCloning value is interesting for ref safety analysis only. Does this node go through the analysis? If it doesn't, then passing ThreeState.Unknown would be the honest and the simplest thing to do. If it does, again, strictly speaking, we should pass ThreeState.True. However, it looks like GetInvocationArgumentsForEscape doesn't like placeholders as receivers and it assumes specific SafeContext value for the future temp, which might or might not reflect the actual SafeContext of the temp introduced during lowering. At the same time, we probably can assume that the KeyValuePair is not a ref struct and the accessor takes no refs and returns no refs, there should be no ref safety issues due to the cloning (a ref to that temp location cannot escape from the accessor), therefore, passing ThreeState.False would be correct (little explanation comment might be helpful for a future code reader).

In my opinion, calling ReceiverIsSubjectToCloning here with placeholder probably doesn't help anyone, and as a follow-up we probably should change it to either assert on placeholders, or return ThreeState.Unknown when type is not known to be a reference type.

Thanks for the detailed explanation. I've switched back to ThreeState.False and added a comment from your explanation.

cston · 2025-03-27T23:53:14Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs

@@ -1473,8 +1475,6 @@ class Program
                }
                """;
            comp = CreateCompilation([sourceA, sourceB2], targetFramework: TargetFramework.Net80);
-            // PROTOTYPE: Update the params-collection spec so it's clear that we require "... a factory method callable with no additional


PROTOTYPE

See dotnet/csharplang#9264.

cston · 2025-03-27T23:53:56Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs

@@ -16309,7 +16309,6 @@ static void Main()
                }
                """;
            comp = CreateCompilation(sourceB, references: new[] { refA }, targetFramework: TargetFramework.Net80);
-            // PROTOTYPE: ERR_CollectionBuilderAttributeMethodNotFound error message is stale. It should say "... with a first parameter of type 'ReadOnlySpan<object> callable with no additional arguments, ...".


PROTOTYPE

The ReadOnlySpan<T> parameter should be last, not first, so the current message seems sufficient.

jcouv

LGTM Thanks (iteration 5)

RikkiGibson · 2025-03-28T16:41:39Z

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

@@ -6631,8 +6631,6 @@ static BoundCall bindKeyOrValue(SyntaxNode syntax, BoundExpression expr, MethodS
                return new BoundCall(
                    syntax,
                    receiverOpt: expr,
-                    // PROTOTYPE: What is the correct value for receiver cloning? Test with expression element


Were the indicated tests added? It wasn't clear to me why these accessors being on well-known type affects what value we should pass here.

It seems like existing places in the binder where False is passed here, are taking care to ensure that the receiver is something we can take a reference to.

RikkiGibson · 2025-03-28T16:42:44Z

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

@@ -907,8 +904,6 @@ private BoundCollectionExpression ConvertCollectionExpression(
                    {
                        var analyzedArguments = AnalyzedArguments.GetInstance();
                        withElement.AddToArguments(analyzedArguments);
-                        // PROTOTYPE: If there are multiple with() elements, should with() elements after


As long as the bad constructor calls say they have errors in the operation tree, this seems fine. That's just us giving the full information about what we think the code means.

RikkiGibson · 2025-03-28T16:46:41Z

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

-            SyntaxKind.SpreadElement,
-            SyntaxKind.KeyValuePairElement,
-            SyntaxKind.WithElement,
+            SyntaxKind.WithElement, // Needed when language version is updated.


I didn't understand what this means. Is the with-element in TestResource.resx not detected at the moment? #Resolved

The parser only generates SyntaxKind.WithElement when compiling with LanguageVersion.Preview, but this test currently uses LanguageVersion.Latest. When Latest is updated, this case can be removed.

RikkiGibson · 2025-03-28T16:51:44Z

src/Compilers/CSharp/Test/Emit3/Semantics/DictionaryExpressionTests.cs

+            string sourceB = """
+                class Program
+                {
+                    static MyDictionary<K, V> FromPair1<K, V>(K k, V v)


Some tests maybe worth adding after feature merge:

MyDictionary<K, V> d = [k:v]; return d;

FromPair(ref K k, ref V v) => [k:v]

MyDictionary<K, V> d = [MakeKey():v]; return d; #Resolved

RikkiGibson · 2025-03-28T17:11:49Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs

-            // PROTOTYPE: [with(x)] and [with(x), y] should result in errors since x should not be included in the params argument.
+            // https://github.com/dotnet/roslyn/issues/77866: [with(x)] and [with(x), y] should
+            // result in errors since x should not be included in the params argument. Should
+            // be fixed when the last parameter of the builder method is the items parameter.


Is this saying we're going to add some enforcement later which will avoid including the 'with' arguments for the last parameter of Create? It might be good to demonstrate what arguments are actually being passed for items in Create(items), e.g. with expectedOutput. But if it's thought to be too much of a corner case, we can just wait to take the fix instead. #Resolved

Yes, I expect the change for parameter order will include some enforcement to catch cases where with arguments may be used for the last parameter.

This reverts commit d4ae37e.

AlekseyTs · 2025-03-29T00:54:15Z

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

@@ -6666,6 +6664,10 @@ private BoundExpression BindDictionaryItemAssignment(
            return new BoundCall(
                syntax,
                receiverOpt: implicitReceiver,
+                // initialBindingReceiverIsSubjectToCloning is used for ref safety analysis, KeyValuePair<,> is


// initialBindingReceiverIsSubjectToCloning is used for ref safety analysis, KeyValuePair<,> is

It looks like this comment belongs in the previous local function instead #Closed

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 27, 2025

Dictionary expressions: remove PROTOTYPE comments

781c167

cston force-pushed the dictionary-4 branch from 9c8b871 to 781c167 Compare March 27, 2025 19:14

cston added 4 commits March 27, 2025 12:28

Add to AllInOneCSharpCode

4853b74

await

5319175

Add to AllInOneCSharpCode

e401f27

Test ref safety

b9f8c82

cston commented Mar 27, 2025

View reviewed changes

cston marked this pull request as ready for review March 27, 2025 23:54

cston requested review from a team as code owners March 27, 2025 23:54

jcouv approved these changes Mar 28, 2025

View reviewed changes

jcouv self-assigned this Mar 28, 2025

jcouv added the Feature - Dictionary Expressions label Mar 28, 2025

cston requested a review from a team March 28, 2025 14:01

RikkiGibson self-assigned this Mar 28, 2025

RikkiGibson approved these changes Mar 28, 2025

View reviewed changes

cston added 4 commits March 28, 2025 15:10

More tests

c837913

Use ReceiverIsSubjectToCloning()

d4ae37e

Revert "Use ReceiverIsSubjectToCloning()"

7ce5d0a

This reverts commit d4ae37e.

Add comment for initialBindingReceiverIsSubjectToCloning

bb4f605

AlekseyTs reviewed Mar 29, 2025

View reviewed changes

Move comment

dd124ae

cston merged commit 9705bdf into dotnet:features/dictionary-expressions Mar 31, 2025
28 checks passed

cston deleted the dictionary-4 branch March 31, 2025 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dictionary expressions: remove PROTOTYPE comments #77876

Dictionary expressions: remove PROTOTYPE comments #77876

cston commented Mar 27, 2025

cston Mar 27, 2025 •

edited

Loading

RikkiGibson Mar 28, 2025

cston Mar 27, 2025

cston Mar 27, 2025 •

edited

Loading

RikkiGibson Mar 28, 2025

cston Mar 28, 2025

AlekseyTs Mar 29, 2025 •

edited

Loading

cston Mar 29, 2025

cston Mar 27, 2025

cston Mar 27, 2025

jcouv left a comment

RikkiGibson Mar 28, 2025

RikkiGibson Mar 28, 2025

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

cston Mar 28, 2025

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

cston Mar 28, 2025

AlekseyTs Mar 29, 2025 •

edited

Loading

Dictionary expressions: remove PROTOTYPE comments #77876

Dictionary expressions: remove PROTOTYPE comments #77876

Conversation

cston commented Mar 27, 2025

cston Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2025 • 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

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 28, 2025 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 28, 2025 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Mar 28, 2025 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

cston Mar 27, 2025 •

edited

Loading

cston Mar 27, 2025 •

edited

Loading

AlekseyTs Mar 29, 2025 •

edited

Loading

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

RikkiGibson Mar 28, 2025 •

edited by cston

Loading

AlekseyTs Mar 29, 2025 •

edited

Loading