Bind native integers in cref

[jcouv]

Test plan #60578

[Neme12]

"A platform-specific type that is used to represent a pointer or a handle" I think they should update that description 😄

[Neme12]

I'm just curious, why does this have to be specialized here and cannot go through the same code path that handles other type keywords like int? 🤔 #Closed

[jcouv]

nint could bind to a type or alias. It's just an identifier, not a keyword.
We had to special case this in BindNonGenericSimpleNamespaceOrTypeOrAliasSymbol as well.

[Neme12]

Oh, I see, back compat 😔

[cston]

!IsViableType(result)

Are we testing members other than types? For instance:

/// <see cref="nint"/>
class C { int @nint; }
``` #Closed

[cston]

symbol.IsNativeIntegerType

If we're not binding to the field in this case, when would we expect cref="nint" to bind to a member? What if the member is class C { public class nint { } }? #Closed

[cston]

In short, what cases does the !IsViableType(result) check allow?

[jcouv]

See XmlDoc_Cref_Alias, XmlDoc_Cref_NamedType and XmlDoc_Cref_TypeParameter for IsViableType(result) being true.
See XmlDoc_Cref_Member for IsViableType(result) being false.

The IsViableType check is the same we use in BindNonGenericSimpleNamespaceOrTypeOrAliasSymbol for binding "nint".

[cston]

Thanks. Given that cref="nint" binds to a peer type or a type parameter, I expected it to also bind to a field.

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[jcouv]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[jcouv]

@333fred @dotnet/roslyn-compiler for second review. Thanks

[333fred]

This appears to be a breaking change, as this code will currently reference the nint field. Is this what you expected, and if so, do we need to document it? #Resolved

[jcouv]

Yes, that's an expected change.
Since this is narrow (requires something named nint and within xmldoc as opposed to code), this is more in the bug fix category and I think this doesn't warrant additional documentation (aside from the issue already filed for the record).

[Neme12]

My 2 cents: I find it understandable and reasonable to make a breaking change like this, especially since it only relates to xmldoc. But I find it odd that this will still work for types named nint (no breaking change there), but not for fields named nint. It's much more likely that someone will have a field named nint than having a type named nint, because having lowercase types is much more rare than lowercase fields, where camel case is the standard naming convention. If it's OK to break lowercase fields, then I'd say that it's definitely OK to break lowercase types, because the latter is a lot less likely to occur in real code than the former.

[Neme12]

Also, with this break, I would expect to be able to add @ and make it @nint to make it refer to the field (just like it's possible with all predefined keyword types). Does that work? If so, is there a test for that? If not, I think a test should be added.

[333fred]

Done review pass (commit 2)

[Neme12]

I think a test should be added where this is <see cref="@nint"/> and make sure that it does refer to the field in that case. Also, I would probably remove the @ from the field in that case just to make sure that it isn't just plain string matching. #Closed

[jcouv]

That was a good scenario. Fixed a bug. Thanks!

[Neme12]

This should also likely be memberNameText? A test for nuint that would catch this would be useful as well. #Resolved

[Neme12]

It would be a good idea to make this a Theory and test nuint as well and verify that it does indeed correspond to
IntPtr vs UIntPtr. Right now it seems like this won't work because of an omission in the code. #Resolved

[jcouv]

From discussion with @333fred and @cston, we don't want to apply the IsViableType filtering in cref scenario because we could be binding to things that aren't type (unlike in BindType).