Report diagnostic for field and value identifiers in property accessors where the meaning would change as contextual keywords
#73570
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
cston
changed the title
field and value identifiers in property accessors where the meaning would change as contextual keywords
cston
force-pushed
the
keyword-diagnostic
branch
3 times, most recently
from
96ec041
to
80b4999
Compare
…rs where the meaning would change as contextual keywords
|
|
||
| [Theory] | ||
| [CombinatorialData] | ||
| public void Event_01( |
There was a problem hiding this comment.
I think value should be a keyword as that parameter exists in the add and remove accessors today (see SynthesizedEventAccessorValueParameterSymbol), like it does in a property setter (SynthesizedPropertyAccessorValueParameterSymbol).
But I don't think field will take any particular meaning in events with the C#Next feature, so I don't think it should be a keyword.
What do you think?
There was a problem hiding this comment.
this matches my takeaway from last LDM on this topic.
| default: | ||
| // If we reach here, add the unhandled identifier case to the | ||
| // switch cases above (and test in FieldAndValueKeywordTests). | ||
| Debug.Assert(false, $"Unhandled identifier: parent {syntax.Kind()}, token {token}"); |
There was a problem hiding this comment.
I don't think we should need to update this method when new syntax nodes are added to C# 14, 15, ... (we'll produce an error already when the LangVer is too old) #Closed
| // (14,21): info CS9248: 'value' is a contextual keyword, with a specific meaning, starting in language version preview. Use '@value' to avoid a breaking change when compiling with language version preview or later. | ||
| // void G1(object value) { } | ||
| Diagnostic(ErrorCode.INF_IdentifierConflictWithContextualKeyword, "object value").WithArguments("value", "preview").WithLocation(14, 21)); | ||
| } |
There was a problem hiding this comment.
nit: Consider adding a test for attribute scenarios, even though they are being kept out-of-scope for this PR:
- [MyAttribute(nameof(value))]
- attribute target scenario:
[field: MyAttribute]and[@field: MyAttribute], on accessors #Closed
| break; | ||
| } | ||
| break; | ||
| default: |
There was a problem hiding this comment.
TBH i find hte approach taken in this pr extremely surprising. It's not how i would have expected this to be done.
Instead, i would expect us to treat his how we treat query-keywords or the await-keyword. Specifically, the parser should enter into a new contextual parsing state when we enter into these specific contexts.
Within that appropriate context, we then have the following behavior:
- in c# 13, the word becomes a keyword, and the new semantics take effect.
- prior to c# 12, when eating an identifier, if we have one of these keywords, we convert it to an identifier, and report the diagnostics about the potential future issues.
I think we also need this for incremental parsing to work properly. If we reinterpret the statements from an accessor into a different context (or vice-versa), then we need to reparse since field/value will change from keywords to identifiers.
--
TLDR: as a contextual keyword, this should work the same way as async/await and query-keywords.
There was a problem hiding this comment.
Note: i was able to get us additional bits on NodeFlags. So we can store this info there as well for incremental parsing purposes. A
There was a problem hiding this comment.
- prior to c# 12, when eating an identifier, if we have one of these keywords, we convert it to an identifier, and report the diagnostics about the potential future issues.
The context from the parser might help, but the diagnostic may need to be reported in binding since we only want a diagnostic when the identifier would bind differently as a keyword.
There was a problem hiding this comment.
my approach supports that. these would be identifiers whose contextual kind would either be 'nothing' (which means it's a legal usage of field/value as an identifier), or it has a contextual kind of SyntaxKind.FieldKeyword or SyntaxKind.ValueKeyword. IN that case, in binding you report a warning saying this will change meaning.
basically, the parser has the core logic for figuring out if it's an identifier or keyword. if it's a true identifier, great, you get an identifier and everything works as normal. If it's contextual keyword, then in C# 13 you get a keyword and the new semantics. In C# 12 you get an identifier, marked as being a contextual keyword, and the binder warns you.
Note: i don't actually see why it as to be the binder that needs to care. Sicne C# 13 will have these rules set up at the syntactic level, it feels fine for the parser to warn here for me. But i'm fine with it beign the binder if you prefer that.
There was a problem hiding this comment.
I think what Chuck is getting at is that, for example, there's no problem with using identifier 'value' to reference the setter value parameter. The parsing will technically change from C# 12 to 13, but it will still refer to the same thing in its keyword form.
So in order to know whether to give diagnostics, compiler needs to know whether 'value' is referring to that parameter, or referring to something else such as a 'value' local declared in a nested function.
FWIW, in the case I just described, I feel ok with saying you did something really weird and you don't get to have this warning. It's the .01% case.
Maybe we should only warn for use of identifier 'field' in property accessors and just do it in the parser.
There was a problem hiding this comment.
my approach would work for this afaict. the node indicates the information needed by the binder to warn.
| } | ||
| else | ||
| { | ||
| comp.VerifyEmitDiagnostics( |
| } | ||
| else | ||
| { | ||
| comp.VerifyEmitDiagnostics( |
| Diagnostic(ErrorCode.INF_IdentifierConflictWithContextualKeyword, "int value").WithArguments("value", "preview").WithLocation(21, 40), | ||
| // (28,23): info CS9248: 'value' is a contextual keyword, with a specific meaning, starting in language version preview. Use '@value' to avoid a breaking change when compiling with language version preview or later. | ||
| // [A(nameof(value))] void F3() { } | ||
| Diagnostic(ErrorCode.INF_IdentifierConflictWithContextualKeyword, "value").WithArguments("value", "preview").WithLocation(28, 23)); |
|
Done with review pass (commit 15) |
| @@ -220,6 +220,11 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno | |||
| CheckFeatureAvailability(_syntax.AwaitKeyword, MessageID.IDS_FeatureAsyncStreams, diagnostics); | |||
| } | |||
|
|
|||
| if (_syntax is ForEachStatementSyntax forEachStatement) | |||
| { | |||
| ReportFieldOrValueContextualKeywordConflictIfAny(forEachStatement, forEachStatement.Identifier, diagnostics); | |||
There was a problem hiding this comment.
nit: have you considered placing this check in the switch below that already differentiates regular foreach from deconstruction foreach? (line 277)
Report a diagnostic for
fieldandvaluein contexts where the identifiers will be treated as keywords in a later language version and where the meaning will change as a result:fieldin a property or event accessorvaluein a property or indexersetorinitaccessor, or event accessor, and when the identifier does not refer to the implicit parameterRelates to test plan #57012