`defaultTraverse` for `SynPat` in `SyntaxTraverse.Traverse` doesn't walk down all `SynPat`s

[Booksbaum]

SyntaxTraverse.Traverse(pos, ast, SyntaxVisitorBase<_>) passes defaultTraverse to all visitor.VisitXXX(...) -> implementation of SyntaxVisitorBase doesn't have to handle all Syntax constructs, but can pass current element on to defaultTraverse.

But for SynPats (VisitPat), defaultTraverse doesn't handle all cases with nested Syntax:
traversePat.defaultTraverse is missing:

• ~~SynPat.As(lhsPat: SynPat; rhsPat: SynPat)~~
• SynPat.Record(fieldPats: (_*_*SynPat) list
• SynPat.IsInst(pat: SynType)
• SynPat.QuoteExpr(expr: SynExpr)
• (SynPat.FromParseError(pat: SynPat))

Repro steps

Given F# source (-> text=):

let (value1) = 42
let (_ as value2) = 42
let (value3 as _) = 42

type R = { Value: int option }
let ({ Value = Some value4 }) = { Value = Some 42 }

and seeking SynPat.Named that starts with value:

let walkToNamed pos (name: string) input =
  SyntaxTraversal.Traverse(pos, input, { new SyntaxVisitorBase<_>() with
    member _.VisitPat(_, defaultTraverse, pat) =
      match pat with
      | SynPat.Named (ident=ident) when ident.idText.StartsWith name ->
          Some ident.idText
      | _ -> defaultTraverse pat
  })
let tree = getUntypedTree (SourceText.ofString text)
let poss = [
  Position.mkPos 2 5    // value1
  Position.mkPos 3 10   // value2
  Position.mkPos 4 5    // value3

  Position.mkPos 7 20   // value4
]

for pos in poss do
  let res = walkToNamed pos "value" tree
  printfn "%A -> %A" pos res

source with nuget package (without SynPat.Record example)
(unfortunately not as single Script File for F# interactive because I ran into #12703)
source for current main (with SynPat.Record example)

Expected behavior

Walk down to each value and output:

(2,5) -> Some "value1"
(3,10) -> Some "value2"
(4,5) -> Some "value3"
(7,20) -> Some "value4"

Actual behavior

Only finds first Named:

(2,5) -> Some "value1"
(3,10) -> None
(4,5) -> None
(7,20) -> None

(current nuget package)

and doesn't find Record on current main:

(2,5) -> Some "value1"
(3,10) -> Some "value2"
(4,5) -> Some "value3"
(7,20) -> None

Known workarounds

Custom handling of ~~SynPat.As~~ SynPat.Record (etc. when necessary).

But I think we now have to custom handle everything because defaultTraverse is

1. just for SynPat
2. its path is fixed to current VisitPat invocation (-> just for current pat, but not its children)
3. no traverseXXX is exposed (-> cannot get back into SyntaxTraversal.Traverse.traverseXXX calls)

(traverseSynExpr exposes itself to visitor.VisitExpr -- maybe there's a way to always expose all traverseXXX functions to visitors so visitor implementation can handle children?)

Related information

Provide any related information (optional):

• dotnet --version: 6.0.202

---

Edit: SynPat.As is handled in current main (https://github.com/dotnet/fsharp/issues/13114#issuecomment-1120449166)
-> Updated example to include SynPat.Record

[kerams]

As and IsInst (behind As) have been fixed in https://github.com/dotnet/fsharp/pull/12837 / https://github.com/dotnet/fsharp/pull/12930

[Booksbaum]

You're right.
I got nuget package and main branch mixed up: I tested with nuget, and only checked defaultTraverse in main branch -- and overlooked SynPat.As. Sorry about that 😞

For other patterns it's of course still the case
-> Added example for SynPat.Record to description

---

I cannot find SynPat.IsInst in defaultTraverse?

[kerams]

I cannot find SynPat.IsInst in defaultTraverse?

Now I am sorry :P. It's not part of the default traverse, but I am overriding it in a custom walker. I don't see why traverseSynType should not be called on the node by default though. As for Record, I don't know. Where do we draw the line of what should be visited by default?

[Booksbaum]

As for Record, I don't know. Where do we draw the line of what should be visited by default?

I'm currently trying to detect if parens are necessary when a type annotation gets added.
What I do is: Walk down to SynPat.Named (+ a couple of other locations -- but Named is most common) and then inspect the parent (or ancestors) and decide if parens are necessary.

match ... with
| { Data = 42; Value = value } -> ()
| _ -> ()

To get to SynPat.Named("value") I have to go down SynPat.Record
(Ast)
(Probably not the most common location to have a type annotation -- but I like to keep it general)
-> without defaultTraverse handling SynPat.Record I have to implement a custom traversePat -- which is basically the same as existing defaultTraverse but with added SynPat.Record handling (and removed SynType traversal because I don't have access to existing traversal methods but fortunately don't need to traverse SynType...) -- I literally just copied defaultTraverse for SynPats from dotnet/fsharp and just applied these two changes....

And: It's unexpected when it doesn't go down a path. It should at least be documented which elements gets traversed and which don't.
At least: there should be a traverseSynXXX to visit children like it's available in VisitExpr (though just for SynExpr, not for other elements)

---

but I am overriding it in a custom walker

The issue with a custom walker is: There's no available default traversal
-> you have to implement all traverse functions yourself:

member _.VisitPat (_, _, pat) =
  match pat with
  | SynPat.IsInst(pat=pat: SynType) -> ...
  | _ -> ...

-> We have to traverse SynType -> must implement a defaultTraverse for SynType (-> to use directly to traverse or to call VisitType(path, defaultTraverse, ty)).

But to go further down we have to implement more traversals:
SynType.StaticConstantExpr(expr=expr: SynExpr)
-> must implement traversal for SynExpr too

And SynExpr contains a couple of other SynXXXs too -> again additional traversals

And now we basically have our own SyntaxTraversal.Traverse implementation....

hmm...maybe SyntaxTraversal should make its traverseSynXXX functions public?

---

BTW: same issue when you want to select something that's not traversed by default.
I wanted to go down SynExpr.Lambda(parsedData=Some (pats, expr)) -- but what gets traversed is SynExpr.Lambda(args; body). Fortunately: pats is relative small and I don't need to go down Types (-> relatively simple custom traversal) and VisitExpr provides a traverseSynExpr which allows to walk down children :)