Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow newlines in the holes inside interpolated strings #56853

Merged
merged 21 commits into from
Oct 25, 2021
Merged

Allow newlines in the holes inside interpolated strings #56853

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8ed75ec
Allow newlines in the holes inside interpolated strings
CyrusNajmabadi Sep 29, 2021
fe7c2d9
Update tests
CyrusNajmabadi Sep 29, 2021
10a0fb5
Update tests
CyrusNajmabadi Sep 29, 2021
06afc0b
Remove unnecessary extra error
CyrusNajmabadi Sep 29, 2021
3a8f99b
Use simple using
CyrusNajmabadi Sep 29, 2021
7dffb4b
Inline variables
CyrusNajmabadi Sep 29, 2021
dde9fdc
Use named parameters
CyrusNajmabadi Sep 29, 2021
61bcfbb
Simplify
CyrusNajmabadi Sep 29, 2021
bde11ad
Move error to binding phase
CyrusNajmabadi Oct 14, 2021
437fe79
Add tests
CyrusNajmabadi Oct 14, 2021
edd0497
Update optinos
CyrusNajmabadi Oct 14, 2021
da75881
remove
CyrusNajmabadi Oct 14, 2021
9dd3189
Update comment
CyrusNajmabadi Oct 14, 2021
d5eb8f6
Remove prototype
CyrusNajmabadi Oct 15, 2021
1b05285
PR feedback and comment
CyrusNajmabadi Oct 15, 2021
f0b1cd8
fix loc strings
CyrusNajmabadi Oct 15, 2021
3ec40db
Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs
CyrusNajmabadi Oct 25, 2021
2459df5
Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs
CyrusNajmabadi Oct 25, 2021
1e445cb
Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs
CyrusNajmabadi Oct 25, 2021
65e91b2
Update language string
CyrusNajmabadi Oct 25, 2021
59e4649
Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs
CyrusNajmabadi Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ private BoundExpression BindInterpolatedString(InterpolatedStringExpressionSynta
}
else
{
var isNonVerbatimInterpolatedString = node.StringStartToken.Kind() != SyntaxKind.InterpolatedVerbatimStringStartToken;
var newLinesInInterpolationsDiagnosticInfo =
MessageID.IDS_FeatureNewLinesInInterpolations.GetFeatureAvailabilityDiagnosticInfo((CSharpParseOptions)node.SyntaxTree.Options);
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved

var intType = GetSpecialType(SpecialType.System_Int32, diagnostics, node);
foreach (var content in node.Contents)
{
Expand All @@ -37,6 +41,23 @@ private BoundExpression BindInterpolatedString(InterpolatedStringExpressionSynta
case SyntaxKind.Interpolation:
{
var interpolation = (InterpolationSyntax)content;
if (isNonVerbatimInterpolatedString &&
!interpolation.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error) &&
newLinesInInterpolationsDiagnosticInfo != null &&
!interpolation.OpenBraceToken.IsMissing &&
!interpolation.CloseBraceToken.IsMissing)
{
var text = node.SyntaxTree.GetText();
if (text.Lines.GetLineFromPosition(interpolation.OpenBraceToken.SpanStart).LineNumber !=
text.Lines.GetLineFromPosition(interpolation.CloseBraceToken.SpanStart).LineNumber)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: i can also do this by just walking the tree. up to you if want me to go that route or not. this is def the easiest and most fool proof mechanism to determine this though.

{
diagnostics.Add(
ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString,
Copy link
Member

@jcouv jcouv Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remembered... We'll need to add this error code the the UpgradeProject fixer. That's the fixer that handles diagnostics that include a CSharpRequiredLanguageVersion.
This can be tracked by a follow-up issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

interpolation.CloseBraceToken.GetLocation(),
new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureNewLinesInInterpolations.RequiredVersion()));
}
}

var value = BindValue(interpolation.Expression, diagnostics, BindValueKind.RValue);

// We need to ensure the argument is not a lambda, method group, etc. It isn't nice to wait until lowering,
Expand Down
7 changes: 5 additions & 2 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6852,7 +6852,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>At least one top-level statement must be non-empty.</value>
</data>
<data name="ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString" xml:space="preserve">
<value>Newlines are not allowed inside a non-verbatim interpolated string</value>
<value>Please use language version {0} or later to use newlines inside a non-verbatim interpolated string</value>
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="IDS_FeatureGenericAttributes" xml:space="preserve">
<value>generic attributes</value>
Expand All @@ -6872,4 +6872,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_CompileTimeCheckedOverflow_Title" xml:space="preserve">
<value>The operation may overflow at runtime (use 'unchecked' syntax to override)</value>
</data>
</root>
<data name="IDS_FeatureNewLinesInInterpolations" xml:space="preserve">
<value>new-lines in interpolations</value>
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,7 +1308,8 @@ internal enum ErrorCode
ERR_DictionaryInitializerInExpressionTree = 8074,
ERR_ExtensionCollectionElementInitializerInExpressionTree = 8075,
ERR_UnclosedExpressionHole = 8076,
ERR_SingleLineCommentInExpressionHole = 8077,
// This is now handled by the single ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString error.
// ERR_SingleLineCommentInExpressionHole = 8077,
ERR_InsufficientStack = 8078,
ERR_UseDefViolationProperty = 8079,
ERR_AutoPropertyMustOverrideSet = 8080,
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,9 @@ internal enum MessageID
IDS_FeatureParameterlessStructConstructors = MessageBase + 12810,
IDS_FeatureStructFieldInitializers = MessageBase + 12811,
IDS_FeatureGenericAttributes = MessageBase + 12812,

// PROTOTYPE: Update this number
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
IDS_FeatureNewLinesInInterpolations = MessageBase + 12813,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -345,6 +348,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
// C# preview features.
case MessageID.IDS_FeatureStaticAbstractMembersInInterfaces: // semantic check
case MessageID.IDS_FeatureGenericAttributes: // semantic check
case MessageID.IDS_FeatureNewLinesInInterpolations: // semantic check
return LanguageVersion.Preview;

// C# 10.0 features.
Expand Down
41 changes: 19 additions & 22 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser_InterpolatedString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ private ExpressionSyntax ParseInterpolatedStringToken()
}
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view PR with whitespace changes off.


// Add a token for text following the last interpolation
var lastText = Substring(originalText, interpolations[interpolations.Count - 1].CloseBracePosition + 1, closeQuoteIndex - 1);
var lastText = Substring(originalText, interpolations[^1].CloseBracePosition + 1, closeQuoteIndex - 1);
if (lastText.Length > 0)
{
var token = MakeStringToken(lastText, lastText, isVerbatim, SyntaxKind.InterpolatedStringTextToken);
Expand Down Expand Up @@ -171,29 +171,26 @@ private InterpolationSyntax ParseInterpolation(string text, Lexer.Interpolation
using (var tempLexer = new Lexer(Text.SourceText.From(parsedText), this.Options, allowPreprocessorDirectives: false, interpolationFollowedByColon: interpolation.HasColon))
{
// TODO: some of the trivia in the interpolation maybe should be trailing trivia of the openBraceToken
using (var tempParser = new LanguageParser(tempLexer, null, null))
using var tempParser = new LanguageParser(tempLexer, oldTree: null, changes: null);

tempParser.ParseInterpolationStart(out openBraceToken, out expression, out var commaToken, out var alignmentExpression);
if (alignmentExpression != null)
{
SyntaxToken commaToken = null;
ExpressionSyntax alignmentExpression = null;
tempParser.ParseInterpolationStart(out openBraceToken, out expression, out commaToken, out alignmentExpression);
if (alignmentExpression != null)
{
alignment = SyntaxFactory.InterpolationAlignmentClause(commaToken, alignmentExpression);
}
alignment = SyntaxFactory.InterpolationAlignmentClause(commaToken, alignmentExpression);
}

var extraTrivia = tempParser.CurrentToken.GetLeadingTrivia();
if (interpolation.HasColon)
{
var colonToken = SyntaxFactory.Token(SyntaxKind.ColonToken).TokenWithLeadingTrivia(extraTrivia);
var formatText = Substring(text, interpolation.ColonPosition + 1, interpolation.FormatEndPosition);
var formatString = MakeStringToken(formatText, formatText, isVerbatim, SyntaxKind.InterpolatedStringTextToken);
format = SyntaxFactory.InterpolationFormatClause(colonToken, formatString);
}
else
{
// Move the leading trivia from the insertion's EOF token to the following token.
closeBraceToken = closeBraceToken.TokenWithLeadingTrivia(extraTrivia);
}
var extraTrivia = tempParser.CurrentToken.GetLeadingTrivia();
if (interpolation.HasColon)
{
var colonToken = SyntaxFactory.Token(SyntaxKind.ColonToken).TokenWithLeadingTrivia(extraTrivia);
var formatText = Substring(text, interpolation.ColonPosition + 1, interpolation.FormatEndPosition);
var formatString = MakeStringToken(formatText, formatText, isVerbatim, SyntaxKind.InterpolatedStringTextToken);
format = SyntaxFactory.InterpolationFormatClause(colonToken, formatString);
}
else
{
// Move the leading trivia from the insertion's EOF token to the following token.
closeBraceToken = closeBraceToken.TokenWithLeadingTrivia(extraTrivia);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Parser/Lexer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@ private void ScanSyntaxToken(ref TokenInfo info)
case '@':
if (TextWindow.PeekChar(1) == '"')
{
var errorCode = this.ScanVerbatimStringLiteral(ref info, allowNewlines: true);
var errorCode = this.ScanVerbatimStringLiteral(ref info);
if (errorCode is ErrorCode code)
this.AddError(code);
}
Expand Down
69 changes: 16 additions & 53 deletions src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private char ScanEscapeSequence(out char surrogateCharacter)
/// <summary>
/// Returns an appropriate error code if scanning this verbatim literal ran into an error.
/// </summary>
private ErrorCode? ScanVerbatimStringLiteral(ref TokenInfo info, bool allowNewlines)
private ErrorCode? ScanVerbatimStringLiteral(ref TokenInfo info)
{
_builder.Length = 0;

Expand Down Expand Up @@ -180,14 +180,6 @@ private char ScanEscapeSequence(out char surrogateCharacter)
break;
}

// If we hit a new line when it's not allowed. Give an error at that new line, but keep on consuming
// the verbatim literal to the end to avoid the contents of the string being lexed as C# (which will
// cause a ton of cascaded errors). Only need to do this on the first newline we hit.
if (!allowNewlines && SyntaxFacts.IsNewLine(ch))
{
error ??= ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString;
}

TextWindow.AdvanceChar();
_builder.Append(ch);
}
Expand Down Expand Up @@ -272,7 +264,6 @@ private class InterpolatedStringScanner
{
private readonly Lexer _lexer;
private bool _isVerbatim;
private bool _allowNewlines;

/// <summary>
/// There are two types of errors we can encounter when trying to scan out an interpolated string (and its
Expand All @@ -285,18 +276,15 @@ private class InterpolatedStringScanner
public SyntaxDiagnosticInfo? Error;
private bool EncounteredUnrecoverableError;

public InterpolatedStringScanner(
Lexer lexer,
bool isVerbatim)
public InterpolatedStringScanner(Lexer lexer, bool isVerbatim)
{
_lexer = lexer;
_isVerbatim = isVerbatim;
_allowNewlines = isVerbatim;
}

private bool IsAtEnd()
{
return IsAtEnd(_isVerbatim && _allowNewlines);
return IsAtEnd(_isVerbatim);
}

private bool IsAtEnd(bool allowNewline)
Expand Down Expand Up @@ -529,15 +517,8 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool
{
char ch = _lexer.TextWindow.PeekChar();

// See if we ran into a disallowed new line. If so, this is recoverable, so just skip past it but
// give a good message about the issue. This will prevent a lot of cascading issues with the
// remainder of the interpolated string that comes on the following lines.
var allowNewLines = _isVerbatim && _allowNewlines;
if (!allowNewLines && SyntaxFacts.IsNewLine(ch))
{
TrySetRecoverableError(_lexer.MakeError(_lexer.TextWindow.Position, width: 0, ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString));
}

// Note: within a hole new-lines are always allowed. The retriction on if new-lines are allowed or not
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
// is only within a text-portion of the interpolated stirng.
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
if (IsAtEnd(allowNewline: true))
{
// the caller will complain
Expand All @@ -558,17 +539,14 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool
var interpolations = (ArrayBuilder<Interpolation>?)null;
var info = default(TokenInfo);
bool wasVerbatim = _isVerbatim;
bool wasAllowNewlines = _allowNewlines;
try
{
_isVerbatim = isVerbatimSubstring;
_allowNewlines &= _isVerbatim;
ScanInterpolatedStringLiteralTop(interpolations, ref info, closeQuoteMissing: out _);
}
finally
{
_isVerbatim = wasVerbatim;
_allowNewlines = wasAllowNewlines;
}
continue;
}
Expand Down Expand Up @@ -618,7 +596,7 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool
// to be a normal verbatim string, so we can continue on an attempt to understand the
// outer interpolated string properly.
var discarded = default(TokenInfo);
var errorCode = _lexer.ScanVerbatimStringLiteral(ref discarded, _allowNewlines);
var errorCode = _lexer.ScanVerbatimStringLiteral(ref discarded);
if (errorCode is ErrorCode code)
{
TrySetRecoverableError(_lexer.MakeError(nestedStringPosition, width: 2, code));
Expand All @@ -632,17 +610,14 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool
var interpolations = (ArrayBuilder<Interpolation>?)null;
var info = default(TokenInfo);
bool wasVerbatim = _isVerbatim;
bool wasAllowNewlines = _allowNewlines;
try
{
_isVerbatim = true;
_allowNewlines = true;
ScanInterpolatedStringLiteralTop(interpolations, ref info, closeQuoteMissing: out _);
}
finally
{
_isVerbatim = wasVerbatim;
_allowNewlines = wasAllowNewlines;
}
continue;
}
Expand All @@ -652,15 +627,10 @@ private void ScanInterpolatedStringLiteralHoleBalancedText(char endingChar, bool
switch (_lexer.TextWindow.PeekChar(1))
{
case '/':
if (!_isVerbatim || !_allowNewlines)
{
// error: single-line comment not allowed in an interpolated string.
// report the error but keep going for good error recovery.
TrySetRecoverableError(_lexer.MakeError(_lexer.TextWindow.Position, 2, ErrorCode.ERR_SingleLineCommentInExpressionHole));
}

_lexer.TextWindow.AdvanceChar(); // skip /
_lexer.TextWindow.AdvanceChar(); // skip /

// read up to the end of the line.
while (!IsAtEnd(allowNewline: false))
{
_lexer.TextWindow.AdvanceChar(); // skip // comment character
Expand Down Expand Up @@ -710,26 +680,19 @@ private void ScanInterpolatedStringLiteralNestedComment()
_lexer.TextWindow.AdvanceChar();
while (true)
{
var ch = _lexer.TextWindow.PeekChar();

// See if we ran into a disallowed new line. If so, this is recoverable, so just skip past it but
// give a good message about the issue. This will prevent a lot of cascading issues with the remainder
// of the interpolated string that comes on the following lines.
var allowNewLines = _isVerbatim && _allowNewlines;
if (!allowNewLines && SyntaxFacts.IsNewLine(ch))
{
TrySetRecoverableError(_lexer.MakeError(_lexer.TextWindow.Position, width: 0, ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString));
}

// Note: if we reach the end of the file without hitting */ just bail out. It's not necessary for
// us to report any issues, as this code is just being used to find the end of teh interpolation hole.
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
// When the full parse happens, the lexer will grab the string inside the interpolation hole and
// pass it to the regular parser. This parser will then see the unterminated /* and will report the
// error for it.
if (IsAtEnd(allowNewline: true))
{
return; // let the caller complain about the unterminated quote
}
return;

var ch = _lexer.TextWindow.PeekChar();
_lexer.TextWindow.AdvanceChar();
if (ch == '*' && _lexer.TextWindow.PeekChar() == '/')
{
_lexer.TextWindow.AdvanceChar(); // skip */
_lexer.TextWindow.AdvanceChar();
return;
}
}
Expand Down
Loading