Fixing crash with ValueTuple alias when VT is missing (#12032), more tests.

[jcouv]

Fixes #12032 (crash when looking up static members and VT alias is aliased by missing)

Resolves #11302 (no repro, no crash)
Resolves #12082 (no repro, conversion to tuple of dynamic elements works)
Opened #12468 (allow invoking a ref-return var method with workaround)
Adds some tests

@dotnet/roslyn-compiler for review for preview 4.

[VSadov]

LGTM

[jcouv]

Thanks Vlad.
@dotnet/roslyn-compiler for a second review of this preview 4 bug fix and extra tests.

[khyperia]

Are we sure that this is the right place to put the check if it's an error type? Seems a bit weird that if an error type happens to have the name System.ValueTuple with an arity >0, .IsTupleCompatible() returns true.

If that's the intended behavior, sure, but I feel like it's a bit odd that error symbols are sometimes considered to be tuple-compatible. (Not sure what the exact crash was, but there might be other places that depend on the type not being an error when that method returns true)

[jcouv]

@khyperia
Let me first describe the crash: you define an alias for ValueTuple, but don't reference the ValueTuple library. The LookupStaticMember semantic model API will attempt to list static members on this alias (for instance when you invoke completion in your code). What it got (prior to the fix) was a TupleTypeSymbol (which is a NamedTypeSymbol) which wraps an ErrorType.
For error types, the lookup is cut short. But for NamedTypeSymbol (which appear valid), there is inspection of the containing assembly, which is improper in this case (we didn't find ValueTuple in any specific assembly).

So in the case when the ValueTuple type doesn't exist, I think we should not wrap it into a seemingly valid NamedTypeSymbol as we normally do.
You raise a good question: should this check only be during wrapping, or should it be upstream (when considering if the type looks like a tuple-compatible type)?
Looking at the code paths that check for tuple-compatibility, there may not be a big difference (the primary purpose of checking for tuple-compatibility is to wrap). But I think we still want know that the type appears tuple-compatible.

I'll take a note to run that by Aleksey when he is back as well.

[agocke]

Should we check if the target is an error type first? Checking for tuple compatibility is slightly more expensive, I think.

[agocke]

Alternatively, should we just fold this into IsTupleCompatible? I can see a solid justification that ErrorTypes are not tuple compatible.

[jcouv]

Error checking first makes total sense.
For folding into IsTupleCompatible that is also the question Evan raised. Let me think and discuss further.

[jcouv]

I gave the second suggestion a try, and as I feared it had a lot more implications. So I'll just take the first suggestion (error checking first).
Thanks!

[agocke]

Aside from comment, LGTM

[jcouv]

@MattGertz Could you approve for preview 4? This fixes a crash.

[MattGertz]

Approved pending successful tests.