[API proposal] Attribute for passing caller identity implicitly

[jkotas]

Rationale

Number of libraries, including the new minimal ASP.NET host, use Assembly.GetCallingAssembly() for convenience. Assembly.GetCallingAssembly() is slow and unreliable API, and also not compatible with AOT technologies (e.g. dotnet/runtime#53825). We need a fast, reliable and AOT-friendly replacement of Assembly.GetCallingAssembly().

Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class CallerIdentityAttribute : Attribute
    {
        public CallerIdentityAttribute();
    }
}

This attribute can be applied to parameters of type System.Reflection.Assembly, System.Reflection.Type, System.Reflection.MethodBase. When the C# compiler sees parameter tagged with this attribute, it will provide default value for it, according to the caller context:

• System.Reflection.Assembly: typeof(containing_type).Assembly
• System.Reflection.Type: typeof(containing_type)
• System.Reflection.MethodBase: MethodBase.GetMethodFromHandle(ldtoken containing_method)

Design meetings

• https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-09-22.md#calleridentityattribute
• https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-01-24.md#attribute-for-passing-caller-identity-implicitly

[ghost]

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

Issue Details

---

Rationale

Number of libraries, including the new minimal ASP.NET host, use Assembly.GetCallingAssembly() for convenience. Assembly.GetCallingAssembly() is slow and unreliable API, and also not compatible with AOT technologies (e.g. dotnet/runtime#53825). We need a fast, reliable and AOT-friendly replacement of Assembly.GetCallingAssembly().

Proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class CallerIdentityAttribute : Attribute
    {
        public CallerIdentityAttribute();
    }
}

This attribute can be applied to parameters of type System.Reflection.Assembly, System.Reflection.Type, System.Reflection.MethodBase. When the C# compiler sees parameter tagged with this attribute, it will provide default value for it, according to the caller context:

• System.Reflection.Assembly: typeof(containing_type).Assembly
• System.Reflection.Type: typeof(containing_type)
• System.Reflection.MethodBase: MethodBase.GetMethodFromHandle(ldtoken containing_method)

Author:	jkotas
Assignees:	-
Labels:	area-System.Runtime
Milestone:	-

[jkotas]

@jaredpar Thoughts on this proposal?

cc @davidfowl

[davidfowl]

So in the case of ASP.NET Core, it would look like this:

public class WebApplication
{
    public static WebApplicationBuilder CreateBuilder(string[] args, [CallerIdentityAttribute]Assembly assembly = null);
}

Usage:

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

app.MapGet("/", context => context.Response.WriteAsync("Hello World!"));

app.Run();

The compiler would do this (since we're in Main):

var builder = WebApplication.CreateBuilder(args, typeof(Program).Assembly);

var app = builder.Build();

app.MapGet("/", context => context.Response.WriteAsync("Hello World!"));

app.Run();

That's slick. I like it.

[jkotas]

We should consider marking Assembly.GetCallingAssembly as obsolete as part of this feature.

[stephentoub]

Should this be in csharplang?

[jkotas]

Agree. This should start in csharplang.

[HaloFour]

Also see: #87

If those attributes were collapsed into a single attribute that passed a CallerInfo-like struct and the compiler were to pass the runtime method handle in that struct you should be able to derive the assembly from the declaring type of the calling method.

[jkotas]

I expect that we will frequently need to add trimming annotations like DynamicallyAccessedMembersAttribute to this argument. CallerInfo-like struct would not work well with these trimming annotations.

[jaredpar]

Overall this looks pretty reasonable to me.

For the Type and MethodBase values do we want to emit the actual types / methods in which the call occurs or the declared types. Consider as a concrete example the following:

class Program {
    static void Main() {
      PrintMe();
      var x = () => PrintMe(); 
      x();
   }

    static void PrintMe([CallerIdentityAttribute]Type type = null) => Console.WriteLine(type.FullName);
}

Does this print

Program
Program

Or

Program
<generated type name>

[davidfowl]

Good question. I think it depends on the usage. Should this work?

class Program {
    static void Main() {
      PrintMe();
      var x = () => PrintMe(); 
      x();
   }

    static void PrintMe([CallerIdentityAttribute]Type type = null) 
    {
       var m = type.GetMethod(nameof(PrintMe), BindingFlags.Public | BindingFlags.Static); // Does this return null?
    }
}

[jaredpar]

var m = type.GetMethod(nameof(PrintMe), BindingFlags.Public | BindingFlags.Static); // Does this return null?

That is explicitly undefined 😄.

The compiler does not specify how lambdas are emitted, except in special cases, because we want to be free to change it between versions. Hence the relationship in terms of metadata between PrintMe and the method generated for x is not defined. If the code does work in one version it doesn't imply it will work in the next version.

I know that often the language / framework says an item in metadata is undefined but in reality it never changes hence developers feel safe in taking dependencies on it. Lambda metadata is one area we've changed several times though and broken developers who took dependencies on it.

[jkotas]

do we want to emit the actual types / methods in which the call occurs or the declared types

I think it makes more sense to emit the declared types/methods, given that it is not specified how lambdas and other similar constructs are emitted.

[jkotas]

To make example in #4984 (comment) work as written, how are we going to retrofit the existing method to use the new pattern?

public class WebApplication
{
    // Existing method
    public static WebApplicationBuilder CreateBuilder(string[] args);
    // New method
    public static WebApplicationBuilder CreateBuilder(string[] args, [CallerIdentityAttribute] Assembly assembly = null);
}

I assume that WebApplication.CreateBuilder(args) is going to bind to the existing method. What is the best way to switch the existing code to the overload that uses this feature?

[jaredpar]

Believe we had the same overload issue with CallerArgumentExpression but not sure exactly what conclusion we came to. @333fred should remember

[stephentoub]

Believe we had the same overload issue with CallerArgumentExpression but not sure exactly what conclusion we came to

Yeah, it'd be nice if you had:

Debug.Assert(condition);

that we could change the method:

public static void Assert([DoesNotReturnIf(false)] bool condition, string? message)

to instead be:

public static void Assert([DoesNotReturnIf(false)] bool condition, [CallerArgumentExpression("condition")] string? message = null)

such that the compiler would emit:

Debug.Assert(condition, "condition");

Unfortunately, we already have the void Assert(bool condition) overload, so the compiler will bind to the "wrong" one.

[davidfowl]

So we need to add this overload now (to WebApplication) so we can make this work in the future.

[stephentoub]

So we need to add this overload now (to WebApplication) so we can make this work in the future.

We already have the existing overload. How does adding the new overload now help (assuming the attribute would have similar semantics to the existing attributes in terms of overload resolution)?

[davidfowl]

I mean for WebApplication (a new API being added by ASP.NET Core), not Debug.Assert

[stephentoub]

I mean for WebApplication (a new API being added by ASP.NET Core), not Debug.Assert

Ok, but if you add both:

M(string[] args);
M(string[] args, Assembly? assembly = null);

today, calls to M(args) are going to bind to M(string[] args) today, and in the future when the latter argument is attributed, the compiler will still prefer to bind to M(string[] args).

If what you mean is you would need to add M(string[] args, Assembly? assembly = null) and not add (or remove if it's already added but hasn't shipped) M(string[] args), then yes.

[davidfowl]

If what you mean is you would need to add M(string[] args, Assembly? assembly = null) and not add (or remove if it's already added but hasn't shipped) M(string[] args), then yes.

This is what I meant, yes

[jaredpar]

The overload issue is a problem here because we're effectively stuck with the overload we don't want to be used anymore. With Debug.Assert / [CallerInfoExpression] it was more of a convenience problem. We'd really like to use it in Debug.Assert but the product can move forward without it. With [CallerIdentity] it could be important to the success of AOT. Basically if we can't move existing APIs to leverage this feature then it seemingly puts AOT at a bit of risk. That ups the stakes a bit.

The imperfect solution I'd been thinking of in my head was ... yet another feature!!! 😄

Essentially imagine an attribute named [BinaryCompatOnlyAttribute]. When a method is marked with this attribute then the compiler will never include it in a candidate set for purposes of overload resolution. The method exists purely for binary compatibility reasons.

That would allow us to solve this problem rather easily:

public class Debug {  
  [BinaryCompatOnly]
  public static void Assert([DoesNotReturnIf(false)] bool condition) { ... }
  public static void Assert([DoesNotReturnIf(false)] bool condition, [CallerArgumentExpression("condition")] string? message = null) { ... }

The burden would be on the API author to ensure they didn't create source breaks when doing this. For the majority use case here though that works out just fine. Given that the extra parameters are optional and we fill in the values existing invocations will just work.

The problem comes in though when we get into method group conversions.

Action<string> action = Debug.Assert;

That would not work because the new overload isn't compatible with Action<string>. That is where this idea starts to fall apart. I thought a bit about saying we only consider [BinaryCompatOnly] for method invocation, not method group conversions but that seems wrong + incomplete.

[alrz]

Action<bool> action = Debug.Assert; is already an error because it's conditional.

[333fred]

Essentially imagine an attribute named [BinaryCompatOnlyAttribute]. When a method is marked with this attribute then the compiler will never include it in a candidate set for purposes of overload resolution. The method exists purely for binary compatibility reasons.

That's a fascinating idea, and I'd want to look at it in conjunction with other ideas that could affect overload resolution. We've had other ideas around attributes that could serve as tiebreakers if two APIs would otherwise be considered ambiguous, and this seems like it might either be a good proving ground for such an attribute, or a good thing to bundle at the same time.

[jaredpar]

Action action = Debug.Assert; is already an error because it's conditional.

In my defense, I've only been back from vacation for three hours now. 😄

[svick]

What if the old overload was only kept in runtime assemblies, but removed from reference assemblies? I think this would mean switching to the new overloads with just recompilation, while maintaining both binary and source compatibility (except for method group conversions, as mentioned above).

It's only a practical solution for large libraries (where it's worth having separate reference assemblies), but that's also where compatibility matters the most.

We've had other ideas around attributes that could serve as tiebreakers if two APIs would otherwise be considered ambiguous

For reference, #4867 is a recent discussion on that topic.

[jaredpar]

What if the old overload was only kept in runtime assemblies, but removed from reference assemblies?

That is a viable option but only in a specific subset of scenarios. Mostly when the code is written in C# and it involves method invocation. It becomes unviable in C# method group conversions, F# (likely at least because they have diff overload resolution and method group rules), when the method is an instance method on a non-sealed type, etc ...

The instance method on an unsealed type is the easiest deal breaker. Have to assume that someone derived from you and bound that method to an interface implementation.

[alrz]

It would be still a problem if it was NOT conditional though.

To be fair, at the moment, overloading M(object) with M(object, [AnyCallerInfo] string = null) is just wrong because the compiler is expected to fill in caller info arguments (read "optional args with special semantics").

So the only reason for this to exist is to maintain backward compatibility.

I'd say we should never remove those from the candidate set and prefer the ones with caller info because, as far as the compiler concerns, those are "more specific" overloads since we know how to provide the missing arg(s) with proper values.

[TonyValenti]

In that case, I'd recommend three different attributes. CallerAssemblyInfo, TypeInfo, MethodInfo.

[bradwilson]

In the case of xUnit.net, we'd probably also want to hide the version that has all the extra junk on it, because the extra junk causes usability problems (and, specifically, I mean in concert with [CallerArgumentExpression], but it would apply to this scenario as well). So it'd be more like this for us:

public class Assert {
  [BinaryCompatOnly]
  public static void Equal<T>(T expected, T actual) { ... }

  [EditorBrowsable(false)]
  public static void Equal<T>(
    T expected,
    T actual,
    [CAE("expected") string? expectedExpression = null,
    [CAE("actual") string? actualExpression = null) { ... }

I assume this would give us the "illusion" in usability of the end user only ever seeing Assert.Equal<T>(T, T) but the call only ever actually ending up calling Assert.Eqwual<T>(T, T, string? string?), yes?

(Related discussion: xunit/xunit#1596)

[agocke]

@HaloFour Fair. It's probably robust to just have the compiler fill in null for cases where the answer isn't well defined.

[uecasm]

Outside of caller parameters, and more in line with #6041 (which was closed as a duplicate of this, hence posting here, though it does seem not entirely related), I often find myself needing the assembly name of the current type -- for example, to compose RCL wwwroot content paths of the form $"/_content/{AssemblyName}/some/sub/path.ext". It irks me that this is only available through reflection and not a compile-time const value, unless I hard-code a value which may become incorrect and introduce bugs due to a later rename or move refactoring.

[am11]

With MethodInfo, the callee can still obtain the type and assembly of the caller. (i.e. injectedMI?.DeclaringType?.Assembly)

[rickbrew]

I have an additional scenario/request for this feature, but I'm not sure it's an easy or even reasonable one, but I'd like to at least describe the scenario so it's on the radar. I am not using NativeAOT nor trimming in this context. Just crossgen/R2R.

This is in the context of Paint.NET's plugin systems and APIs. I very occasionally have a need to establish the identity of the calling method -- but, I also want to make sure that the caller cannot lie, spoof, or just get it wrong, and it'd be nice to not add additional visible complexity to things.

My proposal is to add a SkipFrames property onto the CallerIdentityAttribute so I can get the caller-of-the-caller. I think it's sufficient if this is limited to 0 or 1, it doesn't need the capability to go back 10 frames or whatever (so really it could be bool SkipOneFrame instead of an int).

public sealed class ClassThatPluginsUse
{
    // Maybe this would be required to have [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public ClassThatPluginsUse() : this((object?)null) { }

    private ClassThatPluginsUse(object? ignored, [CallerIdentity(SkipFrames = 1)] Assembly? callingAssembly = null)
    {
        // ... do something that depends on who the caller is
    }
}

I want the plugin to do new ClassThatPluginsUse(), which will call the other constructor, and then the SkipFrames property on the attribute will give me the identity of what called the parameterless constructor (i.o.w., the plugin). This permits me to get the identity of the plugin in a way that the plugin cannot spoof or get wrong. Today I would have to use Assembly.GetCallingAssembly() and [MethodImpl(MethodImplOptions.NoInlining)], which feels hacky and fragile.

I'm not going to make the argument that this would be a security feature 😂 This about wanting to be able to change behavior based on the app version that the plugin is compiled against. Call it a "plugin version contract" if you will. I can then call into callingAssembly.GetReferencedAssemblies() and dig through that list to see what version of (e.g.) PaintDotNet.Effects.dll the plugin was compiled against. This will allow me to version my plugin API in ways that are currently very difficult or impossible to achieve otherwise. This isn't something I need to use often, but when I need it I really need it.

I currently have a need for this in an API where I forgot to check a constructor parameter for null. This parameter is passed to the base class' private-protected constructor which does permit null. The proper way to get null down to that base constructor is to use the derived class' parameterless constructor:

public sealed class TabContainerControlInfo
    : StatefulControlInfo<TabContainerState, TabContainerStateProperty>
{
    // The private-protected base class constructor accepts `null` and this is normal. This parameterless constructor is the right way to do that.
    public TabContainerControlInfo()
        : base((TabContainerStateProperty?)null)
    {
    }

    // But I forgot to do a null-check here on the stateProperty parameter
    public TabContainerControlInfo(TabContainerStateProperty stateProperty)
        : base(stateProperty) // should be e.g. `stateProperty ?? throw new ArgumentNullException(nameof(stateProperty))`
    {
    }
    ...

This has resulted in some confusion and a bunch of wasted debugging time by plugin authors who were then getting a NullReferenceException later when they were using this TabContainerControlInfo in another part of the plugin system. You'll note that the constructor parameter is not annotated with a ?, so why didn't the compiler flag this? Well, these plugin authors are not enabling #nullable because ... they don't care, they don't know about it, they don't want to, they don't like it, etc. That part of the gospel has not reached them yet, in other words, and I can't force it upon them.

But I can't add the null check here without breaking the 10+ plugins that are already using this API incorrectly. Those plugins work fine today because they weren't using the TabContainerControlInfo in the other plugin APIs that would need the TabContainerStateProperty to be non-null (I realize this is confusing). These plugins should just be using the parameterless constructor. They're doing it the wrong way because they were following a tutorial that was doing this wrong, and the tutorial was wrong because it was carved out of another tutorial that did need to use the other plugin APIs that needed TabContainerStateProperty to be non-null (and it was doing all of that correctly).

I want to add the null-check, but only if the caller (the plugin) was compiled against the new version of my app. It's very hard to achieve this otherwise; I prototyped a system where I would use a ThreadLocal and some other Rube-Goldberg apparatuses and it kinda works but is fragile and also doesn't work in certain transitive cases (e.g. a plugin directly instantiating another plugin -- which is possible).

An alternative way of versioning this API would be to [Obsolete] the constructors and require folks to use a static Create method, but that's clunky and inconsistent -- everything else in this area is using constructors, not factory methods.

In Paint.NET I try to maintain strict binary compatibility for plugins, and there are plugins that are almost 20 years old that still work fine today. This is the best experience for the end-user/customer: breaking a plugin to correct an API does not really affect the plugin author, it only makes things worse for the end-user/customer by breaking their work/creative-flow. Having additional tools for versioning the plugin APIs makes it easier for me to uphold all of this.

[333fred]

@rickbrew I'm not sure how that would work though. This attribute, like all other caller info attributes, would need to be filled in at the call site by the compiler. How would the compiler insert the info from 2 frames up? And moreover, as with all caller info attributes, the user can simply provide whatever value they want manually if they so desire, so it really wouldn't be spoof-free either. It very much feels like, to get the functionality you want, you need to actually do the stack walk with a StackTrace to derive the info that way.

[rickbrew]

@rickbrew I'm not sure how that would work though. This attribute, like all other caller info attributes, would need to be filled in at the call site by the compiler. How would the compiler insert the info from 2 frames up? And moreover, as with all caller info attributes, the user can simply provide whatever value they want manually if they so desire, so it really wouldn't be spoof-free either. It very much feels like, to get the functionality you want, you need to actually do the stack walk with a StackTrace to derive the info that way.

Right, I understand -- but note that the constructor taking the Assembly is private, and I did also note in the code above that the middle-man constructor might be required to have [MethodImpl(MethodImplOptions.AggressiveInlining)]. Otherwise I don't think it's possible for the JIT or crossgen to have the right information to work with.

[rickbrew]

Also, by describing my scenario it may result in someone realizing "oh that can actually be achieved in this other way that is feasible to add support for".

[333fred]

[MethodImpl(MethodImplOptions.AggressiveInlining)]

This doesn't seem to help though? What is the compiler actually going to insert at the call site of the method?

[rickbrew]

[MethodImpl(MethodImplOptions.AggressiveInlining)]

This doesn't seem to help though? What is the compiler actually going to insert at the call site of the method?

I don't know I'm not really a compiler person ¯\(ツ)/¯ I'm not really trying to propose an implementation strategy, and I did note that I didn't know if it was feasible. I thought the scenario could be an interesting one to chew on. It's not unheard of for an API to forget to validate parameters but in a way that doesn't cause problems because they're passed on to other APIs where those parameter values are okay.

[rickbrew]

I'm also describing this because it seems there's a push to deprecate things like Assembly.GetCallingAssembly(), and I would be surprised if StackTrace wasn't in the crosshairs too, which means having alternatives that are compatible with modern .NET ideology (e.g. NativeAOT, trimming, reflection-free) could be important.

[MichalStrehovsky]

I'm also describing this because it seems there's a push to deprecate things like Assembly.GetCallingAssembly(), and I would be surprised if StackTrace wasn't in the crosshairs too, which means having alternatives that are compatible with modern .NET ideology (e.g. NativeAOT, trimming, reflection-free) could be important.

The problem with GetCallingAssembly is that it will do a stackwalk at runtime (same way as StackTrace) and give you the assembly of the calling previous frame. It may not be the assembly that called the current method (e.g. if the method got tail called, or the caller got inlined into something else). The API is basically just a trap that will look like it works most of the time, but then it doesn't. The "then it doesn't" part is going to be aggravated by tiered JIT and non-deterministic optimizations and those bugs are not fun bugs. This API can really only be used if you don't actually care about the result.

The API proposed here would be guaranteed to give the caller (unless the caller goes out of the way to spoof this but then ¯(ツ)/¯). The SkipFrames extension would just reintroduce the same problem because it would have to do a stack walk at runtime.

There are no plans to deprecate StackTrace. It can still be used wrong (like someone could use it to interpret it as a call chain and fall into the same trap as GetCallingAssembly), but this is a diagnostic API and returns what it says it returns - the current stack trace.

[JonathanBout]

It's been a year now. Is any progress made on this?

[CyrusNajmabadi]

It's been a year now. Is any progress made on this?

No. When we make progress we update the issues and discussions on the topic.