Add with keyword to Syntax tree

[nojaf]

This PR adds the with keyword range to various nodes in the Syntax tree.
//cc @dsyme

[auduchinok]

I wonder if we could move ranges with keywords and punctuation like this into separate aggregating fields?
It'd simplify using AST (since there would be less fields to reason about) and for most FCS users it'll be easier to fix their code after new data is added to that additional field. We previosly didn't keep that much token-related data, but now it's becoming a bit too many fields in many common AST node types.

[nojaf]

separate aggregating fields

How would that look like? If it is something entirely outside of the tree then that already has little value as I would need to piece these things together again somehow. Unless you mean something else.

FCS users it'll be easier to fix their code

You don't need any changes if you use named pattern matching.
Example:

match expr with
| SynExpr.TryWith (tryExpr=innerComp; withCases=clauses; range=mTryToLast; tryDebugPoint=spTry) -> ...

I'm updating all the code in the compiler like this as I go and for one commit I changed the tree and parser and no other changes were necessary.

[dsyme]

Great work @nojaf

How would that look like? If it is something entirely outside of the tree ..

I think @auduchinok means a systematic typed "trivia" field, as a version resilient class, in each node. We could in principle though not as part of this PR of course :)

[auduchinok]

I think @auduchinok means a systematic typed "trivia" field, as a version resilient class, in each node. We could in principle though not as part of this PR of course :)

Yes, just to move it out of the main node types, so there're less fields to pattern match and update on FCS changes.

[auduchinok]

You don't need any changes if you use named pattern matching.

I think this shouldn't be a requirement for FCS clients.

I'm updating all the code in the compiler like this as I go and for one commit I changed the tree and parser and no other changes were necessary.

It'd likely be better if it was possible without that many changes. 🙂

[nojaf]

Ok, thanks for elaborating. For better or worse, I'm getting a lot of ideas actually by this 🙈.
Interesting train of thought.

[nojaf]

@dsyme this is ready for review.

[dsyme]

This is really great! Just a few nits, nothing much

Thanks

[nojaf]

This is really great! Just a few nits, nothing much

Thanks, never ever doubt about raising nitpicks. All good, always, any time ☺!

[auduchinok]

@nojaf What problem does a separate match range field solve here? The keyword starts at the same range as the whole match expression and is a single token of a known width. Given each added range is rather a big struct, it seems wasteful to add ranges that can be calculated from the nodes.

Maybe we could add some helpers that would calculate keyword ranges from their positions instead of storing them in trees? We could use computed properties and only store positions in Trivia/Appendix types for keywords where storing position is required.

[nojaf]

Feels like we are talking in circles here:

I already expressed why ranges are preferred over calculations with positions.
@dsyme what do you think?

[auduchinok]

This time I'm suggesting this calculation to be the details of implementation in FCS, while keeping range properties that Fantomas could work with. 🙂

Calculating a range from position doesn't look too bad perf-wise and ranges are structs, so creating them shouldn't add GC pressure. On the other hand, the two keywords in match expr use 8 ints to store data that could use 2 ints. And if we're going to add this for all keywords, it becomes a bit too much.

[nojaf]

Well, I'm slowly getting on board with this.
Although, is there a way to get a position inside the parser?
Places where I now use rhs parseState 1, is there something similar that just gets the start position of 1?

[nojaf]

@auduchinok would that then look something like:

#r "nuget: FSharp.Compiler.Service"

open FSharp.Compiler.Text
open FSharp.Compiler.Text.Range
open FSharp.Compiler.Text.Position

type NodeTrivia =
    { StartPos: pos
      FileName: string }

    member this.Range =
        mkRange this.FileName this.StartPos (mkPos this.StartPos.Line (this.StartPos.Column + 4))

let myTrivia =
    { StartPos = mkPos 1 0
      FileName = "MyFile" }

myTrivia.Range

?

[auduchinok]

@nojaf Yes, the idea for getting a range is roughly like this. I'm also trying to find a better way to keep the keyword position only and to get the file name from the overall node range. Something like this could work:

let getKeywordRange (containingRange: range) startPos length =
    mkRange containingRange.FileName startPos (mkPos startPos.Line (startPos.Column + length))

[<Struct>]
type SynExprMatchTrivia =
    { WithPos: pos }

    member this.GetMatchKeywordRange(exprRange: range) =
        getKeywordRange exprRange exprRange.Start 5

    member this.GetWithKeywordRange(exprRange: range) =
        getKeywordRange exprRange this.WithPos 4

[dsyme]

I would prefer a range than a position. The position on its own is of no use, we would always reconstruct the range. These particular nodes are of no intrinsic significance when it comes to memory performance

[dsyme]

_mWith to keep it consistent

[dsyme]

We should lift all this code out into SyntaxTreeOps.fs

[nojaf]

This is quite tricky to just port.
reportParseErrorAt and raiseParseErrorAt are being used inside it.
Could we do this another time perhaps?

[dsyme]

Yes sure, I meant to say that

[dsyme]

Nice work, just a couple of very minor nits