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

[API Proposal]: Zero-overhead member access with suppressed visibility checks #81741

Closed
jkotas opened this issue Feb 7, 2023 · 48 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Feb 7, 2023

Background and motivation

Number of existing .NET serializers depend on skipping member visibility checks for data serialization. Examples include System.Text.Json or EF Core. In order to skip the visibility checks, the serializers typically use dynamically emitted code (Reflection.Emit or Linq.Expressions) and classic reflection APIs as slow fallback. Neither of these two options are great for source generated serializers and native AOT compilation. This API proposal introduces a first class zero-overhead mechanism for skipping visibility checks.

API Proposal

namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
public class UnsafeAccessorAttribute : Attribute
{
    public UnsafeAccessorAttribute(UnsafeAccessorKind kind);

    public UnsafeAccessorKind Kind { get; }

    // The name defaults to the annotated method name if not specified.
    // The name must be null for constructors
    public string? Name { get; set; }
}

public enum UnsafeAccessorKind
{
    Constructor, // call instance constructor (`newobj` in IL)
    Method, // call instance method (`callvirt` in IL)
    StaticMethod, // call static method (`call` in IL)
    Field, // address of instance field (`ldflda` in IL)
    StaticField // address of static field (`ldsflda` in IL)

    // Potential additions to handle niche cases
    // FieldGet, // get value of instance field (`ldfld` in IL). Required for `ref` fields.  
    // FieldSet, // get value of instance field (`stfld` in IL). Required for `ref` fields.  
    // NonVirtualMethod, // call instance method non-virtually (`call` in IL).
};

This attribute will be applied on extern static method. The implementation of the extern static method annotated with this attribute will be provided by the runtime based on the information in the attribute and the signature of the method that the attribute is applied to. The runtime will try to find the matching method or field and forward the call to it. If the matching method or field is not found, the body of the extern method will throw MissingFieldException or MissingMethodException.

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.

The generic parameters of the extern static method are concatenation of the type and method generic arguments of the target method. For example, extern static void Method1<T1, T2>(Class1<T1> @this) can be used to call Class1<T1>.Method1<T2>(). The generic constraints of the extern static method must match generic constraints of the target type, field or method.

Return type is considered for the signature match. modreqs and modopts are not considered for the signature match.

API Usage

class UserData {
  private UserData() { }
  public string Name { get; set; }
}

[UnsafeAccessor(UnsafeAccessorKind.Constructor)]
extern static UserData CallPrivateConstructor();

// This API allows accessing backing fields for auto-implemented properties with unspeakable names
[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "<Name>k__BackingField")]
extern static ref string GetName(UserData @this);

UserData ud = CallPrivateConstructor();
GetName(ud) = "Joe";

Alternative Designs

  • Expand unsafe accessors in Roslyn The primary disadvantages is that compile time decision can be invalidated at runtime because the assembly can change. For example, Roslyn could be looking at a ref assembly where privates are stripped, NuGet version changes could cause compiler to see version 1 of an assembly at one phase in the build but at runtime we end up using version 2, etc.
  • Allow suppressing visibility checks in Roslyn. This option was proposed and rejected in Add public methods to supress visibility checks for compilation roslyn#11149. It violates fundamental language design principles.
  • Sidecar .dll with suppressed visibility checks. Runtime allows suppressing visibility checks in IL via IgnoresAccessChecksToAttribute. Build can generate sidecar .dll with trivial wrappers to access the private fields and methods with visibility checks suppressed. The source generated serializer would then call these wrappers to access the private members. It would be zero-cost way to circumvent visibility checks, assuming that the trivial wrapper methods get inlined by the JIT. The challenge with this option is integration into the overall build flow.
  • Better Reflection APIs. We have work planned on new faster reflection APIs in .NET 8. The challenge with this option is that reflection APIs are always going to be significantly slower than a direct method invocation or a simple field access. We would be leaving performance on the table.
  • Optimize reflection invoke call via pattern-matching The challenge with this option is that reflection invoke tends to be a complex code. Pattern matching them in the JIT or some other dedicated optimization step would be complex and fragile. For example, consider what it would take to pattern match typeof(MyType).GetMethod("set_IntProperty", BindingFlags.Public | BindingFlags.Instance).Invoke(BindingFlags.DoNotWrapExceptions , ptr, new object[] { (object)intValue });, optimize it to p.IntProperty = intValue;.
  • Do nothing Live with the fact that there is no cheap way to circumvent visibility checks without Reflection.Emit. Encourage serializers that want to be lean, fast and AOT friendly to avoid these patterns.

Risks

  • The proposed API has to be recognized in all linker, AOT compilers and runtimes.
  • The proposed API addressed skipping member visibility checks only. Skipping of type visibility checks can be added later (look for UnsafeAccessorType in the discussion below for the potential design).
@jkotas jkotas added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices labels Feb 7, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 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

Background and motivation

Number of existing .NET serializers depend on skipping member visibility checks for data serialization. Examples include System.Text.Json or EF Core. In order to skip the visibility checks, the serializers typically use dynamically emitted code (Reflection.Emit or Linq.Expressions) and classic reflection APIs as slow fallback. Neither of these two options are great for source generated serializers and native AOT compilation. This API proposal introduces a first class zero-overhead mechanism for skipping visibility checks.

API Proposal

namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
public class UnsafeAccessorAttribute : Attribute
{
    public LibraryImportAttribute(string name, UnsafeAccessorKind kind)
    {
        Name = name;
        Kind = kind;
    }
}

public enum UnsafeAccessorKind
{
    Constructor = 1,
    InstanceMethod = 2,
    StaticMethod = 3,
    FieldGetter = 4,
    FieldSetter = 5
};

This attribute will be applied on extern static method. The implementation of the extern static method annotated with this attribute will be provided by the runtime based on the information in the attribute and the signature of the method that the attribute is applied to. The runtime will try to find the matching method or field and forward the call to it. If the matching method or field is not found, the body of the extern method will throw MissingFieldException or MissingMethodException.

API Usage

class UserData {
  private UserData() { }
  public string Name { get; set; }
}

[UnsafeAccessor(".ctor", UnsafeAccessorKind.Constructor)]
extern UserData CallPrivateConstructor()

// This API allows accessing backing fields for auto-implemented properties with unspeakable names
[UnsafeAccessor("<Name>k__BackingField", UnsafeAccessorKind.FieldSetter)]
extern static void SetName(UserData ud, string v);

UserData ud = CallPrivateConstructor();
ud.SetName(ud, "Joe");

Alternative Designs

  • Expand unsafe accessors in Roslyn The primary disadvantages is that compile time decision can be invalidated at runtime because the assembly can change. For example, Roslyn could be looking at a ref assembly where privates are stripped, NuGet version changes could cause compiler to see version 1 of an assembly at one phase in the build but at runtime we end up using version 2, etc.
  • Allow suppressing visibility checks in Roslyn. This option was proposed and rejected in Add public methods to supress visibility checks for compilation roslyn#11149. It violates fundamental language design principles.
  • Sidecar .dll with suppressed visibility checks. Runtime allows suppressing visibility checks in IL via IgnoresAccessChecksToAttribute. Build can generate sidecar .dll with trivial wrappers to access the private fields and methods with visibility checks suppressed. The source generated serializer would then call these wrappers to access the private members. It would be zero-cost way to circumvent visibility checks, assuming that the trivial wrapper methods get inlined by the JIT. The challenge with this option is integration into the overall build flow.
  • Better Reflection APIs. We have work planned on new faster reflection APIs in .NET 8. The challenge with this option is that reflection APIs are always going to be significantly slower than a direct method invocation or a simple field access. We would be leaving performance on the table.
  • Optimize reflection invoke call via pattern-matching The challenge with this option is that reflection invoke tends to be a complex code. Pattern matching them in the JIT or some other dedicated optimization step would be complex and fragile. For example, consider what it would take to pattern match typeof(MyType).GetMethod("set_IntProperty", BindingFlags.Public | BindingFlags.Instance).Invoke(BindingFlags.DoNotWrapExceptions , ptr, new object[] { (object)intValue });, optimize it to p.IntProperty = intValue;.
  • Do nothing Live with the fact that there is no cheap way to circumvent visibility checks without Reflection.Emit. Encourage serializers that want to be lean, fast and AOT friendly to avoid these patterns.

Risks

  • The proposed API has to be recognized in all linker, AOT compilers and runtimes.
  • The proposed API addressed skipping member visibility checks only. It does not address skipping type visibility checks.
Author: jkotas
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices

Milestone: -

@jkotas jkotas changed the title [API Proposal]: [API Proposal]: Zero-overhead member access with suppressed visibility checks Feb 7, 2023
@DaZombieKiller
Copy link
Contributor

Is there an advantage in providing FieldGetter/FieldSetter accessor kinds over a single Field kind that returns a ref? Keeping them separate allows for the system to error if you try to mutate a readonly field, but that ability is needed in many serializers so I'm not sure if it's useful.

public enum UnsafeAccessorKind
{
    Constructor = 1,
    InstanceMethod = 2,
    StaticMethod = 3,
    Field = 4,
};
[UnsafeAccessor("<Name>k__BackingField", UnsafeAccessorKind.Field)]
extern static ref int GetName(ref StructType @this);

I'm also curious, should the return type need to match the field type? If not, how would this work for fixed buffers or fields of non-public types?

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

single Field kind that returns a ref?

Thanks. Applied your suggestion.

how would this work for fixed buffers or fields of non-public types?

The proposal does not work for types that you do not have access to. It is called out in the risks section.

@DaZombieKiller
Copy link
Contributor

DaZombieKiller commented Feb 7, 2023

The proposal does not work for types that you do not have access to. It is called out in the risks section.

Do fixed buffers also fall under that, since the type is unspeakable and "hidden" by the compiler?

Since fixed buffer fields are annotated with [FixedBuffer(typeof(T), length)], maybe a Span could be used?

That would extend the proposal to:

public enum UnsafeAccessorKind
{
    Constructor = 1,
    InstanceMethod = 2,
    StaticMethod = 3,
    Field = 4,
    FixedBufferField = 5,
};

public struct Example
{
    private unsafe fixed int _buffer[5];
}

[UnsafeAccessor("_buffer", UnsafeAccessorKind.FixedBufferField)]
static extern Span<int> GetBuffer(ref Example @this);

Alternatively, allowing the element type to be used as the accessor's return type could work. Callers would then need to query the length with reflection and use Unsafe.Add. I suppose this also applies to [InlineArray] if that is implemented in the future.

@EgorBo
Copy link
Member

EgorBo commented Feb 7, 2023

What if my type has a private field, and the type of that field is also private (e.g. private struct MyStruct or private enum, etc) ?

@DaZombieKiller
Copy link
Contributor

I was thinking about how non-public and unspeakable types could be handled here, and the idea of using TypedReference came to mind. However, there's a blocker: methods are seemingly not allowed to return TypedReference.

It can be passed through a pointer though. So if TypedReference is leveraged and UnsafeAccessor supports providing a fully-qualified type name, it should be possible to handle private types to some extent:

// ExampleAssembly
internal struct CustomString
{
    public string ToSerializedString() => throw null;
}

public class UserData
{
    private UserData() { }
    internal CustomString Name { get; set; }
}
// Generated
[UnsafeAccessor("<Name>k__BackingField", UnsafeAccessorKind.Field)]
extern static unsafe void GetName(UserData @this, TypedReference* outReference);

[UnsafeAccessor("ToSerializedString", UnsafeAccessorKind.InstanceMethod, DeclaringTypeName = "CustomString, ExampleAssembly")]
extern static string ToSerializedString(TypedReference @this);
TypedReference field;
UserData ud = CallPrivateConstructor();
GetName(ud, &field);
string name = ToSerializedString(field);

Something about this feels really nasty though, I'm not sure that I like it. Even if it's only intended to be used by generated code, it seems like "too much".

@kant2002
Copy link
Contributor

kant2002 commented Feb 7, 2023

Isn't this would be fragile if used without source generator. what if we have class defined like this.

// v1
class UserData {
  private UserData() { }
  public string Name { get; set; }
}

[UnsafeAccessor("<Name>k__BackingField", UnsafeAccessorKind.Field)]
extern static ref int GetName(UserData @this);

and then change it to

// v2
class UserData {
  private string name;
  private UserData() { }
  public string Name { get => this.name; set => this.name = value; }
}

[UnsafeAccessor("<Name>k__BackingField", UnsafeAccessorKind.Field)]
extern static ref int GetName(UserData @this);

Now it's easy to forget to update "<Name>k__BackingField"

@kant2002
Copy link
Contributor

kant2002 commented Feb 7, 2023

I initially miss namespace where this would be living, and probably I less worry about wrong people misusing stuff. anyway what about multiple construtors and constructors with parameters?

V1. Custom parameter

class UserData {
  private UserData(string name) { }
  public string Name { get; set; }
}

[UnsafeAccessor(".ctor", UnsafeAccessorKind.Constructor)]
extern UserData CallPrivateConstructor(string param)

Is this possible?

V2. Multiple construtors

class UserData {
  private UserData() { }
  private UserData(string name) { }
  public string Name { get; set; }
}

[UnsafeAccessor(".ctor", UnsafeAccessorKind.Constructor)]
extern UserData CallPrivateConstructor(string param)

how to disambiguate between construtors?

@BreyerW
Copy link

BreyerW commented Feb 7, 2023

Not sure how feasible/desirable but another alternative would be to extend unsafer unsafe proposal like i proposed here dotnet/csharplang#6476 (comment) namely to allow unsafe to skip visibility checks.

@lambdageek
Copy link
Member

/cc @vargaz @SamMonoRT FYI

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

@DaZombieKiller

Do fixed buffers also fall under that, since the type is unspeakable and "hidden" by the compiler?

Yes, fixed buffers fall under that. I do not think it makes sense to create a special solution for fixed buffers. If we do anything here, it should be a general solution for all unaccessible types.

if TypedReference is leveraged and UnsafeAccessor supports providing a fully-qualified type name, it should be possible to handle private types to some extent

Yes, there are ways to naturally extend this design to support inaccessible types. It is not necessary for the first iteration. The serializers scenarios that are motivating this proposal do not need it.

TypedReference is one of the tools that can be part of the solution. The design you have proposed would only work for fields, it would not work for methods that may have multiple types in signatures.

@kant2002

Isn't this would be fragile if used without source generator

Yes, this has the exact same problems as private reflection has today.

how to disambiguate between constructors?

The accessor method has to have signature that matches the target method. It means that accessor method with UnsafeAccessorKind.Constructor with one string argument is always going to match a constructor with one string argument and no other constructors.

@BreyerW

Not sure how feasible/desirable but another alternative would be to extend unsafer unsafe proposal like i proposed here dotnet/csharplang#6476 (comment) namely to allow unsafe to skip visibility checks.

This is not feasible. It is variant of "Allow suppressing visibility checks in Roslyn" and "Expand unsafe accessors in Roslyn" mentioned in the alternative designs.

@lambdageek
Copy link
Member

It would be nice (for the runtime - but would require probably a lot of changes in Roslyn) if instead of Name the runtime could just see a Metadata token (member ref, I think).

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

It would be nice (for the runtime - but would require probably a lot of changes in Roslyn) if instead of Name the runtime could just see a Metadata token (member ref, I think).

It is "Expand unsafe accessors in Roslyn" in alternative designs. In general case, it would require Roslyn to create the token out of a thin air without actually seeing the member in reference assembly. It sounded problematic.

@AaronRobinsonMSFT
Copy link
Member

@jkotas Would these special accessor functions be defined in the assembly containing the type for serialization? I assume so, but I want to make sure I understand the workflow here.

  1. User defines a type.
    class UserData {
      private UserData() { }
      public string Name { get; set; }
    }
  2. There is some gesture that indicates this type is intended to be serialized by a serializer.
  3. A source generator, Roslyn, or user defines extern functions that permit trimmer friendly access to these fields/methods.
    [UnsafeAccessor(".ctor", UnsafeAccessorKind.Constructor)]
    extern UserData CallPrivateConstructor()
  4. Serializers can now access non-public fields/methods using these generated functions that is explicit rather than implicit.

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

Would these special accessor functions be defined in the assembly containing the type for serialization?

Not necessarily. For example, EF Core has situations where the type is defined in one assembly and the source generated serializer lives in a different assembly.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Feb 7, 2023

name parameter in UnsafeAccessorAttribute can be made optional as it is known for constructors and can default to the name of the annotated method for methods.

How would the usage for static methods look like? How the type would be specified? Would it require another parameter for the attribute?

Should UnsafeAccessorKind.Field be called UnsafeAccessorKind.InstanceField for consistency and future-proofness?

Alternatively, whether the target is static could be inferred from the annotated method signature.

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

name parameter in UnsafeAccessorAttribute can be made optional as it is known for constructors and can default to the name of the annotated method for methods.

Good point. Updated the proposal.

name parameter in UnsafeAccessorAttribute can be made optional as it is known for constructors and can default to the name of the annotated method for methods.

I have updated the proposal to say that the first argument has to identify the owning type for both instance and static fields. It will work better with eventual extension for skipping type visibility.

@danmoseley
Copy link
Member

why is this one not extern static?

[UnsafeAccessor(".ctor", UnsafeAccessorKind.Constructor)]
extern UserData CallPrivateConstructor()

@AaronRobinsonMSFT
Copy link
Member

Would these special accessor functions be defined in the assembly containing the type for serialization?

Not necessarily. For example, EF Core has situations where the type is defined in one assembly and the source generated serializer lives in a different assembly.

Alright. If I am reading this correctly this looks like a convenient "macro" for pattern matching, in the JIT for optimizations and in the trimmer for precise trimming, because it is going to be identical to simply using reflection. Is that fair?

@jkotas
Copy link
Member Author

jkotas commented Feb 7, 2023

why is this one not extern static?

Oversight - fixed.

this looks like a convenient "macro" for pattern matching, in the JIT for optimizations and in the trimmer for precise trimming, because it is going to be identical to simply using reflection. Is that fair?

Yep.

@AndriySvyryd
Copy link
Member

I have updated the proposal to say that the first argument has to identify the owning type for both instance and static fields. It will work better with eventual extension for skipping type visibility.

This will make the static calls look awkward as they would need to pass in null with a cast to the target type. Also, how would this help with inaccessible types?

@jkotas
Copy link
Member Author

jkotas commented Feb 8, 2023

This will make the static calls look awkward as they would need to pass in null with a cast to the target type.

This is not meant to look pretty. A lot of this looks awkward. For example, setting the field by calling a method that returns a byref for the field looks awkward too.

Also, how would this help with inaccessible types?

I think that the most straightforward design for inaccessible types would be to use object or TypeReference in the extern method signature, and describe the actual type via attribute on the parameter or return value. Something like this:

[UnsafeAccessor(UnsafeAccessorKind.Method)]
extern static [return:UnsafeAccessorType("InaccessibleTypeA, AssemblyQ")] object MyMethod([UnsafeAccessorType("InaccesibleTypeB, AssemblyW")] @this, [UnsafeAccessorType("InaccesibleTypeC, AssemblyE")] TypeReference argument);

If the owning type was to be specified by some other means, we would need to duplicate the UnsafeAccessorType descriptor there.

@svick
Copy link
Contributor

svick commented Feb 9, 2023

I think this design won't work for accessing members of static classes. For example, if I wanted to access the field System.Console.s_out, I would need to write:

[UnsafeAccessor(UnsafeAccessorKind.Field)]
extern static ref TextWriter s_out(Console @this);

But this won't compile:

error CS0721: 'Console': static types cannot be used as parameters

Though I guess the UnsafeAccessorType approach would work here.

@tannergooding
Copy link
Member

@jkotas, is there any other feedback needed here or is this available to be marked "api-ready-for-review"?

It sounds like there is no language work intended to be done here and so this won't require any cross-team coordination, is that right?

@DaZombieKiller
Copy link
Contributor

Would it make sense to allow defining a void-returning Constructor UnsafeAccessor that takes the this type as the first parameter? This would be useful when you are creating instances with RuntimeHelpers.GetUninitializedObject and need to initialize them later.

class UserData
{
  private UserData() {}
  public string Name { get; set; }
}

[UnsafeAccessor(UnsafeAccessorKind.Constructor)]
extern static void CallPrivateConstructor(UserData @this);
UserData ud = (UserData)RuntimeHelpers.GetUninitializedObject(typeof(UserData));
CallPrivateConstructor(ud);

@hamarb123
Copy link
Contributor

hamarb123 commented Jun 6, 2023

If NonVirtualMethod was approved then it would be able to work with that I would think.

It could be done like this:

[UnsafeAccessor(UnsafeAccessorKind.NonVirtualMethod, Name = ".ctor")]
extern static void CallPrivateConstructor(UserData @this);

@jkotas
Copy link
Member Author

jkotas commented Jun 6, 2023

This would be useful when you are creating instances with RuntimeHelpers.GetUninitializedObject and need to initialize them later.

UnsafeAccessor(UnsafeAccessorKind.Method, Name=".ctor") should work for this case.

UserData ud = (UserData)RuntimeHelpers.GetUninitializedObject(typeof(UserData));
CallPrivateConstructor(ud);

[UnsafeAccessor(UnsafeAccessorKind.Method, Name=".ctor")]
extern static void CallPrivateConstructor(UserData @this);

@AaronRobinsonMSFT We may want to add a test for this case to #86932.

@AaronRobinsonMSFT
Copy link
Member

UnsafeAccessor(UnsafeAccessorKind.Method, Name=".ctor") should work for this case.

It does.

@lambdageek

This comment was marked as resolved.

@jkotas
Copy link
Member Author

jkotas commented Jun 7, 2023

does the ref string keep ud alive and pinned?

Yes, ref string keeps ud alive. It does not keep it pinned. (It would be pinned in Mono due to conservative stack scanning.)

@AaronRobinsonMSFT
Copy link
Member

@jkotas I am going to set this to .NET 8 so we don't forget to move this to .NET 9 when the tag is available.

@sbomer
Copy link
Member

sbomer commented Jul 17, 2023

Question about the use case for this: the unsafe accessors are supposed to be generated by source generators right? How will source generators discover private members that need unsafe accessors, if the private members don't show up in ref assemblies?

@AaronRobinsonMSFT
Copy link
Member

Question about the use case for this: the unsafe accessors are supposed to be generated by source generators right? How will source generators discover private members that need unsafe accessors, if the private members don't show up in ref assemblies?

This isn't about discovery. Source generators (Roslyn and otherwise) often generate source that needs to be private for reason X or Y. In this case those source generator can continue to do what they were designed to do and create private APIs. Discovery isn't the scenario here and shouldn't be performed by the source generator for the ref scenario. This is about providing a mechanism that is akin to the private reflection scenario but with lower overhead. Whether the API is present or not is not the domain of this API, only that is fails in the same way if private reflection was used.

To that end no verification should be performed in any analyzer to validate the target is there if that analyzer consumes ref assemblies.

@sbomer
Copy link
Member

sbomer commented Jul 17, 2023

Thanks, that makes sense! What I was missing is that this isn't designed to be a general solution for all reflection-based serializers, but only for those which don't have to do discovery at runtime in the first place. And from our conversation it sounds like there are cases for example in EF.Core where the schema is known at compilation time, so discovery isn't a problem.

@jkotas
Copy link
Member Author

jkotas commented Jul 18, 2023

How will source generators discover private members that need unsafe accessors, if the private members don't show up in ref assemblies?

Source generators that wish to discover private members in referenced assemblies need to instruct their users to disable use of reference assemblies for compilation, e.g. by setting <CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies>.

vitek-karas added a commit that referenced this issue Jul 18, 2023
The core of the change is that `UnsafeAccessor` creates a code dependency from the accessor method to the target specified by the attribute. The trimmer needs to follow this dependency and preserve the target. Additionally, because the trimmer operates at the IL level, it needs to make sure that the target will keep its name and some other properties intact (so that the runtime implementation of the `UnsafeAccessor` can still work).

Implementation choices:
* The trimmer will mark the target as "accessed via reflection", this is a simple way to make sure that name and other properties about the target are preserved. This could be optimized in the future, but the savings are probably not that interesting.
* The implementation ran into a problem when trying to precisely match the signature overload resolution. Due to Cecil issues and the fact that Cecil's resolution algorithm is not extensible, it was not possible to match the runtime's behavior without adding lot more complexity (currently it seems we would have to reimplement method resolution in the trimmer). So, to simplify the implementation, trimmer will mark all methods of a given name. This means it will mark more than necessary. This is fixable by adding more complexity to the code base if we think there's a good reason for it.
* Due to the above choices, there are some behavioral differences:
  * Trimmer will warn if the target has data flow annotations, always. There's no way to "fix" this in the code without a suppression.
  * Trimmer will produce different warning codes even if there is a true data flow mismatch - this is because it treats the access as "reflection access" which produces different warning codes from direct access.
  * These differences are fixable, but it was not deemed necessary right now.
* We decided that analyzer will not react to the attribute at all, and thus will not produce any diagnostics around it.

The guiding reason to keep the implementation simple is that we don't expect the unsafe accessor to be used by developers directly, instead we assume that vast majority of its usages will be from source generators. So, developer UX is not as important.

Test changes:
* Adds directed tests for the marking behavior
* Adds tests to verify that `Requires*` attributes behave correctly
* Adds tests to verify that data flow annotations behave as expected (described above)
* The tests are effectively a second validation of the NativeAOT implementation as they cover NativeAOT as well.

Fixes in CoreCLR/NativeAOT:
This change fixes one bug in the CoreCLR/NativeAOT implementation, unsafe accessor on a instance method of a value type must use "by-ref" parameter for the `this` parameter. Without the "by-ref" the accessor is considered invalid and will throw.
This change also adds some tests to the CoreCLR/NativeAOT test suite.

Part of #86161.
Related to #86438.
Feature design in #81741.
@tannergooding
Copy link
Member

Is this still being worked on for .NET 8, or can we move it out to .NET 9/future?

@jkotas
Copy link
Member Author

jkotas commented Jul 24, 2023

This work was completed in .NET 8.

@jkotas jkotas closed this as completed Jul 24, 2023
@AaronRobinsonMSFT
Copy link
Member

This work was completed in .NET 8.

Not entirely. None of the runtimes have Generics support - that was documented. CoreCLR and NativeAOT are at parity. Mono has a few work items outstanding.

Anyone looking to follow the additional work-items:
#86161
#86040

@AaronRobinsonMSFT
Copy link
Member

Tracking of Generic support across all runtimes is being tracked with #89439.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests