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

[CoreCLR and native AOT] UnsafeAccessorAttribute supports generic parameters #99468

Merged
merged 14 commits into from Mar 20, 2024
Merged

[CoreCLR and native AOT] UnsafeAccessorAttribute supports generic parameters #99468

merged 14 commits into from Mar 20, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Mar 8, 2024 •
edited

Support in CoreCLR and native AOT.

Contributes to #89439

The behavior expects type parameters to be supplied from a type and method generic parameters supplied from method declaration. Examples below. See also tests in PR.

// Target class has no generic type parameters
class Accessors
{
    // class B
    // {
    //     private T M<T>(T arg);
    // }
    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "M")]
    public extern static T CallM<T>(B b, T t);
}

// Target class has generic type parameters
class Accessors<T>
{
    [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_items")]
    public extern static ref T[] GetItems(List<T> list);

    [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name = "s_emptyArray")]
    public extern static ref T[] GetEmptyArray(List<T> list);

    // Case where a type parameter and method parameter both exist.
    // class C<T>
    // {
    //     private U M<U>(U arg);
    // }
    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "M")]
    public extern static U CallM<U>(C<T> c, U arg);
}

@AaronRobinsonMSFT
Generic type support
d223b6f 
Instance and static fields on generic types
Re-enable and add to field tests
@AaronRobinsonMSFT
Generic methods
86bc15f 
Generic types with non-generic methods.
@AaronRobinsonMSFT
Generic static methods
7495477
@AaronRobinsonMSFT
Add testing for generic scenarios
4c09918
@AaronRobinsonMSFT
Remove canonical check to VAR and MVAR
b1b22cf
@AaronRobinsonMSFT
Native AOT support
6271919 
Update tests
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Mar 8, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Mar 8, 2024
Open
4 tasks
@AaronRobinsonMSFT
Copy link
Member Author

/cc @fanyang-mono @lambdageek @jkotas @davidwrighton @steveisok

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review March 9, 2024 20:37
@AaronRobinsonMSFT
Copy link
Member Author

/azp list

@azure-pipelines Azure Pipelines
Copy link

CI/CD Pipelines for this repository:

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime-coreclr ilasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@AaronRobinsonMSFT AaronRobinsonMSFT changed the title UnsafeAccessorAttribute supports generic attributes [CoreCLR and native AOT] UnsafeAccessorAttribute supports generic attributes Mar 9, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added the runtime-coreclr specific to the CoreCLR runtime label Mar 9, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title [CoreCLR and native AOT] UnsafeAccessorAttribute supports generic attributes [CoreCLR and native AOT] UnsafeAccessorAttribute supports generic parameters Mar 10, 2024
@MichalPetryka
Copy link
Contributor

MichalPetryka commented Mar 12, 2024 •
edited

What's the behaviour with constraints here? I assume they have to match? Asking since exposing the idea from #89439 (comment) might be interesting, either by default or with some new , DontMatchConstraints = true (EDIT: or maybe LazyConstraintChecks would be a better name) that'd cause UnsafeAccessor to match methods with different constraints and throw if they don't match at runtime.

@AaronRobinsonMSFT
Copy link
Member Author

What's the behaviour with constraints here? I assume they have to match? Asking since exposing the idea from #89439 (comment) might be interesting, either by default or with some new , DontMatchConstraints = true that'd cause UnsafeAccessor to match methods with different constraints and throw if they don't match at runtime.

Still discussing that at present. The constraint aspect is not being addressed in this PR. Since there are a number of assumptions about using this API in general and it is considered "unsafe", it is possible it is something the user needs to ensure is correct. An official decision will be made for .NET 9 though.

@MichalPetryka
Copy link
Contributor

Since there are a number of assumptions about using this API in general and it is considered "unsafe", it is possible it is something the user needs to ensure is correct.

@SingleAccretion mentioned before that checking at runtime would be an additional cost for shared generics so exposing both options could be the best solution.

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member Author

Ah, I assumed the default would rather be that you need to match the constraints at declaration:

This is what I am currently exploring now.

@AaronRobinsonMSFT
Add design document
9b26c9d 
Add strict check for precisely matched Generic constraints
@AaronRobinsonMSFT
Check for null prior to constraint check.
7946c69
@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek Nope. I am defining "lookup" very precisely. Happy to relax this if we see compelling scenarios but I think, like the first iteration, strictness is helpful. It is for the same reason I am requiring constraints to match exactly.

    // also ok? - all variables, but some are rearranged
   [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "M")]
   public extern static Tuple<U,T,V,W> CallM2<V,W>(C<U,T> c, U arg1, T arg2, V arg3, W arg4);

No. They must match precisely.

   // also ok? - some type args are constructed types
   [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "M")]
   public extern static Tuple<List<T>,U, V, W> CallM3<V, W>(C<List<T>, U> c, List<T> arg1, U arg2, V arg3, W arg4);

No, the target signature doesn't match so lookup for M will fail.

   // also ok? - we need to guess the method instantiation based on the arguments?
   [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "M")]
   public extern static Tuple<T,U, V[], W[]> CallM4<V, W>(C<T, U> c, T arg1, U arg2, V[] arg3, W[] arg4);

No, the target signature doesn't match so lookup for M will fail.

This was referenced Mar 14, 2024
Closed
Closed
jkotas
jkotas reviewed Mar 14, 2024
View reviewed changes
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Mar 15, 2024
Open
jkotas
jkotas reviewed Mar 15, 2024
View reviewed changes
src/coreclr/vm/prestub.cpp Outdated Show resolved Hide resolved
@AaronRobinsonMSFT
Update design doc
adbfc52 
Add constraint test
@AaronRobinsonMSFT
Add constraints check in native AOT
c34b61e 
Update tests to use unique parameter names
jkotas
jkotas approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you

@jkotas
Copy link
Member

jkotas commented Mar 19, 2024

We should make sure to update the docs: https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute

@AaronRobinsonMSFT
Copy link
Member Author

We should make sure to update the docs: https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute

Yep, that is being tracked in #89439.

davidwrighton
davidwrighton approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

:shipit:

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit d58c541 into dotnet:main Mar 20, 2024
136 of 154 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the generic_unsafe_accessors branch March 20, 2024 20:25
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@davidwrighton davidwrighton davidwrighton approved these changes

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@fanyang-mono fanyang-mono Awaiting requested review from fanyang-mono

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels
area-System.Runtime.CompilerServices runtime-coreclr specific to the CoreCLR runtime
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AaronRobinsonMSFT @MichalPetryka @jkotas @lambdageek @davidwrighton @MichalStrehovsky