Ensure to get the name of nameof argument #52016
Conversation
jcouv
added
the
Community
| @@ -1874,7 +1874,7 @@ private void EnsureNameofExpressionSymbols(BoundMethodGroup methodGroup, Binding | |||
| /// <summary> | |||
| /// Returns true if syntax form is OK (so no errors were reported) | |||
| /// </summary> | |||
| private bool CheckSyntaxForNameofArgument(ExpressionSyntax argument, out string name, BindingDiagnosticBag diagnostics, bool top = true) | |||
| private bool CheckSyntaxForNameofArgument(ExpressionSyntax argument, out string name, BindingDiagnosticBag diagnostics, bool boundArgumentHasAnyErrors, bool top = true) | |||
There was a problem hiding this comment.
bool boundArgumentHasAnyErrors [](start = 128, length = 30)
Instead of adding this parameter we should consider passing BindingDiagnosticBag.Discarded when we are trying to suppress cascading diagnostics. #Closed
There was a problem hiding this comment.
@AlekseyTs That simplifies it alot! 🎉
| // We relax the instance-vs-static requirement for top-level member access expressions by creating a NameofBinder binder. | ||
| var nameofBinder = new NameofBinder(argument, this); | ||
| var boundArgument = nameofBinder.BindExpression(argument, diagnostics); | ||
| if (!boundArgument.HasAnyErrors && CheckSyntaxForNameofArgument(argument, out name, diagnostics) && boundArgument.Kind == BoundKind.MethodGroup) | ||
| var syntaxIsOk = CheckSyntaxForNameofArgument(argument, out string name, diagnostics, boundArgument.HasAnyErrors); |
There was a problem hiding this comment.
var syntaxIsOk = CheckSyntaxForNameofArgument(argument, out string name, diagnostics, boundArgument.HasAnyErrors); [](start = 12, length = 114)
We should add tests that observe this behavior change via SemanticModel/IOperation and possibly other APIs. #Closed
|
Done with review pass (commit 3) #Closed |
| // We relax the instance-vs-static requirement for top-level member access expressions by creating a NameofBinder binder. | ||
| var nameofBinder = new NameofBinder(argument, this); | ||
| var boundArgument = nameofBinder.BindExpression(argument, diagnostics); | ||
| if (!boundArgument.HasAnyErrors && CheckSyntaxForNameofArgument(argument, out name, diagnostics) && boundArgument.Kind == BoundKind.MethodGroup) | ||
|
|
||
| bool syntaxIsOk = CheckSyntaxForNameofArgument(argument, out string name, boundArgument.HasAnyErrors ? BindingDiagnosticBag.Discarded : diagnostics); |
There was a problem hiding this comment.
I didn't want to introduce a local for this conditional expression to prevent it from being used accidentally. (See few lines later which uses diagnostics regardless syntaxIsOk)
| } | ||
| Block[B2] - Exit | ||
| Predecessors: [B1] | ||
| Statements (0) | ||
| "; | ||
| VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(source, expectedFlowGraph, expectedDiagnostics); |
There was a problem hiding this comment.
VerifyFlowGraphAndDiagnosticsForTest [](start = 12, length = 36)
Did you really mean to validate a flow graph or just copied "wrong" test?
There was a problem hiding this comment.
@AlekseyTs, Ah I think I wanted to validate the operation tree instead. But probably okay to keep it since the ILiteralOperation's constant value should be affected by this change. Let me know if this alone is sufficient, or if I should validate both flow graph and operation tree, or operation tree only.
|
@dotnet/roslyn-compiler Please review. Need a second sign-off for a small community PR. |
3 similar comments
|
@dotnet/roslyn-compiler Please review. Need a second sign-off for a small community PR. |
|
@dotnet/roslyn-compiler Please review. Need a second sign-off for a small community PR. |
|
@dotnet/roslyn-compiler Please review. Need a second sign-off for a small community PR. |
ghost
added this to the 
|
Thanks @Youssef1313! |
Fixes #52013.