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

Collection expression argument parsing ([with(x, y, z), a, b, c]). #76003

Merged

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Nov 21, 2024

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-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 21, 2024
Copy link
Contributor

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.

@dotnet-policy-service dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Nov 21, 2024
var element = syntax.Elements[i];
if (element is WithElementSyntax withElement)
{
Error(diagnostics, ErrorCode.ERR_WithElementMustBeFirst, withElement.WithKeyword.GetLocation());
Copy link
Member

@cston cston Jan 23, 2025

Choose a reason for hiding this comment

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

ERR_WithElementMustBeFirst

Are we testing this? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

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

@cston cston Jan 23, 2025

Choose a reason for hiding this comment

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

IDS_FeatureDictionaryExpressions.CheckFeatureAvailability

This is checked in BindKeyValuePair(). #Closed

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

Choose a reason for hiding this comment

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

IDS_FeatureCollectionExpressionConstructionArguments

Are we testing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


// 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());
Copy link
Member

@cston cston Jan 23, 2025

Choose a reason for hiding this comment

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

return new BoundBadExpression

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

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 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.

@@ -295,6 +295,7 @@ internal enum MessageID

IDS_FeatureFirstClassSpan = MessageBase + 12849,
IDS_FeatureDictionaryExpressions = MessageBase + 12850,
IDS_FeatureCollectionExpressionConstructionArguments = MessageBase + 12851,
Copy link
Member

@cston cston Jan 23, 2025

Choose a reason for hiding this comment

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

Consider shortening, perhaps to IDS_FeatureCollectionExpressionArguments. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

LGTM.

Please squash commits when merging, thanks.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) January 23, 2025 21:23
return (WithElementSyntax)Syntax.InternalSyntax.SyntaxFactory.WithElement((Syntax.InternalSyntax.SyntaxToken)withKeyword.Node!, (Syntax.InternalSyntax.ArgumentListSyntax)argumentList.Green).CreateRed();
}

#pragma warning disable RS0027
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

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

@jcouv jcouv Jan 23, 2025

Choose a reason for hiding this comment

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

Why are we breaking out of the loop? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -1688,8 +1688,8 @@ private void WriteRedMinimalFactory(Node nd, bool withStringNames = false)

var hasOptional = minimalFactoryfields.Any(f => !IsRequiredFactoryField(nd, f));
var hasAttributeOrModifiersList = nd.Fields.Any(f => IsAttributeOrModifiersList(f));

if (hasOptional && hasAttributeOrModifiersList)
var writePragma = hasOptional && (hasAttributeOrModifiersList || nd.Name == "WithElementSyntax");
Copy link
Member

@jcouv jcouv Jan 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

correct.

[Fact]
public void WithElement2()
{
UsingExpression("[with()]");
Copy link
Member

@jcouv jcouv Jan 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 .

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 is part of hte final syntax/sign-off discussion there.

Copy link
Member

Choose a reason for hiding this comment

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

Should it be made LangVer-dependent?

This is currently an open question.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

open LDM question.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Done.

Copy link
Member

@jcouv jcouv Jan 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) January 23, 2025 23:38
@CyrusNajmabadi CyrusNajmabadi merged commit f806fd4 into dotnet:features/dictionary-expressions Jan 24, 2025
25 of 28 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the withElement branch January 24, 2025 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Dictionary Expressions Needs API Review Needs to be reviewed by the API review council 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