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

Proposal: Allow expressions of method parameters to be stringified #205

Open
jamesqo opened this issue Feb 28, 2017 · 23 comments

Comments

@jamesqo
Copy link
Contributor

commented Feb 28, 2017

Motivation

When an exception is thrown or an assert/unit test fails, it's not always obvious what went wrong just from looking at the stack trace. Part of the reason for this is that you can't see the condition that failed. If you write Debug.Assert(array != null) and it fails, the stack trace will only contain the filename and the line number. You will have to open up a text editor and navigate to that file to see what went wrong. It would be ideal if Debug.Assert actually showed you the string "array != null" in the failure message.

Proposal

Add a CallerArgumentExpression attribute allowing callees to 'stringify' the expressions passed in at a callsite. The constructor of the attribute will take an string argument specifying the name of the argument to stringify.

public static class BetterDebug
{
    public static void Assert(bool condition, [CallerArgumentExpression("condition")] string expression = null)
    {
        Debug.Assert(condition, expression);
    }
}

public void MyArrayMethod(T[] array)
{
    BetterDebug.Assert(array != null); // Upon failure, the message will be "array != null"
}
  • The attribute will work the same as other Caller* attributes like CallerMemberName, CallerFilePath, etc. The compiler will replace the optional parameter with the string of the expression of the argument having the name specified in the attribute.

  • The compiler will verify that the name is valid at compile-time, e.g. [CallerArgumentExpression(null)] or [CallerArgumentExpression("cnodition")] in this example will raise an error.

The use cases for this are not limited to Debug.Assert. Consider the possibility of creating a helper class for argument validation without needing to use nameof:

public static class Requires
{
    public static void NotNull<T>(T argument, [CallerArgumentExpression("argument")] string expression = null)
    {
        if (argument == null) throw new ArgumentNullException(expression);
    }

    public static void Range(bool condition, [CallerArgumentExpression("condition")] string expression = null)
    {
        if (!condition) throw new ArgumentOutOfRangeException(message: $"{expression} failed.");
    }

    public static void CanRead(Stream stream, [CallerArgumentExpression("stream")] string expression = null)
    {
        if (!stream.CanRead) throw new InvalidOperationException(message: $"{expression} can't read.");
    }
}

Now we can validate parameters as follows

public static void MyHighPerfByteArrayStreamMethod(
    byte[] buffer, int index, int count, Stream stream)
{
    // No nameof! Each parameter name only has to be typed once.
    Requires.NotNull(buffer); // paramName: "buffer"
    Requires.Range(index >= 0 && count >= 0); // message: "index >= 0 && count >= 0 failed."
    Requires.Range(buffer.Length - index >= count); // message: "buffer.Length - index >= count failed."
    Requires.CanRead(stream); // message: "stream can't read."
}

One could also see how this attribute could be used by test frameworks to provide better error messages when passing in a boolean, e.g. in the case of xUnit Assert.True or Assert.False could be more helpful.

@ig-sinicyn

This comment has been minimized.

Copy link

commented Feb 28, 2017

There was a similar issue at roslyn repo, dotnet/roslyn#2178.

Happy to know I'm not alone:)

@jamesqo jamesqo changed the title Proposal: ExpressionStringAttribute for stringifying the expression passed at a callsite Proposal: Allow expressions of method parameters to be stringified Feb 28, 2017

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

@ig-sinicyn Nice proposal! I hadn't seen that before. I like your choice of having the attribute taking a string rather than an index better, and prefixing the attribute name with Caller would make it fit in more with the other special attributes. I'll update my proposal to incorporate some of your ideas when I get time.

@gafter gafter added the Discussion label Feb 28, 2017

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

Updated the proposal.

@chrisaut

This comment has been minimized.

Copy link

commented Mar 7, 2017

I don't have much to add, but I really like this. Would be very useful indeed.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

@jamesqo, I do like the idea, but I had a couple of questions...

Are you expecting the raw code passed in for the argument be stringified?

  • I think if yes, then this would need a lot more thought as users could call an API and not be aware that their raw code is being preserved as a constant in their library (not everyone is open source).

How would this work with localization (maybe same as nameof, no loc support)?

Also what are your thoughts on having a little helper function such as:

class DebugEx
{
    [Conditional("DEBUG")]
    void AssertNotNull(bool condition, string paramName)
    {
        Debug.Assert(condition, $"{paramName} was null");
    }
}

In the case of exceptions, most of the core types already have some support for this. For example:

throw new ArgumentNullException(nameof(someParam));

will give you a message of:

Value cannot be null.
Parameter name: x

In the case of the latter two, all that is missing is automatic passing of the parameter name (which, I think, is less likely to be a concern for consumers who aren't open-source).

@ig-sinicyn

This comment has been minimized.

Copy link

commented Mar 7, 2017

@tannergooding

We are actively using assertions through our codebase so I think I can answer to both of your questions:)

Are you expecting the raw code passed in for the argument be stringified?

yes, raw code as is.

Also what are your thoughts on having a little helper function ...

Copied from dotnet/roslyn#2178


...
Main issue with this approach is that you need to write the assert condition twice. First, as a code and second - as a text. The second one never shown to user and used to ease issue investigation on failure.
If there are tens or hundreds of assertions across the code it's not a problem to maintain assertions and text messages in sync.

But when there are a thousand of them there's always a chance someone will just copy assertion block without correcting the text message.


In short, the content of [CallerArgumentExpression("condition")] arg is not meant to be localized or exposed to customers.

It contains nothing more than technical details for troubleshooting and there's as many chances anyone will want to prettify it as with content of the memory dump ;)

@VictorBlomberg

This comment has been minimized.

Copy link

commented Mar 7, 2017

Would another approach be a hybrid between
Syste.Linq.Expressions.Expression<TDelegate> where TDelegate : Func<T> and Func<T>. Where the expression is "precompiled", and thus inexpensive to call.

That would give the same info, at roughly the same cost, with

  • the drawback that one would be forced to use functions/lambdas, and it's less simple,
  • the advantages being that the info isn't stringified, which means we lose less details, and it could be used for different scenarios too (thinking e.g. of NotifyOfPropertyChange)
@neoKushan

This comment has been minimized.

Copy link

commented Mar 7, 2017

This is a very interesting proposal. I wonder if it could be used to allow logging libraries to get direct access to interpolated string data? I love C#6's interpolated strings, but most logging libraries still require a separation of log string and arguments (particularly thinking of Serilog in this instance).

I'm not sure what the impact of that would necessarily be.

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

@tannergooding

I think if yes, then this would need a lot more thought as users could call an API and not be aware that their raw code is being preserved as a constant in their library (not everyone is open source).

That's a good point. Hopefully that should not be an issue because:

  • During Release builds, Debug.Assert calls are wiped out so the strings won't be present in the IL
  • Besides Debug.Assert, there will not be other framework methods that use this attribute
  • People will be able to see the attribute in IntelliSense and Google what it means

That said, I'm willing to consider making this an opt-in feature if you're not convinced. We could introduce a new attribute such as [assembly: EnableCallerArgumentExpression] and have people put that in AssemblyInfo.cs. That way closed-source people won't leak source code if they do something like ship their Debug builds.

How would this work with localization (maybe same as nameof, no loc support)?

Yep, as @ig-sinicyn says, no loc support. It's just a dump of the expression for purely diagnostic purposes.

In the case of the latter two, all that is missing is automatic passing of the parameter name

Automatic passing of the parameter name is pretty much the motivation for proposing this attribute.

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

@neoKushan

I wonder if it could be used to allow logging libraries to get direct access to interpolated string data? I love C#6's interpolated strings, but most logging libraries still require a separation of log string and arguments (particularly thinking of Serilog in this instance).

Hm, I don't think this proposal would let logs access interpolated string data. It would however let them access the names of the arguments you pass in, so if you call LoggingFunction(user.DateCreated) you will be able to see user.DateCreated is mm-dd-yy in the log.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

During Release builds, Debug.Assert calls are wiped out so the strings won't be present in the IL

This is only true if the method that consumes the attribute has a [Conditional("DEBUG")] attribute.

Besides Debug.Assert, there will not be other framework methods that use this attribute

That may be true, but what prevents a third party from creating a function that uses the attribute. Any consumers could then be unknowingly sharing their source code (yes, not likely to happen, especially if you properly read the documentation on functions you are calling, but still).

People will be able to see the attribute in IntelliSense and Google what it means

I would hope they would, but you never know.

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

@tannergooding

That may be true, but what prevents a third party from creating a function that uses the attribute. Any consumers could then be unknowingly sharing their source code (yes, not likely to happen, especially if you properly read the documentation on functions you are calling, but still).

They aren't "sharing" their source code: you will have to look through the IL to see the strings. Chances are if you know how to look through IL then you also know how to use a decompiler, so there will be little new information revealed from the extra strings.

@svick

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

I think the people who want to protect their source code and so would mind if some expressions from their source code were shared are the same kind of people who use obfuscators. And if they're willing to use an obfuscator, either the obfuscator could take care of this, or they could use an analyzer that makes sure they don't call such methods unknowingly.

So, I think this would make life slightly worse for those people, but it would make life better for everyone else. And that sounds like an acceptable tradeoff to me.

@nesteruk

This comment has been minimized.

Copy link

commented Mar 18, 2017

Seems like a sensible proposal, especially considering that in C++ land we already have this functionality, albeit in a quirky way (macros). In addition to debugging, I would also appreciate this approach in unit testing: instead of the never-ending Assert.That(thisThing, Is.Really.ThatThing(orNot)) I'd prefer to just write Assert (x==y) and have the error message state "Assertion x==y failed".

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@gafter What do you think of this proposal? It seems like everyone so far has been in favor of it one way or another (even @tannergooding thumbed-up @svick's comment). It isn't adding any new syntax to the language, has a clear, common use case, and isn't unprecedented because the compiler already does something similar for the other Caller* attributes. Would it be OK to open a pull request adding this proposal?

@tannergooding

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Just to be clear, I'm not on the language design team, so my thumbs up means very little 😄

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@tannergooding I meant it as you had pointed out a potential issue with the proposal, but you still were in support of it.

@jnm2

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

Can the CallerArgumentExpression string be language agnostic, possibly? A text serialization of the expression, with a BCL parser to make it easy for your Assert.That(x, Is.Not.EqualTo(y)) to be replaced with Assert.That(x != y) no matter what the calling language is?

@svick

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

@jnm2 I'm not sure I understand what you are asking. Are you saying that Assert.That(x != y); in C# and Assert.That(x <> y) in VB should produce the same CallerArgumentExpression string?

What is the reason for that? The string is meant to be viewed by humans, who I think prefer to see expressions in their language of choice. If you want a language agnostic description of an expression, that's what expression trees are for. (That currently requires you to use a lambda, but maybe it doesn't have to, see F# auto-quotations.)

@jnm2

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

@svick Yes, that's what I'm asking. But further. The example was brought up of Assert.That(x != y). Assert.That(x, Is.Not.EqualTo(y)) currently results in Expected: 1 But was: 2. So the example would not really be able to replace it; Assert.That(x != y) would result in Expected: x != y with no idea what the values of x and y are.

@svick

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

@jnm2 How would you imagine that string to look? Like I said, it sounds to me like you're designing expression trees, which already do pretty much what you're asking for.

@jamesqo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@jnm2

Assert.That(x, Is.Not.EqualTo(y)) currently results in Expected: 1 But was: 2. So the example would not really be able to replace it; Assert.That(x != y) would result in Expected: x != y with no idea what the values of x and y are.

Yep. To be clear, the new feature doesn't entirely supplant passing in arguments directly, e.g. With xUnit Assert.Equal(e, a) will still be more descriptive than Assert.True(e == a). However, it does make Assert.True more descriptive, and without much added complexity (there is no serialization/parsing/expression tree creation involved).

@gafter

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

@gafter What do you think of this proposal?

I think it's OK. I don't imagine it would rank very high among the other small features we're looking at. I'll have the LDM triage it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.