Skip to content

Comments

Allow nameof to access instance members.#48754

Closed
YairHalberstadt wants to merge 19 commits intodotnet:mainfrom
YairHalberstadt:allow-nameof-to-access-instance-members
Closed

Allow nameof to access instance members.#48754
YairHalberstadt wants to merge 19 commits intodotnet:mainfrom
YairHalberstadt:allow-nameof-to-access-instance-members

Conversation

@YairHalberstadt
Copy link
Contributor

@YairHalberstadt YairHalberstadt commented Oct 19, 2020

Fixes #40229

nameof is allowed to access instance members in a static context, and to capture ref / struct this variables. This fixes a bug where members of instance members could not be accessed inside a nameof in a static context.

The relevant part of the spec is this:

However, where the lookup described in Simple names and Member access results in an error because an instance member was found in a static context, a nameof_expression produces no such error.

For example, currently the following is allowed:

using System;
public struct C {
    public string P;
    public static string M1() => nameof(P);
    public Func<string> M2() => () => nameof(P);
    public Func<string> M3(ref string x) => () => nameof(x.Length);
}

But the following isn't:

using System;
public struct C {
    public string P;
    public static string M1() => nameof(P.Length);
    public Func<string> M2() => () => nameof(P.Length);
}

This PR fixes these cases.

There was also an inconsistency in the previous implementation, where the following was allowed:

public class C
{
    public C Prop { get; }
    public static void StaticMethod(){}
    public string M() => nameof(Prop.StaticMethod);
}

But this produces an error:

public class C
{
    public C Prop { get; }
    public static int StaticProp { get; }
    public string M() => nameof(Prop.StaticProp);
}

error CS0176: Member 'C.StaticProp' cannot be accessed with an instance reference; qualify it with a type name instead

It is probably a bug that this is allowed for methods, but this cannot be changed due to back compat. Whilst I'm not sure whether this is worth fixing on it's own for various reasons, I think it fits well enough under the umbrella of the other changes being made here that it's worth allowing in C#10 for consistency.

Things this PR doesn't fix

The following is still illegal:

public struct C {
    public string M();
    public static string M1() => nameof(M()); 
    public static string M2() => nameof(M().Length);
}

error CS0120: An object reference is required for the non-static field, method, or property 'C.M()'

This is because even if we removed that error, we would still get the following error:

error CS8081: Expression does not have a name.
error CS8082: Sub-expression cannot be used in an argument to nameof.

So I didn't think it worth fixing.

I also don't allow use of this and base in nameof since I'm not sure if that's a bug.

… to capture ref/struct this variables. This fixes a bug where members of instance members could not be accessed inside a nameof in a static context.
@YairHalberstadt YairHalberstadt requested a review from a team as a code owner October 19, 2020 05:45
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

(currentType.IsInterface && (declaringType.IsObjectType() || currentType.AllInterfacesNoUseSiteDiagnostics.Contains(declaringType))))
{
bool hasErrors = false;
if (EnclosingNameofArgument != node)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gafter I think you wrote this code originally. Was it intentional to not allow nameof(InstanceProperty.Member) in a static context, but allow nameof(InstanceProperty)?

@AlekseyTs
Copy link
Contributor

I am not sure I agree with interpretation of the spec. I think the quote about Simple names and Member access talks about them at the top level of the named_entity part rather than about them on any level. In that sense errors about P in the example:

using System;
public struct C {
    public string P;
    public static string M1() => nameof(P.Length);
    public Func<string> M2() => () => nameof(P.Length);
}

are expected because, even though P is a simple name, it not the named_entity. Therefore, the exception doesn't apply.

@YairHalberstadt
Copy link
Contributor Author

@AlekseyTs

If you look at the grammar given in the spec, both "P" and "Length" are parsed as named_entity

@AlekseyTs
Copy link
Contributor

Here is the quote in context:

The meaning of the named_entity of a nameof_expression is the meaning of it as an expression; that is, either as a simple_name, a base_access or a member_access. However, where the lookup described in Simple names and Member access results in an error because an instance member was found in a static context, a nameof_expression produces no such error.

The paragraph is talking about the named_entity of a nameof_expression, i.e. the one in the grammar

nameof_expression
    : 'nameof' '(' named_entity ')'
    ; 

Not about any named entity

@alrz
Copy link
Member

alrz commented Oct 19, 2020

So either both of these should be an error or none?

public class C
{
    [Attr(nameof(Other.Length))] // error 
    public string Other;
    string M() => nameof(Other.Length); // ok
}

@alrz
Copy link
Member

alrz commented Oct 19, 2020

Ah I think attribute case is considered a static context so the error is correct.

@YairHalberstadt
Copy link
Contributor Author

I've opened an issue on csharplang to clarify the meaning of the spec, and consider changing it in a future language version if the current implementation is not a bug: dotnet/csharplang#4037

@jinujoseph jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 27, 2020
@YairHalberstadt
Copy link
Contributor Author

I've now changed this to only work in preview language version, thereby implementing dotnet/csharplang#4037

@333fred
Copy link
Member

333fred commented Dec 11, 2020

Done review pass (commit 3). I'm not certain what IDE features will be impacted by this, will need a buddy to advise. #Closed

@333fred
Copy link
Member

333fred commented Dec 28, 2020

Done review pass (commit 7). Looking good overall, couple of small comments. If the recursive tests do end up becoming an issue let me know on discord and I can help you troubleshoot. #Closed

@333fred
Copy link
Member

333fred commented Jan 28, 2022

@AlekseyTs for a second review.

@AlekseyTs
Copy link
Contributor

@YairHalberstadt It looks like there are legitimate test failures and a merge conflict. Could you please address those?

…ess-instance-members

* upstream/main: (669 commits)
  Fix 'hasStaticConstructor' check in MethodCompiler (dotnet#59116)
  Update dependencies from https://github.com/dotnet/arcade build 20220127.8 (dotnet#59134)
  Update StreamJsonRpc (dotnet#59073)
  Resources
  Filter cancellation exceptions in generator driver (dotnet#58843)
  Revert "Remove dependency on EditorFeatures from Remote.ServiceHub project (dotnet#59059)"
  Strings
  Inline
  Convert to switch expression
  Explicitly test empty string case.
  Fix comment
  Simplify test code
  Run all
  Add tests
  Delete test generator
  Add support for specifying server in tests
  Remove options
  review feedback
  Format document after each provider (dotnet#59091)
  [main] Update dependencies from dotnet/arcade (dotnet#59015)
  ...
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 14)

@333fred
Copy link
Member

333fred commented Mar 14, 2023

Moving this to draft to take it off the backlog. @YairHalberstadt, do you think you'll have time to address feedback here?

@333fred 333fred marked this pull request as draft March 14, 2023 16:52
@YairHalberstadt
Copy link
Contributor Author

@333fred probably not

@333fred 333fred requested a review from jjonescz March 21, 2023 17:15
@333fred
Copy link
Member

333fred commented Mar 21, 2023

Alright. We're going to have @jjonescz pick this up to get it across the finish line.

@jjonescz jjonescz force-pushed the allow-nameof-to-access-instance-members branch from 8250658 to ad3e699 Compare March 23, 2023 12:51
@jjonescz jjonescz marked this pull request as ready for review March 23, 2023 14:02
@AlekseyTs
Copy link
Contributor

For some reason CodeFlow crashes while trying to load this PR. Could be due to very old commits in the PR. In order to confirm this theory, could you clone the branch in a new branch, rebase it on top of main and create a different PR from that new branch?

@jjonescz
Copy link
Member

For some reason CodeFlow crashes while trying to load this PR. Could be due to very old commits in the PR.

Yes, happens to me, as well.

In order to confirm this theory, could you clone the branch in a new branch, rebase it on top of main and create a different PR from that new branch?

#67461

@AlekseyTs
Copy link
Contributor

#67461

CodeFlow works for that PR. I suggest continue working in that branch.

@jjonescz
Copy link
Member

CodeFlow works for that PR. I suggest continue working in that branch.

Alright. Closing this in favor of #67461.

@jjonescz jjonescz closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An object reference is required in nameof in attribute usage

7 participants