MethodSymbol and FieldSymbol equality fixes:#40939
MethodSymbol and FieldSymbol equality fixes:#40939chsienki wants to merge 3 commits intodotnet:masterfrom
Conversation
- Seal the equals method for field and method symbols, and define the explicitly allowed comparisons - Add a generic hashcode implementaiton for method and field symbols - Replace overriden equals methods with a type specific equals
| expectedIncludeNullability: true | ||
| ); | ||
|
|
||
| var method1ParamType = method1.Parameters.First().Type; // A<T!> |
There was a problem hiding this comment.
Please also test the ParameterSymbols directly.
There was a problem hiding this comment.
Can you think of a way of getting different parameter symbols, such that one is A and the other A<T!>? I tried to come up with a way, but couldn't.
Do you want me to just break it out and test as is even if we cant?
In reply to: 366075635 [](ancestors = 366075635)
| ); | ||
| } | ||
|
|
||
| private void VerifyEquality(ISymbol symbol1, ISymbol symbol2, bool expectedDefault, bool expectedIncludeNullability) |
There was a problem hiding this comment.
expectedDefault seems to never be false. Consider either adding negative tests and adjusting hashcode verification accordingly, or removing the parameter.
| return (object)other != null && TypeSymbol.Equals(_containingType, other._containingType, compareKind) && OriginalDefinition == other.OriginalDefinition; | ||
| } | ||
|
|
||
| public override int GetHashCode() |
There was a problem hiding this comment.
Why is GetHashCode getting removed here?
| return (object)other != null && _reducedFrom.Equals(other._reducedFrom, compareKind); | ||
| } | ||
|
|
||
| public override int GetHashCode() |
There was a problem hiding this comment.
Why is GetHashCode getting removed here?
| var localFunction = symbol as LocalFunctionSymbol; | ||
| return (object)localFunction != null | ||
| && localFunction._syntax == _syntax; | ||
| return localFunction._syntax == _syntax; |
There was a problem hiding this comment.
This doesn't seem to take into account the CompareKind, and seems like it will be wrong in the face of substituted nullability. Please add tests to verify that this works as expected.
| return (object)other != null && | ||
| _tupleElementIndex == other._tupleElementIndex && | ||
| TypeSymbol.Equals(ContainingType, other.ContainingType, compareKind); | ||
| RoslynDebug.Assert(other is object); |
There was a problem hiding this comment.
Please either add these assertions to the other modified Equals methods or remove from this one.
|
Done review pass (commit 2) |
|
Oh, sorry @chsienki. The github extension for vscode didn't show the |
With nullable enabled, it's now valid to compare some method and field symbols across their respective hierarchies.
This PR makes the equals for method and field symbols sealed, and instead contains an explicit list of valid sub-type comparisons. This also requires implementing hashcode for the base method and field symbol. This will potentially have a small perf hit, but should be functionally correct.