Collection expressions: analyze nullability of element assignments

[jcouv]

Addresses parts of #68786

The parts that are not addressed in this PR:

• method type re-inference with nullability (added some scenarios to illustrate)
• spreads

Relates to test plan #66418

[RikkiGibson]

Is there anything to do besides visit the spread, and warn if the result may be null? (since we want to enumerate the operand value, it should be non-null.) #Resolved

[RikkiGibson]

I think we may want VisitRvalue here because if a spread puts us into a split state after visiting (like in an error scenario) that could mess things up later on.

[cston]

FWIW, with #70197, NullableWalker.VisitCollectionExpressionSpreadElement() relies on base.VisitCollectionExpressionSpreadElement(node) which simply calls VisitRvalue(node.Expression).

[RikkiGibson]

I'd be comfortable with this assuming we have a test demonstrating that things don't fall over when an expression inside a spread splits the state. e.g. [..(x is not null)]

[jcouv]

Spreads are not in scope for this PR, so I don't want to go too far. I'd be okay with removing this entirely if you prefer.

As things stand, ..(x is null) hits an assertion: System.InvalidOperationException : Analyzed 11 nodes in NullableWalker, but DebugVerifier expects 10. Example of unverified node: BoundCollectionExpressionSpreadElement ..(x is null)

In release mode, the test produces the expected diagnsotic (binding error). Added a test

[jcouv]

Note: a similar assertion is hit if I remove this case, even on simple cases. I still think it's beneficial to keep it in the interim (before we have fully coverage on spreads)

System.InvalidOperationException : Analyzed 33 nodes in NullableWalker, but DebugVerifier expects 27. Example of unverified node: BoundCollectionExpressionSpreadElement ..a

[RikkiGibson]

As long as the compiler doesn't blow up in the retail build in these cases, I'm happy. Since it looks like we are actually visiting the spread operand, I think we are just missing the top-level nullable warning for when the operand may be null. That's OK to address later. Thanks!

[jcouv]

Chuck pointed out the assertion problem can be solved by undoing the s_skippedExpressions change. Done

Want to wait until the suggested Visit->VisitRValue change is investigated

[RikkiGibson]

Analyzers leg failed

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs(18732,1): error IDE0055: (NETCORE_ENGINEERING_TELEMETRY=Build) Fix formatting (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0055)

[RikkiGibson]

Formatting warning is being reported here. I honestly don't know what's wrong. Maybe a bug due to presence of the if-directive above? Might want to use compilation options to enable nullability for this test, or use attributes to only run the test in release mode.

I think @Youssef1313 may have reported something like this recently. #Resolved

[Youssef1313]

@RikkiGibson Yup, that's the same root cause as dotnet/csharplang#7520.

For "Debug" where the #if RELEASE is false, the compiler will parse everything in the #if RELEASE block as disabled trivia, and it will parse #nullable enable as NullableDirectiveTrivia even if the human thinking is that it's part of a string literal and not a nullable directive trivia.

[jjonescz]

Consider using [ConditionalFact(typeof(IsRelease))] instead. #Resolved

[jcouv]

Yup. Done. Thanks