Parse attributes on local function declarations #38808
Conversation
Definitely a way to go! Alternatively, previously, i made it so that any MemberDeclarationSyntax could have Attributes. If the impl is not too complex here, you might want to consider making it so that any statement could have attributes on it, and that we just bail on them for anything but local functions. This has hte virtue of embedding them in our syntactic model consistently, and making it much easier to represent in the tree for editing/recovery scenario. If this is complex, do not do it. BUt def something to maybe consider :) |
| @@ -7177,8 +7182,9 @@ private bool IsPossibleStatement(bool acceptAccessibilityMods) | |||
| case SyntaxKind.InternalKeyword: | |||
| case SyntaxKind.ProtectedKeyword: | |||
| case SyntaxKind.PrivateKeyword: | |||
| // could be a local function with attributes | |||
| case SyntaxKind.OpenBracketToken: // PROTOTYPE reuse this or always allow the OpenBracketToken? | |||
There was a problem hiding this comment.
i would always allow it.
There was a problem hiding this comment.
Will revisit this when it comes time to remove the prototype comments before merging to master.
src/Compilers/CSharp/Portable/Syntax/LocalFunctionStatementSyntax.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Syntax/LocalFunctionStatementSyntax.cs
Outdated
Show resolved
Hide resolved
|
Looking good! |
|
Thanks for your feedback, will look into adding AttributeLists to StatementSyntax. (instinctively I wonder if that would be a significant amount of churn in LanguageParser?) Would this make it so if we have some error recovery scenario where maybe some local declaration doesn't parse as a local function, but it happens to have attributes, we could give better diagnostics? |
|
I don't think this PR should target |
RikkiGibson
changed the base branch from
master
to
feature/local-function-attributes
|
I used the wrong naming convention for the base branch 🤦♂ |
RikkiGibson
changed the base branch from
feature/local-function-attributes
to
features/local-function-attributes
|
@dotnet/roslyn-compiler Please review |
src/Compilers/CSharp/Test/Syntax/Parsing/LocalFunctionParsingTests.cs
Outdated
Show resolved
Hide resolved
|
Could I please get a second review @dotnet/roslyn-compiler |
What happens if an attribute is applied to a standard local variable? Looks like the parser will parse out the attribute but then not attach it to any other node. Won't that cause a consistency issue in the parse tree? Feel like we should have tests for this. Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:4145 in 2c251ee. [](commit_id = 2c251ee, deletion_comment = False) |
|
Review pass complete (iteration 9) In reply to: 535713832 [](ancestors = 535713832) |
|
We have tests which include an unexpected attribute on a local variable declaration, but I can go into more detail in them to show exactly how to access the unexpected attribute if you’d like. |
|
Where are the tests? If the tests do exist why are they not tripping up the new error you just added about requiring a new version of C#? I would presume the old tests are still using langversion 8.0 hence they should trip up the new error. Also is there a test where an attribute is used on a local declaration statement that has multiple locals? In reply to: 536000459 [](ancestors = 536000459) |
|
I have a few local declaration with attribute parsing tests in here but some already exist such as The LangVersion error is only going to happen if the attribute was on a local function. The diagnostics for putting an attribute on any statement besides LocalFunctionStatement aren't expected to change. I'm not aware of a test which tries to parse a local variable declaration with multiple declarators and with attributes. If you think that would be useful I can add it. |
| { | ||
| void M() | ||
| { | ||
| [A] object local1, local2; |
There was a problem hiding this comment.
What hapens if the attribute is before the second local?
There was a problem hiding this comment.
When the attribute comes after the type the parser believes it's some kind of bad array declarator.
|
I ended up sticking with AttributeLists on the LocalFunctionStatement and the main reason is that Modifiers is also declared on LocalFunctionStatement (not inherited) and it feels like the two should be consistent with each other. |
I agree. Though note this is something that can change in the future. Fields can "move up" the inheritance hierachy without problems. As i mentioned before, i did this for modifiers/attributes on MemberDeclarationSyntax. If you're ok with it, i may try to make the attributes-approach more generalized here in your branch. |
|
That makes sense, if you send the PR I am happy to review 😄 |
Related to #38801
Attributes on local function parameters or type parameters are already parsed as expected but they simply give errors. This PR adds an AttributeLists property to LocalFunctionStatementSyntax and parses the attributes into that property.