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.

Sign up for GitHub

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

Jump to bottom

Initial errors on UnmanagedCallersOnly #47008

Merged
merged 8 commits into from
Aug 28, 2020
Merged

Initial errors on UnmanagedCallersOnly #47008

merged 8 commits into from
Aug 28, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Aug 20, 2020

We now error when a signature that is not compatible with UnmanagedCallersOnly is used. Specifically, these requirements are:

  • The method must be static.
  • The method must be ordinary, or a local function.
  • The method must use only unmanaged types in its parameters and return type.
  • The set of valid types passed to the CallConv property is the same as the set of types considered calling convention modifiers for function pointers.

@333fred
Initial errors on UnmanagedCallersOnly
db634b0 
We now error when a signature that is not compatible with UnmanagedCallersOnly is used. Specifically, these requirements are:
* The method must be static.
* The method must be ordinary, or a local function.
* The method must use only unmanaged types in its parameters and return type.
* The set of valid types passed to the `CallConv` property is the same as the set of types considered calling convention modifiers for function pointers.
@333fred 333fred requested a review from a team as a code owner August 20, 2020 18:20
@333fred
Copy link
Member Author

333fred commented Aug 20, 2020

https://github.com/dotnet/csharplang/blob/master/proposals/csharp-9.0/function-pointers.md#systemruntimeinteropservicesunmanagedcallersonlyattribute is the spec for this. Note that this PR doesn't implement the restrictions on directly calling the method, only invalid signatures.

@333fred
Copy link
Member Author

333fred commented Aug 20, 2020

@dotnet/roslyn-compiler for review.

jcouv
jcouv reviewed Aug 21, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 1)

@jcouv jcouv self-assigned this Aug 21, 2020
@333fred
Copy link
Member Author

333fred commented Aug 21, 2020

@jcouv @cston addressed feedback.

jcouv
jcouv approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@333fred
Copy link
Member Author

333fred commented Aug 24, 2020

@dotnet/roslyn-compiler for a second review please.

1 similar comment
@333fred
Copy link
Member Author

333fred commented Aug 25, 2020

@dotnet/roslyn-compiler for a second review please.

@333fred
Clarify UnmanagedCallersOnly requirements to be no type parameters, a…
769d52a 
…nd all parameter/return types must be truly unmanaged.
@333fred 333fred mentioned this pull request Aug 25, 2020
Merged
@333fred
Copy link
Member Author

333fred commented Aug 25, 2020

@cston addressed feedback.
@elinor-fung, could you please take a look at the test cases and see if the errors and cases make sense to you?

@333fred
Allow UnmanagedWithGenerics parameter/return types, and mark the us…
49274a3 
…age invalid in types with generic parameters.
@333fred
Copy link
Member Author

333fred commented Aug 26, 2020

@cston addressed feedback.

@jkoritzinsky
Copy link
Member

@333fred the error messages and cases make sense to me.

@333fred
Copy link
Member Author

333fred commented Aug 26, 2020

@cston addressed feedback.

@333fred
Copy link
Member Author

333fred commented Aug 27, 2020

@cston addressed feedback.

@333fred
Merge remote-tracking branch 'upstream/master' into unmanaged-callers…
a25ef2d 
…-only-errors

* upstream/master: (236 commits)
  Fix bug when "End statement" is used in single-line if (dotnet#47062)
  Solution asset cache refactoring (dotnet#46948)
  add specific tests to validate behavior between keys and snapshots
  Extract into separate files
  rename parameters
  rename parameters
  rename parameters
  rename parameters
  Add CancellationToken parameters to SyntaxTreeOptionsProvider
  Reuse nullable override checks for delegate conversions (dotnet#46953)
  Introduce warning for multiple entry points (sync + async) (dotnet#46832)
  Switch from throwing NotImplementedException and return E_NOTIMPL
  Delete Building for Core CLR.md (dotnet#47146)
  Adjust PrintMembers to avoid boxing and avoid extra space (dotnet#47095)
  Track asynchronous operation in InProcLanguageServer
  Use Task.FromCanceled where appropriate
  Apply suggestions from code review
  Address feedback
  Expose ParseOptions on generator context (dotnet#46919)
  Remove redundant statement in added tests
  ...
@ghost
Copy link

ghost commented Aug 27, 2020

Hello @333fred!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost
ghost approved these changes Aug 27, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@333fred 333fred merged commit e98d42c into dotnet:master Aug 28, 2020
@333fred 333fred deleted the unmanaged-callers-only-errors branch August 28, 2020 02:36
@ghost ghost added this to the Next milestone Aug 28, 2020
@allisonchou allisonchou modified the milestones: Next, 16.8.P3 Aug 31, 2020
@am11 am11 mentioned this pull request Sep 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers auto-merge Feature - Function Pointers Adding Function Pointers
Projects
None yet
Milestone
16.8.P3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@333fred @jkoritzinsky @cston @jcouv @allisonchou