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 for method parameter names in nameof() #373

Open
Tracked by #9
taori opened this issue Mar 28, 2017 · 33 comments
Open
Tracked by #9

Support for method parameter names in nameof() #373

taori opened this issue Mar 28, 2017 · 33 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 Proposal
Milestone

Comments

@taori
Copy link

taori commented Mar 28, 2017

Update (jcouv): speclet (https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/extended-nameof-scope.md)


image

it would be fairly amazing if nameof also worked for MethodName arguments.

I think i've talked about this 1-2 years ago and some aspnet member also chimed in and mentioned how this could be useful for magic EntityFramework where the appropriate name must be passed too for connection strings to work.

So essentially i would love it if, instead of strings, i could be typing this:

[AuthGrantFilter(
  AppFunction = ApplicationFunctionTypes.ReadProjectStructure, 
  TopLevelProjectId = nameof(topLevelProjectId), 
  SiteId = nameof(siteId))]
[AuthDenyFilter(
  Context = DenyContext.Site, 
  TopLevelProjectId = nameof(topLevelProjectId), 
  SiteId = nameof(siteId))]

or this

[AuthGrantFilter(AppFunction =
  ApplicationFunctionTypes.ReadProjectStructure, 
  TopLevelProjectId = nameof(GetChildrenOf.topLevelProjectId), 
  SiteId = nameof(GetChildrenOf.siteId))]
[AuthDenyFilter(
  Context = DenyContext.Site, 
  TopLevelProjectId = nameof(GetChildrenOf.topLevelProjectId), 
  SiteId = nameof(GetChildrenOf.siteId))]

Update (jcouv):
This proposal (in its most likely and straightforward design) would affect scoping rules and thus would introduce a breaking change. It is worth considering whether this breaking change is severe. Here's an example to illustrate the problem:

const int p = 3;

[Attribute(Property = p)] // currently binds to the constant, but the parameter `p` would be in scope after this proposal
void M(int p) { }

Some alternatives for LDM to consider:

  1. Change the scoping rules and take the breaking change
  2. Only affect the scoping rules for nameof
  3. Allow nameof to reference parameters with the method name as a qualifier [Attr(nameof(M.p)] void M(int p) { }

LDM history:

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2017

Why is GetChildrenOf.nameof(topLevelProjectId) better than nameof(GetChildrenOf.topLevelProjectId)? Putting nameof first makes it easier to read.

Or nameof(GetChildrenOf, topLevelProjectId).

@alrz
Copy link
Member

alrz commented Mar 28, 2017

How would you resolve multiple method overloads? It's not ambiguous in the use-site, but when you rename either of parameters in the method declaration, it's not clear which one nameof is referring to.

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2017

@alrz nameof(GetChildrenOf(int, int?), topLevelProjectId) perhaps.

@alrz
Copy link
Member

alrz commented Mar 28, 2017

That seems like a lot of new syntax, however, the attributes in the example above are on the method itself. I think it would be nice to just bring them into the scope, e.g.

[Attribute(nameof(parameter))]
void M(object parameter, [Attribute(nameof(parameter))] object p) { }

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2017

I agree.

@taori
Copy link
Author

taori commented Mar 29, 2017

@jnm2 you could a typo of me there unfortunately. i meant to write it like nameof(GetChildrenOf.siteId). I'd rather stay in line with the ordinary nameof syntax

@taori
Copy link
Author

taori commented Mar 29, 2017

@alrz I would probably resolve it like this given we have the following methods

class Sample {
Method1(string a, string b) {}
Method2(string a, string b) {}
Method2(string a, string b, string c) {}
Method2(int aint, int bint, int cint) {}
}

Which would resolve in options to use nameof like this:

// methodname unique, not need to specify signature to remain uniquely identifyable
nameof(Sample.Method1.a) nameof(Sample.Method1.b)

nameof(Sample.Method2.a, string, string) nameof(Sample.Method2.b, string, string)

nameof(Sample.Method2.a, string, string, string) nameof(Sample.Method2.b, string, string, string) nameof(Sample.Method2.c, string, string, string)

// argument names unique - no need to be more specific to stay unique
// given that i don't quite like this one from an intellisense perspective since "Method2" should then suggest "aint" and "a" at the same time
nameof(Sample.Method2.aint) nameof(Sample.Method2.bint) nameof(Sample.Method2.cint)

I agree that merely bringing them into scope would already be a significant improvement for most use cases - but rather than having to bypass data through attributes, one might just go all the way from the start then.

@taori
Copy link
Author

taori commented Aug 8, 2017

This would be great for unit testing methods throwing ArgumentExceptions.

Quoting @perholje #771 (comment)

@cartermp
Copy link

cartermp commented Jul 15, 2019

cc @jcouv

Note that the NotNullIfNotNull(string) attribute, when specified for the return value, makes this a bit more desirable:

class Path
{
    [return: NotNullIfNotNull(nameof(path))] // This fails to compile, needs to be "path" instead
    public static string? GetFileName(string? path);
}

In fact, the LDM notes inadvertently specified code that won't compile due to this 🙂 - https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md#nullness-dependencies-between-inputs-and-outputs

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jul 15, 2019

The main challenge here is back compat. How do you do this without breaking existing code. You'd need a way to say "try with the existing binding first, and if that doesn't work, look at parameters". But then that's super weird and will not match any sort of intuition about what should be going on when i see something like that.

Retracted. You will always get the same IL no matter what this binds to (since the nameof is just replaced with the stirng constant value, and that value doesn't change here). The only difference is what impact it has on things like renames and whatnot. i.e. if this bound to a field before and you rename the field, this would now no longer update since it bound to the parameter.

I think tha'ts actually totally acceptable. I don't think the language should be restricted from changing here just because it might end up causing a downstream IDE tooling change.

IMO, the new behavior would be entirely intuitive and would be in line with what people want anyways. So i change my position to 👍 on this.

@TylerBrinkley
Copy link

Just putting my 2 cents in as well that this would be useful, especially with the addition of the NotNullIfNotNullAttribute as @cartermp points out. It wouldn't be surprising to me if more attributes like that get added in future versions for the nullable reference types feature.

@RikkiGibson
Copy link
Contributor

@jaredpar brought up a scenario could break depending on how we implement this:

class A : System.Attribute
{
    public A(int i, string s) { }
}

class C
{
    const int item1 = 123;
    int item2;

    [A(item1, nameof(item2))]
    void M(int item1, int item2) { }
}

If we don't specify/implement it carefully, item1 will start binding to the parameter and the code will no longer compile. So one suggestion is that we could attempt to bind the attribute argument using basically the "binder we already have today", and if the lookup fails, attempt to bind again using a binder that knows about the method parameters. Apologies if I'm mangling the terms here.

Alternatively we could decide no, the user needs to change their code to say [A(C.item1, nameof(item2))] instead, specifying the constant member of C instead of the parameter of M.

@Joe4evr
Copy link
Contributor

Joe4evr commented Sep 27, 2019

If we don't specify/implement it carefully, item1 will start binding to the parameter and the code will no longer compile.

Is it possible to specify it'll only bind to a parameter of the method an attribute is applied on if it's inside a nameof() expression?

@taori
Copy link
Author

taori commented Sep 28, 2019

@RikkiGibson That sure is an interesting edge case he came up with. In that scenario i would however say that the feature should just raise a compile error if it can't distinguish between symbols. After all you are free to pick and choose argument names without major impacts on your code, so utilizing the benefits of forwarding argument names to attributes would still exist even if you have that case.

  • afaik naming convention for most private members is still a leading underscore, so there should be no collisions if you follow conventions.

@yaakov-h
Copy link
Member

@taori that's just one naming convention, and not even the default VS one.

@taori
Copy link
Author

taori commented Sep 28, 2019

@yaakov-h Yeah - sure. Well i guess the language team will wrap their head around this one since there seems to be quite some interest in implementing this feature to ensure compilation safety for the nullness safety feature.

@CyrusNajmabadi
Copy link
Member

Is it possible to specify it'll only bind to a parameter of the method an attribute is applied on if it's inside a nameof() expression?

Yes. That would be my preference.

@jcouv jcouv self-assigned this Oct 15, 2019
@Joe4evr
Copy link
Contributor

Joe4evr commented Oct 21, 2019

Allow nameof to reference parameters with the method name as a qualifier [Attr(nameof(M.p))] void M(int p) { }

Funny thing, I thought of this exact option to be the final disambiguator, if nothing else will do. I even had thought out that the user experience when in the context [Attr(nameof(M.$$))] should be to have IntelliSense pop up regarding M as a "quasi-object" (hopefully simple to do in Roslyn's design) and listing M's parameters as its members.

@gafter gafter added this to the Any Time milestone Dec 16, 2019
@gafter
Copy link
Member

gafter commented Dec 16, 2019

We probably would want to do this either before or at the same time as #287

@CleanCodeX
Copy link

The compiler could detect if there is a already a private member with same name as the param name and enforce the user to either prefix its methodname => nameof(Method1.Param1) or rename the private member variable. (e. g. adding an underscore).
the benefits are ommitting the Methodname where no ambiguity exists and keeping backwards compatibility.
Am I missing a major downside here?
(I am sure you already thought about that idea, but I don't see it mentioned here so I throw it in)

@CyrusNajmabadi
Copy link
Member

Why would it matter if there was a field in scope with the same name as the parameter?

@HaloFour
Copy link
Contributor

@CyrusNajmabadi

Why would it matter if there was a field in scope with the same name as the parameter?

Could affect refactoring if you wanted to rename that parameter or field. This is something that tooling could handle without language/compiler changes, though, by asking for disambiguation.

@CyrusNajmabadi
Copy link
Member

Yeah. Not a lang issue afaict.

@mikernet
Copy link

mikernet commented Oct 25, 2021

The refactoring issue is already kind of a problem with method overloads that have the same name so tooling improvements in this area would be beneficial and could address both those issues.

@batzen
Copy link

batzen commented Nov 4, 2021

How about nameof(parameters.xy) or something like that?
That would prevent all scoping issues.

@333fred
Copy link
Member

333fred commented Nov 4, 2021

@batzen what if I have a field named parameters with a property or field named xy?

@batzen
Copy link

batzen commented Nov 4, 2021

Sorry, meant params...

@CommonLoon102
Copy link

With the new CallerArgumentExpression this is needed even more than before:
dotnet/roslyn#57792

@jcouv
Copy link
Member

jcouv commented Feb 15, 2022

FYI, added a speclet link to OP: https://github.com/dotnet/csharplang/blob/main/proposals/extended-nameof-scope.md

@jcouv
Copy link
Member

jcouv commented Apr 27, 2022

Feature was merged for VS 17.3p2

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

No branches or pull requests