dotnet / csharplang

The official repo for the design of the C# programming language
11.49k stars 1.02k forks source link

Support for method parameter names in nameof() #373

Open taori opened 7 years ago

taori commented 7 years ago

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 commented 7 years ago

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

Or nameof(GetChildrenOf, topLevelProjectId).

alrz commented 7 years ago

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 commented 7 years ago

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

alrz commented 7 years ago

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 commented 7 years ago

I agree.

taori commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

This would be great for unit testing methods throwing ArgumentExceptions.

Quoting @perholje https://github.com/dotnet/csharplang/issues/771#issuecomment-320895137

cartermp commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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.

yaakov-h commented 5 years ago

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

taori commented 5 years ago

@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 commented 5 years ago

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.

Joe4evr commented 5 years ago

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 commented 4 years ago

We probably would want to do this either before or at the same time as https://github.com/dotnet/csharplang/issues/287

AartBluestoke commented 4 years ago

option 3 could have a problem of concerning class members. It is likely we could have a class of the same name as a function in scope (eg: decorating a constructor). This is more likely than having a class member of the same name as a function parameter name in scope as almost all code styles would name members and locals differently.

Class C{
    int my_int;
    // do you expect my_int, the_int, both or neither to work here?
    [Attr(nameof(C.my_int),nameof(C.the_int))] 
    C(int the_int){ ... };
}
jcouv commented 4 years ago

We discussed this in LDM this week and landed on option [2] (special scoping rules for nameof in attributes). This will be backwards compatible.

mikernet commented 4 years ago

This would be rather useful for some NRT annotations:

[return: NotNullIfNotNull(nameof(source))]
public static string? Resolve(object? source)

Just needs to also work when applied to the return value or other method parameters, not just the method itself.

CleanCodeX commented 3 years ago

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 commented 3 years ago

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

HaloFour commented 3 years ago

@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 commented 3 years ago

Yeah. Not a lang issue afaict.

mikernet commented 3 years ago

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 commented 2 years ago

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

333fred commented 2 years ago

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

batzen commented 2 years ago

Sorry, meant params...

CommonLoon102 commented 2 years ago

With the new CallerArgumentExpression this is needed even more than before: https://github.com/dotnet/roslyn/discussions/57792

jcouv commented 2 years ago

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

jcouv commented 2 years ago

Feature was merged for VS 17.3p2