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

Champion "CallerArgumentExpression" #287

Open
4 of 5 tasks
gafter opened this issue Mar 19, 2017 · 82 comments
Open
4 of 5 tasks

Champion "CallerArgumentExpression" #287

gafter opened this issue Mar 19, 2017 · 82 comments
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Soon candidate
Milestone

Comments

@gafter
Copy link
Member

gafter commented Mar 19, 2017

Allow developers to capture the expressions passed to a method, to enable better error messages in diagnostic/testing APIs and reduce keystrokes.

See #205

@gafter gafter self-assigned this Mar 19, 2017
@HaloFour
Copy link
Contributor

If expansion of the supported caller info attributes is going to be discussed in the LDM is there a chance that some quantity of #87 be mentioned as well?

@CyrusNajmabadi
Copy link
Member

@HaloFour I think that would be reasonable. If we're going to do more in this space, we probably want to do it all at once.

@jamesqo
Copy link
Contributor

jamesqo commented Mar 21, 2017

Related corefx proposal that would take advantage of this feature: dotnet/corefx#17068

@jamesqo
Copy link
Contributor

jamesqo commented Mar 21, 2017

@CyrusNajmabadi

I think that would be reasonable. If we're going to do more in this space, we probably want to do it all at once.

Do I need to modify the proposal I'm working on to include his suggestions, or will they be reviewed just by looking at the issue?

@gafter
Copy link
Member Author

gafter commented Mar 22, 2017

My intent is to bring up this one request to the design group. It sounds like @CyrusNajmabadi will bring up the others in that discussion.

@alrz
Copy link
Contributor

alrz commented May 1, 2017

Related: #443

void M(object param, [CallerArgumentExpression(nameof(param))] string arg = null) {}

@jamesqo
Copy link
Contributor

jamesqo commented May 15, 2017

@alrz I agree with the proposal, but it isn't strictly necessary for this to move forward. The end user will not have to touch this attribute. Only a handful of methods in the framework will use it, and for those it's fine to simply hard-code the string.

@jamesqo
Copy link
Contributor

jamesqo commented Jul 1, 2017

Great to see that this has been discussed in the LDM, I wish I'd commented just 2 days later so I would have seen that 😄 I see this is the text for the CallerArgumentExpression section:

Push to 7.X and start process for getting the attribute in place.

Does that mean I should open a proposal in the corefx repo?

@jamesqo
Copy link
Contributor

jamesqo commented Jul 2, 2017

I opened a corefx proposal for this, see https://github.com/dotnet/corefx/issues/21809.

@paulomorgado
Copy link

Call me strange, but I still don't feel comfortable with optional arguments in public APIs.

To the proposal in question, the callee shouldn't be required to know anything about the caller. And the caller should know what it was doing just by the call stack of the exception. Otherwise, you have worst problems in your hands and this is not the solution.

@alrz
Copy link
Contributor

alrz commented Jul 3, 2017

Caller attributes are not a new concept so I think we should be past that argument by now.

@yaakov-h
Copy link
Member

yaakov-h commented Jul 3, 2017

How would this work on extension methods?

I'm thinking specifically of something like Shouldly here.

@jamesqo
Copy link
Contributor

jamesqo commented Jul 3, 2017

@yaakov-h Oh wow, I hadn't even thought about this feature working in that way! I guess it'll do basically the same thing for extension methods-- capture everything before the dot that represents the expression is being invoked on. e.g. Off the top of my head:

public static class FooExtensions {
    public static void Bar(this Foo f, [CallerArgumentExpression("f")] string fExpression = null) {}
}

(this as Foo).Bar(); // fExpression = "(this as Foo)"

I have no idea how Shouldly works (I just read the README and it seems like dark magic), but it should be able to use CAE to get the string before the dot instead of whatever they use.

@yaakov-h
Copy link
Member

yaakov-h commented Jul 4, 2017

@jamesqo it's definitely dark magic: it walks the stack, interprets the PDBs, then interprets the source code file. 😄

Thanks for the spec update, looks reasonable to me. 👍

@alrz
Copy link
Contributor

alrz commented Jul 4, 2017

Debug.Assert can't take advantage of this feature, because there is already an overload that takes an string,

public static void Assert(bool condition);
public static void Assert(bool condition, string message);

If you make message an optional parameter, the compiler would still prefer the first overload.

I didn't come up with a solution to light up CallerArgumentExpression without losing source compatibility.

@jamesqo
Copy link
Contributor

jamesqo commented Jul 4, 2017

@alrz We can maintain source compatibility. However, we may have to break binary compatibility by removing the first overload.

@alrz
Copy link
Contributor

alrz commented Jul 4, 2017

Maybe you could do it in a library you use but that's simply not an option for Debug.Assert.

@paulomorgado
Copy link

There could be an helper type that takes all the values in the constructor as optional arguments:

public static void Assert(AssertContext context, bool condition);
public static void Assert(AssertContext context, bool condition, string message);

@alrz
Copy link
Contributor

alrz commented Jul 4, 2017

Maybe we could do this with a static extension method in "extention everything" feature,

namespace System.Diagnostics {
  public extension class DebugExtensions for Debug {
    public new static void Assert(bool condition,
      [CallerArgumetExpression("condition")] string message = default) { .. }
  }
}

If we were able to define new extension methods to hide the original, we're there.

To make it opt-in we could move the extension to another namespace like System.Diagnostics.Extensions.

@paulomorgado
Copy link

@alrz, are you expecting the extension method to win over the class method?

@benfoster
Copy link

Are there plans to provide a shim package for .NET Standard?

@333fred
Copy link
Member

333fred commented Sep 10, 2021

Are there plans to provide a shim package for .NET Standard?

Not AFAIK.

@jnm2
Copy link
Contributor

jnm2 commented Sep 10, 2021

I prefer internal attribute class declarations over shim packages, myself.

@vbcodec
Copy link

vbcodec commented Oct 14, 2021

simpler version, without unnecessary clutter

    void fun (int arg)
    {
        string expr = GetCallingExpression(arg);
    }

where GetCallingExpression is build-in function that return stringified expression and can be called only with function's parameters.

@CyrusNajmabadi
Copy link
Member

@vbcodec this feature is already done.

@jnm2
Copy link
Contributor

jnm2 commented Oct 14, 2021

@vbcodec That signature would be misleading, since the actually emitted method must have an additional string parameter. There's no other general mechanism for obtaining the string value than having the string value passed via parameter. (Ultimately, the compiler must emit something expressible in past runtimes.)

@vbcodec
Copy link

vbcodec commented Oct 15, 2021

There's no other general mechanism for obtaining the string value than having the string value passed via parameter.

It is matter of compiler. Compiler can generate such additional parameters, or use thread-based ambient context (must be added to NET 6 BCL), to transfer these strings

@CyrusNajmabadi
Copy link
Member

It is matter of compiler.

It can't. Source code doesn't exist at runtime.

or use thread-based ambient context (must be added to NET 6 BCL), to transfer these strings

There's no thread based ambient context that contains the original source text to transfer.

@jnm2
Copy link
Contributor

jnm2 commented Oct 15, 2021

@vbcodec is suggesting either of two things, both of which are technically possible but both of which are problematic IMO:

  1. No parameter is visible in the signature of the method declaration syntax, but it exists when compiled/invoked via reflection/used with delegates/etc. The compiler at the call site has the syntax and passes it as an argument to this implicitly declared method parameter. This is not good IMO because you can no longer look at the method declaration syntax and reason about what will actually be seen when the code compiles, what delegates can be used with the method, etc.

  2. Or, there is no method parameter. The compiler at the call site doesn't pass the syntax as a method argument but instead emits a call to some unspeakable static method generated alongside the method which receives the syntax and stores it in thread-local storage. Then the first thing the method does is retrieve the syntax using the corresponding unspeakable static method that reads and clears the thread-local storage. (Or some similar version of this.) This is not good IMO. First, because it's heavy in additional metadata and interop complexity. What if you want to invoke the method via a delegate? Second, because it's not good for performance, especially not for logging frameworks that need method calls like this to have extremely low overhead.

@vbcodec
Copy link

vbcodec commented Oct 15, 2021

@jnm2

Thanks for expanding my statement. My two corrections:

  1. while these parameters won't be explicitly defined, developer should know that they exist in generated code, and function in source behave as they were defined explicitly.

  2. Perf can be optimized, and delegates may use special attributes (for example CallerArgumentExpressionRequired) on particular parameters or for whole delegate, so caller of delegate will know when pass stringified expressions.

@IanKemp
Copy link

IanKemp commented Nov 9, 2021

For future visitors: https://docs.microsoft.com/dotnet/api/system.runtime.compilerservices.callerargumentexpressionattribute

Since .NET Standard is dead, sadly this attribute will never appear there.

@jnm2
Copy link
Contributor

jnm2 commented Nov 12, 2021

Since .NET Standard is dead, sadly this attribute will never appear there.

@IanKemp This actually doesn't matter. .NET Standard projects can do this:

#if !NETCOREAPP3_0_OR_GREATER // This line is only needed if you are multitargeting with netcoreapp3.0+
namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
internal sealed class CallerArgumentExpressionAttribute : Attribute
{
    public CallerArgumentExpressionAttribute(string parameterName)
    {
        ParameterName = parameterName;
    }

    public string ParameterName { get; }
}
#endif

This general approach works for all compiler-recognized attributes. I first used it with CallerMemberNameAttribute, and SerializableAttribute was another example of this before it was added to .NET Standard.

It's very important to declare these attributes as internal so that consuming projects don't end up seeing conflicting definitions of System.Runtime.CompilerServices.CallerArgumentExpressionAttribute. Declaring the attribute as internal does not restrict where it can be used within the project that declared it.

@NetMage
Copy link

NetMage commented Nov 13, 2021

That is no help in gaining the actual functionality of the attribute, which means it has no purpose if you are targeting .Net Standard.

@jnm2
Copy link
Contributor

jnm2 commented Nov 13, 2021

@NetMage That's incorrect. The actual functionality of the attribute is in full effect so long as you are using the .NET 6 SDK. The same thing has been true of all compiler-recognized attributes; the target framework doesn't affect compiler behavior so long as somebody declares the correct types.

You can demonstrate for yourself that this works when targeting .NET Standard. Here's one way:

Setup (click to expand)
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>netstandard2.0</TargetFrameworks>
    <LangVersion>10</LangVersion>
  </PropertyGroup>

</Project>
Program.cs
using System;
using System.Runtime.CompilerServices;

WriteExpression(1 + 2);

static void WriteExpression(object p, [CallerArgumentExpression("p")] string expression = null)
{
    Console.WriteLine(expression);
}
System.Runtime.CompilerServices.CallerArgumentExpressionAttribute.cs
#if !NETCOREAPP3_0_OR_GREATER // This line is only needed if you are multitargeting with netcoreapp3.0+
namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
internal sealed class CallerArgumentExpressionAttribute : Attribute
{
    public CallerArgumentExpressionAttribute(string parameterName)
    {
        ParameterName = parameterName;
    }

    public string ParameterName { get; }
}
#endif

Result:

image

You can verify the same thing by going a longer route involving a netstandard2.0 library that isn't necessarily a console app and verify that the caller argument expression is indeed created as a string literal by decompiling the assembly with ILSpy or dotPeek etc.

This works within the same assembly no matter what the assembly's target framework is, and it works across the boundary between any two assemblies no matter what each target framework is.

@bradwilson
Copy link

I can verify this works with .NET 4.7.2, .NET Core 3.1, and .NET Standard 2.0 given the example here. I've already updated my Guard class in xUnit.net: xunit/xunit@d53d076

Thanks! 🎉

@dominikjeske
Copy link

Why is this issue still open? Something missing?

@bernd5
Copy link
Contributor

bernd5 commented Nov 14, 2021

ECMA Spec is missing

@333fred
Copy link
Member

333fred commented Nov 14, 2021

We keep issues open until they are documented in the ECMA specification over at dotnet/csharpstandard.

@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Jan 11, 2022
@abelbraaksma
Copy link

abelbraaksma commented May 2, 2024

That's now 3 years later, is the ECMA spec that far behind, or was this simply forgotten? Where should this go? I don't mind giving it a go, as I'm looking at implementing this for F# (see fsharp/fslang-suggestions#966).

It looks like the latest ECMA version is from 2023, but this isn't in. I assume it should go into section 22.5?

@colejohnson66
Copy link

colejohnson66 commented May 2, 2024

The 2023 ECMA is for C# 7. Yes, it’s the far behind. Progress is being made at @dotnet/csharplang. They’re working on C# 8 ATM. As this is for C# 10, it’s gonna be a bit before this is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Soon candidate
Projects
None yet
Development

No branches or pull requests