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.

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

Represent enhanced #line directives in syntax API #54318

Open
cston opened this issue Jun 23, 2021 · 12 comments
Open

Represent enhanced #line directives in syntax API #54318

cston opened this issue Jun 23, 2021 · 12 comments
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Milestone

Comments

@cston
Copy link
Member

cston commented Jun 23, 2021

Background and Motivation

Represent enhanced #line directives (see proposal) in the syntax model.

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

Proposed API

Current #line directives are represented by Microsoft.CodeAnalysis.CSharp.Syntax.LineDirectiveTriviaSyntax.

One possible approach is to introduce an additional class to represent enhanced #line directives and a common base class for both. The existing LineDirectiveTriviaSyntax class would be unchanged other than some members becoming overrides of virtual base class members.

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
    // Base class for #line directives
    public abstract class LineOrSpanDirectiveTriviaSyntax : DirectiveTriviaSyntax
    {
        public abstract SyntaxToken LineKeyword { get; }
        public abstract SyntaxToken File { get; }

        public new LineOrSpanDirectiveTriviaSyntax WithHashToken(SyntaxToken hashToken);
        public LineOrSpanDirectiveTriviaSyntax WithLineKeyword(SyntaxToken lineKeyword);
        public LineOrSpanDirectiveTriviaSyntax WithFile(SyntaxToken file);
        public new LineOrSpanDirectiveTriviaSyntax WithEndOfDirectiveToken(SyntaxToken endOfDirectiveToken);
    }

    // Current directive: #line default | hidden | line file
    public sealed class LineDirectiveTriviaSyntax : LineOrSpanDirectiveTriviaSyntax
    {
        public override bool IsActive { get; }
        public override SyntaxToken HashToken { get; }
        public override SyntaxToken LineKeyword { get; }
        public SyntaxToken Line { get; }
        public override SyntaxToken File { get; }
        public override SyntaxToken EndOfDirectiveToken { get; }

        public LineDirectiveTriviaSyntax WithIsActive(bool isActive);
        public new LineDirectiveTriviaSyntax WithHashToken(SyntaxToken hashToken);
        public LineDirectiveTriviaSyntax WithLine(SyntaxToken line);
        public new LineDirectiveTriviaSyntax WithLineKeyword(SyntaxToken lineKeyword);
        public new LineDirectiveTriviaSyntax WithFile(SyntaxToken file);
        public new LineDirectiveTriviaSyntax WithEndOfDirectiveToken(SyntaxToken endOfDirectiveToken);
    }

    // Enhanced directive: #line (startLine, startChar) - (endLine, endChar) charOffset file
    public sealed class LineSpanDirectiveTriviaSyntax : LineOrSpanDirectiveTriviaSyntax
    {
        public override bool IsActive { get; }
        public override SyntaxToken HashToken { get; }
        public override SyntaxToken LineKeyword { get; }
        public LineDirectivePositionSyntax Start { get; }
        public SyntaxToken MinusToken { get; }
        public LineDirectivePositionSyntax End { get; }
        public SyntaxToken CharacterOffset { get; }
        public override SyntaxToken File { get; }
        public override SyntaxToken EndOfDirectiveToken { get; }

        public LineSpanDirectiveTriviaSyntax WithIsActive(bool isActive);
        public new LineSpanDirectiveTriviaSyntax WithHashToken(SyntaxToken hashToken);
        public new LineSpanDirectiveTriviaSyntax WithLineKeyword(SyntaxToken lineKeyword);
        public LineSpanDirectiveTriviaSyntax WithStart(LineDirectivePositionSyntax start);
        public LineSpanDirectiveTriviaSyntax WithMinusToken(SyntaxToken minusToken);
        public LineSpanDirectiveTriviaSyntax WithEnd(LineDirectivePositionSyntax end);
        public LineSpanDirectiveTriviaSyntax WithCharacterOffset(SyntaxToken characterOffset);
        public new LineSpanDirectiveTriviaSyntax WithFile(SyntaxToken file);
        public new LineSpanDirectiveTriviaSyntax WithEndOfDirectiveToken(SyntaxToken endOfDirectiveToken);
    }

    public sealed class LineDirectivePositionSyntax : CSharpSyntaxNode
    {
        public SyntaxToken OpenParenToken { get; }
        public SyntaxToken Line { get; }
        public SyntaxToken CommaToken { get; }
        public SyntaxToken Character { get; }
        public SyntaxToken CloseParenToken { get; }

        public LineDirectivePositionSyntax WithOpenParenToken(SyntaxToken openParenToken);
        public LineDirectivePositionSyntax WithLine(SyntaxToken line);
        public LineDirectivePositionSyntax WithCommaToken(SyntaxToken commaToken);
        public LineDirectivePositionSyntax WithCharacter(SyntaxToken character);
        public LineDirectivePositionSyntax WithCloseParenToken(SyntaxToken closeParenToken);
    }
}

A CharacterOffset property should be added to LineMapping as well.

namespace Microsoft.CodeAnalysis
{
    public readonly struct LineMapping : IEquatable<LineMapping>
    {
        public readonly LinePositionSpan Span { get; }
        public readonly int? CharacterOffset { get; }
        public readonly FileLinePositionSpan MappedSpan { get; }

        public LineMapping(LinePositionSpan span, int? characterOffset, FileLinePositionSpan mappedSpan);

        public void Deconstruct(
            out LinePositionSpan span, out int? characterOffset, out FileLinePositionSpan mappedSpan);

        // ...
    }
}
@cston cston added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Jun 23, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 23, 2021
@cston
Copy link
Member Author

cston commented Jun 23, 2021

cc @tmat, @333fred

@cston cston added the blocking API needs to reviewed with priority to unblock work label Jun 23, 2021
@CyrusNajmabadi
Copy link
Member

These feel pretty differnet (to me at least). Do we expect any code to operate on hte base type itself?

@tmat
Copy link
Member

tmat commented Jun 23, 2021

Code that is looking for mapped file paths can use the base class.

@333fred
Copy link
Member

333fred commented Jun 23, 2021

Base class feels like the right solution to me. Even if we don't expect to see a large number of users of the base type, it would feel weird to me if the two forms of #line weren't related in some fashion.

@cston
Copy link
Member Author

cston commented Jun 23, 2021

Updated proposal to extract a LineDirectivePositionSyntax type for LineSpanDirectiveTriviaSyntax.Start and LineSpanDirectiveTriviaSyntax.End.

@333fred 333fred added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jun 23, 2021
@cston
Copy link
Member Author

cston commented Jun 30, 2021

Added LineMapping.CharacterOffset property to proposal (see #53752).

@333fred
Copy link
Member

333fred commented Jun 30, 2021

API Review

API Approved

  • Remove Deconstruct from LineMapping. If we want to add Deconstruct methods to our simple structs, we can add them in general.
  • Before merge, see if CharacterOffset on LineMapping can be an int instead of an int?. Are there scenarios where the information loss is actually important?

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking API needs to reviewed with priority to unblock work labels Jun 30, 2021
@tmat
Copy link
Member

tmat commented Jun 30, 2021

Re CharacterOffset:
What if we don't need int? now but will need it after we ship the API? Would we add bool HasCharacterOffset in that case?

@333fred
Copy link
Member

333fred commented Jun 30, 2021

Would we add bool HasCharacterOffset in that case?

Presumably yes, but main question is whether we think that will ever happen. If you think that it's reasonable someone would want to know this from LineMapping alone, then we can keep as int?. My main question is whether that's actually important information for LineMapping users, or if that's more of a syntax question.

@tmat
Copy link
Member

tmat commented Jun 30, 2021

I can't predict future. I think it's ok to not expose it as long as we have a reasonable path forward if we need it.
It doesn't cost anything to expose, so not sure why we wouldn't just expose int? rightaway.

@333fred
Copy link
Member

333fred commented Jun 30, 2021

so not sure why we wouldn't just expose int? rightaway.

It's potentially a more onerous API to use.

@tmat
Copy link
Member

tmat commented Jun 30, 2021

If someone is interested in knowing the offset at all they presumably have a reason to distinguish between #line directive that doesn't specify it and #line directive that does. Since 0 is a valid value for the offset when specified, I think it would be more confusing to return 0 when it's not specified as well. We could return -1, e.g. but at that point it seems that int? is better than having to know a special value for "it's not specified".

@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 9, 2021
@jaredpar jaredpar added this to the 17.0 milestone Jul 9, 2021
@333fred 333fred modified the milestones: 17.0, 17.1 Sep 13, 2021
@jcouv jcouv modified the milestones: 17.1, Compiler.Next Mar 17, 2022
@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Projects
None yet
Development

No branches or pull requests

6 participants