Updating usages of IntPtr/UIntPtr to be the language keyword nint/nuint now that they are equivalent

[tannergooding]

This is mostly just a simple find/replace. The cases where is isn't is IntPtr.Zero were converted to just use 0, cases of IntPtr.Size were converted to just use sizeof(nint), and cases of new IntPtr(...) and x.ToInt32() were changed to use (nint)value and (int)value respectively. The latter had checked inserted where it could have potentially overflowed, but most of these usages were simply handling void* or other cases where overflow wasn't possible.

This also notably only touches S.P.Corelib right now, as that one is simple and known to not impact anything other than .NET 7

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is mostly just a simple find/replace. The cases where is isn't is IntPtr.Zero were converted to just use 0, cases of IntPtr.Size were converted to just use sizeof(nint), and cases of new IntPtr(...) and x.ToInt32() were changed to use (nint)value and (int)value respectively. The latter had checked inserted where it could have potentially overflowed, but most of these usages were simply handling void* or other cases where overflow wasn't possible.

Author:	tannergooding
Assignees:	tannergooding
Labels:	area-Meta
Milestone:	-

[jkotas]

This and many other places are pointers. nint makes them look odd. We should be changing these to void* instead.

[tannergooding]

I'm happy to do that, but just noting its going to make this a much more in depth change.

Also noting that this isn't critical for .NET 7, its mostly "stylistic" cleanup outside the places where IntPtr.Zero and similar were swapped to something more efficient.

[jkotas]

I'm happy to do that, but just noting its going to make this a much more in depth change.

That's sounds fine to me. It can be done in chunks to make it reviewable.

I would switch to void* or other appropriate pointer type where possible, and keep using IntPtr for pointers where it is not possible to switch. Switch to nint/nuint only in places where the value is not pointer.

[tannergooding]

and keep using IntPtr for pointers where it is not possible to switch

This is going to conflict, longer term, with editorconfig options and what IntelliSense/docs display. As of C# 11 (when targeting .NET 7 or later), IntPtr/nint and UIntPtr/nuint are true aliases of eachother, in the same way as Int32/int and UInt32/uint. They are identical in IL (not even a NativeInteger attribute any longer) and all the same style rules and support will exist/apply.

[eerhardt]

In my opinion it is about intent. Yes, in IL they are identical aliases for each other. But the important thing here is for the code reader, not the compiler or tools.

What is the intent of this variable? Is it "just a number" like a size or a counter? Or is it a pointer?

I find it super confusing to use nint for "pointer". As a reader, you need to inspect further to figure out what the intention of the variable is than just looking at its type.

[tannergooding]

C# does not have one universally accepted style.

Yes. But IntelliSense does and IntelliSense is going to display nint everywhere, just as it displays int everywhere.

IntelliSense "could" respect https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0049, but that would be IntPtr and Int32 everywhere with nint and int nowhere.

Nothing says that we have to enable style rule that says use nint/uint everywhere.

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md.

11. We use language keywords instead of BCL types (e.g. int, string, float instead of Int32, String, Single, etc) for both type references as well as method calls (e.g. int.Parse instead of Int32.Parse). See issue Language or BCL data types #13976 for examples.

Now that IntPtr and nint are true aliases of eachother, nint is a keyword and is per our rules and the general styling/editorconfig rules, preferred.

---

Users are going to have to update their incorrect expectations here and change. This is and has always been the case for many languages outside of C#.

We can improve some things on our own end by utilizing pointers instead, where feasible. But trying to continue perpetuating the leaky abstraction that IntPtr is somehow different from nint is flawed and not going to end up in a positive area.

This was already discussed in depth by API review and C# LDM with consideration for how other languages do things, how APIs are exposed, and what our official guidance for exposing such APIs is.

[tannergooding]

Whatever precedence we set in our own code is one that, for better or worse, is going to be used as a representative example for other codebases.

The handful of APIs that fall into the weird category of "this should have been void* but the API review committee at the time had an aversion to unsafe code" will be exposed to the end user as nint and that is how everyone is going to see that API in VS, regardless of what we use in source.

Because of that, just being consistent with our own existing rules and guidelines should be fine and will help ensure that source link vs decompilation match up.

[jkotas]

Whatever precedence we set in our own code is one that, for better or worse, is going to be used as a representative example for other codebases.

Agree. I believe that blank find&replace of IntPtr with nint is a bad precedence to set. We should set the precedence by converting IntPtr pointer values to use proper unmanaged pointer values where possible.

As the language evolves, the coding style has to evolve as well. I am proposing to add a new rule to the repo style rule:

We use unmanaged pointers (e.g. void*, byte*) for pointer values where possible. IntPtr/nint pointer values used by legacy APIs should be converted to and from unmanaged pointers at once (e.g. void* p = (void*)Marshal.AllocHGlobal(...); Marshal.FreeGlobal((IntPtr)p);).

[tannergooding]

Right, I agree with that. The remaining bit is whether AllocHGlobal is declared in source as IntPtr AllocHGlobal(nint cb) or as nint AllocHGlobal(nint cb).

IntelliSense, tooling, and other bits are all going to show and default to the latter for .NET 7+ and so I think we should be consistent there. The proposed rule you gave should be that we convert the returned nint to void* at all usage sites, since we can't actually change the API signature to return void*.

It might be beneficial to have an analyzer to help flag these edge cases, but that's not a strict need.

[jkotas]

The remaining bit is whether AllocHGlobal is declared in source as IntPtr AllocHGlobal(nint cb) or as nint AllocHGlobal(nint cb).

I do not have a strong opinion on that. It is going to need a comment that explains that it is actually a pointer. It can be also declared as nint /* IntPtr */ AllocHGlobal(nint cb) or nint /* void* */ AllocHGlobal(nint cb) to get the point across.

[tannergooding]

Going through and finding all the places where IntPtr can/should be void* is going to take a significant effort so I'm going to close this for now and will re-open if/when I have enough time to do that.