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

Function pointer design #282

Merged
merged 17 commits into from
Feb 22, 2023
Merged

Conversation

steveharter
Copy link
Member

Design that should be approved before implementation is continued and before we update API issues.

accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
accepted/2022/FunctionPointers.md Show resolved Hide resolved
accepted/2022/FunctionPointers.md Show resolved Hide resolved
accepted/2022/FunctionPointers.md Outdated Show resolved Hide resolved
+ Type functionPointerType)
}
}
```
Copy link
Member

@jkotas jkotas Dec 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflection.Emit will need some work to deal with function pointers correctly beyond the new EmitCalli overloads.

Consider this example that tries to emit a call to a method that takes function pointer https://gist.github.com/jkotas/79c7c673b7d892f41f6be5f16d380aed . It does not work today. MethodInfo returns IntPtr instead of actual type, Reflection.Emit encodes method reference with a wrong signature, and we get MissingMethodException at runtime.

The problem is that this example cannot work even once this proposal is implemented. The example contains a function pointer with modreqs, but Reflection.Emit has no way to see the modreqs. It means that it is still going to emit reference with a wrong signature and we will get MissingMethodException at runtime.

This suggests that we cannot actually omit function pointer modopts/modreqs in runtime reflection object model if we want to have full support for function pointers in Reflection.Emit.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Dec 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we run into the same problem if we wrap access to a get; init; setter like that? I think the problem is really that the runtime reflection system was not designed to introspect modifiers. It was designed to ignore them.

Edit: maybe not. We can still read that one. But we still cannot e.g. reflection-invoke C++/CLI methods with pointers to const for the same reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to emit init-only properties today. (We had bugs reported in it that got fixed by #40587.)

reflection-invoke C++/CLI methods with pointers to const for the same reason.

Yes, it is similar to #282 (comment) .

```

# Breaking changes
The breaking change is: **`typeof(delegate*<...>)` and `Type.GetType()` will return a `Type` instance instead of `IntPtr`**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: This breaking change is not actually strictly required to make the function pointers inspectable by reflection. We can keep the runtime current behavior and tell people to use signature types to gather any function pointer details.

(I am not trying to oppose the breaking change. Just sharing an observation.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting; but I would say doing that goes against the goal of "correcting" the Type system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants