SyntaxTree.GetLineMappings

[tmat]

Background and Motivation

EnC analyzer requires the ability to enumerate all #line mappings in a source file.

Proposed API

class SyntaxTree
{
…
        /// <summary>
        /// Returns empty sequence if there are no line mapping directives in the tree.
        /// Otherwise, returns a sequence of pairs of spans: each describing a mapping of a span of the tree between two consecutive #line directives.
        /// If the first directive is not on the first line the first pair describes mapping of the span preceding the first directive.
        /// The last pair of the sequence describes mapping of the span following the last #line directive.
        /// </summary>
        /// <returns>
        /// Empty sequence if the tree does not contain a line mapping directive.
        /// Otherwise a non-empty sequence of <see cref="LineMapping"/>.
        /// </returns>
        public abstract IEnumerable<LineMapping> GetLineMappings(CancellationToken cancellationToken = default);
…
}
 
class CSharpSyntaxTree
{
…
        public override IEnumerable<LineMapping> GetLineMappings(CancellationToken cancellationToken = default);
…
}
 
class VisualBasicSyntaxTree
{
…
        public override IEnumerable<LineMapping> GetLineMappings(CancellationToken cancellationToken = default);
…
}
 
namespace Microsoft.CodeAnalysis
{
    /// <summary>
    /// Represents a line mapping defined by a single line mapping directive (<c>#line</c> in C# or <c>#ExternalSource</c> in VB).
    /// </summary>
    [DataContract]
    public readonly struct LineMapping : IEquatable<LineMapping>
    {
        /// <summary>
        /// The span in the syntax tree containing the line mapping directive.
        /// </summary>
        [DataMember(Order = 0)]
        public readonly LinePositionSpan Span { get; }
 
        /// <summary>
        /// If the line mapping directive maps the span into an explicitly specified file the <see cref="FileLinePositionSpan.HasMappedPath"/> is true.
        /// If the path is not mapped <see cref="FileLinePositionSpan.Path"/> is empty and <see cref="FileLinePositionSpan.HasMappedPath"/> is false.
        /// If the line mapping directive marks hidden code <see cref="FileLinePositionSpan.IsValid"/> is false.
        /// </summary>
        [DataMember(Order = 1)]
        public readonly FileLinePositionSpan MappedSpan { get; }
 
        public LineMapping(LinePositionSpan span, FileLinePositionSpan mappedSpan);
 
        public void Deconstruct(out LinePositionSpan span, out FileLinePositionSpan mappedSpan);
 
        /// <summary>
        /// True if the line mapping marks hidden code.
        /// </summary>
        public bool IsHidden { get; }
 
        public override bool Equals(object? obj);
        public bool Equals(LineMapping other);
        public override int GetHashCode();
        public static bool operator ==(LineMapping left, LineMapping right);
        public static bool operator !=(LineMapping left, LineMapping right);;
        public override string? ToString()
    }
}

Usage Examples

            foreach (var (unmappedSection, mappedSection) in oldTree.GetLineMappings(cancellationToken))
            {
               var targetPath = mappedSection.HasMappedPath ? mappedSection.Path : oldTree.FilePath;

                ...
            }

https://github.com/dotnet/roslyn/pull/53735/files#diff-5ee00551d4ea54b98dfc33510c5ea4be279cd1cc11d40a2329a9ad506e75681bR183

Alternative Designs

Risks

[RikkiGibson]

Ping @333fred @dotnet/roslyn-api-owners

[333fred]

Why make GetLineMappings return IEnumerable? I would expect them to return ImmutableArray.

[333fred]

Also, API owners should be the ones applying ready-for-review, just as an FYI. @CyrusNajmabadi as the other API owner of syntax.

[tmat]

@333fred The caller might not necessarily need to enumerate all of the mappings if it only looks for the first one matching some criteria.

[333fred]

@333fred The caller might not necessarily need to enumerate all of the mappings if it only looks for the first one matching some criteria.

Is calculating the mappings expensive? I guess I don't see the benefits that IEnumerable gets us here.

[tmat]

There can be 1000s of #line directives in a file. Why allocate and calculate all that if the caller needs to find the first one, for example?

[333fred]

There can be 1000s of #line directives in a file. Why allocate and calculate all that if the caller needs to find the first one, for example?

Do we think that's a common use case? If needing to find just one span is going to be the thing people need to do, it seems to me like the appropriate API shape would instead be bool TryGetLineMapping(this CSharpSyntaxTree tree, LinePositionSpan originalSpan, out FileLinePositionSpan mappedSpan).

[tmat]

I don't see why IEnumerable is bad.

[CyrusNajmabadi]

We have plenty of precedence here for using IEnumerable in our apis. Usually teh core question is: will this be iterated many times during expected consumption. In that case, we def want IMmutableArray to effectively make the later consumption 'effectively free'. In other words, no allocations to consume. No virtual indirecitons, etc.

However, for stuff that really may be computed lazily, and which is only intended to be read a tiny handful of times per user scenario, an IEnumerable is fine. So it comes down to how @tmat thinks this will be used for EnC. If it's just going ot be read once/twice while doing some mapping operation against other data, then that's fine. If we think this data will need to be read a lot though, then we should realize this as an ImmutableArray.

[tmat]

The method provides another view of data already stored in an array in the underlying implementation (essentially just a translation). EnC currently uses it in multiple ways: 1) enumerates all items 2) finds first mapping that contains given mapped span (note that this requires O(n) since the underlying array is sorted by unmapped location, not mapped), 3) checks if there are any significant line mappings by calling .Any().

I believe IEnumerable is good for all of these use cases.

[333fred]

EnC currently uses it in multiple ways: 1) enumerates all items 2) finds first mapping that contains given mapped span (note that this requires O(n) since the underlying array is sorted by unmapped location, not mapped), 3) checks if there are any significant line mappings by calling .Any().

Thanks, this was the context I was missing. Provided @CyrusNajmabadi has no objections, I think this is fine to move ahead. We'll review in the next API review session, but implementation work doesn't need to be gated on it.

[333fred]

API Review

• How will the new #line changes be incorporated?
  • Offset should be added before we ship.
• Otherwise looks good.