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

Support CallerArgumentExpression in assertions #127

Closed
drewnoakes opened this issue Nov 23, 2021 · 10 comments · Fixed by #165 or dotnet/project-system#8828
Closed

Support CallerArgumentExpression in assertions #127

drewnoakes opened this issue Nov 23, 2021 · 10 comments · Fixed by #165 or dotnet/project-system#8828
Assignees

Comments

@drewnoakes
Copy link
Member

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);
@AArnott
Copy link
Member

AArnott commented Nov 23, 2021

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.

@AArnott
Copy link
Member

AArnott commented Nov 23, 2021

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.

@drewnoakes
Copy link
Member Author

One approach would be to add an analyzer.

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

@AArnott
Copy link
Member

AArnott commented Nov 29, 2021

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.

@MichaelPeter
Copy link

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.

@AArnott
Copy link
Member

AArnott commented Jan 27, 2023

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.

@AArnott
Copy link
Member

AArnott commented Jan 27, 2023

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.

@MichaelPeter
Copy link

MichaelPeter commented Jan 27, 2023

@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

@AArnott
Copy link
Member

AArnott commented Jan 27, 2023

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.

drewnoakes added a commit to drewnoakes/project-system that referenced this issue Jan 28, 2023
The new release of vs-validation fixed microsoft/vs-validation#127 which means our call sites can be simplified.

These changes were made via regular expressions.
drewnoakes added a commit to drewnoakes/project-system that referenced this issue Jan 28, 2023
The new release of vs-validation fixed microsoft/vs-validation#127 which means our call sites can be simplified.

These changes were made via regular expressions.
drewnoakes added a commit to drewnoakes/project-system that referenced this issue Jan 29, 2023
The new release of vs-validation fixed microsoft/vs-validation#127 which means our call sites can be simplified.

These changes were made via regular expressions.
@drewnoakes
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment