Refine parsing of potentially ambiguous casts/index expressions with collection literals #68456

CyrusNajmabadi · 2023-06-06T17:15:09Z

This changes parsing of (X)[1] back to the original C#1 interpretation of a parenthesized expression that is indexed into.

Importantly though, the parsing of expressions like (List<X>)[1, 2, 3] continues to parsed as a cast. This is because in the latter case, we know that the item in the parentheses must be a type, and so this could only be a cast. As collection literals are most commonly generic (or arrays) this means the common casts that people will perform will continue to be parsed as casts.

One area this would have an impact on is:

using IntList = List<int>;

// Parsed as an indexing expression, not a cast:
var v = (IntList)[1, 2, 3];

The workaround on the user side is to write:

var v = (IntList)([1, 2, 3]);

If we feel like this is a bitter pill (especially if lots of users alias complex collection instantiations) we could consider a breaking change in the language here to consider (X)[...] to be a cast now. This feels like this could be fairly safe to to as it seems highly unlikely in practice that someone would parenthesize a simple identifier only to index into it. In other words, we would expect people to be writing X[...] not (X)[...] in the real world.

…collection literals

CyrusNajmabadi · 2023-06-06T17:16:00Z

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

    [Fact]
    public void CastVersusIndexAmbiguity1()
    {
+        // As a non-generic expression, we assume that `(type)` is an expression, and we are indexing into it.


this preserves compat with existing code.

CyrusNajmabadi · 2023-06-06T17:16:27Z

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

+    [Fact]
+    public void CastVersusIndexAmbiguity12()
+    {
+        UsingExpression("(int[]?)[1, 2, 3]");


adding a bunch more interesting variants to ensure we know the behavior and we don't accidentally regress here.

CyrusNajmabadi · 2023-06-06T17:17:03Z

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

+    public void CastVersusIndexAmbiguity14()
+    {
+        // Parenthesizing RHS should make this a cast.
+        UsingExpression("(type)([1, 2, 3])");


explicit test to ensure that the user can take this route if they do happen to have a collection type that is non-generic (the likely case where thsi woudl arise would be aliases).

this may be worth bringing up to LDM to discuss this. we coudl consider parenthesizing a simple identifier and indexing into it something we shoudl parse as a cast of a collection literal. it seems unlikely that people would need to parenthesize the identifier in that case.

CyrusNajmabadi · 2023-06-06T17:18:48Z

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

+    [Fact]
+    public void CastVersusIndexAmbiguity15()
+    {
+        UsingExpression("(alias::type)[1, 2, 3]");


this is always a cast, as x::y can only refer to a type.

CyrusNajmabadi · 2023-06-06T17:19:24Z

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

+    [Fact]
+    public void CastVersusIndexAmbiguity20()
+    {
+        UsingExpression("(alias::type.member)[1, 2, 3]");


unlike x::y, x::y.z can now be referring to a member.

CyrusNajmabadi · 2023-06-06T17:20:13Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                // PROTOTYPE: We allow `(X)[...]` as a cast of a collection expression.  This could break existing
-                // code like (A.B.C)[0].
-                // case SyntaxKind.OpenBracketToken:
+                case SyntaxKind.OpenBracketToken:


restored this to the original logic before i changed it in the feature branch. always thinking of [ as starting a literal after a (...) is likely too heavy a hammer. But it might be good to bring to LDM.

instead, we now do a more refined check above where we only think of this as a cast if we're sure what is in the (...) is a type. As nearly all collection types will be arrays/generic, this will just work.

cston · 2023-06-06T19:32:18Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                        SyntaxKind.DotDotToken => true,
-                        var tk => CanFollowCast(tk)
+                        SyntaxKind.DotDotToken or
+                        SyntaxKind.OpenBracketToken // for `(int[])[1, 2, 3]` or `(List<int>?)[1, 2, 3]`


Does this change affect any tests?

it preserves existing tests passing.

I haven't found any parsing tests that depend on or SyntaxKind.OpenBracketToken. Please add a parsing test that depends on this if there is not one already.

CastVersusIndexAmbiguity8 validates this.

CyrusNajmabadi · 2023-06-06T20:25:34Z

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

            UsingExpression("(F) [A] () => { }", TestOptions.RegularPreview,
                // (1,1): error CS1073: Unexpected token '=>'
                // (F) [A] () => { }
                Diagnostic(ErrorCode.ERR_UnexpectedToken, "(F) [A] ()").WithArguments("=>").WithLocation(1, 1));


this restores the parsing behavior (in an error recovery case) here to the prior behavior we had here that i changed in the original parsing PR: https://github.com/dotnet/roslyn/pull/66417/files#diff-e77ec53b27dcea093de7ebc648dd17ff814931a0e44e001d5b6ab5d0a3403f0bR1255

CyrusNajmabadi · 2023-06-07T04:52:43Z

/azp run

azure-pipelines · 2023-06-07T04:52:58Z

Azure Pipelines successfully started running 2 pipeline(s).

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

CyrusNajmabadi · 2023-06-07T17:59:32Z

@RikkiGibson ptal.

cston · 2023-06-07T18:05:49Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                case ScanTypeFlags.TupleType:
+                    // If we have `(X<Y>)[...` then we know this must be a cast of a collection literal, not an index
+                    // into some expr. As most collections are generic this means a good experience most of the time in
+                    // this ambiguous scenario.


This sentence seems to indicate the generic case is ambiguous, even though the previous sentence implies it is not. Would it make sense to reword this second sentence? Perhaps: "As most collections are generic, the common case is not ambiguous."

cston · 2023-06-07T18:10:24Z

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

+    public void CastVersusIndexAmbiguity19()
+    {
+        // something that starts looking generic, but isn't.
+        UsingExpression("(a < b > c)[1, 2, 3]");


"(a < b > c)[1, 2, 3]"

Consider also testing (a < b > . c)[0]

RikkiGibson · 2023-06-07T18:17:03Z

The bitter pill to me is the arbitrary nature of it. (A)[1] is an element access but (A<int>)[1] is a cast of a collection literal. I would not enjoy explaining that to users. It's a compromise a bit like "one _ parameter is a usable parameter, two _ parameters is two discards".

I definitely think this is a situation where we should be looking at making a breaking change.

CyrusNajmabadi · 2023-06-07T18:18:58Z

@RikkiGibson would you like to drive this with LDM?

RikkiGibson · 2023-06-07T18:26:41Z

@RikkiGibson would you like to drive this with LDM?

Yes

RikkiGibson · 2023-06-07T18:27:14Z

I think auto-merge should be disabled until second review is complete.

…nto castVsIndexParsing

CyrusNajmabadi · 2023-06-08T21:13:04Z

@RikkiGibson Ptal :)

CyrusNajmabadi · 2023-06-12T17:10:37Z

@dotnet/roslyn-compiler @333fred for another pair of eyes plz.

RikkiGibson · 2023-06-12T18:44:34Z

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

+        UsingExpression("(A)[]",
+            // (1,5): error CS0443: Syntax error; value expected
+            // (A)[]
+            Diagnostic(ErrorCode.ERR_ValueExpected, "]").WithLocation(1, 5));


Please do leave a prototype comment here about the diagnostic experience. If LDM approves this design the diagnostics in error cases (either "element access lacks a value" or "lookup didn't find an indexer" errors) should hint that user might want to parenthesize the [] if they meant to use a collection literal.

will add in followup :)

Refine parsing of potentially ambiguous casts/index expressions with …

0108867

…collection literals

CyrusNajmabadi requested review from RikkiGibson and cston June 6, 2023 17:15

CyrusNajmabadi requested a review from a team as a code owner June 6, 2023 17:15

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 6, 2023

CyrusNajmabadi commented Jun 6, 2023

View reviewed changes

cston reviewed Jun 6, 2023

View reviewed changes

CyrusNajmabadi added 2 commits June 6, 2023 13:17

Update tests

a72bbec

Update tests

a5d4bfc

CyrusNajmabadi commented Jun 6, 2023

View reviewed changes

cston reviewed Jun 7, 2023

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/CollectionLiteralParsingTests.cs Show resolved Hide resolved

Add tests

de20ac2

CyrusNajmabadi requested a review from cston June 7, 2023 17:58

cston reviewed Jun 7, 2023

View reviewed changes

remove branch

8aa5855

tweak wording

6f6eaad

cston approved these changes Jun 7, 2023

View reviewed changes

CyrusNajmabadi enabled auto-merge (squash) June 7, 2023 18:25

RikkiGibson disabled auto-merge June 7, 2023 18:31

CyrusNajmabadi added 2 commits June 7, 2023 11:40

Add test

8823ffd

Merge remote-tracking branch 'upstream/features/CollectionLiterals' i…

dd6bc1e

…nto castVsIndexParsing

RikkiGibson reviewed Jun 12, 2023

View reviewed changes

RikkiGibson approved these changes Jun 12, 2023

View reviewed changes

CyrusNajmabadi merged commit 2955139 into dotnet:features/CollectionLiterals Jun 12, 2023

CyrusNajmabadi deleted the castVsIndexParsing branch June 12, 2023 18:47

Refine parsing of potentially ambiguous casts/index expressions with collection literals #68456

Refine parsing of potentially ambiguous casts/index expressions with collection literals #68456

Uh oh!

Conversation

CyrusNajmabadi commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CyrusNajmabadi Jun 6, 2023 • 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!

CyrusNajmabadi commented Jun 7, 2023

Uh oh!

azure-pipelines bot commented Jun 7, 2023

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 7, 2023

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!

RikkiGibson commented Jun 7, 2023

Uh oh!

CyrusNajmabadi commented Jun 7, 2023

Uh oh!

RikkiGibson commented Jun 7, 2023

Uh oh!

RikkiGibson commented Jun 7, 2023

Uh oh!

CyrusNajmabadi commented Jun 8, 2023

Uh oh!

CyrusNajmabadi commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi commented Jun 6, 2023 •

edited

Loading

CyrusNajmabadi Jun 6, 2023 •

edited

Loading