Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.IntPtr and System.UIntPtr Improvements #21943

Closed
tannergooding opened this issue May 24, 2017 · 37 comments
Closed

System.IntPtr and System.UIntPtr Improvements #21943

tannergooding opened this issue May 24, 2017 · 37 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Milestone

Comments

@tannergooding
Copy link
Member

Rationale

The .NET framework provides the System.IntPtr and System.UIntPtr types which wrap the native int IL primitive (as per ECMA-335).

These types are frequently used to represent both handles/pointers and integers whose size depend on the underlying platform.

When being used to represent the latter (platform sized integer) type, users may find a difficult time performing some basic operations, such as comparing two native ints to determine sort order.

Additionally, with the proposed C# Native Sized Number Types (dotnet/csharplang#435) where language support for the primitive operations will be exposed (currently this will be done via partial erasure), the use of System.IntPtr and System.UIntPtr as native int types will become more prevalent (F# is one language that already exposes these operators).

As such, these types should be modified to expose a set of APIs that make other common operations easier to perform.

Proposed API

The proposed API here only shows the members that would be new to the types.

System.IntPtr

[StructLayout(LayoutKind.Sequential)] // This is implicitly set by the compiler, but should be explicitly declared, as it is done for System.Int32
public struct IntPtr : IComparable, IComparable<IntPtr>, IFormattable
{
    public static IntPtr MaxValue { get; }
    public static IntPtr MinValue { get; }

    public static int ShiftMask { get; } // Runtime constant equivalent to (sizeof(nint) * 8) - 1

    int IComparable.CompareTo(object value);    // Explicitly implemented to ensure that users still get compile-time type checking
    public int CompareTo(IntPtr value);

    public bool Equals(IntPtr other);           // This is currently explicitly implemented as IEquatable<IntPtr>.Equals(IntPtr other)

    public string ToString(IFormatProvider provider);
    public string ToString(string format, IFormatProvider provider);

    public static IntPtr Parse(string s);
    public static IntPtr Parse(string s, NumberStyles style);
    public static IntPtr Parse(string s, IFormatProvider provider);
    public static IntPtr Parse(string s, NumberStyles style, IFormatProvider provider);

    public static bool TryParse(string s, out IntPtr result);
    public static bool TryParse(string s, NumberStyles style, IFormatProvider provider, out IntPtr result);
}

System.UIntPtr

public struct UIntPtr : IComparable, IComparable<UIntPtr>, IFormattable
{
    public static UIntPtr MaxValue { get; }
    public static UIntPtr MinValue { get; }

    public static int ShiftMask { get; } // Runtime constant equivalent to (sizeof(nint) * 8) - 1

    int IComparable.CompareTo(object value);    // Explicitly implemented to ensure that users still get compile-time type checking
    public int CompareTo(UIntPtr value);

    public bool Equals(UIntPtr other);          // This is currently explicitly implemented as IEquatable<UIntPtr>.Equals(UIntPtr other)

    public string ToString(string format);      // This is currently exposed on System.IntPtr, but not on System.UIntPtr
    public string ToString(IFormatProvider provider);
    public string ToString(string format, IFormatProvider provider);

    public static UIntPtr Parse(string s);
    public static UIntPtr Parse(string s, NumberStyles style);
    public static UIntPtr Parse(string s, IFormatProvider provider);
    public static UIntPtr Parse(string s, NumberStyles style, IFormatProvider provider);

    public static bool TryParse(string s, out UIntPtr result);
    public static bool TryParse(string s, NumberStyles style, IFormatProvider provider, out UIntPtr result);
}

Other Thoughts

Implementing System.IConvertible is done by the other primitive types and may be useful in some cases here as well.

It might be worthwhile having int implement IComparable<IntPtr> and IEquatable<IntPtr>, given that these are valid operations as well.

@tannergooding
Copy link
Member Author

Ported from https://github.com/dotnet/coreclr/issues/11768. I have an implementation of this proposal here: tannergooding#1. If this is approved, I can submit a PR to dotnet/coreclr directly.

@MichalStrehovsky
Copy link
Member

It might be better to make MinValue and MaxValue a property to avoid adding a static constructor to the type.

These are different from e.g. Int32.MaxValue anyway because those are all literal fields (const).

@tannergooding
Copy link
Member Author

I had done it to match the current Zero definition, but you make a good point. I've updated the above.

@GSPP
Copy link

GSPP commented May 24, 2017

Can IntPtr.ToString() be changed to something more appropriate for pointers and other "opaque values"? Maybe 0x12345678 (i.e. return ((uint)this).ToString("X8"); and similar for 64 bit). Right now it returns ((int)this).ToString().

Not sure how this can cause compat issues since I have never seen the ToString output of an IntPtr in my life.

@tannergooding
Copy link
Member Author

@GSPP, changing the default is likely a breaking change (but I can't say for certain on that). However the existing IntPtr.ToString(string) function (and the proposed UIntPtr.ToString(string)) function can be used to give you the desired behavior (IntPtr.ToString("X8") will give you a hex formatted number).

Additionally, the purpose of this proposal is to expose some interfaces and methods to make IntPtr less opaque and less tied to being a pointer-type, because it isn't a pointer type. In my opinion, (due to lots of reasons I could list, ranging from documentation, to comments, to specifications, to corresponding types with the same name in the set of standard Windows Data Types) It is a platform sized integer (an Integer that is the size of a PoinTeR). The size and shape of IntPtr just makes it convenient to use when needing to expose opaque types (handles) when you don't want to require the user to use unsafe code. As such, treating it as an integer first and foremost is likely the correct way to go, but recognizing that it is also used to represent opaque pointers and providing helper functions for that scenario is also a good idea.

@pentp
Copy link
Contributor

pentp commented May 25, 2017

  1. Maybe I'm missing something here, but it seems like implementing IComparable<int> and IEquatable<int> on IntPtr isn't very useful (they won't be used in any normal generic code). For the same reasons, adding IComparable<IntPtr> and IEquatable<IntPtr> to int isn't useful. CompareTo(int)/Equals(int) might be questionable without these interfaces and after changing CompareTo(IntPtr)/Equals(IntPtr) to CompareTo(nint)/Equals(nint).

  2. Implementing IConvertible would be useful for example when upgrading existing code to use nint instead of long while some other part of the application uses Convert.ToInt64(object) - without IConvertible it will fail with an exception.

@tannergooding
Copy link
Member Author

@pentp, for 1, I only included it because those comparisons represent actual IL instructions supported by the runtime. I'm definitely not sold on including them 😄

For 2, I agree it would be useful in some cases (but not when upgrading existing code from long to nint -- that is an API break 😉), but the primary reason I left it off was because it doesn't expose any way to convert back to IntPtr (and adding a new interface member will be a breaking change, at least until we get default interface members).

@tannergooding
Copy link
Member Author

@joperezr, @AlexGhiondea (since it looks like you are the area owners).

Can this be moved to ready-for-review. It looks like the biggest question is around the IComparable<int> and IEquatable<int> contracts, but those should be resolvable during review.

@joperezr
Copy link
Member

that seems reasonable. Marking it as ready-for-review.

@terrajobst
Copy link
Member

We decided to table this until the nint, andnuint discussion is closed.

@karelz
Copy link
Member

karelz commented Oct 17, 2017

FYI: The API review discussion was recorded - see https://youtu.be/BEBq3__WfDc?t=245 (10 min duration)

@tannergooding
Copy link
Member Author

Watched the API review.

There was a lot of discussion around both the nint and nuint proposal and around why these particular APIs were proposed.

There were two sets of APIs included in this proposal:

  • API additions to System.UIntPtr which gave it parity with System.IntPtr
  • API additions which are new to both IntPtr and UIntPtr

I think the former should be done regardless, so that UIntPtr and IntPtr have the same exposed surface area. The singular API is: public string ToString(string format);

The remaining APIs should not be impacted by the nint/nuint decision. Several of them represent standard interface contracts and would need to be implemented regardless.

MaxValue and MinValue represent the logical min/max and can vary based on platform. This makes writing platform agnostic code easier (requiring platform checks and accesses to int vs long otherwise)

The IntPtr, int equality operators match the existing IntPtr, IntPtr equality operators and represents the valid IL comparison between IntPtr and int. long versions are not suggested as they are not portable and represent invalid IL. -- I am not convinced these are needed, since the user could explicitly cast from int to IntPtr and compare then, this just makes that easier, since implicit cast from int to IntPtr doesn't exist today.

The CompareTo methods represent the IComparable interface, which should be implemented regardless of the language decision.

The Equals methods represent the IEquatable interface, which should be implemented regardless of the language decision.

The ToString interfaces finish implementing the core overloads the other primitive types have (and also implements IFormattable).

The Parse and TryParse methods represent core string methods that are on the other primitive types.

@tannergooding
Copy link
Member Author

long overloads were not suggested since they are:

  1. Not Portable
  2. Represent invalid IL (long and IntPtr comparisons are illegal, would need to explicitly convert to long and then compare).

@pentp
Copy link
Contributor

pentp commented Oct 18, 2017

I still think that IComparable<int> and IEquatable<int> don't really fit here - can't think of any realistic code that could call these on an IntPtr and no other type has such type-mismatched interfaces implemented.

@tannergooding
Copy link
Member Author

no other type has such type-mismatched interfaces implemented.

@pentp, I would normally agree. However, there is one key difference which I think makes there be an argument:

  • Performing binary operations on primitive types is strictly limited to having both values on the evaluation stack being the same (int32, int64, or F). Because of this, when operating on two operands of different types, you are required to explicitly convert one of them before performing the operation. native int and & are the exception in that they are allowed to have the other operand be int32 and do not require an explicit conversion before performing the operation.

image

image

image

So, the argument is that IComparable<int> and IEquatable<int> represent legal IL instructions and a scenario that does not exist for the other primitive types.

@pentp
Copy link
Contributor

pentp commented Oct 18, 2017

But what would that scenario be? Comparing IntPtr and int is fine, but what I don't understand is how would someone call these through an interface? It can't be a generic dictionary/set/sorting operation because these always use the type itself (IntPtr) for the interface type parameter.

To put it another way: why doesn't int implement IComparable<short> - that's also perfectly OK from the IL perspective, but equally useless.

@tannergooding
Copy link
Member Author

why doesn't int implement IComparable - that's also perfectly OK from the IL perspective, but equally useless.

There is no int16 for the evaluation stack and short is implicitly convertible to int.

I'm definitely not convinced they are needed, but they do represent valid IL instructions and valid comparisons for two types which are not implicitly convertible to each other.

@jkotas
Copy link
Member

jkotas commented Oct 18, 2017

The reduced set of types on the IL evaluation stack has been a very local simplifying assumption to make the IL simpler and more compact. I do not think it makes sense to use it to base the API design on.

@karelz
Copy link
Member

karelz commented Nov 7, 2017

Please mark it api-ready-for-review when it is unblocked and ready.

@tannergooding
Copy link
Member Author

Marking this as "ready-for-review" again. This was originally marked as blocked due to belief that it was pending the language team decision (https://youtu.be/HnKmLJcEM74?t=30). However, that should not be the case and System.IntPtr can freely undergo API changes. The language, due to the existing operators already exposed in net40, will already need to special-case whatever they do (whether it is via partial erasure, a new type, or something else). CC. @jaredpar

@tannergooding
Copy link
Member Author

Having these additional functions, and the rest of the operators exposed on these types would greatly simplify some code in CoreCLR and externally in other libraries that need to deal with raw memory addresses or native sized integers (such as ML.NET). It would also facilitate more code-sharing with CoreFX (CC. @GrabYourPitchforks).

@jaredpar
Copy link
Member

I agree I don't really see any language blockers here. The only way I could see this being a language issue is if we collectively decided to break compat on IntPtr here. Essentially delete the existing operators entirely and ask the compiler to emit the proper IL instructions in that case. Or also collectively decide the compiler should just ignore them and emit the proper IL.

@jkotas
Copy link
Member

jkotas commented Oct 10, 2018

Having these additional functions, and the rest of the operators exposed on these types would greatly simplify some code in CoreCLR and externally in other libraries that need to deal with raw memory addresses or native sized integers

The APIs at the top are not operators (except for the equal operator). I do not think this proposal would help the code sharing or portability much. If you would like to see what helps with code sharing/portability, replace using nuint = ... and using nint = ... in CoreLib to point to UIntPtr/IntPtr, and then make it compile and functional with same performance as before.

I think you may want to split this into two different issues: One that is for Parse, Format, etc. And second one for the operators. Each of these is for a very different scenario.

@tannergooding
Copy link
Member Author

@jaredpar, @jkotas, @stephentoub. Now that we appear to have stabilized on the plans for the language feature.

Do you see any problems with exposing the proposed helpers on System.IntPtr and System.UIntPtr?

@jkotas
Copy link
Member

jkotas commented Oct 16, 2019

MaxValue, MinValue, IComparable<IntPtr>

Makes sense.

IComparable<int>, IEquatable<int>

I do not think this makes sense. E.g. long does not implemement IComparable<int> either.

operator ==
operator !=

Why these two and not other operators? If somebody needs more convenient way to work with IntPtr, they should use nint instead. I do not think we should be adding any new operators.

Parsing, formatting

Some parsing/formatting may be ok for convenience, but I am not sure whether we need the whole set proposed above. I see these types as low-level interop types that are never directly displayed in UI, etc.

@tannergooding
Copy link
Member Author

I do not think this makes sense. E.g. long does not implemement IComparable either.

👍

Why these two and not other operators? If somebody needs more convenient way to work with IntPtr, they should use nint instead. I do not think we should be adding any new operators.

I had originally proposed them, but split them off at your request: https://github.com/dotnet/corefx/issues/20256#issuecomment-428766266.

However, with nint and it being erased, I agree that using nint will be preferred and will remove these from the proposal.

Some parsing/formatting may be ok for convenience, but I am not sure whether we need the whole set proposed above. I see these types as low-level interop types that are never directly displayed in UI, etc.

For formatting: from this perspective, it plays into how nint can be used/displayed in debugging/logs and how it works in interpolated strings vs ToString() calls. It may be desirable for some values to be displayed as hex and others to be displayed as "regular" integral values and this would allow that.

For parsing: I think not supporting parsing might be understandable, especially since that would encourage people to consider int vs long, etc.

The alternative would be to just tell users wanting to parse/format to use (nint.Size == 4) ? ((int)value).ToString() : ((long)value).ToString(), etc.

@tannergooding
Copy link
Member Author

CC. @agocke, @MadsTorgersen, @cston

It was also raised whether we could expose a property that could be used by the language for making left and right shift deterministic.

@tannergooding
Copy link
Member Author

CC. @333fred

@terrajobst
Copy link
Member

  • I've tagged this as a breaking change because we have concerns that now supporting format strings will change behavior when people used format strings with IntPtr/UIntPtr (before it was ignored, now it would be honored).
  • C# has new types nint and nuint that will compile down to IntPtr/UIntPtr (encoded similar to dynamic). When the value is typed as these new types, C# will support common operators and emit them like they do for int. If they value is typed as IntPtr/UIntPtr, then the only supported are the user defined operators on those types. nint and nuint will never use these custom operators. There are identity conversions between nint and IntPtr and between nuint and UIntPtr. We assume that static methods dotted into from nint and nuint will have IntPtr and UIntPtr instances to be replaced by nint and nuint respectively. For example, nint.TryParse(text, out var result) should type result as nint, nuint.MaxValue should be of type nuint etc.
  • Currently, C# will not allow using aliases like using SomeType = nint.
  • Today, IntPtr/UIntPtr can't be used as base types for enum. Should we allow nint and nuint? If so, we should probably have an analyzer that warns when people use nint and have a constant with the high-bit set to avoid surprising sign extensions.
  • It might be beneficial to let the compiler not requiring ShiftMask because otherwise the compiler would have to disallow shifting nint and nuint when targeting a framework that doesn't have the ShiftMask property. Alternatively, the compiler could light up when the property exists.
  • We should review the custom attribute we'll be using to encode nint and nuint
  • Should we always synthesize the NativeIntegerAttribute (akin to NativeIntegerAttribute) or should be put one in-box (akin to DynamicAttribute)?
  • We propose to make MaxValue, MinValue, and ShiftMask read-only field because they are really runtime constants. But we're open to do whatever the JIT team thinks is more optimal.
  • We should review existing usage of IntPtr/UIntPtr and decide which one we want to mark as nint/unint.
#nullable enable;

namespace System
{
    [StructLayout(LayoutKind.Sequential)] // This is implicitly set by the compiler, but should be explicitly declared, as it is done for System.Int32
    public partial struct IntPtr : IComparable, IComparable<IntPtr>, IFormattable
    {
        public static readonly IntPtr MaxValue;
        public static readonly IntPtr MinValue;

        public static readonly int ShiftMask; // Runtime constant equivalent to (sizeof(nint) * 8) - 1

        int IComparable.CompareTo(object value);    // Explicitly implemented to ensure that users still get compile-time type checking
        public int CompareTo(IntPtr value);

        public bool Equals(IntPtr other);           // This is currently explicitly implemented as IEquatable<IntPtr>.Equals(IntPtr other)

        public string ToString(IFormatProvider? provider);
        public string ToString(string format, IFormatProvider? provider);

        public static IntPtr Parse(string s);
        public static IntPtr Parse(string s, NumberStyles style);
        public static IntPtr Parse(string s, IFormatProvider? provider);
        public static IntPtr Parse(string s, NumberStyles style, IFormatProvider? provider);

        public static bool TryParse(string? s, out IntPtr result);
        public static bool TryParse(string? s, NumberStyles style, IFormatProvider? provider, out IntPtr result);
    }
    public partial struct UIntPtr : IComparable, IComparable<UIntPtr>, IFormattable
    {
        public static readonly UIntPtr MaxValue;
        public static readonly UIntPtr MinValue;

        public static readonly int ShiftMask; // Runtime constant equivalent to (sizeof(nint) * 8) - 1

        int IComparable.CompareTo(object value);    // Explicitly implemented to ensure that users still get compile-time type checking
        public int CompareTo(UIntPtr value);

        public bool Equals(UIntPtr other);          // This is currently explicitly implemented as IEquatable<UIntPtr>.Equals(UIntPtr other)

        public string ToString(string format);      // This is currently exposed on System.IntPtr, but not on System.UIntPtr
        public string ToString(IFormatProvider? provider);
        public string ToString(string format, IFormatProvider? provider);

        public static UIntPtr Parse(string s);
        public static UIntPtr Parse(string s, NumberStyles style);
        public static UIntPtr Parse(string s, IFormatProvider? provider);
        public static UIntPtr Parse(string s, NumberStyles style, IFormatProvider? provider);

        public static bool TryParse(string? s, out UIntPtr result);
        public static bool TryParse(string? s, NumberStyles style, IFormatProvider? provider, out UIntPtr result);
    }    
}
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Class |
                    AttributeTargets.Field |
                    AttributeTargets.Parameter |
                    AttributeTargets.Property |
                    AttributeTargets.ReturnValue |
                    AttributeTargets.Struct)]
    public sealed class NativeIntegerAttribute : Attribute
    {
        public NativeIntegerAttribute() { }
        public NativeIntegerAttribute(byte[] flags) { }
    }
}

@tannergooding
Copy link
Member Author

@terrajobst, we said that IComparable.CompareTo should not be explicit and I don't see the notes around the parsing/formatting APIs (where I raised the concerns Jan had and we indicated we thought they were still fine to expose).

@pentp
Copy link
Contributor

pentp commented Nov 19, 2019

For enums, the current proposal is to allow nint/nuint but not allow constants that could have different meaning on different platforms: dotnet/csharplang#2833 (comment)

@jkotas
Copy link
Member

jkotas commented Nov 20, 2019

We propose to make MaxValue, MinValue, and ShiftMask read-only field because they are really runtime constants. But we're open to do whatever the JIT team thinks is more optimal.

If these are properties, the C# implementation for these will "just work" and generate good code predictably. No JIT or VM changes are required.

If these are readonly fields, a lot of ceremony is required to predictably generate good code for these, custom built in each runtime, AOT compiler, etc. We can make it work, but it is pain.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2019

FWIW, we do have a prior art for both styles of runtime constants on IntPtr/UIntPtr:

  • IntPtr.Zero is reaonly field
  • IntPtr.Size is a property

@cston
Copy link
Member

cston commented Apr 14, 2020

Regarding ShiftMask:

public static int ShiftMask { get; } // equivalent to (sizeof(nint) * 8) - 1

The C# compiler will probably emit (sizeof(nint) * 8) - 1 explicitly, instead of using the ShiftMask member, since the compiler should fallback to the explicit value if the member is missing anyway.

@tannergooding
Copy link
Member Author

Resolved with #307

@ericstj
Copy link
Member

ericstj commented Jul 24, 2020

@tannergooding can you please file a breaking-change doc issue? https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

@tannergooding
Copy link
Member Author

dotnet/docs#19679

@ghost ghost locked as resolved and limited conversation to collaborators Dec 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

No branches or pull requests