Add higher-order-function-based API for working with untyped AST

[brianrourkeboll]

Description

As part of my refactoring of the parenthesization API (#16461), I needed to publicly expose some way to traverse the entire untyped AST. One option was to make public a version of the SyntaxTraversal.traverseAll function that I added in #16079 taking a SyntaxVisitorBase<'T> implementation. This PR introduces (either instead or in addition) an alternative API based on higher-order functions that I think has advantages in ergonomics and composability.

I think a focus on exposing composable, orthogonal APIs for FCS primitives, from ParsedInput or SyntaxNode/SyntaxNodes/Ast to SynExpr and SynPat, could help greatly simplify current and future usage.

I'd be interested in feedback on this from current internal and external consumers of the untyped AST, including @baronfel, @nojaf, etc.

Edit: I went ahead and tried to update much of FSAC's usage of the existing syntax visitor API to the new one, and I think the results are encouraging: https://gist.github.com/brianrourkeboll/96bedaaed7dd7c22ddbe2047ee937633

Edit: I also went ahead and updated the uses of SyntaxTraversal.Traverse in FSharpParseFileResults to use the new API. That surfaced the need for ParsedInput.exists, ParsedInput.tryNode, and ParsedInput.tryPickLast. It also happened to lead me to make a small improvement to the signature help functionality:

Before, no signature help was offered in a situation like this, where a function in the middle of a pipeline already had a complex curried parameter (e.g., a lambda) applied to it:

[1..10]
|> List.fold (fun acc _ -> acc) ‸
|> List.filter (fun x -> x > 3)

The service will now offer help for the state parameter when the
cursor ‸ is in that location:

Provisional API surface area (updated)

module ParsedInput =
    /// Applies the given predicate to each node of the AST and its context (path)
    /// down to a given position, returning true if a matching node is found, otherwise false.
    /// Traversal is short-circuited if no matching node is found through the given position.
    val exists:
        predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) ->
        position: pos ->
        parsedInput: ParsedInput ->
            bool

    /// Applies a function to each node of the AST and its context (path),
    /// threading an accumulator through the computation.
    val fold:
        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State) ->
        state: 'State ->
        ast: Ast ->
            'State

    /// Applies a function to each node of the AST and its context (path)
    /// until the folder returns <c>None</c>, threading an accumulator through the computation.
    val foldWhile:
        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State option) ->
        state: 'State ->
        ast: Ast ->
            'State

    /// Dives to the deepest node that contains the given position,
    /// returning the node and its path if found, or <c>None</c> if no
    /// node contains the position.
    val tryNode:
        position: pos ->
        ast: Ast ->
            (SyntaxNode * SyntaxVisitorPath) option

    /// Applies the given function to each node of the AST and its context (path)
    /// down to a given position, returning <c>Some x</c> for the first node
    /// for which the function returns <c>Some x</c> for some value <c>x</c>, otherwise <c>None</c>.
    /// Traversal is short-circuited if no matching node is found through the given position.
    val tryPick:
        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) ->
        position: pos ->
        parsedInput: ParsedInput ->
            'T option

    /// Applies the given function to each node of the AST and its context (path)
    /// down to a given position, returning <c>Some x</c> for the last (deepest) node
    /// for which the function returns <c>Some x</c> for some value <c>x</c>, otherwise <c>None</c>.
    /// Traversal is short-circuited if no matching node is found through the given position.
    val tryPickLast:
        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) ->
        position: pos ->
        parsedInput: ParsedInput ->
            'T option

Original proposed API

module ParsedInput =
    val contents: parsedInput: ParsedInput -> SyntaxNode list

    val find:
        predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) ->
        parsedInput: ParsedInput ->
            SyntaxNode

    val fold:
        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State) ->
        state: 'State ->
        parsedInput: ParsedInput ->
            'State

    val foldWhile:
        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State option) ->
        state: 'State ->
        parsedInput: ParsedInput ->
            'State

    val pick:
        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) ->
        parsedInput: ParsedInput ->
            'T

    val tryFind:
        predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) ->
        parsedInput: ParsedInput ->
            SyntaxNode option

    val tryPick:
        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) ->
        parsedInput: ParsedInput ->
            'T option

    val tryPickDownTo:
        position: pos ->
        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) ->
        parsedInput: ParsedInput ->
            'T option

    val walk:
        walker: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State * SyntaxNode list option) ->
        state: 'State ->
        parsedInput: ParsedInput ->
            'State

Notes

• See the diff for ServiceParseTreeWalk.fsi for doc comments with additional descriptions.
• tryPick effectively corresponds to the existing SyntaxTraversal.Traverse API. It could also be named tryPickDownTo, tryPickUntil, or tryPickThrough.
• Many of the functions take a position parameter, because the common case is that a position of interest is already known when they are called, and so unnecessary traversal of extra nodes can be minimized. I have considered adding some kind of suffix to these functions (tryPickDownTo/tryPickAt, tryNodeAt, existsAt, etc.) in case we might also want to add variants without a position parameter (for when a position of interest is not already known), but I don't have a compelling use-case for such variants yet.
• In the future, it might make sense to add APIs for "editing" the AST: Ast.mapExpr, Ast.mapPat… This could be very useful in, e.g., certain bulk codefix scenarios.

Examples

let unnecessaryParentheses =
   (HashSet Range.comparer, parseResults.ParseTree) ||> ParsedInput.fold (fun ranges path node ->
       match node with
       | SyntaxNode.SynExpr(SynExpr.Paren(expr = inner; rightParenRange = Some _; range = range)) when
           not (SynExpr.shouldBeParenthesizedInContext getLineString path inner)
           ->
           ignore (ranges.Add range)
           ranges

       | SyntaxNode.SynPat(SynPat.Paren(inner, range)) when
           not (SynPat.shouldBeParenthesizedInContext path inner)
           ->
           ignore (ranges.Add range)
           ranges

       | _ -> ranges)

let range =
    (pos, parseResults.ParseTree) ||> ParsedInput.tryPick (fun _path node ->
        match node with
        | SyntaxNode.SynExpr (SynExpr.InterpolatedString (range = range)) when
            rangeContainsPos range pos
            -> Some range
        | _ -> None)

let isTypeName =
    (range.Start, parsedInput) ||> ParsedInput.exists (fun _path node ->
        match node with
        | SyntaxNode.SynTypeDefn(SynTypeDefn(typeInfo = typeInfo)) -> typeInfo.Range = range
        | _ -> false)

Original examples

• Use ParsedInput.fold to traverse the entire tree and accumulate results.

  let allIdents =
      (Dictionary(), parsedInput) ||> ParsedInput.fold (fun acc path node ->
          match node with
          | SyntaxNode.SynExpr(SynExpr.LongIdent(longDotId = SynLongIdent(id = id; range = range))) ->
              acc[range.End] <- id; acc
  
          | SyntaxNode.SynExpr(SynExpr.Ident id) ->
              acc[id.idRange.End] <- [id]; acc
  
          | _ -> acc)

  let unnecessaryParentheses =
     (HashSet Range.comparer, parseResults.ParseTree) ||> ParsedInput.fold (fun ranges path node ->
         match node with
         | SyntaxNode.SynExpr(SynExpr.Paren(expr = inner; rightParenRange = Some _; range = range)) when
             not (SynExpr.shouldBeParenthesizedInContext getLineString path inner)
             ->
             ignore (ranges.Add range)
             ranges
  
         | SyntaxNode.SynPat(SynPat.Paren(inner, range)) when
             not (SynPat.shouldBeParenthesizedInContext path inner)
             ->
             ignore (ranges.Add range)
             ranges
  
         | _ -> ranges)

• Use ParsedInput.foldWhile to accumulate results, but short-circuit where desired.

  let myPos = …
  let myFunc expr = …
  let myResults =
      ([], parsedInput) ||> ParsedInput.foldWhile (fun acc path node ->
          match node with
          | SyntaxNode.SynExpr expr ->
              if posLt expr.Range myPos then Some (myFunc expr :: acc)
              else None
  
          | _ -> Some acc)

• Use ParsedInput.walk to traverse the tree in a specific way, e.g., lopping off entire branches, changing the order in which a given node's children are traversed, etc.

  let myResults =
      ([], parsedInput) ||> ParsedInput.walk (fun acc path node ->
          match node with
          | SyntaxNode.SynPat(SynPat.Paren(_, range) ->
              range :: acc, None
  
          | SyntaxNode.SynExpr(SynExpr.Match(expr = _ignoreMe; clauses = clauses)) ->
              acc, Some (List.map SyntaxNode.SynMatchClause clauses)
          
          | SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = expr1; expr2 = expr2)) ->
              acc, Some [SyntaxNode.SynExpr expr2; SyntaxNode.SynExpr expr1]
          
          | _ -> acc, None)

• Use ParsedInput.tryPick to dive to the specific node you want (based on position, path, case, whatever) and return some value derived from the node, like the node and its context, or the node's range, or anything else.

  let myPos = …
  let myExpr, myContext =
      parsedInput |> ParsedInput.tryPick (fun path node ->
          match node, path with
          | SyntaxNode.SynExpr expr, context :: _ when rangeContainsPosLeftEdgeInclusive expr.Range myPos -> Some (expr, context)
          | _ -> None)

Checklist

• Test cases added.
  • All existing tests pass for existing uses of the methods on FSharpParseFileResults. These transitively call ParsedInput.tryPick, ParsedInput.tryPickLast, ParsedInput.exists, ParsedInput.tryNode, ParsedInput.foldWhile under the hood.
  • Test cases for ParsedInput.tryPick (corresponding to the existing ones for SyntaxTraversal.Traverse).
  • Basic test cases for ParsedInput.tryPickLast, ParsedInput.exists, ParsedInput.tryNode, ParsedInput.fold, ParsedInput.foldWhile.
• Release notes entry updated.
• Surface area updated.
• FCS usage documentation.

[nojaf]

This looks really promising! What comes to mind here is that the current Visitor doesn't traverse expressions by default and I would assume the new functions don't have this limitation.
Depending on whether you want this, you just use the right helper function right?

It would also be great if you could update https://fsharp.github.io/fsharp-compiler-docs/fcs/syntax-visitor.html. Feel free to swap out or remove any of the existing text there. We should educate FCS consumers about these new functions.

Great work @brianrourkeboll!

[brianrourkeboll]

What comes to mind here is that the current Visitor doesn't traverse expressions by default and I would assume the new functions don't have this limitation.
Depending on whether you want this, you just use the right helper function right?

The idea for these new functions is that every node be traversed in canonical order unless or until otherwise specified. That seems like a good default for, e.g., analyzers that will end up wanting to look at a good portion of the AST anyway via fold. Indeed, since a good portion of all code is either a SynExpr or nested under one, it seems like a good default in general, even for tryPick, etc.

The existing default behavior of Traverse + SyntaxVisitorBase of not traversing SynExpr nodes and their descendants, however, seems like a useful optimization when you know the thing you're looking for will never be found in or under a SynExpr. I assume that it was a purposeful choice when that code was originally written?

If that is truly a very common or important use-case, we could consider exposing some way of specifying desired traversal behavior in these new functions to avoid being excessively wasteful in such scenarios—perhaps a flags enum or something?

[<Flags>]
type SyntaxNodes =
    | None = 0
    | SynModuleOrNamespace = 1
    | SynModule = 2
    | SynBinding = 4
    | SynExpr = 8
    | …
    | All = …

val fold:
    folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State) ->
    nodesOfInterest: SyntaxNodes ->
    state: 'State ->
    parsedInput: ParsedInput ->
        'State

Used like:

(
    SyntaxNodes.SynModuleOrNamespace ||| SyntaxNodes.SynModule ||| SyntaxNodes.SynBinding,
    [],
    parsedInput
)
|||> ParsedInput.fold (fun acc path node -> …)

or

(
    SyntaxNodes.All ^^^ SyntaxNodes.SynExpr,
    [],
    parsedInput
)
|||> ParsedInput.fold (fun acc path node -> …)

or

(
    SyntaxNodes.All ^^^ SyntaxNodes.SynExpr,
    parsedInput
)
||> ParsedInput.tryPickDownTo pos (fun path node -> …)

It would also be great if you could update https://fsharp.github.io/fsharp-compiler-docs/fcs/syntax-visitor.html.

Yes: if others think that this new API looks useful enough to move forward with, I'd probably try to migrate existing usages of SyntaxTraversal.Traverse in this repo (and maybe something like FSAC) to the new API to prove it out, which I think might result in some good example usage.

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
vsintegration/src	docs/release-notes/.VisualStudio/17.10.md

[vzarytovskii]

lgtm, thanks, we can

[nojaf]

This review is a testament to the exceptional work done on this project. It's clear that a lot of thought and effort has been put into it, and the result is impressive.

I have some suggestions that might enhance the public API of this PR, although these are minor compared to the overall quality of the work. It seems more intuitive to continue using ParsedInput as the standard input across all new functions, given its familiarity. While the type Ast = SyntaxNode list serves its purpose internally, perhaps it's best not to expose it publicly.

Regarding the naming of the abbreviation, I personally feel that it might be more beneficial to consider a different naming convention or perhaps remove it altogether.

In the ParsedInputExtensions module, making the Contents member private could be a good move, as its utility seems limited outside the scope of the new code:

module ParsedInputExtensions =
    type ParsedInput with

        /// <summary>
        /// Gets the parsed file's contents as forest of <see cref="T:FSharp.Compiler.Syntax.SyntaxNode"/>s.
        /// </summary>
        member Contents: Ast

The introduction of module Ast = is interesting, but I wonder if it's necessary. It might be more cohesive to integrate these functions into the existing module public SyntaxTraversal, especially if ast: Ast is changed to parsedInput: ParsedInput.

Concerning the function signatures, aligning them with the functions-first rule, as commonly seen in List, Seq, Array, etc., would maintain consistency. For example:

val exists: position: pos -> predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) -> ast: Ast -> bool

// could be revised to

val exists: predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) -> position: pos -> parsedInput: ParsedInput -> bool

Finally, it may be worthwhile to revisit the F# Compiler Service's Syntax Visitor documentation. While a complete overhaul might be too extensive at this stage, adding references to these new functions could provide valuable context and guidance for users.

Overall, these points are minor compared to the excellent quality of the work,

and I believe they could further polish an already impressive project.

[brianrourkeboll]

@nojaf I appreciate the thoughtful feedback!

TL;DR

I'm willing to switch back to ParsedInput and to move the pos parameter if the following notes don't convince you otherwise. And yes, I should update the docs!

Ast (= SyntaxNode list) versus ParsedInput versus SyntaxTraversal

Composability

I admit that requiring the extraction of the underlying AST via ParsedInput.Contents on every usage may seem like an unnecessary step on its own, but what if we ever wanted to add a function like skipWhile (sample implementation here), takeWhile, or the like whose output can then be composed with other functions operating on the same type?

Taking skipWhile as an example, it wouldn't make a whole lot of sense to return a new ParsedInput with updated contents and the same file metadata copied over, since the hash directives, trivia, set of identifiers, etc., would need to be updated as well, which could be relatively expensive, and the result would no longer represent a real "parsed input file" anyway.

All uses of skipWhile or takeWhile in particular, however, could be replaced by conditions in fold, foldWhile, tryPick, etc., depending on the use case. Are there other useful functions that require composability beyond what ParsedInput allows? The answer might be "no."

AST editing?

I have also wondered whether editing functions could be added to the Ast module.

Consider something like the following (names, signatures, feasibility, and actual utility pending further thought):

val updateExprs : mapping:(SyntaxVisitorPath -> SynExpr -> SynExpr) -> ast:Ast -> Ast

val updatePats : mapping:(SyntaxVisitorPath -> SynPat -> SynPat) -> ast:Ast -> Ast

val updateBindings : mapping:(SyntaxVisitorPath -> SynBinding -> SynBinding) -> ast:Ast -> Ast

etc.

Could such relatively straightforward editing of the AST be useful in tooling scenarios? (E.g., bulk application of certain code fixes, etc.)

I think that such editing functions would be a bit awkward if made to operate on and return ParsedInput.

Pros of using ParsedInput

• No need to expose a new type (abbreviation).
• No need to dot into Contents every time.

Cons of using ParsedInput

• Somewhat less composable.
• ParsedInput technically holds several other things beyond the AST itself (contents: SynModuleOrNamespace list/contents: SynModuleOrNamespaceSig list) that something like ParsedInput.exists or ParsedInput.tryPick could theoretically apply to:
  • scopedPragmas: ScopedPragma list
  • hashDirectives: ParsedHashDirective list
  • trivia: ParsedImplFileInputTrivia
  • identifiers: Set<string>
  • …
• There is an existing ParsedInput module in the FSharp.Compiler.EditorServices namespace that exposes relatively specialized functions that are essentially unrelated to the more general ones like exists, fold, tryPick, etc.

Pros of using Ast (or SyntaxNodes/SyntaxNode list)

• More composable, at least in theory: parsedInput.Contents |> Ast.skipWhile predicate |> Ast.tryPick pos chooser, etc.

Cons of using Ast (or SyntaxNodes/SyntaxNode list)

• There is not really any perfect, unambiguous, non-potentially-misleading name for it (Ast, SyntaxNodes, SyntaxNode list…).
• Requires dotting into ParsedInput.Contents every time; ParsedInput.Contents isn't really useful for anything else.
• The extra theoretical composability might not actually be useful.
• SyntaxNode list is a defective representation of the actual AST to begin with. It's nowhere near granular enough to allow constructing a new AST using something like Ast.unfold, for example. Perhaps we ought not attempt to make it seem like a first-class construct by giving it its own type abbreviation and module.
• ParsedInput is already treated as "the AST" in many places.

I don't think we should use the SyntaxTraversal module. It doesn't have the RequireQualifiedAccess attribute, and I don't think exists, tryPick, fold, etc., should be allowed to be brought into scope unqualified. Since the SyntaxTraversal module name doesn't correspond to any type name, it's also relatively hard to discover without seeing existing code or reading the docs.

I admit that I don't really know the right answer here. I'm willing to just use ParsedInput for now, and we can expose Ast later if we ever actually have a compelling use for it.

Parameter order

Hmm. There are a few reasons why I put position first:

1. If we had two versions of each function, one that took a position and one that didn't, I think it would make sense for the pos parameter to come first, if only for naming's sake: ast |> Ast.existsAt pos predicate alongside ast |> Ast.exists predicate; ast |> Ast.tryPickAt pos predicate alongside ast |> Ast.tryPick predicate; etc. The main reason that this PR does not include such pairs is because every existing use-case for these functions already has a useful position in scope.

2. Once a pos has been applied as the first argument, the resulting partially applied function's signature matches its canonical equivalent. This makes sense to me: the versions of exists or tryPick that take a position are just optimized versions of the equivalent functions that don't.

3. Since the higher-order functions passed into these module functions are nearly always single-use multiline lambdas, moving the pos parameter later is likely to require double-piping at the callsite to maintain legibility.

   Compare:

   parsedInput.Contents
   |> Ast.exists pos (fun path node ->
       (* Some
          multiline
          lambda. *))

   versus

   parsedInput.Contents
   |> Ast.exists (fun path node ->
       (* Some
          multiline
          lambda. *)) pos

   or

   (pos, parsedInput.Contents)
   ||> Ast.exists (fun path node ->
       (* Some
          multiline
          lambda. *))

I don't think my opinion is super strong here, though (unless anyone thinks we should use the At suffix in case we want to expose versions of these functions that don't take a pos).

FCS docs

I 100% agree that I should update these, either in this PR or in a followup.

[nojaf]

Thanks, @brianrourkeboll, for putting your thoughts together on this. I remain skeptical about the additional arguments presented.

Composability

Regarding the possibility of adding functions in the future, it's worth noting that the current primary users of SyntaxVisitors—namely FSAC, VS tooling, and analyzers—interact at the file level with ParsedInput. They aim to traverse this tree to detect specific syntax constructs. I am hesitant to advocate for Ast = SyntaxNode list solely based on composability, absent a compelling, concrete use case. My concern is introducing an API that might not entirely meet our practical needs due to an overemphasis on theoretical benefits.

AST Editing

As for the suggestion of editing the AST, I remain unconvinced of its utility in our tooling scenarios. Considering our main consumers, they typically suggest changes in a text format. While manipulating the AST directly might seem appealing, the critical step of converting these changes back to text is non-trivial and is not a process our consumers are equipped to handle efficiently.

Parameter Order

My stance on parameter order is somewhat flexible, but there's a logical argument for placing pos after the predicate. Consider the function signature:

val exists: predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) -> position: pos -> parsedInput: ParsedInput -> bool

The structure facilitates a natural application flow. Start by applying a generic function applicable to any file:

let typeExists = exists (fun path node -> true)
let typeExistsInFile = typeExists cursorPosition parsedInput

Then specify the concrete cursor position and file input. This rationale underpins my preference for arranging the predicate first.

[psfinaki]

My 2Kč on this:

• feels like a step in the right direction
• definitely an improvement on the editor side
• AST side, @nojaf has probably the best perspective, my main preference is to keep the code as clear as possible here
• docs update should probably go into this PR for better trackability and reviewability

[nojaf]

Astonishing work @brianrourkeboll!

[brianrourkeboll]

I'm working on updating the docs and hope to push that today.

[psfinaki]

Thanks, looking forward to have is in :)

[dawedawe]

The docs are just golden like your API work 🥇

[psfinaki]

Thanks Brian! Amazing work - keep rocking :)

[baronfel]

I don't think the FCS in the public feed has this API yet - 43.8.300-preview.24080.5 is that version. Can someone from the team check if packages are flowing to nightly? I only have a single 43.8.300 preview package at all in that feed:

And that version is from 2024/02/13, so I'd expect it to have this API?

Ignore me, I am dumb and didn't realize this was in a new namespace.