microsoft/vs-validation

Support CallerArgumentExpression in assertions

drewnoakes opened this issue · 10 comments

Methods on Requires (and possibly other classes such as Assumes etc.) may benefit from CallerArgumentExpressionAttribute.

https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-10#callerargumentexpression-attribute-diagnostics

// before
Requires.NotNull(name, nameof(name));

// after
Requires.NotNull(name);

That sounds awesome. I wonder though how we can incorporate this without regressing the experience for the great majority of users that are still on C# 9 and older. If we make the second parameter optional then C# 9 would not prompt nor automatically fill in the attribute, leaving a missing parameter name in the exception.

I suppose we could multi-target and when .NET 6 is in use we might assume (unreliably) that the user is using C# 10 and only apply the default parameter value and attribute in the net6 compilation. That would be somewhat tedious to maintain though, and C# 10 users may exist while targeting less than .NET 6 and .NET 6 users may not be using C# 10, so it's an imprecise design too.

One approach would be to add an analyzer.

Personally I'd want this for use in VS, where we still target net472.

True. An analyzer would have all the needed insight. In fact I think an analyzer could be written in a way that works for all calls to any method that uses this parameter attribute.
I don't think I'll have time to do this in the near future though, but we'll keep the issue active and take contributions.

Hi, we are currently looking for a contract framework and without that feature this framework is not an option.

Question: For older Language Versions, the attribute simply has no effect.
So for non C# 10+ Users the experience would be the same as before...

So would it not be an option to just add the attribute for everybody?
See here:
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-10.0/caller-argument-expression

I think even for .NET Framework where the CallerArgumentExpressionAttribute doesn't exist,
You can just declare the type in the project with the same namespace and it works again.

So for non C# 10+ Users the experience would be the same as before...

It wouldn't be though. The attribute is to be applied to an optional parameter. But the parameter will be optional regardless of language version, even though the attribute only works on C# 10+. So all the C# 9- users would be likely to omit the parameter by accident, producing a much less desirable result when the exception is thrown.

I could still add the analyzer and/or only add the attribute and make the parameter optional for the .NET 6 targeted projects to mitigate that.

Ok, so I did some testing, and maybe this is what @MichaelPeter was referring to, but this is not limited by lang version. It works just fine on LangVersion 7.3, for example, and when targeting net472. or netstandard2.0.
I'll prepare this change.

@AArnott I actually did not consider, that the parameterName are is not yet an optional parameter. You are right.

Ok, so I did some testing, and maybe this is what @MichaelPeter was referring to, but this is not limited by lang version. It works just fine on LangVersion 7.3, for example, and when targeting net472. or netstandard2.0. I'll prepare this change.

Ohh really? It works with 7.3? My experience was with previous versions of C# that the parameter value of the parameterName would simply stay null. They Introduced the Attribute in C# 5.0 but only provided support for it in C# 10.

Yes to my knowlege it was independent of the framework version, so you can use it in .net472. Only requirement is you have a visual studio version which supports C# 10 (and have not explicitly set the project to a language version before C# 10.0)
But seems I am wrong when it worked with 7.3

My experience was with previous versions of C# that the parameter value of the parameterName would simply stay null.

You were close. The significant difference is compiler version rather than language version. If the feature came out in C# (language version) 10, then any compiler that supports C# 10 will work, without regard to the language version setting. I believe that means any version of VS 2022 is acceptable.

And yes, the sad fallback from this change is that anyone using an older compiler (i.e. VS 2019) will be tempted to omit arguments and null will be passed in instead of the parameter name, without any warning. But VS 2022 has been out for quite a while now, so I think this is a bigger win than a loss.

For anyone now facing a mass migration, the following RegEx might be helpful. In VS's Replace in files:

  • Requires\.NotNull\((\w+), nameof\(\1\)\);
  • Requires.NotNull($1);

image

Also:

  • Requires.NotNullAllowStructs
  • Requires.NotNullOrEmpty
  • Requires.NotNullOrWhiteSpace
  • Requires.NullOrNotNullElements
  • Requires.NotNullEmptyOrNullElements
  • Requires.NotEmpty
  • Requires.Defined
  • Requires.NotDefault

Calls to Assumes.True(bool, string) and Assumes.False(bool, string) likely require manual review.