Enhanced #line directives #54489

cston · 2021-06-30T05:53:35Z

#line (startLine, startChar) - (endLine, endChar) charOffset "fileName"

Map lines after the #line directive to the span (startLine, startChar) - (endLine, endChar) in "fileName" where the origin of the unmapped span is the first line after the directive at character charOffset.

Proposal: dotnet/csharplang#4747
Test plan: #54509

jcouv · 2021-07-01T01:24:53Z

Please add an entry to feature status page with a test plan link. Thanks

In reply to: 871838112

cston · 2021-07-01T18:06:47Z

cc @captainsafia

RikkiGibson · 2021-07-01T18:18:46Z

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

+                    result = (this.CurrentToken.Kind == SyntaxKind.OpenParenToken) ?
+                        this.ParseLineSpanDirective(hash, lineKeyword, isActive) :
+                        this.ParseLineDirective(hash, lineKeyword, isActive);


nit: consider placing ?: tokens at the beginning of the line

Suggested change

result = (this.CurrentToken.Kind == SyntaxKind.OpenParenToken) ?

this.ParseLineSpanDirective(hash, lineKeyword, isActive) :

this.ParseLineDirective(hash, lineKeyword, isActive);

result = (this.CurrentToken.Kind == SyntaxKind.OpenParenToken)

? this.ParseLineSpanDirective(hash, lineKeyword, isActive)

: this.ParseLineDirective(hash, lineKeyword, isActive);

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

RikkiGibson · 2021-07-01T18:50:50Z

src/Compilers/CSharp/Portable/Syntax/CSharpLineDirectiveMap.cs

+                        var mappedLine = (previous.State == PositionState.RemappedSpan) ?
+                            unmappedLine :
+                            previous.MappedLine + directiveLineNumber - previous.UnmappedLine;


Suggested change

var mappedLine = (previous.State == PositionState.RemappedSpan) ?

unmappedLine :

previous.MappedLine + directiveLineNumber - previous.UnmappedLine;

var mappedLine = (previous.State == PositionState.RemappedSpan)

? unmappedLine

: previous.MappedLine + directiveLineNumber - previous.UnmappedLine;

src/Compilers/Core/Portable/Syntax/LineDirectiveMap.cs

RikkiGibson · 2021-07-01T20:53:46Z

src/Compilers/Core/Portable/Syntax/LineMapping.cs

+        /// <summary>
+        /// The optional offset in the syntax tree for the line immediately following an enhanced <c>#line</c> directive in C#.
+        /// </summary>
+        public readonly int? CharacterOffset { get; }


did API review have any conclusion about the choice to make this int? versus int with a special "not specified" or default value?

This wasn't resolved at the API review (see comment). We might need to close on this separately.

Let's address after preview2.

src/Compilers/Core/Portable/Syntax/LineMapping.cs

src/Compilers/CSharp/Test/Syntax/Parsing/LineSpanDirectiveParsingTests.cs

jcouv · 2021-07-01T23:44:28Z

Looking

jcouv · 2021-07-02T00:15:59Z

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

+            var comma = EatToken(SyntaxKind.CommaToken, reportError);
+            if (comma.IsMissing) reportError = false;
+
+            var character = ParseNumericLiteral(reportError);


The error we'll report here will be ERR_InvalidLineNumber (" error CS1576: The line number specified for #line directive is missing or invalid") but this number doesn't refer to a line #Resolved

jcouv · 2021-07-02T00:23:12Z

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

+            var openParen = EatToken(SyntaxKind.OpenParenToken, reportError);
+            if (openParen.IsMissing) reportError = false;
+
+            var line = ParseNumericLiteral(reportError);


It looks like we have some validation for original #line directives, possibly reporting ERR_InvalidLineNumber or WRN_TooManyLinesForDebugger. Feels like we should keep those #Closed

Another rule from the spec: [if] start line is equal to end line then end character is greater than start character.

jcouv · 2021-07-02T00:31:25Z

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

+            var end = ParseLineDirectivePosition(ref reportError);
+            var characterOffset = (reportError && CurrentToken.Kind == SyntaxKind.NumericLiteralToken) ? this.EatToken() : null;
+
+            var file = EatToken(SyntaxKind.StringLiteralToken, ErrorCode.ERR_MissingPPFile, reportError: reportError);


From offline discussion, we probably need something like line 389 above

jcouv · 2021-07-02T00:38:16Z

src/Compilers/Core/Portable/Syntax/LineDirectiveMap.LineMappingEntry.cs

@@ -78,10 +92,26 @@ public LineMappingEntry(int unmappedLine)
            {
                this.UnmappedLine = unmappedLine;
                this.MappedLine = mappedLine;
+                this.MappedSpan = default;
+                this.UnmappedCharacterOffset = null;
                this.MappedPathOpt = mappedPathOpt;
                this.State = state;


Consider asserting that the state isn't PositionState.RemappedSpan #Closed

jcouv · 2021-07-02T00:54:00Z

src/Compilers/CSharp/Portable/Syntax/CSharpLineDirectiveMap.cs

@@ -21,69 +21,143 @@ public CSharpLineDirectiveMap(SyntaxTree syntaxTree)
        // Add all active #line directives under trivia into the list, in source code order.
        protected override bool ShouldAddDirective(DirectiveTriviaSyntax directive)
        {
-            return directive.IsActive && directive.Kind() == SyntaxKind.LineDirectiveTrivia;
+            return directive.IsActive && (directive.Kind() is SyntaxKind.LineDirectiveTrivia or SyntaxKind.LineSpanDirectiveTrivia);


There's a couple references to SyntaxKind.LineDirectiveTrivia in the IDE layer (relates to classification, in ClassifyTrivia and ClassifyPreprocessorDirective) and one to LineDirectiveTriviaSyntax in CSharpSyntaxFacts.TryGetExternalSourceInfo which will probably need looking at. #Pending

Thanks, added to the test plan.

jcouv · 2021-07-02T01:44:24Z

src/Compilers/CSharp/Portable/Syntax/CSharpLineDirectiveMap.cs

+                case LineDirectiveTriviaSyntax lineDirective:
+                    {
+                        var mappedLine = (previous.State == PositionState.RemappedSpan) ?
+                            unmappedLine :


I didn't understand this. It seems strange to use something from "unmapped" domain directly in "mapped" domain without any sort of mapping.
Consider adding a comment: "only used in error cases" #Closed

jcouv · 2021-07-02T02:43:51Z

src/Compilers/CSharp/Portable/Syntax/CSharpLineDirectiveMap.cs

+                    throw ExceptionUtilities.UnexpectedValue(directive);
+            }
+
+            static bool tryGetOneBasedNumericLiteralValue(in SyntaxToken token, ref int value)


Consider making this an out instead #Resolved

jcouv · 2021-07-02T03:04:18Z

src/Compilers/CSharp/Portable/Syntax/CSharpLineDirectiveMap.cs

+
+            static bool tryGetOneBasedNumericLiteralValue(in SyntaxToken token, ref int value)
+            {
+                if (!token.IsMissing &&


It looks like these two checks (IsMissing and Kind() == NumericLiteralToken) were not in the original source code. Do we need them?
I'm fine if it's for extra safety... #Closed

It looks like these two checks (IsMissing and Kind() == NumericLiteralToken) were not in the original source code. Do we need them?

The original checks are still there above the call to the local function, so these are redundant. In fact, the checks might be redundant from each of the callers but it seems useful for safety as you suggested.

jcouv · 2021-07-02T03:30:29Z

src/Compilers/Core/Portable/Syntax/LineDirectiveMap.cs

+            if (entry.State == PositionState.Hidden ||
+                entry.State == PositionState.Unknown && GetUnknownStateVisibility(currentIndex) == LineVisibility.Hidden)
+            {
+                return new LineMapping(unmapped, null, default);


nit: consider naming last two args #Closed

jcouv · 2021-07-02T03:46:13Z

src/Compilers/CSharp/Test/Syntax/Parsing/LineSpanDirectiveParsingTests.cs

+            EOF();
+        }
+    }
+}


nit: Consider adding a ParseIncompleteSyntax test (like ParseIncompleteRecordSyntax) #Closed

jcouv · 2021-07-02T03:54:24Z

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

+                int value = (int)token.Value;
+                if (value < minValue || value > maxValue)
+                {
+                    token = this.AddError(token, ErrorCode.ERR_LineSpanDirectiveInvalidValue);


For old #line directives, one of these scenarios is just a warning: WRN_TooManyLinesForDebugger. Do we want to maintain that?

jcouv

Done with review pass (iteration 6)

jcouv

LGTM Thanks (iteration 8)

jcouv

LGTM Thanks (iteration 9)

jcouv · 2021-07-02T18:46:55Z

Marked as "Approved to merge" in @jaredpar's absence, given compiler feature review and IDE approval (blocking formatting issue was fixed).

dotnet-issue-labeler bot added the Area-Compilers label Jun 30, 2021

Enhanced #line directive

46c1a22

cston force-pushed the line-directive branch from f77b2a9 to 46c1a22 Compare June 30, 2021 23:52

cston marked this pull request as ready for review July 1, 2021 00:45

cston requested review from a team as code owners July 1, 2021 00:45

cston requested a review from tmat July 1, 2021 00:45

cston marked this pull request as draft July 1, 2021 00:49

Fix tests

3b4d738

cston marked this pull request as ready for review July 1, 2021 07:01

cston added 2 commits July 1, 2021 10:19

Update PublicAPI.Unshipped.txt

a093754

Fix tests

36eb622

RikkiGibson self-assigned this Jul 1, 2021

RikkiGibson approved these changes Jul 1, 2021

View reviewed changes

More tests

2e9ccff

jcouv reviewed Jul 2, 2021

View reviewed changes

Check ranges

a221979

jcouv reviewed Jul 2, 2021

View reviewed changes

cston added 2 commits July 1, 2021 23:23

PR feedback

c57f854

Misc.

e91e4f1

jcouv approved these changes Jul 2, 2021

View reviewed changes

Update IsPreprocessorDirective

d98ae98

jcouv added the Approved to merge label Jul 2, 2021

jcouv approved these changes Jul 2, 2021

View reviewed changes

jcouv self-assigned this Jul 2, 2021

jcouv added this to the C# 10 milestone Jul 2, 2021

RikkiGibson approved these changes Jul 2, 2021

View reviewed changes

cston merged commit ec005c8 into dotnet:release/dev17.0 Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced #line directives #54489

Enhanced #line directives #54489

cston commented Jun 30, 2021 •

edited

Loading

jcouv commented Jul 1, 2021 •

edited by cston

Loading

cston commented Jul 1, 2021

RikkiGibson Jul 1, 2021

RikkiGibson Jul 1, 2021

RikkiGibson Jul 1, 2021

cston Jul 1, 2021 •

edited

Loading

RikkiGibson Jul 1, 2021 •

edited

Loading

jcouv commented Jul 1, 2021

jcouv Jul 2, 2021 •

edited by cston

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021

jcouv Jul 2, 2021

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited by cston

Loading

cston Jul 2, 2021

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited by cston

Loading

jcouv Jul 2, 2021 •

edited

Loading

cston Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021

jcouv left a comment

jcouv left a comment

jcouv left a comment

jcouv commented Jul 2, 2021

Enhanced #line directives #54489

Enhanced #line directives #54489

Conversation

cston commented Jun 30, 2021 • edited Loading

jcouv commented Jul 1, 2021 • edited by cston Loading

cston commented Jul 1, 2021

RikkiGibson Jul 1, 2021

Choose a reason for hiding this comment

RikkiGibson Jul 1, 2021

Choose a reason for hiding this comment

RikkiGibson Jul 1, 2021

Choose a reason for hiding this comment

cston Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jul 1, 2021

jcouv Jul 2, 2021 • edited by cston Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021

Choose a reason for hiding this comment

jcouv Jul 2, 2021

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Jul 2, 2021

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited by cston Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

cston Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 2, 2021

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Jul 2, 2021

cston commented Jun 30, 2021 •

edited

Loading

jcouv commented Jul 1, 2021 •

edited by cston

Loading

cston Jul 1, 2021 •

edited

Loading

RikkiGibson Jul 1, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited by cston

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited by cston

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited by cston

Loading

jcouv Jul 2, 2021 •

edited

Loading

cston Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited

Loading

jcouv Jul 2, 2021 •

edited

Loading