Disallow native ints inside nameof by Youssef1313 · Pull Request #47497 · dotnet/roslyn

Youssef1313 · 2020-09-06T08:48:21Z

Related to #47128

Youssef1313 · 2020-09-06T10:01:29Z

                if (node is IdentifierNameSyntax identifier)
                {
-                    var type = BindNativeIntegerSymbolIfAny(identifier, diagnostics);
+                    var type = IsInsideNameof ? null : BindNativeIntegerSymbolIfAny(identifier, diagnostics);


int isn't allowed in nameof. However, something like nameof(int.MaxValue) is allowed.

Is there similar thing for nint? e.g. nameof(nint.Something) which should be allowed?

If there is something like that, my approach will probably fail.

Edit:

Probably nameof(nint.Equals) should be allowed. Let me add a test for it.

Edit 2:

Added a check for parent node to fix this issue.

Youssef1313 · 2020-09-06T11:56:20Z

-                    var type = BindNativeIntegerSymbolIfAny(identifier, diagnostics);
+                    // Don't bind nameof(nint) or nameof(nuint) so that ERR_NameNotInContext is reported.
+                    // The check for ArgumentSyntax is to allow the binding for something like nameof(nint.Equals).
+                    var type = IsInsideNameof && identifier.Parent is ArgumentSyntax


Do I need to check that the parent of this ArgumentSyntax is an ArgumentListSyntax of an InvocationExpressionSyntax whose Expression is an IdentifierNameSyntax with IdentifierToken.Text equals to nameof?
That would be a large check that I don't know it's necessary or not.

For the above to make some sense, refer to the following visualization while reading it 😄

Youssef1313

In the current master, nameof(dynamic) generates an invalid operation. While nameof(nint) generates "none" operation. I'm not sure if my change will fix that, or if that even needs to be fixed. (Given that nameof(int) generates "none")

cston

Thanks @Youssef1313.

jcouv · 2020-09-09T21:31:49Z

+                        identifier.Parent?.Parent?.Parent is InvocationExpressionSyntax invocation &&
+                        (invocation.Expression as IdentifierNameSyntax)?.Identifier.ContextualKind() == SyntaxKind.NameOfKeyword
+                        ? null
+                        : BindNativeIntegerSymbolIfAny(identifier, diagnostics);


nit: could we move all the extra logic into BindNativeIntegerSymbolIfAny? It would help encapsulate the complexity away from sight a bit.

@jcouv There is another call for BindNativeIntegerSymbolIfAny:

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

Line 893 in 45f2aed

bindingResult = BindNativeIntegerSymbolIfAny(node, diagnostics);

I don't know if there would be unintended effects of changing the method itself. That's why I kept the change outside it to only affect this code path.

If you can confirm that both method calls need this change, I'll do that.

The second caller doesn't need this change, but won't be hurt (the IsInsideNameof check is fast).

@cston Just to confirm, what do you think of my proposal (move the new logic inside BindNativeIntegerSymbolIfAny)?

what do you think of my proposal (move the new logic inside BindNativeIntegerSymbolIfAny)?

Sounds reasonable. And it would mean we could avoid the extra check unless the identifier is "nint" or "nuint".

In reply to: 485950715 [](ancestors = 485950715)

jcouv

Done with review pass (iteration 14). Minor comment

Youssef1313 · 2020-09-10T10:57:32Z

@cston Moved the logic to BindNativeIntegerSymbolIfAny and updated its comment to be clear about that.

Do you want me to squash all the commits to avoid any noise in master branch?

cston · 2020-09-10T16:06:23Z

Do you want me to squash all the commits to avoid any noise in master branch?

No, please avoid squashing commits while the PR is being reviewed. We'll Squash and Merge when merging the change.

Youssef1313 · 2020-09-11T13:56:04Z

@cston An unrelated test has failed. Can you re-run the failed job?

jaredpar · 2020-09-11T14:05:15Z

Hit the re-run on it.

jcouv

LGTM Thanks! (iteration 18)

jcouv · 2020-09-11T18:06:30Z

Merged/squashed. Thanks for the fix @Youssef1313 !

Disallow native ints inside nameof

9623651

Youssef1313 requested a review from a team as a code owner September 6, 2020 08:48

[NameofTests] Tests for dynamic and nint when both defined and not

17bec34

Youssef1313 commented Sep 6, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated

Youssef1313 added 2 commits September 6, 2020 10:57

Update NameOfTests.cs

03c866e

Update NativeIntegerTests.cs

f4ee931

Youssef1313 commented Sep 6, 2020

View reviewed changes

Youssef1313 added 3 commits September 6, 2020 12:36

Update NativeIntegerTests.cs

2ef7eb2

Relax binding restriction to allow for nameof(nint.Equals)

77cbd28

Update NativeIntegerTests.cs

40bf56f

Youssef1313 commented Sep 6, 2020

View reviewed changes

Fix expected diagnostics

942673b

jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 6, 2020

cston reviewed Sep 8, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs Outdated

cston reviewed Sep 8, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs

cston reviewed Sep 8, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated

Youssef1313 added 4 commits September 9, 2020 14:03

Apply suggestion

8b754c2

Add TestTypeArguments

94d8835

Update NameOfTests.cs

9f39249

Update NameOfTests.cs

e8cf43a

Youssef1313 mentioned this pull request Sep 9, 2020

Use the correct feature name for error message #47518

Merged

cston reviewed Sep 9, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated

cston reviewed Sep 9, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs

Youssef1313 added 2 commits September 9, 2020 18:40

Split long line

112a05c

Add TestNameOfTypeOf

d7b4e42

cston approved these changes Sep 9, 2020

View reviewed changes

jcouv reviewed Sep 9, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated

jcouv reviewed Sep 9, 2020

View reviewed changes

jcouv self-assigned this Sep 9, 2020

Add TestNameOfNintWhenTheyAreIdentifierNames

f73a5f6

cston reviewed Sep 9, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs Outdated

Youssef1313 and others added 2 commits September 10, 2020 00:38

Use CreateCompilation

52fb692

Move logic to BindNativeIntegerSymbolIfAny

040be8e

Update Binder_Symbols.cs

7d28fdb

cston approved these changes Sep 10, 2020

View reviewed changes

jcouv approved these changes Sep 11, 2020

View reviewed changes

jcouv merged commit af4fe0d into dotnet:master Sep 11, 2020

ghost added this to the Next milestone Sep 11, 2020

Youssef1313 deleted the patch-27 branch September 11, 2020 18:17

dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020

Conversation

Youssef1313 commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Youssef1313 Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

Youssef1313 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcouv Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

cston Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Youssef1313 commented Sep 10, 2020

Uh oh!

cston commented Sep 10, 2020

Uh oh!

Youssef1313 commented Sep 11, 2020

Uh oh!

jaredpar commented Sep 11, 2020

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Youssef1313 commented Sep 6, 2020 •

edited

Loading

Youssef1313 Sep 6, 2020 •

edited

Loading

Youssef1313 left a comment •

edited

Loading