Fix issue preventing going to def on a complicated metadata symbol involving nullability and generics #45594
Conversation
…volving nullability and generics
| var method = type.GetMembers("GetValue").OfType<IMethodSymbol>().Single(); | ||
| var callbackParamater = method.Parameters[1]; | ||
| var parameterType = callbackParamater.Type; | ||
| Assert.Equal("global::ConditionalWeakTableTest<TKey!, TValue!>.CreateValueCallback!", parameterType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat.WithMiscellaneousOptions(SymbolDisplayMiscellaneousOptions.IncludeNotNullableReferenceTypeModifier))); |
There was a problem hiding this comment.
this line is to validate that we're getting appropriately annotated generic types. in this case, that tehse values are expected to be not-null.
| @@ -344,8 +344,7 @@ private bool HandleNamedTypesWorker(INamedTypeSymbol x, INamedTypeSymbol y, Dict | |||
| { | |||
| Debug.Assert(GetTypeKind(x) == GetTypeKind(y)); | |||
|
|
|||
| if (x.IsDefinition != y.IsDefinition || | |||
| IsConstructedFromSelf(x) != IsConstructedFromSelf(y) || | |||
| if (IsConstructedFromSelf(x) != IsConstructedFromSelf(y) || | |||
There was a problem hiding this comment.
this is the actual change. the original check included something unnecessary (the IsDefinition check). This was unnecessary, but not problematic in teh past. however, with nullable this became an issue as List<T!> woud be not equal to List<T> due to the former having IsDefinition=false. With symbol equivalence we don't want to consider these different.
There was a problem hiding this comment.
Is IsDefinition supposed to be different in those cases? It's unclear from the documentation on the property
/// <summary>
/// Gets a value indicating whether the symbol is the original definition. Returns false
/// if the symbol is derived from another symbol, by type substitution for instance.
/// </summary>
There was a problem hiding this comment.
So I believe we decided that OriginalDefinition would always take you back to an unannotated type; for example if you have string? and string! then you want some way to get back to just "string".
There was a problem hiding this comment.
@CyrusNajmabadi I can never remember the precise definition of "equal" this class generally is trying to use. It's insensitive to nullability because for "is this symbol that symbol" we want it to work cross project/cross language?
It's by design. The 'definition' only considers the original, totally unmutated symbol (i.e. Effectively, from talking to compiler team, this is the expectation for that API, and it was a bug on our end to depend on this in this manner. |
| var symbolKey = SymbolKey.Create(method); | ||
| var resolved = symbolKey.Resolve(compilation).Symbol; | ||
|
|
||
| Assert.Equal(method, resolved); |
There was a problem hiding this comment.
This is testing symbol key which is good, but do we have a test covering the SymbolEquivalenceComparer for this same scenario?
There was a problem hiding this comment.
The symbolkey operates under the covers by using SymbolEquivalenceComparer.
There was a problem hiding this comment.
Chatted with @CyrusNajmabadi offline, didn't realize this still gets used when comparing the parameter symbols during our attempt to find the right overload.
|
Thanks! |
ghost
added this to the 
Fixes #45437.