Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File modifier parsing #60823
File modifier parsing #60823
Changes from 11 commits
4fb5734
fb624a1
e4efb02
eff66b0
24a1f0d
3f854fd
9fee3df
573ff74
0bd3b9e
877ad9f
9698453
228845e
310528e
bb96370
a87fb15
d7c4b69
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't love hte feature flag check actually controlling behavior. i think we should just parse this out if ShouldContextualKeywordBeTreatedAsModifier returns true and then report an issue later if the feature is not available. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. This is also the cause of a breaking change in the new language version. This is also what we are doing for 'required' modifier. See related discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up biasing our error recovery involving 'file' to: you meant to declare a member that uses this modifier, versus "you must have been using it as an identifier". Compare the _CSharp10 versions of tests to their CSharpNext or unsuffixed counterparts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider explicitly stating which contextual keywords this is expected to undertand. i think it's nicer if the reader can know "ok, this is just for async/file/etc." as opposed to "oh... what agbout all the other contextual keywords that we might have??"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, consider passing in the token type we are looking to treat as a modifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now check GetModifier in the assert which achieves what we want--don't need to repeat ourselves here, and we are limited to allowing the contextual keywords which are modifiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this special case of AsyncKeyword correct now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't treat 'file' as a modifier when parsing a local declaration, just like we don't treat 'required' as a modifier on local declarations in the required-members feature branch.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.