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

Test plan for CallerArgumentExpressionAttribute #52745

Closed
42 of 46 tasks
jcouv opened this issue Apr 19, 2021 · 15 comments · Fixed by #57805
Closed
42 of 46 tasks

Test plan for CallerArgumentExpressionAttribute #52745

jcouv opened this issue Apr 19, 2021 · 15 comments · Fixed by #57805
Assignees
Labels
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Apr 19, 2021

Proposal: dotnet/csharplang#287
Spec: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-10.0/caller-argument-expression.md

General Concerns

  • LangVersion - No lang version impact for caller info attributes.
  • VB Interop
    • VB does case-insensitive identifier matching
    • VB emits correct casing, regardless of user's casing in source
  • Update spec with implicit conversion rules
  • Remove feature flag after Aleksey reviews.

Semantics

  • Syntax is reflected exactly
    • Leading comments ignored
    • Trailing comments ignored
    • Middle comments included
    • Leading pragmas ignored
    • Trailing pragmas ignored
    • Middle pragmas included
  • Parameter remapping
    • Extension methods
      • Info of this parameter
      • Info of non-this parameter
    • Delegate Invoke
    • Delegate BeginInvoke
    • Delegate EndInvoke
  • Overridden by existing caller info attributes
    • CallerFilePathAttribute
    • CallerLineNumberAttribute
    • CallerMemberNameAttribute
  • Self-referential attribute
  • Explicit argument reordering
  • Applied multiple times
  • Invalid parameter name
  • No default value provided for parameter
  • Referenced parameter uses default value
  • Attribute added to overridden method
  • Attribute removed from overridden method
  • Attribute applied to type with implicit conversion from string
    • Reference conversion
    • User-defined conversion
    • No conversion
  • Compiler generated calls with user-written code
    • Extension GetEnumerator
    • Extension Deconstruct
    • Implicit Index/Slice (should be default)
    • Extension GetAwaiter
  • Ref/out parameters
    • Out variable declarations
    • Out discards
  • Implicit base constructor calls (should be ignored, like other Caller info attributes)

Open issues:

  • could we add CallerArgumentExpression to Debug.Assert and have that new overload get picked?

FYI @Youssef1313

@jcouv jcouv added this to the C# 10 milestone Apr 19, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 19, 2021
@Youssef1313
Copy link
Member

Youssef1313 commented Apr 20, 2021

There is already Debug.Assert(bool condition) and Debug.Assert(bool condition, string message).

Overload resolution will currently pick Debug.Assert(bool condition) over Debug.Assert(bool condition, [CallerArgumentExpression("condition")] string expression ="").

It could be tweaked to prefer the other overload only because it has caller member attribute, but that will make the language inconsistent.

Another solution I can think of (which I'm not sure if it's good) is introducing a new language feature, a special attribute MethodForwardAttribute which causes the overload resolution to be instructed to pick another specific compatible overload.

I can't think of other use cases of such a new language feature, so I don't think it's a great idea to introduce a whole language feature just for this?

(Is there some easier way I'm missing? 😄)

@jaredpar jaredpar added Feature Request and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 20, 2021
@alrz
Copy link
Contributor

alrz commented Apr 20, 2021

could we add CallerArgumentExpression to Debug.Assert and have that new overload get picked?

Make Assert(bool) obsolete and remove obsolete members from the candidate set? (cons: too scary)

@333fred
Copy link
Member

333fred commented Apr 20, 2021

Make Assert(bool) obsolete and remove obsolete members from the candidate set? (cons: too scary)

Probably a breaking change unfortunately.

@alrz
Copy link
Contributor

alrz commented Apr 20, 2021

Consider existing code with an obsolete overload:

  • In cases currently resolved to the obsolete method, an error is already produced (with error: true). If this rule causes another method to get picked it's not a breaking change by definition.

  • With this rule in place, user would never see the obsolete error for Assert(bool) because it gets removed from the set and Assert(bool, string=null) wins.

  • Lastly, since the method is there, the change is binary compatible anyways.

Now the question is, can this possibly make the overload resolution to pick another method where it already compiles without any error? that is, it's not ambiguous and it's not resolved to the obsolete method.

I'm leaning towards no, it can't but I'm not completely sure.

@AlekseyTs
Copy link
Contributor

  1. I think discussions like that do not belong in a Test Plan issue.
  2. It is fine to consume an obsolete APIs in another obsolete API, not an error, not even a warning

@Youssef1313
Copy link
Member

Youssef1313 commented May 20, 2021

TODO

@Youssef1313
Copy link
Member

Youssef1313 commented May 27, 2021

Open questions:

  • VB support (VB support for CallerArgumentExpression #54132)
  • Should caller argument expression be given an argument that's compiler generated? for example, the argument expression refers to a parameter that is a CallerMemberName. Should the argument expression take the default value or take the value supplied by the compiler for CallerMemberName (my current implementation won't supply compiler generated arguments)
  • Should there be a warning if the caller argument expression refers to itself? void M([CallerArgumentExpression("p") string p = null) (CallerArgumentExpression: Warning for self-referential #54129)
  • Should comments be included as part of the caller argument expression? (Don't include trivia in caller argument expression #54130)
  • If yes to previous point, there are cases where part of the comment doesn't parse as part of the ArgumentSyntax, would that be okay? (See SharpLab for example) (N/A)

@333fred
Copy link
Member

333fred commented Jun 15, 2021

Open questions were answered in https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-06-14.md.

@jcouv
Copy link
Member Author

jcouv commented Jun 25, 2021

FYI LanguageVersion.CSharp10 was to the main branch. Before merging this feature to main please ensure that it is attached to the correct language version.

@Youssef1313
Copy link
Member

@jcouv Thanks. I'll do when #54064 goes in.

@Youssef1313
Copy link
Member

TODO: #54355 (comment)

@333fred
Copy link
Member

333fred commented Jul 19, 2021

Updated the test plan in preparation for review this afternoon. I went through and checked off all the boxes I could find tests for.

@jerviscui
Copy link

Is it available?
For example, the following method:

public static string CheckLength(this string value, [CallerArgumentExpression("value")] string parameterName = Empty)
{
}

string name = "";
name.CheckLength()

The parameterName will be assigned "name"?

@Youssef1313
Copy link
Member

@jerviscui Yes it's available and parameterName will be assigned name. See SharpLab.

You'll need to have the latest Visual Studio 2022 preview to use the feature.

@jerviscui
Copy link

@Youssef1313 Thank you very much!
The tool SharpLab is also very useful. Thank you for your sharing!

And I'm about to try VS 2022.
I think 64bit would be useful to me.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants