Fixing crash with ValueTuple alias when VT is missing (#12032), more tests.#12534
Conversation
… adding more tests.
|
LGTM |
|
Thanks Vlad. |
|
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, 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) |
|
@khyperia So in the case when the I'll take a note to run that by Aleksey when he is back as well. |
| public static TypeSymbol TransformToTupleIfCompatible(TypeSymbol target) | ||
| { | ||
| if (target.IsTupleCompatible()) | ||
| if (target.IsTupleCompatible() && !target.IsErrorType()) |
There was a problem hiding this comment.
Should we check if the target is an error type first? Checking for tuple compatibility is slightly more expensive, I think.
There was a problem hiding this comment.
Alternatively, should we just fold this into IsTupleCompatible? I can see a solid justification that ErrorTypes are not tuple compatible.
There was a problem hiding this comment.
Error checking first makes total sense.
For folding into IsTupleCompatible that is also the question Evan raised. Let me think and discuss further.
There was a problem hiding this comment.
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!
|
Aside from comment, LGTM |
|
@MattGertz Could you approve for preview 4? This fixes a crash. |
|
Approved pending successful tests. |
Fixes dotnet/razor#9829 Part of dotnet/razor#8541 When renaming a component in a Razor file we now call Roslyn so that the component name or namespace is renamed in all C# code in the solution. With these changes, once #81450 merges, we'll do the same when renaming a component type name from a C# file, including updating start and end component tags.
6 participants
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.