dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.13k stars 4.7k forks source link

[API Proposal]: Reflection introspection support for FunctionPointer #69273

Closed steveharter closed 1 year ago

steveharter commented 2 years ago

Background and motivation

This is the proposed API to support introspection of Function Pointers (delegate*) based on the user story, original issue and function pointer design. In summary, when function pointers were added in 5.0 the corresponding support for reflection was not added which resulted in an IntPtr being returned as the type when using typeof or reflection. This feature changes that to now return System.Type which then allows access to function pointer metadata including the calling convention, return type and parameters. This is a breaking change.

UPDATE: the original approved API for .NET 7 was not implemented due to concerns with breaking scenarios with managed C++ and adding unnecessary runtime overhead. This .NET 8 proposal changes the design to return "modified types" that expose custom modifiers on function pointers instead of exposing them on runtime-based types.

Corresponding support will also be added to MetadataLoadContext. The inbox version will have full support but the netstandard version, for simplicity, will throw NotSupportedException for the new members that return new types (since they are not in netstandard). For members that don't return new types, it will be possible to use reflection to invoke them.

This API issue is focused on introspection; the links above have additional features that can be layered on this including support for IL Emit.

API Proposal

namespace System
{
    public abstract class Type
    {
+       public virtual bool IsFunctionPointer { get; }
+       public virtual bool IsUnmanagedFunctionPointer { get; }

        // These throw InvalidOperationException if IsFunctionPointer = false:
+       public virtual Type GetFunctionPointerReturnType();
+       public virtual Type[] GetFunctionPointerParameterTypes();

        // These require a "modified type" to return custom modifier types:
+       public virtual Type[] GetRequiredCustomModifiers();
+       public virtual Type[] GetOptionalCustomModifiers();
+       public virtual Type[] GetFunctionPointerCallingConventions(); // Throws if IsFunctionPointer = false
    }
}

// Return a "modified type" from a field, property or parameter if its type is a:
// - Function pointer type
// - Pointer or array since they may reference a function pointer
// - Parameter or return type from a function pointer
namespace System.Reflection
{
    public abstract class FieldInfo
    {
+       public virtual Type GetModifiedFieldType() => throw new NotSupportedException();
    }

    public abstract class PropertyInfo
    {
+       public virtual Type GetModifiedPropertyType() => throw new NotSupportedException();
    }

    public abstract class ParameterInfo
    {
+       public virtual Type GetModifiedParameterType() => throw new NotSupportedException();
    }
}

A modified type's UnderlyingSystemType property returns the unmodified type and all members on a modified forward to that except:

which instead return the information kept on the modified type.

API Usage

See the design referenced above. Here's some short examples:

Type type = typeof(delegate*<int, bool>);
bool b = type.IsFunctionPointer(); // true
string s = type.ToString(); // "System.Boolean(System.Int32)"
Type returnType = type.GetFunctionPointerReturnType(); // System.Boolean
Type param1Type = type.GetFunctionPointerParameterTypes()[0]; // System.Int32

FieldInfo fieldInfo = typeof(Util).GetField("_field")!;
bool isUnmanaged = fieldInfo.FieldType.IsUnmanagedFunctionPointer; // true

// Get the modified type in order to obtain the custom modifiers
Type modifiedType = fieldInfo.GetModifiedFieldType();

// Get the unmanaged calling conventions
type = modifiedType.GetFunctionPointerCallingConventions()[0]; // CallConvCdecl

// Get the first custom modifier from the first parameter
type = modifiedType.GetFunctionPointerParameterTypes()[0].GetRequiredCustomModifiers()[0]; // OutAttribute

unsafe public class Util
{
    public delegate* unmanaged[CDecl]<out bool, void>; _field;
}

Risks

The breaking change nature of not returning IntPtr any longer.

ghost commented 2 years ago

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

Issue Details
### Background and motivation This is the proposed API to support introspection of Function Pointers (`delegate*`) based on the [user story](https://github.com/dotnet/runtime/issues/44327) and [original issue](https://github.com/dotnet/runtime/issues/11354). In summary, when function pointers were added in 5.0 the corresponding support for reflection was not added which resulted in an `IntPtr` being returned as the type when using `typeof` or reflection. This feature changes that to now return `System.Type` which then allows access to function pointer metadata including the calling convention, return type and parameters. This is a breaking change. Corresponding support will also be added to `MetadataLoadContext`. The v7 version will have full support but the netstandard version, for simplicity, will likely throw `NotSupportedException` like it does today since there are new types and new `System.Type` members not exposed in netstandard. This API issue is focused on introspection. Once this is in we can consider: - Support invoke such as by adding a `DynamicInvoke()`, by returning a bound `Delegate` or by returning a bound `MethodInfo`. - Being able to box for invoke parameter support. This means supporting the existing reflection invoke APIs by allowing a previously created `IntPtr` (by casting the function pointer) to be passed to a method that has a strongly-typed function pointer type argument, where it is unwound. This would be similar to how value type pointers (e.g. `int*`) can be passed as a boxed `IntPtr` (or `System.Reflection.Pointer`) to such methods. - Note that at runtime, outside of reflection, function pointers are still passed as either the strongly-typed pointer itself, a `void*` pointer or an `IntPtr`. This API issue does not expose a unified type+value class such as `System.Reflection.Pointer` although it could be added later. - Emit APIs to create a function pointer. ### API Proposal ```diff namespace System.Reflection { // A function pointer is a stand-alone signature, meaning it does not have an independent, // reusable declaration like a Delegate. + public abstract class FunctionPointerInfo + { + internal FunctionPointerInfo(); // prevent external derivation + + public abstract CallingConvention CallingConvention { get; } + public abstract ParameterInfo ReturnParameter { get; } + public abstract Type UnderlyingType { get; } + public abstract ReadOnlySpan GetParameters(); // Although each function pointer declaration will be its own instance (pending possible caching strategy) // we support Equals to compare against them. + public abstract bool Equals(object? obj); + public abstract int GetHashCode(); + public abstract string? ToString(); // Future invoke and MemberInfo scenarios: // public abstract unsafe object? DynamicInvoke(void* pfnTarget, params object?[]? args); // public abstract unsafe Delegate CreateDelegate(void* pfnTarget, Type delegateType); + } public class Type { + public virtual bool IsFunctionPointer { get; } // Throws InvalidOperationException if IsFunctionPointer is false. + public virtual FunctionPointerInfo GetFunctionPointerInfo(); // Note that IsPointer will return 'false' for function pointers. public virtual bool IsPointer { get; } } // We are not re-using this as it exposes "Any". Ideally this could have been called "AnyManaged" // in which case we may have extended it. // Currently used in limited scenarios on MethodBase primarily to filter via GetMethods() and GetConstructor() [Flags] public enum CallingConventions { Standard = 0x1, VarArgs = 0x2, Any = 0x3, HasThis = 0x20, ExplicitThis = 0x40, // would be set for function pointers } // New CallingConvention that matches System.Reflection.Metadata.SignatureCallingConvention except uses Managed\Unmanaged prefixes + public enum CallingConvention : byte + { + Managed = 0, + UnmanagedCdecl = 1, + UnmanagedStdCall = 2, + UnmanagedThisCall = 3, + UnmanagedFastCall = 4, + ManagedVarArgs = 5, // not supported with function pointers + Unmanaged = 9, + } } ``` ### API Usage ```cs using System.Reflection; namespace ConsoleApp { internal class Program { static void Main(string[] args) { // typeof: Type type = typeof(delegate*); bool b = type.IsFunctionPointer(); // true FunctionPointerInfo fpi = Type.GetFunctionPointerInfo(); string s = fpi.ToString(); // "void ()" (actual is TBD, but will list all parameters if there any) Type returnType = fpi.ReturnParameter.ParameterType; // System.Void // Also works with reflection: MethodInfo method = typeof(Util).GetMethod(nameof(Util.GetLog), BindingFlags.Public | BindingFlags.Instance)!; type = method.ReturnType; b = type.IsFunctionPointer(); // true fpi = Type.GetFunctionPointerInfo(); } } unsafe public class Util { public static void Log() { Console.WriteLine("Log"); } public delegate* GetLog() { return &Log; } } } ``` ### Alternative Designs - A new `MethodInfo`-derived type. - A new `System.Type`-derived type. - A new `System.Reflection.FunctionPointer` type similar to `System.Reflection.Pointer`. This is still possible if we want to wrap the type and value together. ### Risks The breaking change.
Author: steveharter
Assignees: steveharter
Labels: `api-suggestion`, `area-System.Reflection`, `breaking-change`
Milestone: 7.0.0
AaronRobinsonMSFT commented 2 years ago

public abstract CallingConvention CallingConvention { get; }

Instead of reusing that enum, which has aged poorly, I would follow the pattern established by UnmanagedCallersOnlyAttribute.CallConvs and UnmanagedCallConvsAttribute.

teo-tsirpanis commented 2 years ago
jkotas commented 2 years ago

Type.GetFunctionPointerInfo() could return null if the FPI does not exist,

If it does exist, it would end up allocating FunctionPointerInfo instance that the caller may not be interested at all. I think there is a value in having IsFunctionPointer method that just returns bool.

MichalStrehovsky commented 2 years ago

What's the motivation behind FunctionPointerInfo? There's no GenericParameterInfo, ArrayInfo, PointerInfo, ByReferenceInfo, or GenericInstanceInfo - the properties/methods that are only relevant for pareterized types/generic instances are dumped directly onto System.Type. It's up to the caller to decide whether the properties/methods make sense to call for a given System.Type instance.

Function pointers are constructed types like any other constructed type - why do we shape the reflection API differently?

svick commented 2 years ago

@MichalStrehovsky

the properties/methods that are only relevant for pareterized types/generic instances are dumped directly onto System.Type

Personally, I dislike that design: it makes it harder to discover which members you need or to ensure you're using them correctly. To me, this proposal looks good, since it clearly separates the concept of function pointers from the rest of the API surface, while not being too out of place regarding the existing API (unlike e.g. a new System.Type-derived type).

So I see it as a question of choosing something that's better in isolation, but less consistent versus choosing something worse on its own, but more consistent.

MichalStrehovsky commented 2 years ago

I'm not a fan of that design either but it has advantages in some cases. E.g. if I want a Type that tweaks one of the properties of an existing type, I would use a System.Refection.TypeDelegator and override that one property. It's unclear how TypeDelegator fits with FunctionPointerInfo. E.g. if I want the exact same function pointer type but with a different calling convention.

The reflection stack is built around dumping everything onto System.Type.

steveharter commented 2 years ago

What's the motivation behind FunctionPointerInfo? There's no GenericParameterInfo, ArrayInfo, PointerInfo, ByReferenceInfo, or GenericInstanceInfo - the properties/methods that are only relevant for pareterized types/generic instances are dumped directly onto System.Type. It's up to the caller to decide whether the properties/methods make sense to call for a given System.Type instance. Function pointers are constructed types like any other constructed type - why do we shape the reflection API differently?

Function pointers are more like methods and have more metadata than generics, arrays, existing pointer, etc so instead of adding directly to Type some options considered:

  1. Provide a new stand-alone FunctionPointerInfo type
  2. Extend MethodInfo - e.g. FunctionPointerInfo : MethodInfo.
  3. Extend MemberInfo - e.g. FunctionPointerInfo : MemberInfo.
  4. Extend Type with inheritance FunctionPointerType : Type

The stand-alone option has the least friction and doesn't pollute Type.

We may also want to add DynamicInvoke() capabilities, so having a separate type would also be used for that.

steveharter commented 2 years ago

It's unclear how TypeDelegator fits with FunctionPointerInfo. E.g. if I want the exact same function pointer type but with a different calling convention.

Yes thanks to support that we would need to add a Type.GetFunctionPointerInfoImpl(...) similar to Type.GetMethodInfoImpl().

MichalStrehovsky commented 2 years ago

Function pointers are more like methods and have more metadata than generics, arrays, existing pointer, etc.

I think there might be some confusion about the amount of metadata function pointer carries. They really don't carry much.

Also the choice of ParameterInfo is odd - most of the properties on ParameterInfo don't make sense for function pointers - ParameterInfo.MetadataToken can only throw because function pointer parameters don't have tokens (they're not parameters in metadata sense). There's also no ParameterInfo.Member, or Name, or any of the other properties besides ParameterType and modifier-related APIs. The ParameterInfo.MemberImpl field is also not-nullable so it's unclear what value we would put there since there's no associated member.

Here's a list of APIs on Type that are only relevant if System.Type represents a generic parameter as an example of par for the course and why the argument of "polluting" doesn't hold.

I'm discussing pretty much just adding three properties to System.Type - one to get the information about return type, one to get information about parameters, and one for the calling convention:

class Type
{
       public abstract CallingConvention CallingConvention { get; }

       // Not actually proposing using ParameterInfo per above
       public abstract ParameterInfo ReturnParameter { get; }

       // Not ReadOnlySpan for consistency
       public abstract ParameterInfo[] GetParameters();
}

I understand we would also like to add "execution" APIs for this. Those should go on a separate class. There's prior art:

The "execution" APIs never go on Type and we would keep it that way for function pointers.

Yes thanks to support that we would need to add a Type.GetFunctionPointerInfoImpl(...) similar to Type.GetMethodInfoImpl().

That would not address the problem in a consistent way. For the scenario I wrote above (just change calling convention), do I really need to reimplement all of FunctionPointerInfo? Why there's no convenient FuctionPointerInfoDelegator that works as TypeDelegator? As a reflection user, it's what I expect based on how these APIs work for everything else, when I just need to tweak a single aspect of the type.

steveharter commented 2 years ago

Thanks for the detailed feedback @MichalStrehovsky. Per offline discussion, the original intent of FunctionPointerInfo was to hide new members on Type for a better experience, however for the following reasons the API proposal was updated to remove FunctionPointerInfo:

MichalStrehovsky commented 2 years ago

Thank you for the good offline discussion! I have a couple of observations for the new design:

For the FunctionPointerCallingConvention member, how will this look for delegate* unmanaged[Stdcall, SuppressGCTransition]<void>. Will the returned object be an array of two System.Type instances (one for CallConvSuppressGCTransition, the other for CallConvStdcall). Instead of typing FunctionPointerCallingConvention as object, could we type it as Type[] for parity with UnmanagedCallersOnlyAttribute.CallConvs?

steveharter commented 2 years ago

Thank you for the good offline discussion! I have a couple of observations for the new design:

Thanks. Removed Name, Equals, GetHashCode, IsIn, IsOut. Did not add "OwningType";' we could always add that later and I'm concerned with future asks for aliasing that this could either be null or prevent effective caching.

For the FunctionPointerCallingConvention member, ...

The FunctionPointerCallingConvention should have been Type[], not object. I also added some more comments around that. Basically FunctionPointerCallingConvention and FunctionPointerReturnParameter.GetOptionalCustomModifiers() will return the exact same CallConv types however FunctionPointerReturnParameter.GetOptionalCustomModifiers() may return additional non-CallConv mod opts. The idea is that FunctionPointerCallingConvention is a filter on top of FunctionPointerReturnParameter.GetOptionalCustomModifiers() and returns just the CallConv* types, and some of those types may be synthesized based on the non-exposed CallKind -- all to hide CallKind and prevent bugs related to misuse of that.

jkotas commented 2 years ago

FunctionPointerReturnParameter.GetOptionalCustomModifiers() may return additional mod opts like CallConvSuppressGCTransition

CallConvSuppressGCTransition is also a CallConv* type. Did you mean to use a different example?

steveharter commented 2 years ago

CallConvSuppressGCTransition is also a CallConv* type. Did you mean to use a different example?

Yep thanks. I just updated to say "non-CallConv" mod opts.

carlreinke commented 2 years ago

@bartonjs You forgot to hit "Comment".

MichalStrehovsky commented 2 years ago

Watched the recording. If we're proposing IsPointer return true for function pointers, how are we going to reconcile it with places like this:

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Assignability.cs#L101

Will we always have to do IsPointer && !IsFunctionPointer? Or be very careful about the order in which we check the properties (treat function pointers first and the rest is what everyone else considers pointer types)?

Function pointers are not a subset of unmanaged pointers in the ECMA-335 spec either (page 44 of the PDF has a helpful hierarchy - function pointers are pointers same way as byrefs are pointers).

Do we also return true from HasElementType or do we only implement GetElementType to return void?

I'm not sure I agree with the argument that we need IsPointer to be true so that NetStandard serializers can use it to ignore function pointers. Desktop CLR is going to treat them as UIntPtr forever and IsPointer is false for those. We'll get inconsistent behavior no matter what.

bartonjs commented 2 years ago

::sigh::

The API review notes, as recovered from https://youtu.be/viY9djCf4pg?t=6424.

namespace System
{
    public partial class Type
    {
        // Throws InvalidOperationException if IsFunctionPointer is false.
        public virtual Type[] GetFunctionPointerCallingConventions();

        // Throws InvalidOperationException if IsFunctionPointer is false.
        public virtual FunctionPointerParameterInfo GetFunctionPointerReturnParameter();

        // Throws InvalidOperationException if IsFunctionPointer is false.
        public virtual FunctionPointerParameterInfo[] GetFunctionPointerParameterInfos();

        public virtual bool IsUnmanagedFunctionPointer { get; }
        public virtual bool IsFunctionPointer { get; }

        // Note that the existing IsPointer will still return 'true' for function pointers.
        // public virtual bool IsPointer { get; }
    }
}

namespace System.Reflection
{
    public abstract class FunctionPointerParameterInfo
    {
        public abstract Type ParameterType { get; }

        // This will return the same set of CallConv* as Type.FunctionPointerCallingConventions plus any additional mod opts
        public abstract Type[] GetOptionalCustomModifiers();
        public abstract Type[] GetRequiredCustomModifiers();
    }
}
bartonjs commented 2 years ago

Desktop CLR is going to treat them as UIntPtr forever and IsPointer is false for those. We'll get inconsistent behavior no matter what.

The impression that I had was that IsPointer is currently returning true for function pointer types; and maintaining the same "IsX" answers applies to netstandard2.0 libraries whether they are running on NetFX or CoreFX...

MichalStrehovsky commented 2 years ago

No, they're IntPtr. Here's all the cases I tested: https://twitter.com/MStrehovsky/status/1311247635756412928?t=w5_oW1CWfQwR0ozJsNRx7w&s=19

steveharter commented 2 years ago

cc @GrabYourPitchforks

The discussion is that we wanted to change IsPointer from returning false to true but noted that will likely break existing serializers that assume true implies GetElementType() is going to be non-null. So, to address that, we return typeof(void) for GetElementType() instead of null.

Having a non-null "element type" for a FP may not be 100% correct at least a FP can be implicitly cast to void* (in C#).

Will we always have to do IsPointer && !IsFunctionPointer? Or be very careful about the order in which we check the properties (treat function pointers first and the rest is what everyone else considers pointer types)?

Yes IsFunctionPointer is more specific than IsPointer. However, the intent of returning void* mentioned above is that hopefully it addresses most backwards-compat scenarios. If that is not the case, then we should re-visit this.

Function pointers are not a subset of unmanaged pointers in the ECMA-335 spec either (page 44 of the PDF has a helpful hierarchy - function pointers are pointers same way as byrefs are pointers).

Do you see fallout from treating FP as a pointer in Type? This is also trying to improve understandability\consistency in Type since it would be odd at a high level to have a general API IsPointer return false while the more specific API IsFunctionPointer returns true for the same type.

Do we also return true from HasElementType or do we only implement GetElementType to return void?

I think true makes sense assuming we continue down the path of returning a non-null GetElementType().

MichalStrehovsky commented 2 years ago

Having a non-null "element type" for a FP may not be 100% correct at least a FP can be implicitly cast to void* (in C#).

They don't cast as far as reflection is concerned:

Type intStar = typeof(int).MakePointerType();
Type voidStar = typeof(void).MakePointerType();
Type uintStar = typeof(uint).MakePointerType();
Console.WriteLine(intStar.IsAssignableFrom(voidStar)); // false
Console.WriteLine(voidStar.IsAssignableFrom(intStar)); // false
Console.WriteLine(intStar.IsAssignableFrom(uintStar)); // true

I agree that having function pointers pretend to be void* can make some existing code work.

It will also make existing code subtly broken. These are just two examples of code that will be subtly broken in this repo:

https://github.com/dotnet/runtime/blob/8a043bf7adb0fbf5e60a8dd557c98686bc0a8377/src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureTypeExtensions.cs#L131-L134

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Assignability.cs#L101-L115

Do we have any data saying IsPointer=true will be meaningfully less breaking?

In the absence of data, my suggestion is to do the thing that is consistent with how everyone else has been treating function pointers, starting with ECMA-335 spec, Cecil, CCI, dnSpy type system, our managed type system, and other places. They all have pointers and function pointers, and function pointers are not pointers.

The only place that seems to say function pointers are pointers is the Roslyn compiler test tree, but even that one says GetElementType() == null because void is misleading. This probably has to do with the tests being built on top of LMR and this is the best thing they could do since introducing a new property on Type is impossible.

The story on <= .NET 6.0 is broken beyond any repair since function pointers are indistinguishable from an nint.

DaZombieKiller commented 2 years ago

Bit of a nitpick, but would it be better if GetFunctionPointerCallingConventions were named GetFunctionPointerCallConvs? It's more consistent with other instances of CallConv* Type arrays. The current name almost seems to imply that a function pointer can have more than one calling convention, when in reality it has a single calling convention defined by multiple CallConvs.

steveharter commented 2 years ago

During PR review, design discussions were raised about how this API proposal introduces unwanted breaking changes to managed C++ and may cause issues elsewhere in the future. This is because the identity of function pointer types (which include the modifiers) affects how runtime Type handles are created. For example, two identical functions differing only by a parameter having an in modifier for one function and a ref modifier for the other function would be considered two different Type instances and not be equal. This is not expected nor desired in the runtime.

The new proposal is to not consider modifiers or calling conventions as part of the Type identity. This means that:

In addition, to support scenarios that need to obtain the calling conventions and related custom modifiers, the new proposal also will return "Signature" types from PropertyInfo, ParameterInfo and FieldInfo which will be null for non-function pointers. These new APIs will only be exposed when using these types from MetadataLoadContext but could be extended to runtime reflection types if necessary (pending feedback). This signature class will return parameter information likely the same way as GetFunctionPointerCallingConventions() and GetFunctionPointerParameterInfo().

Since V7 is essentially being locked down now for new features\API work, these changes will come in V8. This issue will be updated with the new proposal.

DaZombieKiller commented 2 years ago

In addition, to support scenarios that need to obtain the calling conventions and related custom modifiers, the new proposal also will return "Signature" types from PropertyInfo, ParameterInfo and FieldInfo which will be null for non-function pointers. These new APIs will only be exposed when using these types from MetadataLoadContext but could be extended to runtime reflection types if necessary (pending feedback).

In my view, only working with MetadataLoadContext greatly harms the usability of these APIs. It essentially eliminates any usefulness that these function pointer APIs could have had for my use cases (largely interop and code generation-related scenarios.)

I can no longer take a user-provided function pointer Type and treat it appropriately based on its calling convention and other information. I could potentially go through the Type's Assembly and re-load it through MetadataLoadContext, but now I have a massive performance penalty and my code cannot support in-memory assemblies.

steveharter commented 2 years ago

@DaZombieKiller

I can no longer take a user-provided function pointer Type and treat it appropriately based on its calling convention and other information.

For your scenarios, do you care about non-calling convention custom modifiers?

DaZombieKiller commented 2 years ago

For your scenarios, do you care about non-calling convention custom modifiers?

Certainly not as much, though it would be nice to have the ability to handle them.

alexrp commented 2 years ago

FWIW:

Being able to introspect a function pointer Type is, if we're being realistic, effectively useless if you can't know the calling convention. Any result you produce based on the information you have at hand is strictly speaking going to be incorrect.

Imagine having a nominal Type that you can introspect for its fields, methods, etc but you can't ask if it's a class or a struct. Or a MethodBase where you can introspect the parameter types but you can't know if they're ref vs out. Or a FieldInfo where you have no idea if it's static. That's the kind of situation this would be.

At most, you'd be able to use the information for semi-helpful diagnostic purposes, but that's about where the usefulness would end.

MSDN-WhiteKnight commented 2 years ago

I remember some uses are blocked just because MetadataLoadContext currently dies with NotSupportedException whenever it hits a signature with function pointer, even if it does not try to access the function type itself. They needed to manually exclude some methods to evade this error, which is pretty ugly. So even the limited ability to support function pointers without calling convention could be useful.

steveharter commented 1 year ago

For those interested in the design, please review https://github.com/dotnet/designs/pull/282.

steveharter commented 1 year ago

cc @MichalStrehovsky @GrabYourPitchforks @jkotas

We had a review today and @tannergooding was asking if Type.IsPointer returns true or not for function pointers. The short answer is the current design returns false. Long answer is that returning true was originally proposed early in the 7.0 design but then changed to false based on discussions:

Tanner was suggesting that we do this (as was originally in the 7.0 proposal as well)

-or-

jkotas commented 1 year ago

Have ElementType return typeof(void*) for function pointers.

Should that be typeof(void) instead? What would Type.HasElementType return for function pointers with this plan?

MichalStrehovsky commented 1 year ago

I summed up my arguments about IsPointer here: https://github.com/dotnet/runtime/issues/69273#issuecomment-1142806816

steveharter commented 1 year ago

Should that be typeof(void) instead? What would Type.HasElementType return for function pointers with this plan?

Yes it should be void and not void*. Type.HasElementType would be true.

jkotas commented 1 year ago

My vote is to keep POR proposal (IsPointer is false for function pointers), for similar reasons that Michal shared.

Add a IsPointerOrFunctionPointer

I do not see enough added clarity over type.IsPointer || type.IsFunctionPointer. It is very common in code that operates over reflection object model to have a bool || conditions like this.

tannergooding commented 1 year ago

I do not see enough added clarity over type.IsPointer || type.IsFunctionPointer. It is very common in code that operates over reflection object model to have a bool || conditions like this.

The clarity is that when you look at IntelliSense, you see both IsPointer and IsPointerOrFunctionPointer side by side. This allows you to determine that IsPointer doesn't imply IsFunctionPointer, which is the natural presumption for such APIs (particularly if coming from other compilers that do model them this way).

tannergooding commented 1 year ago

The same applies to docs and any other context where you might be looking at the available Is* APIs for Type

jkotas commented 1 year ago

The same problem exists for IsByRef. Byrefs are pointers in the more general sense as well - they are called managed pointers in number of contexts. Reflection object model has many intricacies that are not discoverable using IntelliSense. One has to read the docs.

tannergooding commented 1 year ago

The difference is they don't have pointer in the name for the API side of things, and so while one might expect IsPointer to return true, there isn't any implication.

However, IsPointer vs IsFunctionPointer being distinct and non-overlapping queries violates several existing conventions in our API surface/design guidelines and directly violates several intuitive assumptions people may immediately form when seeing them.

MichalStrehovsky commented 1 year ago

People who don't read docs are going to interpret this in all sorts of way. There's prior art in Cecil (or CCI, or others). We can just look for how people use this.

E.g. here is someone puzzled why IsFunctionPointer doesn't return true for delegates:

https://github.com/partypooperarchive/DataDumper/blob/98e25bbdb62e45fa61b60f2d026bb765a527cea5/DataDumper/AssemblyParser.cs#L376

They're right, delegates are also conceptually function pointers. One needs to read docs...

bartonjs commented 1 year ago

Video

Looks good as proposed.

namespace System
{
    public partial class Type
    {
       public virtual bool IsFunctionPointer { get; }
       public virtual bool IsUnmanagedFunctionPointer { get; }

        // These throw InvalidOperationException if IsFunctionPointer = false:
       public virtual Type GetFunctionPointerReturnType();
       public virtual Type[] GetFunctionPointerParameterTypes();

        // These require a "modified type" to return custom modifier types:
       public virtual Type[] GetRequiredCustomModifiers();
       public virtual Type[] GetOptionalCustomModifiers();
       public virtual Type[] GetFunctionPointerCallingConventions(); // Throws if IsFunctionPointer = false
    }
}

// Return a "modified type" from a field, property or parameter if its type is a:
// - Function pointer type
// - Pointer or array since they may reference a function pointer
// - Parameter or return type from a function pointer
namespace System.Reflection
{
    public partial class FieldInfo
    {
       public virtual Type GetModifiedFieldType() => throw new NotSupportedException();
    }

    public partial class PropertyInfo
    {
       public virtual Type GetModifiedPropertyType() => throw new NotSupportedException();
    }

    public partial class ParameterInfo
    {
       public virtual Type GetModifiedParameterType() => throw new NotSupportedException();
    }
}