Proposal: CallerArgumentExpressionAttribute

[jamesqo]

Rationale

In order to facilitate the language feature at dotnet/csharplang#287, which has been championed and discussed in the LDM, we should add CallerArgumentExpressionAttribute to the framework. The LDM notes seem to approve of getting this into the framework: see here.

Proposal

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

        // Added in response to @svick's comment
        public string ParameterName { get; }
    }
}

Notes

• The name of the constructor argument is intentionally parameterName and not argumentName. Parameters refer to the variables declared inside the callee, while arguments are the values passed by the caller.

[jamesqo]

cc'ing people from the csharplang threads (dotnet/csharplang#287 and dotnet/csharplang#205): @gafter, @CyrusNajmabadi, @HaloFour, @ig-sinicyn, @tannergooding, @svick, @jnm2

[jjvanzon]

Cool proposal. I have made several frameworks in the past to accomplish something similar, with either less performance or more verbose notation, that might become redundant by this.

https://github.com/jjvanzon/JJ.Framework/tree/master/Framework/Exceptions
https://github.com/jjvanzon/JJ.Framework/tree/master/Framework/Testing

Those are not even the only ones!

[svick]

Should the parameterName also be exposed as a public property? I'm not sure how useful would that actually be (compilers are probably going to directly read the metadata, they're not going to instantiate the attribute object), but I think it's consistent with the rest of BCL. It's also consistent with Framework Design Guidelines.

[jamesqo]

@svick Sure, why not. I think such a property may even have a practical use for reflective code that wants to determine the parameter being referenced. (Although, that may already be possible (in a slightly less cleaner way) through this API which allows you to get the ctor arguments of an attribute.) Updated the proposal.

[jamesqo]

@karelz, I think this api is safe to mark ready-for-review. The C# team has already approved this and explicitly said we should start the process for getting this in, see here. The addition of this api to corefx is what is causing them to push this feature back to 7.x, I do not think we should risk further delays.

[HaloFour]

@CyrusNajmabadi

What of the other caller info attributes?

[karelz]

@AlexGhiondea @joperezr are area owners and should do first-level API review. If they are happy with it, they will be able to mark it 'api-ready-for-review'.

[jamesqo]

@AlexGhiondea @joperezr Please consider marking this ready-for-review: the C# language design team has already approved of it, and this attribute needs to get into the fx for them to take any further action.

[joperezr]

proposal looks reasonable. Marking as ready for review.

[jamesqo]

@karelz When are you planning to do an API review for this issue? There seem to be quite a few issues-- some since January-- marked ready-for-review that are not approved yet.

[karelz]

We have a small backlog accumulated over the summer. Except one special-case exception, all api-ready-for-review issues were marked as such after 6/24.
We are slowly chewing through our backlog every Tuesday (we have 1-2h slot for GitHub triage, with exception of next week when we will focus on larger API review of System.Threading.Tasks.Channels the whole 2 hours).

So far I have been trying to bundle API review issues into similar blocks (Reflection last week, sadly the recording of the session failed to record properly), and prioritize the easy ones. This one should be part of next badge in ~10 days or the week after.
Does it help?

[jamesqo]

@karelz Yeah, it helps. Thanks for the update! 😄

[karelz]

API review: Looks good as proposed, but we should wait for final language feature.
We do not want to ship new API which won't be used by the language feature.

In general, I would recommend to not file API proposals ahead of LDM being further down the road.

[jamesqo]

@karelz I don't think it makes sense to mark this as blocked. The compiler team has said that they are waiting on the type to be added before they will discuss it further... it's like an issue deadlock.

[gafter]

I expect the compiler team to add the relevant language feature in the next release (minor or major) following the addition of this supporting attribute.

/cc @jcouv

[karelz]

@gafter if you can confirm the API shape fits Roslyn needs, we can unblock the API addition in 2.1.

[jcouv]

@karelz From discussion with LDM over email, the API shape was confirmed to be good. I forwarded you the thread. Thanks!

[karelz]

Unblocked, it is ready to roll. Any takers? -- should be fairly straightforward to add

[jcouv]

Tagging @alrz who is working on compiler change to see if he could implement the type as well.

[alrz]

I didn't find other caller info attributes in this repo. sounds like System.Private.CoreLib.csproj is private?

[jcouv]

@alrz The implementation lives in corert (example).

I'm not sure whether adding the new attribute to corert is sufficient for it to appear (or whether some change in corefx is also needed). @karelz can clarify.

[jamesqo]

From discussion with LDM over email, the API shape was confirmed to be good. I forwarded you the thread. Thanks!

WOOT! Been waiting for this for so long 🎉

[jcouv]

@jamesqo glad to hear. I must say there is one more language issue that came up afterwards: how can existing APIs adopt the attribute? Adding an optional string parameter causes problems in typical methods we're thinking of (such as asserts).

[jamesqo]

@jcouv Just gave you a really long response about binary compatibility on the csharplang issue.

[karelz]

Marking as blocked again, until we resolve the language feature.

[jamesqo]

@karelz I think it's resolved. They said there wouldn't be a binary compatibility issue.

[jcouv]

Yes, here are the relevant LDM notes. Thanks

[jcouv]

Removed the "blocked" label accordingly.

[jswolf19]

It looks like this API is approved, but how it'll actually manifest is still under debate. However, if putting this in'll get things moving, this is something I'd like to see.

I'm not sure where this would be added in corefx, though. There are some CompilerServices Attributes in Common\src\CoreLib\System\Runtime\CompilerServices, but these files are just for including in other projects, if I understand correctly.

I'd be glad to add it if I know where to ^_^

[karelz]

@jswolf19 I didn't read the LDM discussion carefully. My naive understanding was that it is ready to be added into CoreFX.
CoreLib is shared with CoreCLR and CoreRT repos. There are members of the namespace which are in various ref assemblies (see search). I am not quite sure where this one would go.

@danmosemsft would probably know more ...

[jswolf19]

@karelz
I got the impression from the discussion at csharplang issue 287 that the general consensus is to apply this to a string argument and supply a parameter name, but there was some discussion of dealing with multiple arguments, using expressions over strings, or some other method which might affect the makeup of this class.

So, if I add to the Common\src\CoreLib\System\Runtime\CompilerServices, it'll eventually make its way to the CoreCLR and CoreRT repos?

[karelz]

I admit I don't know which way the mirror goes :( ... either check history of such additions (I suspect CoreCLR is the primary source), or wait for someone knowledgeable to chime in ...

[jswolf19]

The LDM linked is newer than much of the discussion at the issue, so it would seem that this API is what has gotten consensus. I'll see what I can find as far as addition history.

[jswolf19]

@karelz @danmosemsft Looking at IntrinsicAttribute, it would appear that CoreCLR is the start point, so I'll look at adding this there?

CoreCLR
CoreFX
CoreRT

[jswolf19]

So this is what I'm perceiving the order of things that needs to get done is. Please let me know if I should do things differently, otherwise, I'll submit the CoreCLR pull request this week.

• Add CallerArgumentExpressionAttribute to System.Private.CoreLib (CoreCLR)
• CoreCLR gets merged to CoreFX (by someone else)
• Add CallerArgumentExpressionAttribute to the ref assembly in System.Runtime (CoreFX)
• Add a simple test showing usage of CallerArgumentExpressionAttribute asserting that it shows up in the reflection metadata in System.Runtime.Tests (CoreFX)
• Add disabled tests for the expected result once the compiler handles the CallerArgumentExpressionAttribute (CoreFX)

For the last step, I have questions about whitespace handling and corner cases (see below). If there are any other cases that I should include, please let me know.

void IntParamMethod(int val, [CallerArgumentExpression("val")] string expr = null) {...}

IntParamMethod(x); // "x"
IntParamMethod( x ); // "x"
IntParamMethod(x, "y"); // "y"
IntParamMethod( x  + 20); // "x  + 20" (2 spaces after x)

void NullOptionalParamMethod(string val = null, [CallerArgumentExpression("val")] string expr = null) {...}

NullOptionalParamMethod(); // "null"

void ConstOptionalParamMethod(string val = StringConst + "string literal", [CallerArgumentExpression("val")] string expr = null) {...}

ConstOptionalParamMethod(); // "StringConst + \"string literal\""

void CompilerSuppliedParamMethod([CallerFilePath()] string val = "", [CallerArgumentExpression("val")] string expr = null) {...}

CompilerSuppliedParamMethod(); // "\"\"" (???)
CompilerSuppliedParamMethod("not file path"); // "\"not file path\""

void ExtensionMethod(this int val, [CallerArgumentExpression("val")] string expr = null) {...}

Math.Min( x  + 20, //comment
          x * x
        ) . ExtensionMethod(); // "Math.Min( x  + 20, //comment\r\n          x * x\r\n        )" (??? assuming windows crlf)

void IntParamMethod(int val, [CallerArgumentExpression("notVal")] string expr = null) {...}

IntParamMethod(x); // "" as per proposal.

void IntParamMethod(int val, [CallerArgumentExpression("this")] string expr = null) {...}

IntParamMethod(x); // "" ?
this.IntParamMethod(x); // "this" ?

[danmoseley]

@jswolf19 somehow I missed this issue. Yes your sequence looks correct. Note the mirroring you care about from coreclr to corefx is the insertion of the coreclr SHA not the mirroring of coreclr source code (which is for code reuse)

[danmoseley]

Your questions about test expectations are msybe questions for Roslyn repo since they will be producing the result.

[jamesqo]

@jswolf19 I've answered your questions in parentheses below:

void IntParamMethod(int val, [CallerArgumentExpression("val")] string expr = null) {...}

IntParamMethod(x); // "x" (yes)
IntParamMethod( x ); // "x" (yes)
IntParamMethod(x, "y"); // "y" (yes)
IntParamMethod( x  + 20); // "x  + 20" (2 spaces after x) (yes)

void NullOptionalParamMethod(string val = null, [CallerArgumentExpression("val")] string expr = null) {...}

NullOptionalParamMethod(); // "null" (yes)

void ConstOptionalParamMethod(string val = StringConst + "string literal", [CallerArgumentExpression("val")] string expr = null) {...}

ConstOptionalParamMethod(); // "StringConst + \"string literal\"" (yes)

void CompilerSuppliedParamMethod([CallerFilePath()] string val = "", [CallerArgumentExpression("val")] string expr = null) {...}

CompilerSuppliedParamMethod(); // "\"\"" (yes)
CompilerSuppliedParamMethod("not file path"); // "\"not file path\"" (yes)

void ExtensionMethod(this int val, [CallerArgumentExpression("val")] string expr = null) {...}

Math.Min( x  + 20, //comment
          x * x
        ) . ExtensionMethod(); // "Math.Min( x  + 20, //comment\r\n          x * x\r\n        )" (??? assuming windows crlf) (yes)

void IntParamMethod(int val, [CallerArgumentExpression("notVal")] string expr = null) {...}

IntParamMethod(x); // "" as per proposal. (yes)

(no: using CAE to find `this` expression will not be supported for instance methods, only extension methods)
void IntParamMethod(int val, [CallerArgumentExpression("this")] string expr = null) {...}

IntParamMethod(x); // "" ? (yes: will be "" since "this" is not recognized)
this.IntParamMethod(x); // "this" ? (no: will be "" since "this" is not recognized)

(note: for `ExtensionMethod(this int val, string expr = null)`, if it's possible to call it like `ExtensionMethod()` without prefixing with `this.` then the expression passed should be "".
 not sure if this is possible since extension methods normally require `this.` to call them even from the class they target, but if it is that should be the behavior)

[jswolf19]

@jamesqo Thanks for your reply. There's an inline comment in the ExtensionMethod example, so I'm guessing that should not be included? (2 instead of 1 below)

Math.Min( x  + 20, //comment
          x * x
        ) . ExtensionMethod(); // assuming windows crlf

//1. "Math.Min( x  + 20, //comment\r\n          x * x\r\n        )"
//2. "Math.Min( x  + 20, \r\n          x * x\r\n        )"

(note: for ExtensionMethod(this int val, string expr = null), if it's possible to call it like ExtensionMethod() without prefixing with this. then the expression passed should be "".
not sure if this is possible since extension methods normally require this. to call them even from the class they target, but if it is that should be the behavior)

It doesn't appear to be possible in c#, but I'm pretty sure it is possible in vb.

[CyrusNajmabadi]

@jamesqo Thanks for your reply. There's an inline comment in the ExtensionMethod example, so I'm guessing that should not be included? (2 instead of 1 below)

It should be included (IMO). The compiler will simply give you the unprocessed span of the code from the start of the first token in the arg to the end of the last token in the arg. All intermediate 'trivia' (including comments, whitespace, newlines, and even preprocessor directives) would be included.

[CyrusNajmabadi]

Note: in Roslyn parlance, this would be equivalent to calling: sourceText.GetText(argNode.Expression.Span)

Note: another important case to consider is:

Foo(ref SomeArg)

Presumably we want "SomeArg" on the other end of that, not "ref SomeArg". This is why my Roslyn-parlance code is sourceText.GetText(argNode.Expression.Span) and not sourceText.GetText(argNode.Span)