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

Add conversions spec for function pointers #3263

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 10, 2020

The summary for these specs are:

  • Value parameters are treated as contravariant.
  • Value return types are treated as covariant.
  • Reference parameter/return types and modifiers must match exactly.
  • Calling convention must match exactly.
  • An address-of expression that contains method group with exactly 1 static method is convertible to void*.

- For each `ref`, `out`, or `in` parameter, the parameter type in `M` is the same as the corresponding parameter type in `F`.
- If the return type is by value (no `ref` or `ref readonly`), an identity, implicit reference, or implicit pointer conversion exists from the return type of `F` to the return type of `M`.
- If the return type is by reference (`ref` or `ref readonly`), the return type and `ref` modifiers of `F` are the same as the return type and `ref` modifiers of `M`.
- The calling convention of `M` is the same as the calling convention of `F`.
Copy link
Member

Choose a reason for hiding this comment

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

How you determine calling convention of M ? Is this going to take into account the NativeCallable attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this going to take into account the NativeCallable attribute?

I was planning on it doing so, yes, but I'm not sure if that's something that should be explicitly called out in our spec or if should be an implementation detail. @gafter for thoughts on that?

Copy link
Member

Choose a reason for hiding this comment

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

It does not feel right to me to talk about calling convention in the spec, but not explain where it comes from.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that if the spec depends on a concept, it should define it. However, that can be left for a future refinement of this spec. Put it in an open issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the open issues list.

- [_Existing conversions_](https://github.com/dotnet/csharplang/blob/master/spec/unsafe-code.md#pointer-conversions)
- From _funcptr\_type_ `F0` to another _funcptr\_type_ `F1`, provided all of the following are true:
- `F0` and `F1` have the same number of parameters, and each parameter `D0n` in `F0` has the same `ref`, `out`, or `in` modifiers as the corresponding parameter `D1n` in `F1`.
- For each value parameter (a parameter with no `ref`, `out`, or `in` modifier), an identity conversion, implicit reference conversion, or implicit pointer conversion exists from the parameter type in `F0` to the corresponding parameter type in `F1`.
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2020

Choose a reason for hiding this comment

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

implicit reference conversion, or implicit pointer conversion exists from the parameter type in F0 to the corresponding parameter type in F1. [](start = 109, length = 145)

Does runtime support this kind of mismatch natively, or will compiler have to generate a stub to make this work? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Does runtime support this kind of mismatch natively, or will compiler have to generate a stub to make this work?

Natively. We don't even emit a cast for explicit conversions.

- Only a `delegate*` with a managed calling convention can be the target of such a conversion.
In an unsafe context, a method `M` is compatible with a function pointer type `F` if all of the following are true:
- `M` and `F` have the same number of parameters, and each parameter in `D` has the same `ref`, `out`, or `in` modifiers as the corresponding parameter in `F`.
- For each value parameter (a parameter with no `ref`, `out`, or `in` modifier), an identity conversion, implicit reference conversion, or implicit pointer conversion exists from the parameter type in `M` to the corresponding parameter type in `F`.
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 10, 2020

Choose a reason for hiding this comment

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

implicit reference conversion, or implicit pointer conversion exists from the parameter type in M to the corresponding parameter type in F. [](start = 105, length = 143)

Same question here. #Closed

@AlekseyTs
Copy link
Contributor

Do we want to/is it possible to support a conversion from a function pointer to a delegate?

@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

@AlekseyTs the runtime has an API for this today, Marshal.GetFunctionPointerForDelegate<TDelegate>(TDelegate). I don't think we need to try and do anything special in the language for that.

@AlekseyTs
Copy link
Contributor

the runtime has an API for this today, Marshal.GetFunctionPointerForDelegate(TDelegate). I don't think we need to try and do anything special in the language for that.

I think I am talking about conversion in the opposite direction. I.e. passing a pointer to delegate's constructor.

@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

I think I am talking about conversion in the opposite direction.

The runtime has a method going in the other direction as well, Marshal.GetDelegateForFunctionPointer<TDelegate>(IntPtr). While it's not ideal as it's not a strongly-typed guarantee that the function pointer is compatible with the type of the delegate, unless I hear strong desire for these to be compatible I don't think it merits special language treatment either.

@AlekseyTs
Copy link
Contributor

The runtime has a method going in the other direction as well, Marshal.GetDelegateForFunctionPointer<TDelegate>(IntPtr).

I am not sure why would we want to force a user to use this API and not have a benefit of type safety checks performed by a compiler. A delegate constructor takes a pointer (which we obtain for a method today) and, in function pointer case, we would have a pointer available. We also already have rules for method signature to delegate compatibility, which we can simply reuse.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2020

  • Marshal.GetDelegateForFunctionPointer is an expensive API. I do not think you want compiler to add calls to it.
  • Marshal.GetDelegateForFunctionPointer works for unmanaged methods only
  • Users who are using function pointers are unlikely to use Marshal.GetDelegateForFunctionPointer. Function pointers are better.

@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

I added a note to the open issues to consider it.

@333fred
Copy link
Member Author

333fred commented Mar 11, 2020

@AlekseyTs @jkotas did you have any more feedback? I'll address the typo when I'm confident that all the rest of the minor changes are accounted for.

@jkotas
Copy link
Member

jkotas commented Mar 11, 2020

Looks fine to me otherwise.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

Co-Authored-By: Jan Kotas <[email protected]>
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.

4 participants