Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it by CyrusNajmabadi · Pull Request #57966 · dotnet/roslyn

CyrusNajmabadi · 2021-11-24T06:46:41Z

Followup to #57945. This changes the interpolation struct to be simpler and not contain information that can be computed when the interpolation is needed.

It also moves the internal data to be in terms of Ranges instead of positions for things like where the { and } are in an interpolation. This is needed for raw-interpolated-strings as those braces may be more than one character long. Usage of spans also makes length/substring computation trivial. Prior to this we had helpers that took in two positions but which was inclusive on both. This lead to a lot of complex math and didn't follow any of the slicing math patterns we use everywhere else in roslyn. Switching to Range just makes all this complexity fall out.

…onPArsing4

CyrusNajmabadi · 2021-11-24T19:33:44Z

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

    internal partial class LanguageParser
    {
-        /// <summary>
-        /// "Safe" substring using start and end positions rather than start and length.


First, 'safe' is crazy here. We should never be in a situation where we need to munge positions. Everything shoudl be known with exact locations and we should always be able to safely use the data collected. This approach to specifying inclusive end positions, but then rectifying them is just bad.

Second, using inclusive for start/end just violated all of our (and the BCL) patterns around ranges/spans. Nothing else works this way (esp. string slicing), so we really don't want to do this.

CyrusNajmabadi · 2021-11-24T19:33:57Z

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

-            return (last > s.Length || len <= 0) ? string.Empty : s.Substring(first, len);
-        }
+        private static string Substring(string str, TextSpan span)
+            => str.Substring(span.Start, span.Length);


No fuss, no muss. You ask for a slice, you get it.

CyrusNajmabadi · 2021-11-24T19:34:40Z

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

                // with no inserts. We must still use String.Format to get its handling of escapes such as {{,
                // so we still treat it as a composite format string.
-                var text = Substring(originalText, openQuoteIndex + 1, closeQuoteIndex - 1);
+                var text = Substring(originalText, TextSpan.FromBounds(openQuoteIndex + 1, closeQuoteIndex));


no need for weird -1. THis is the standard inclusive/exclusive slice logic.

CyrusNajmabadi · 2021-11-24T19:35:27Z

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

+                    var text = Substring(originalText,
+                        TextSpan.FromBounds(
+                            i == 0 ? openQuoteIndex + 1 : interpolations[i - 1].CloseBraceSpan.End,
+                            interpolation.OpenBraceSpan.Start));


instead of needing +1 and -1 on the curly locations, we can actually just use known concepts like .End and .Start to specify what we care about. (we should probably do this for the openQuote part as well, but i haven't had need to just yet).

CyrusNajmabadi · 2021-11-24T19:36:00Z

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

            return CheckFeatureAvailability(result, MessageID.IDS_FeatureInterpolatedStrings);
        }

+        private static InterpolationSyntax ParseInterpolation(CSharpParseOptions options, string text, Lexer.Interpolation interpolation, bool isVerbatim)


made static so it's clear that you can't call instance methods both on the original 'parser' instance and the 'tempParser' created within.

CyrusNajmabadi · 2021-11-24T19:36:52Z

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

-            using (var tempLexer = new Lexer(Text.SourceText.From(parsedText), this.Options, allowPreprocessorDirectives: false, interpolationFollowedByColon: interpolation.HasColon))
+            var openBraceToken = this.EatToken(SyntaxKind.OpenBraceToken);
+            var (expression, alignment) = getExpressionAndAlignment();
+            var (format, closeBraceToken) = getFormatAndCloseBrace();


complex interpolation parsing was broken into it's three successive segments. The parts taht return pairs of items do so as there's a relation between those two pieces (esp. wrt to how trivia is associated with either part).

CyrusNajmabadi · 2021-11-24T19:37:17Z

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

+#endif
+            return result;
+
+            (ExpressionSyntax expression, InterpolationAlignmentClauseSyntax alignment) getExpressionAndAlignment()


diff is awful. i'm not sure a good way to review before/after. i recommend just trying to understand the after part.

CyrusNajmabadi · 2021-11-24T19:38:53Z

@RikkiGibson @jcouv @chsienki this is ready for review.

CyrusNajmabadi · 2021-11-24T19:59:13Z

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

                // with no inserts. We must still use String.Format to get its handling of escapes such as {{,
                // so we still treat it as a composite format string.
-                var text = Substring(originalText, openQuoteIndex + 1, closeQuoteIndex - 1);
+                var text = originalText[new Range(openQuoteIndex + 1, closeQuoteIndex)];


no need for -1, Range math is sane math.

Note: we might want to move the open-quote part to be a range as well, but i haven't needed that so far.

CyrusNajmabadi · 2021-11-24T19:59:37Z

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

-                    var text = Substring(originalText, (i == 0) ? (openQuoteIndex + 1) : (interpolations[i - 1].CloseBracePosition + 1), interpolation.OpenBracePosition - 1);
+                    var text = originalText[new Range(
+                        i == 0 ? openQuoteIndex + 1 : interpolations[i - 1].CloseBraceRange.End,
+                        interpolation.OpenBraceRange.Start)];


range math is much saner. you can just use .Start and .End easily and have things just make sense.

jcouv · 2021-11-29T20:47:41Z

                    var errorCode = this.ScanVerbatimStringLiteral(ref info);

There's a bit of an asymmetry which I didn't understand. In the @ case (here), we call ScanVerbatimStringLiteral or ScanInterpolatedStringLiteral. But in the $ case (line 785), we use TryScanInterpolatedString which always does ScanInterpolatedStringLiteral.

In reply to: 982008924

Refers to: src/Compilers/CSharp/Portable/Parser/Lexer.cs:767 in bb55d90. [](commit_id = bb55d90, deletion_comment = False)

jcouv · 2021-11-29T21:03:33Z

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

-                _isVerbatim = isVerbatim;
+
+                _isVerbatim = (lexer.TextWindow.PeekChar(0) == '$' && lexer.TextWindow.PeekChar(1) == '@') ||
+                              (lexer.TextWindow.PeekChar(0) == '@' && lexer.TextWindow.PeekChar(1) == '$');


Too bad we can't do something like is ['$', '@', ..] or ['@', '$', ..], because we probably don't want to pull on the length here, but it would look really cool ;-) #Closed

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

…onPArsing4

CyrusNajmabadi · 2021-11-30T00:56:01Z

There's a bit of an asymmetry

Yup. it's asymmetric. I'm happy to try to unify those in followup as well.

jcouv · 2021-11-30T01:15:15Z

Yup. it's asymmetric.

Is it a bug, or just some redundant code?

jcouv

LGTM Thanks (iteration 25)

333fred · 2021-11-30T01:30:11Z

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

+                using var tempLexer = new Lexer(SourceText.From(originalText), this.Options, allowPreprocessorDirectives: false);
                var info = default(Lexer.TokenInfo);
-                tempLexer.ScanInterpolatedStringLiteralTop(interpolations, isVerbatim, ref info, out error, out closeQuoteMissing);
+                tempLexer.ScanInterpolatedStringLiteralTop(ref info, out error, out openQuoteRange, interpolations, out closeQuoteRange);


interpolations

Is this the only thing that this local function needs from the surrounding context? If so, consider making the function static and passing this in. It's far enough away from the usage that it's not immediately obvious that interpolations is used in here when reading the main method body, and since the builder is immediately freed there's some refactoring risk there.

yeah, i wasn't sure the best structure here. i've changed it uip a bit now to hopefully make it a bit clearer. now the data that is in/out should be more obvious/relevant (i think).

333fred · 2021-11-30T01:31:50Z

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

-            bool closeQuoteMissing;
-            using (var tempLexer = new Lexer(Text.SourceText.From(originalText), this.Options, allowPreprocessorDirectives: false))
+
+            rescanInterpolation(out var openQuoteRange, out var error, out var closeQuoteRange);


openQuoteRange

Can we make any assertions that these quote ranges are within the original string?

we can. though we'll also horrifically crash if they're not since we slice using those ranges. so teh assertion won't be doing much more than just precrashing :)

CyrusNajmabadi · 2021-11-30T01:40:51Z

Is it a bug, or just some redundant code?

i believe the latter.

333fred · 2021-11-30T01:41:50Z

Done review pass (commit 25). Just a couple of minor comments.

…onPArsing4

CyrusNajmabadi · 2021-11-30T01:52:52Z

There's a bit of an asymmetry which I didn't understand.

Ok, fixed that :) was needed in the past, but not anymore with lots of the refactoring that have gone on here :)

CyrusNajmabadi · 2021-11-30T02:03:59Z

Ok. I'm going to try this as a squash. :)

333fred

LGTM (commit 17)

…rovements * upstream/main: (310 commits) Read SourceLink info and call service to retrieve source from there (dotnet#57978) Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) (dotnet#58050) Snap 17.1 P2 (dotnet#58041) Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (dotnet#57576) Shorten paths in VS installation (dotnet#57726) Add comments Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) Fix await completion for expression body lambda Add tests Fix comment Honor option, and also improve formatting with comment Skip TestLargeStringConcatenation (dotnet#58035) Log runtime framework of remote host Mark EqualityContract property accessor as not auto-implemented (dotnet#57917) Fix typo in XML doc for GeneratorExtensions (dotnet#58020) Hold Receiver directly in bound node for implicit indexer access (dotnet#58009) Pass AnalysisKind instead of int Enable nullable reference types for TableDataSource Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it (dotnet#57966) Add missing test for CallerArgumentExpression (dotnet#57805) ...

Simplify the interpolation struct

762655d

CyrusNajmabadi requested a review from a team as a code owner November 24, 2021 06:46

ghost added the Area-Compilers label Nov 24, 2021

Merge remote-tracking branch 'upstream/main' into simplifyInterpolati…

5506c2d

…onPArsing4

CyrusNajmabadi marked this pull request as draft November 24, 2021 06:49

runfoapp bot mentioned this pull request Nov 24, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.FastDoubleInvoke #57551

Closed

CyrusNajmabadi added 3 commits November 24, 2021 11:23

Use spans

ca7451b

Merge remote-tracking branch 'upstream/main' into simplifyInterpolati…

1ecac79

…onPArsing4

Merge branch 'simplifyInterpolationPArsing5' into simplifyInterpolati…

dffb52d

…onPArsing4

CyrusNajmabadi changed the title ~~Simplify interpolation parsing~~ Simplify 'interpolation' data, and move to an easier to consume data model for it Nov 24, 2021

CyrusNajmabadi commented Nov 24, 2021

View reviewed changes

CyrusNajmabadi changed the title ~~Simplify 'interpolation' data, and move to an easier to consume data model for it~~ Simplify 'interpolation' data, and move to an easier to consume TextSpan approach for it Nov 24, 2021

CyrusNajmabadi marked this pull request as ready for review November 24, 2021 19:38

CyrusNajmabadi requested review from RikkiGibson, chsienki and jcouv November 24, 2021 19:38

CyrusNajmabadi marked this pull request as draft November 24, 2021 19:57

use ranges

e80b01b

CyrusNajmabadi changed the title ~~Simplify 'interpolation' data, and move to an easier to consume TextSpan approach for it~~ Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it Nov 24, 2021

CyrusNajmabadi commented Nov 24, 2021

View reviewed changes

CyrusNajmabadi added 2 commits November 24, 2021 12:00

Update comment

a08d6e2

Use range in more places

86e2f0c

jcouv reviewed Nov 29, 2021

View reviewed changes

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

jcouv self-assigned this Nov 29, 2021

CyrusNajmabadi added 3 commits November 29, 2021 16:32

Merge remote-tracking branch 'upstream/main' into simplifyInterpolati…

f29b5e8

…onPArsing4

Add named arg

7d1a492

Fix comments

cb65c80

CyrusNajmabadi requested a review from jcouv November 30, 2021 00:47

jcouv approved these changes Nov 30, 2021

View reviewed changes

333fred reviewed Nov 30, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into simplifyInterpolati…

3f6fd30

…onPArsing4

Reorder

f0ba8a5

CyrusNajmabadi requested a review from 333fred November 30, 2021 01:55

CyrusNajmabadi enabled auto-merge (squash) November 30, 2021 02:03

jcouv self-requested a review November 30, 2021 02:05

333fred approved these changes Nov 30, 2021

View reviewed changes

CyrusNajmabadi merged commit 2df14d4 into dotnet:main Nov 30, 2021

ghost added this to the Next milestone Nov 30, 2021

pawchen mentioned this pull request Nov 30, 2021

ExpressionEvaluator.*.UnitTests are not run in CI #58030

Closed

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

CyrusNajmabadi deleted the simplifyInterpolationPArsing4 branch February 1, 2022 18:26

Conversation

CyrusNajmabadi commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 30, 2021

Uh oh!

jcouv commented Nov 30, 2021

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 30, 2021

Uh oh!

333fred commented Nov 30, 2021

Uh oh!

CyrusNajmabadi commented Nov 30, 2021

Uh oh!

CyrusNajmabadi commented Nov 30, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Nov 24, 2021 •

edited

Loading

CyrusNajmabadi Nov 24, 2021 •

edited

Loading

jcouv commented Nov 29, 2021 •

edited

Loading

jcouv Nov 29, 2021 •

edited

Loading