Move feature check from lexer to binder. #57113
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CyrusNajmabadi
commented
Oct 12, 2021
| @@ -18,6 +18,10 @@ internal partial class Binder | |||
| { | |||
| private BoundExpression BindInterpolatedString(InterpolatedStringExpressionSyntax node, BindingDiagnosticBag diagnostics) | |||
| { | |||
| var startText = node.StringStartToken.Text; | |||
| if (startText.StartsWith("@$\"")) | |||
| CheckFeatureAvailability(node, MessageID.IDS_FeatureAltInterpolatedVerbatimStrings, diagnostics, node.StringStartToken.GetLocation()); | |||
There was a problem hiding this comment.
one single check in one location.
CyrusNajmabadi
commented
Oct 12, 2021
| if (feature == MessageID.IDS_FeatureAltInterpolatedVerbatimStrings) | ||
| { | ||
| return new CSDiagnosticInfo(ErrorCode.ERR_AltInterpolatedVerbatimStringsNotAvailable, new CSharpRequiredLanguageVersion(feature.RequiredVersion())); | ||
| } |
There was a problem hiding this comment.
preserves the special error message we give here (the parser used to special case this as well).
CyrusNajmabadi
commented
Oct 12, 2021
| if (isAltInterpolatedVerbatim) | ||
| { | ||
| openQuote = CheckFeatureAvailability(openQuote, MessageID.IDS_FeatureAltInterpolatedVerbatimStrings); | ||
| } |
CyrusNajmabadi
commented
Oct 12, 2021
| @@ -771,7 +771,6 @@ private void ScanSyntaxToken(ref TokenInfo info) | |||
| else if (TextWindow.PeekChar(1) == '$' && TextWindow.PeekChar(2) == '"') | |||
| { | |||
| this.ScanInterpolatedStringLiteral(isVerbatim: true, ref info); | |||
| CheckFeatureAvailability(MessageID.IDS_FeatureAltInterpolatedVerbatimStrings); | |||
There was a problem hiding this comment.
lexer check #1. intended to be overridden by parser.
CyrusNajmabadi
commented
Oct 12, 2021
| @@ -628,7 +628,6 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool | |||
| } | |||
| else if (_lexer.TextWindow.PeekChar(1) == '$' && _lexer.TextWindow.PeekChar(2) == '"') | |||
| { | |||
| _lexer.CheckFeatureAvailability(MessageID.IDS_FeatureAltInterpolatedVerbatimStrings); | |||
There was a problem hiding this comment.
lexer check #2, during recursive interpolated string lexing. Intended to be overridden by parser.
CyrusNajmabadi
commented
Oct 12, 2021
| return availableVersion >= requiredVersion | ||
| ? node | ||
| : this.AddError(node, ErrorCode.ERR_AltInterpolatedVerbatimStringsNotAvailable, | ||
| new CSharpRequiredLanguageVersion(requiredVersion)); |
There was a problem hiding this comment.
special case error message. moved to https://github.com/dotnet/roslyn/pull/57113/files#diff-a1cb0e0d2615ca34e6c6cfc96a0300cb85c046ca6f90bf0d8931021ec8ccc480R334
cston
reviewed
Oct 12, 2021
cston
reviewed
Oct 12, 2021
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
cston
reviewed
Oct 13, 2021
CyrusNajmabadi
force-pushed
the
moveFeatureCheck
branch
from
e22c1a1 to
267fc94
Compare
cston
approved these changes
Oct 13, 2021
|
@dotnet/roslyn-compiler @jcouv for another set of eyes. Thanks! |
333fred
approved these changes
Oct 13, 2021
ghost
added this to the
|
Thanks! |
|
Please remember to squash compiler changes. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note: this is not part of more work to do this in other locations.
This change is very specific as the way this current check works is complex, difficult to sustain for raw-string-literals, and also (IMO) violates how the lexer/parser should work.
Specifically, the lexer, today, needs to recurse back into itself when dealing with interpolated string literals. It does this both at lex time (to make the initial token and figure out interpolation boundaries), and it then does it again when teh parser comes in and makes the real InterpolationExpressionSyntax. As such, it is both confusing, and error-prone for errors to be added here in the interpolation holes as both the lexer and parser take over the same responsibility. Indeed, it is necessary for these lexer errors to not bubble up as they must get reported now by the parser. This complicates lexing (esp. interpolation lexing) by effectively making it so that some errors are reported internally, but not bubbled up. And some errors are reported internally, stored, and bubbled up to the parser.
I do not want to try to maintain/duplicate that conceptual complexity. As such, i'm moving this logic entirely to the binder. This has the benefit of reverse-smearing things. What was three checks, with a complex relation betwen them, becomes one simple check.