Fix nullable type pattern parsing for better error recovery

[alrz]

Fixes #48112

[CyrusNajmabadi]

can we have some explicit tests of foo is int ? x : y to ensure these don't regress. thanks!

[alrz]

IsNullableType01 and IsNullableType02 already do that for o is A ? b : c and o is A? ? b : c

Is there any other scenario worth adding test?

[alrz]

Note this doesn't affect is. Actually T? is allowed in is for historical reasons. Semantically it's identical to T, but in patterns we have an explicit diagnostic on Nullable<T>. This fix reports the same error on T? syntax.

[CyrusNajmabadi]

@gafter would be a good person to weigh in on where we should focus tests. Even if this isn't directly impacted, those tests will help prevent regressions as this code gets refactored in the future.

[333fred]

I'm actually not sure the compiler as it is today complies with the spec here. switch expression patterns can be a constant_pattern, and constant_pattern is just a constant_expression. That means we should accept code of this form: o switch { S ? true : false => false } (when S is a constant), but we do not today. Do we have tests for similar scenarios in switch statements (which do accept this code)? For example, this code:

    switch (o)
    {
        case S ? true : false: // Totally legal if S is constant
            break;
        case int?: // Error: ':' expected
            break;
    }

While I agree with this in principle, we definitely need to have a lot of tests.

[333fred]

To expand on my concern: The first thing that we'd need to do to correctly implement the spec is revert this change. I think that if we wanted to limit the error recovery to just scenarios that can result from a bad pattern, such as int? =>, I would be more comfortable with that. So, if the character after the ? is a =, it seems reasonable to recover there. It also seems like it would be reasonable to recover after int?: in a case label: today, the error you get for case int?: is : expected, which is just confusing.

[alrz]

📝 The cases you mentioned happen only in the top level patterns - if we're going to look only for those two characters to disambiguate, we need to make sure it won't break when these appear deep nested in recursive patterns.

btw, is this an outstanding issue or was there an actual reason or parsing difficulties that we didn't cover these the first time?

[333fred]

btw, is this an outstanding issue or was there an actual reason or parsing difficulties that we didn't cover these the first time?

I suspect it's just a bug.

[alrz]

One last note. If we fix the conditional parsing, we're practically breaking those using an older compiler to compile a program written in a newer version but both with the same langversion. I guess it should be fine considering this is just an edge case unlikely to actually happen.

[333fred]

Yeah, I wouldn't be worried about that. It's a pretty edge of edge case.

[alrz]

@333fred turns out it was actually by-design.

roslyn/src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

Lines 645 to 649 in a09a5a2

	// We use a precedence that excludes lambdas, assignments, and a conditional which could have a
	// lambda on the right, because we need the parser to leave the EqualsGreaterThanToken
	// to be consumed by the switch arm. The strange side-effect of that is that the conditional
	// expression is not permitted as a constant expression here; it would have to be parenthesized.
	var pattern = ParsePattern(Precedence.Coalescing, whenIsKeyword: true);

[333fred]

Interesting. That does still seem like a bug though... the spec makes no mention of the constant_expr having different rules than any other constant_expr here.

[CyrusNajmabadi]

I haven't quite paid attention to all the discussion. I just want to check on the following:

1. things that are legal as per hte language remain legal (no errors at parse time, or binding time).
2. things that are illegal per hte language remain illegal. however, errors can be pushed to whatever phase we prefer.
3. if something is illegal, we can certainly improve error recovery, as long as we ensure an error is still reported at some phase.

Importantly: it is not at all necessary (and is often undesirable) for the parsing phase to produce an error. In particular, if the user text can cleanly fit into the syntactic model, it's often nicer to just fit htat in (with no 'missing token' or 'skipped token' diagnostics), and report the problem later.

So, if that's what this PR is generally doing, then i'm good with this. Happy to take a deeper look later. However, as i don't actually know the raw rules of hte language here, i'm not totally comfortable with validating if this is implementing those rules properly.

[alrz]

That does still seem like a bug though... the spec makes no mention of the constant_expr having different rules than any other constant_expr here.

How would you propose we disambiguate? i.e. stop parsing a constant_expr further when we get to =>.

The current impl used a specific precedence to leave that out.

[333fred]

We certainly could attempt to do that, but it's of course a pretty bad case. I think I'd be happy with the simpler, narrowly-scoped change of only doing this recovery when it's unambiguous. I think there are likely scenarios involving constant expressions here that are truly syntactically undecidable.

[alrz]

I think I'd be happy with the simpler, narrowly-scoped change of only doing this recovery when it's unambiguous.

Well it's always ambiguous? The simplest case a?b:c=>d is parsed as a?b:(c=>d). The precedence here stop that from happening.

[333fred]

@dotnet/roslyn-compiler for a second review

[gafter]

😷

[333fred]

@dotnet/roslyn-compiler for a second review

Thanks for the look Neal!

[333fred]

Thanks @alrz!