apicompat: treat IntPtr as nint and UIntPtr as nuint

[smasher164]

Previously, ApiCompat would consider IntPtr and nint (as well as UIntPtr and nuint) as different types, emitting diagnostics where there shouldn't have been. This change updates the display string used by the SymbolComparer to replace all instances of IntPtr with nint and UIntPtr with nuint.

Fixes #25566

[ericstj]

Will this create confusing diagnostics when someone is actually using IntPtr? Say I had an API that used IntPtr and I removed it on the right, what diagnostic do we emit?

[ViktorHofer]

This code path doesn't impact the Target that is being emitted by rules and APICompat. The ToComparisonDisplayString helper function is only used by the mapping infrastructure to map members together.

I.e.

sdk/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/AttributesMustMatch.cs

Line 73 in 0975222

$"{itemRef}:[{docId}]"),

doesn't use that function to for encoding the target.

[smasher164]

This string is only used for comparing symbols, so the diagnostic that is emitted in that case is still "CP0002 : Member 'CompatTests.First.H(System.IntPtr)' exists on left but not on right".

[ericstj]

If we do end up having to normalize this in comparisons, it might be better to do here:

sdk/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/DefaultSymbolsEqualityComparer.cs

Line 14 in 0975222

string.Equals(x != null ? GetKey(x) : null, y != null ? GetKey(y) : null, StringComparison.OrdinalIgnoreCase);

And only after the equality check fails, and only if references are not provided. I'd like us to first see if we can get roslyn to behave as if the feature is supported.

[ViktorHofer]

IMO the ToComparisonDisplayString method is the right place as the name already suggests that it calculates a display string for comparison.

[smasher164]

Testing this on .NET 472 and .NET 7,

if (RuntimeFeature.IsSupported("NumericIntPtr"))
{
    return symbol.ToDisplayString(Format);
}
return symbol.ToDisplayString(Format)
    .Replace("nint", "System.IntPtr") // Treat nint and IntPtr as the same
    .Replace("nuint", "System.UIntPtr"); // Treat nuint and UIntPtr as the same

tests fail on .NET 7. It seems like even when NumericIntPtr is defined, Roslyn returns different display strings.

[bartonjs]

Off the top of my head the two concerning places are nested types (which probably here use a + instead of .) and explicit interface implementations (e.g. the full method name of IEnumerable.GetEnumerator() => GetEnumerator(); I believe is System.IEnumerable.GetEnumerator. So while "nint" => "System.IntPtr" probably wouldn't create any false positives, '.' probably isn't a guaranteed "eh, it's fine".

[ViktorHofer]

Agreed that nint is fairly new but it's not unlikely that nint and nuint word parts are used in a member's name. I would expect that replacing "nint" with "System.IntPtr" would result in way more false positives than the other way around. That said, these false positives shouldn't have any negative impact as far as I can tell.

[ericstj]

if (RuntimeFeature.IsSupported("NumericIntPtr"))

This is mixing tooling runtime with target runtime. The check would need to be against whatever the target references that were used when compiling the assembly. Calling the feature RuntimeFeature.IsSupported in the tool runtime has no relevance since we might be using an older runtime to test assemblies built for a newer one.

The way I was imagining this to work was we would add some parameter to DefaultSymbolsEqualityComparer like bool honorRuntimeFeatures and set that as withReferences == true. Then we could say that when it is false we'd do the hack-up in DefaultSymbolsEqualityComparer.GetKey to normalize those symbols.

it's not unlikely that nint and nuint word parts are used in a member's name

That's true. I see we're doing arbitrary replacements here wherever they occur in the string. I'll relent on my suggestion to change the direction of replacement.

[ericstj]

Hmm, the premise of the withReferences suggestion was based on the fact that we only face this problem when we don't have the matching references. However, we probably are in this state even when references are provided, since we don't have references for the left-hand-side and that might be the runtime that doesn't support the feature :(

I was really trying to get this to a place where we didn't need this hack indefinitely but I guess that might not be possible at the moment.

[ViktorHofer]

Looks good. Thanks for adding a test for it.

[ericstj]

Approved this as-is. I think given our current architecture (references present only for right hand side) and the only source for this feature information in the compiler coming from references - we'll always be in a state where we need to normalize these types of metadata differences ourselves. That's unfortunate but I can't think of a better solution.