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

UnsafeAccessor and static classes #90048

Closed
MichalPetryka opened this issue Aug 4, 2023 · 15 comments
Closed

UnsafeAccessor and static classes #90048

MichalPetryka opened this issue Aug 4, 2023 · 15 comments

Comments

@MichalPetryka
Copy link
Contributor

As per #81741:

For UnsafeAccessorKind.{Static}Method and UnsafeAccessorKind.{Static}Field, the type of the first argument of the annotated extern method identifies the owning type.
The value of the first argument is treated as @this pointer for instance fields and methods.
The first argument must be passed as ref for instance fields and methods on structs.
The value of the first argument is not used by the implementation for static fields and methods.

This requirement however makes StaticMethod and StaticField impossible to use with static classes, since you can't have a parameter of their type.

Is there any way to specify the type there today? If not, could the api maybe be changed to take a typeof in the attribute fields to allow specifying the type that way?

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 4, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 4, 2023
@MihaZupan MihaZupan added area-System.Runtime.CompilerServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 4, 2023
@ghost
Copy link

ghost commented Aug 4, 2023

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

Issue Details

As per #81741:

For UnsafeAccessorKind.{Static}Method and UnsafeAccessorKind.{Static}Field, the type of the first argument of the annotated extern method identifies the owning type.
The value of the first argument is treated as @this pointer for instance fields and methods.
The first argument must be passed as ref for instance fields and methods on structs.
The value of the first argument is not used by the implementation for static fields and methods.

This requirement however makes StaticMethod and StaticField impossible to use with static classes, since you can't have a parameter of their type.

Is there any way to specify the type there today? If not, could the api maybe be changed to take a typeof in the attribute fields to allow specifying the type that way?

Author: MichalPetryka
Assignees: -
Labels:

area-System.Runtime.CompilerServices, untriaged

Milestone: -

@jkoritzinsky
Copy link
Member

Alternatively, this could be an additional motivation for allowing static classes to be used as type parameters (part of dotnet/csharplang#5783).

@MichalPetryka
Copy link
Contributor Author

MichalPetryka commented Aug 4, 2023

Alternatively, this could be an additional motivation for allowing static classes to be used as type parameters (part of dotnet/csharplang#5783).

How would that relate to this? UnsafeAccessor isn't a generic attribute and making it so would be a big breaking change.

@jkoritzinsky
Copy link
Member

I was thinking that this was implemented with a generic parameter for the static class, my mistake. I misinterpreted it, sorry.

I don't think allowing the first parameter to be a System.Type for static classes would work though. It would effectively require runtime introspection, bringing the cost back to just about the same as reflection (it couldn't be jitted to be a direct call to the underlying method).

@hamarb123
Copy link
Contributor

It would just be a property on the attribute.

@jkotas
Copy link
Member

jkotas commented Aug 5, 2023

@AaronRobinsonMSFT Should we reconsider the design for StaticMethod and StaticField and specify the owning type via an optional Attribute argument instead of the dummy parameter?

@jkotas
Copy link
Member

jkotas commented Aug 5, 2023

(The problem was mentioned in #81741 (comment), but I have missed it somehow.)

@jkotas
Copy link
Member

jkotas commented Aug 5, 2023

Ah, I remember now after reading the whole discussion in #81741. The idea was to cover this case using UnsafeAccessorType extension that would enable skipping type visibility, and make static classes work as a side-effect.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Aug 5, 2023

Ah, I remember now after reading the whole discussion in #81741. The idea was to cover this case using UnsafeAccessorType extension that would enable skipping type visibility, and make static classes work as a side-effect.

Yes, that was the design. I don't think this is needed for the primary stakeholders for UnsafeAccessorAttribute at present. It wouldn't be a ton of work to enable this with a new attribute, but getting it through API review for .NET 8 would be a heavy lift I think.

@jkotas Do you have an opinion here about .NET 8 or .NET 9?

@stephentoub
Copy link
Member

stephentoub commented Aug 5, 2023

getting it through API review for .NET 8 would be a heavy lift I think

If there's a simple design you're confident in and you want it in for 8, we can make it happen.

@AaronRobinsonMSFT
Copy link
Member

If we decide to do this for .NET 8 we would need the following done.

  • Update CoreCLR and NativeAOT - I can handle both.
  • Update trimmer to track the new attribute - @vitek-karas could you handle this?
  • Update mono - @fanyang-mono or @lambdageek Would either of you have a concern here?

@AaronRobinsonMSFT
Copy link
Member

A straw man proposal for UnsafeAccessorTypeAttribute could be:

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
    public sealed class UnsafeAccessorTypeAttribute : Attribute
    {
        public UnsafeAccessorTypeAttribute(Type target)
        {
            Target = target;
        }
    
        public Type Target { get; init; }
    }
}

We could also make this a generic attribute.

@jkotas
Copy link
Member

jkotas commented Aug 5, 2023

The challenge with this design is that it would not extend to the type visibility skipping using UnsafeAccessorType.

We could also make this a generic attribute.

Nit: That would not work for the static classes. C# does not allow static classes to be used as generic type arguments.

@jkotas Do you have an opinion here about .NET 8 or .NET 9?

I think what we have is good to .NET 8. We should open a tracking issue for the type visibility skipping using UnsafeAccessorType and mention that it would address accessing methods on static classes as well.

@MichalPetryka
Copy link
Contributor Author

I think what we have is good to .NET 8.

Yeah I'd say that this can wait for .Net 9, I'd rather see #89439 be prioritized first.

@AaronRobinsonMSFT
Copy link
Member

Closing in lieu of #90081.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants