Make ModifierComparer use type references instead of definition #7557
Conversation
When comparing definitions this was failing if the type couldn't be resolved, for instance if a type-forwarded dependency was missing or references were specified at all. This was inconsistent with our other parameter comparisons which just evaluate the compare the type reference as a string without considering assembly. This does raise a concerning issue, that APICompat doesn't actually evaluate assembly equivalence of parameters but that's an existing problem.
safern
left a comment
There was a problem hiding this comment.
LGTM, thanks!
This does raise a concerning issue, that APICompat doesn't actually
evaluate assembly equivalence of parameters but that's an existing
problem.
For that we can discuss to add this kind of validation for the new API Compat.
dougbu
left a comment
There was a problem hiding this comment.
Looks like this is my bad because I added ModifierComparer in d6dd7a0#diff-ab264297f1e0d7296c8d69384a224e589d95ced59d203663d968dcc20f7d1e13R131
I'm slightly concerned this isn't the only instance of the problem because I did a lot of copying and pasting in #4585 Pretty sure I copied most of that class from somewhere else but have no idea where…
|
|
||
| return x.IsOptional == y.IsOptional && | ||
| TypeComparer.Equals(x.Modifier.ResolvedType, y.Modifier.ResolvedType); | ||
| TypeComparer.Equals(x.Modifier, y.Modifier); |
There was a problem hiding this comment.
Does this allow both ICustomModifiers to be unresolved but disallow one or the other (and not both) to be resolved❔
There was a problem hiding this comment.
It's just using the type-reference to do the comparison so it's irrelevant if either type is/can be resolved. This is consistent with how we already compare parameter types.
|
@dougbu you did a solid by fixing the original issue, this just tweaks your fix to not require TypeDefinitions to be equal. I think this is the only place you introduced that's doing TypeDefinition comparisons. I didn't see any other places where we were doing this (looking for uses of |
When comparing definitions this was failing if the type couldn't be resolved,
for instance if a type-forwarded dependency was missing or references
were specified at all.
This was inconsistent with our other parameter comparisons which just
evaluate the compare the type reference as a string without considering
assembly.
This does raise a concerning issue, that APICompat doesn't actually
evaluate assembly equivalence of parameters but that's an existing
problem.
Fixes #2102, #7511.
Related #2136
cc @utpilla