Allow newlines in the holes inside interpolated strings #56853

CyrusNajmabadi · 2021-09-29T18:12:26Z

Fixes dotnet/csharplang#4935 (approved by LDM).

Review with whitespace changes off.

Relates to test plan #57154

RikkiGibson · 2021-09-29T18:25:54Z

It looks like the PR doesn't include any modified or new tests. Are those coming as part of this PR?

CyrusNajmabadi · 2021-09-29T18:32:38Z

src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs

-                Diagnostic(ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString, "").WithLocation(5, 77),
+                // (7,2): error CS1035: End-of-file found, '*/' expected
+                // }
+                Diagnostic(ErrorCode.ERR_OpenEndedComment, "").WithLocation(7, 2),


getting a duplicated error. investigating. #Closed

CyrusNajmabadi · 2021-09-29T19:06:21Z

@RikkiGibson yes. lots of test updates here :)

azure-pipelines · 2021-09-29T21:57:30Z

Azure Pipelines successfully started running 3 pipeline(s).

CyrusNajmabadi · 2021-09-29T22:21:37Z

Note: i still need to make the specification change doc on this.

jcouv · 2021-10-14T18:11:34Z

Kindly reminder to document the feature (with link to test plan and spec) in https://github.com/dotnet/roslyn/blob/main/docs/Language%20Feature%20Status.md
This helps us keep track of all the features and also communicate with partners and users.

In reply to: 943600359

src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs

CyrusNajmabadi · 2021-10-14T20:59:59Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

+                                {
+                                    var text = node.SyntaxTree.GetText();
+                                    if (text.Lines.GetLineFromPosition(interpolation.OpenBraceToken.SpanStart).LineNumber !=
+                                        text.Lines.GetLineFromPosition(interpolation.CloseBraceToken.SpanStart).LineNumber)


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.

CyrusNajmabadi · 2021-10-14T21:01:18Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_InterpolatedString.cs

@@ -137,7 +137,7 @@ private ExpressionSyntax ParseInterpolatedStringToken()
                }


view PR with whitespace changes off.

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalErrorTests.cs

CyrusNajmabadi · 2021-10-23T15:34:38Z

@chsienki PTAL. Thanks! :)

src/Compilers/CSharp/Portable/CSharpResources.resx

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

chsienki

LGTM minus a couple of spelling nits

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>

CyrusNajmabadi · 2021-10-25T18:46:36Z

As this just touched comments/loc-string, i made the chnage and set to auto-merge. LMK if there's an issue with that.

jcouv · 2021-10-25T19:01:31Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

+                                        text.Lines.GetLineFromPosition(interpolation.CloseBraceToken.SpanStart).LineNumber)
+                                    {
+                                        diagnostics.Add(
+                                            ErrorCode.ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString,


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.

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>

jcouv

LGTM Thanks (iteration 21). IDE portion can be a follow-up

… implemented as part of dotnet#50799 and later realized via dotnet#56853

* Add test "TestInterpolatedStringWithNewLinesInExpression()" which was implemented as part of #50799 and later realized via #56853

Allow newlines in the holes inside interpolated strings

8ed75ec

CyrusNajmabadi requested a review from jcouv September 29, 2021 18:12

CyrusNajmabadi requested a review from a team as a code owner September 29, 2021 18:12

dotnet-issue-labeler bot added the Area-Compilers label Sep 29, 2021

jcouv added this to the Compiler.Next milestone Sep 29, 2021

Update tests

fe7c2d9

Update tests

10a0fb5

CyrusNajmabadi commented Sep 29, 2021

View reviewed changes

CyrusNajmabadi added 5 commits September 29, 2021 11:48

Remove unnecessary extra error

06afc0b

Use simple using

3a8f99b

Inline variables

7dffb4b

Use named parameters

dde9fdc

Simplify

61bcfbb

CyrusNajmabadi mentioned this pull request Sep 29, 2021

[Proposal]: Remove restriction that interpolations within a non-verbatim interpolated string cannot contain new-lines. dotnet/csharplang#4935

Closed

3 tasks

runfoapp bot mentioned this pull request Sep 29, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

jcouv self-assigned this Oct 14, 2021

jcouv reviewed Oct 14, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs Show resolved Hide resolved

CyrusNajmabadi added 4 commits October 14, 2021 13:28

Move error to binding phase

bde11ad

Add tests

437fe79

Update optinos

edd0497

remove

da75881

CyrusNajmabadi commented Oct 14, 2021

View reviewed changes

Update comment

9dd3189

CyrusNajmabadi commented Oct 14, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalErrorTests.cs Show resolved Hide resolved

jcouv requested a review from chsienki October 19, 2021 18:09

chsienki reviewed Oct 25, 2021

View reviewed changes

src/Compilers/CSharp/Portable/CSharpResources.resx Outdated Show resolved Hide resolved

chsienki reviewed Oct 25, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs Outdated Show resolved Hide resolved

chsienki reviewed Oct 25, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs Outdated Show resolved Hide resolved

chsienki reviewed Oct 25, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs Outdated Show resolved Hide resolved

chsienki approved these changes Oct 25, 2021

View reviewed changes

CyrusNajmabadi and others added 4 commits October 25, 2021 12:33

Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

3ec40db

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>

Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

2459df5

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>

Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

1e445cb

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>

Update language string

65e91b2

CyrusNajmabadi force-pushed the features/verbatim-hole-newline branch from e316868 to 65e91b2 Compare October 25, 2021 18:45

CyrusNajmabadi enabled auto-merge (squash) October 25, 2021 18:46

jcouv reviewed Oct 25, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Parser/Lexer_StringLiteral.cs

59e4649

Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>

jcouv approved these changes Oct 25, 2021

View reviewed changes

CyrusNajmabadi merged commit 35546d0 into dotnet:main Oct 25, 2021

ghost modified the milestones: Compiler.Next, Next Oct 25, 2021

CyrusNajmabadi deleted the features/verbatim-hole-newline branch October 25, 2021 20:40

RikkiGibson modified the milestones: Next, 17.1.P1 Oct 25, 2021

This was referenced Nov 13, 2021

Parse-Error: new-line in string interpolation expressions #50799

Closed

Parse-Error: new-line in string interpolation expressions #50742

Closed

bernd5 added a commit to bernd5/roslyn that referenced this pull request Nov 13, 2021

Add test "TestInterpolatedStringWithNewLinesInExpression()" which was…

f378dbe

… implemented as part of dotnet#50799 and later realized via dotnet#56853

bernd5 mentioned this pull request Nov 13, 2021

Add test from PR 50799 for impl PR 56853 #57752

Merged

jcouv mentioned this pull request Dec 12, 2021

Record list-patterns and newlines in interpolations as done #58250

Merged

333fred pushed a commit that referenced this pull request Dec 13, 2021

Add test from PR 50799 for impl PR 56853 (#57752)

498d5b1

* Add test "TestInterpolatedStringWithNewLinesInExpression()" which was implemented as part of #50799 and later realized via #56853

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow newlines in the holes inside interpolated strings #56853

Allow newlines in the holes inside interpolated strings #56853

CyrusNajmabadi commented Sep 29, 2021 •

edited by jcouv

Loading

RikkiGibson commented Sep 29, 2021

CyrusNajmabadi Sep 29, 2021 •

edited by jcouv

Loading

CyrusNajmabadi commented Sep 29, 2021

azure-pipelines bot commented Sep 29, 2021

CyrusNajmabadi commented Sep 29, 2021

jcouv commented Oct 14, 2021 •

edited

Loading

CyrusNajmabadi Oct 14, 2021

CyrusNajmabadi Oct 14, 2021 •

edited

Loading

CyrusNajmabadi commented Oct 23, 2021

chsienki left a comment

CyrusNajmabadi commented Oct 25, 2021

jcouv Oct 25, 2021 •

edited

Loading

CyrusNajmabadi Oct 25, 2021

jcouv left a comment

		@@ -137,7 +137,7 @@ private ExpressionSyntax ParseInterpolatedStringToken()
		}

Allow newlines in the holes inside interpolated strings #56853

Allow newlines in the holes inside interpolated strings #56853

Conversation

CyrusNajmabadi commented Sep 29, 2021 • edited by jcouv Loading

RikkiGibson commented Sep 29, 2021

CyrusNajmabadi Sep 29, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 29, 2021

azure-pipelines bot commented Sep 29, 2021

CyrusNajmabadi commented Sep 29, 2021

jcouv commented Oct 14, 2021 • edited Loading

CyrusNajmabadi Oct 14, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 23, 2021

chsienki left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 25, 2021

jcouv Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 25, 2021

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 29, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Sep 29, 2021 •

edited by jcouv

Loading

jcouv commented Oct 14, 2021 •

edited

Loading

CyrusNajmabadi Oct 14, 2021 •

edited

Loading

jcouv Oct 25, 2021 •

edited

Loading