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

Extract out AST-collecting-walker to a separate function + abstract class #1154

Merged
merged 2 commits into from
Aug 26, 2023

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented Aug 25, 2023

WHAT

🤖 Generated by Copilot at c7037be

Refactored the code for finding folding ranges in F# files. Moved the logic to a new FoldingRange module and used a custom syntax walker to collect the ranges. Split the UntypedAstUtils.fsi file into Syntax.fs and UntypedAstUtils.fs for better modularity.

🤖 Generated by Copilot at c7037be

FoldingRange walks
Syntax trees in separate file
Winter of refactor

🔧📦📑

WHY

I logged an issue at dotnet/fsharp a while ago to pull out this walker from UntypedAstUtils into a reusable component because I thought it would be helpful. This walker gives the opportunity to do an action at every node which is helpful for 'collecting' style operations, like folding ranges and the soon-to-be-implemented nested language support.

This PR is the infrastructure for that - it pulls out the collecting walker into an abstract class for the caller to override, and a walking function that the walker is passed to - very much like the existing position-finding walker. It then uses that walker to reimplement the folding-range detection, which now is very simple to understand.

HOW

🤖 Generated by Copilot at c7037be

  • Refactor the logic for finding folding ranges to a separate module and use a custom syntax walker (link, link)
  • Split the UntypedAstUtils.fsi file into two files: Syntax.fs and UntypedAstUtils.fs (link)

Comment on lines +1 to +37
namespace FSharp.Compiler

module Syntax =
open FSharp.Compiler.Syntax

type SyntaxCollectorBase =
new: unit -> SyntaxCollectorBase
abstract WalkSynModuleOrNamespace: SynModuleOrNamespace -> unit
abstract WalkAttribute: SynAttribute -> unit
abstract WalkSynModuleDecl: SynModuleDecl -> unit
abstract WalkExpr: SynExpr -> unit
abstract WalkTypar: SynTypar -> unit
abstract WalkTyparDecl: SynTyparDecl -> unit
abstract WalkTypeConstraint: SynTypeConstraint -> unit
abstract WalkType: SynType -> unit
abstract WalkMemberSig: SynMemberSig -> unit
abstract WalkPat: SynPat -> unit
abstract WalkValTyparDecls: SynValTyparDecls -> unit
abstract WalkBinding: SynBinding -> unit
abstract WalkSimplePat: SynSimplePat -> unit
abstract WalkInterfaceImpl: SynInterfaceImpl -> unit
abstract WalkClause: SynMatchClause -> unit
abstract WalkInterpolatedStringPart: SynInterpolatedStringPart -> unit
abstract WalkMeasure: SynMeasure -> unit
abstract WalkComponentInfo: SynComponentInfo -> unit
abstract WalkTypeDefnSigRepr: SynTypeDefnSigRepr -> unit
abstract WalkUnionCaseType: SynUnionCaseKind -> unit
abstract WalkEnumCase: SynEnumCase -> unit
abstract WalkField: SynField -> unit
abstract WalkTypeDefnSimple: SynTypeDefnSimpleRepr -> unit
abstract WalkValSig: SynValSig -> unit
abstract WalkMember: SynMemberDefn -> unit
abstract WalkUnionCase: SynUnionCase -> unit
abstract WalkTypeDefnRepr: SynTypeDefnRepr -> unit
abstract WalkTypeDefn: SynTypeDefn -> unit

val walkAst: walker: SyntaxCollectorBase -> input: ParsedInput -> unit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the bit that I would contribute upstream at some point (maybe the Active Patterns as well but they aren't strictly necessary).

Comment on lines +69 to +77
module FoldingRange =
open FSharp.Compiler.Syntax
open FSharp.Compiler.Text

val getRangesAtPosition: input: ParsedInput -> r: Position -> Range list

module Completion =
open FSharp.Compiler.Syntax
open FSharp.Compiler.Text
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at these nice, compact modules with no extraneous information in them.


loop [] pats

type SyntaxCollectorBase() =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this type I looked at every walkX member in the walker function from the previous version and made a WalkX member on this type. The walker function handles going 'deeper' into the structure, so the member overrides that users write here should be strictly focused on looking at this particular syntax node, not doing traversal themselves.

abstract WalkTypeDefn: SynTypeDefn -> unit
default _.WalkTypeDefn s = ()

let walkAst (walker: SyntaxCollectorBase) (input: ParsedInput) : unit =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation here is the same as before, but with all of the addIfInside calls replace with calls to walker.WalkX as appropriate.

if (Range.rangeContainsPos m pos) then
ranges.Add m

override _.WalkSynModuleOrNamespace m = addIfInside m.Range
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we pulled out all of the noise of walking the tree, these individual node decision become really easy to reason about. Many AST nodes provide a helpful Range member so that we don't have to unpack too many of the nodes.

Comment on lines +653 to +656
let getRangesAtPosition input (r: Position) : Range list =
let walker = new RangeCollectorWalker(r)
walkAst walker input
walker.Ranges |> Seq.toList
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now the method becomes more understandable.

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@baronfel baronfel merged commit a68786a into ionide:main Aug 26, 2023
10 checks passed
nojaf pushed a commit to nojaf/FsAutoComplete that referenced this pull request Nov 3, 2023
…lass (ionide#1154)

* extract out AST-collecting-walker to a separate function + abstract class and use it for range aggregation

* better factoring of the syntax-walking code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants