Fix duplicated trivias in combination with double nested try-with statements #1969

[KarolBajkowski]

No description provided.

[nojaf]

Hello @KarolBajkowski, you are on the right track here.
Keep it up!

[nojaf]

This check is a bit too vague I'm afraid.
It will fail if the with keyword is higher up:

[<Test>]
let ``comment is still lost`` () =
    formatSourceString
        false
        """
try
    a
// comment
with


| b -> c
"""
        config
    |> prepend newline
    |> should
        equal
        """
try
    a
// comment
with


| b -> c
"""

I would add mWithToLast to the active pattern:
(parser source)

let (|TryWith|_|) =
    function
    | SynExpr.TryWith (e, _, cs, mWithToLast, _, _, _) -> Some(e, mWithToLast, cs)
    | _ -> None

and extract the with range as:

| TryWith (e, mWithToLast, cs) ->
 ....
                    let lookupRange =
                        ctx.MkRangeWith
                            (mWithToLast.StartLine, mWithToLast.StartColumn)
                            (mWithToLast.StartLine, mWithToLast.StartColumn + 4)

[KarolBajkowski]

I updated the code with your suggestion. Thank you for your help :) I hope that will work now, even though it's probably my worst PR in my life :D

[KarolBajkowski]

I had to miss your suggested code :/ I'm wondering why this code works even with +3 like (mWithToLast.StartLine, mWithToLast.StartColumn + 3)

[nojaf]

Yeah, well that might be an offset by 1 error on the token side.
The trivia node that has the comment is going from 3,0 - 3,3

mWithToLast shall in this case span from 3,0--6,8.
tokN will try and look within lookupRange whether the WITH token exists.
Inclusively, so that is why it is dangerous to look in a range that is too long.
I took + 4 because the with keyword is 4 characters long.

Once dotnet/fsharp#12400 is merged, we can look for that exact range (via ASTTransformer) and use range contains. Don't worry about this last bit, that is something for in the future.

[KarolBajkowski]

Thanks for explanation! It really helps. But I must admit for me it was too much rush. I have to slowly finish fantomas youtube videos and look again at the source code and assembly all those puzzles together. I think I intuitively understand what's going on, but I need better understanding... TBH this is my first contact with compiler source code especially in F# (which I think basics I know very well but still I'm wrapping my head around some F# features/patterns which are not common in OOP)

[nojaf]

Yes, it took me a lot of time to understand this project when I first started out.
That is why I made these videos, to speed up this process for newcomers.
But I'm well aware not everything immediately clicks. Especially all those custom operators.
At first you sorta conceptually understand what they do, and later you really start understanding the events they add to the context.
I'm in the same boot that before this project, I never saw a compiler up close.
That was also a very overwhelming brave new world, but with all things, eventually, you get the hang of it.

Thanks again for all your patience and for engaging in conversation.

[nojaf]

Hi Karol, don't be so hard on yourself.
This is a good first PR!
Fantomas is just one of those codebases where things can be delicate from time to time.

[nojaf]

Close, but this still doesn't cover all cases:

[<Test>]
let ``nested try/with with comment on with`` () =
    formatSourceString
        false
        """
try
    a
with
| b ->
    try c
    // inner comment
    with
    | d -> ()
"""
        config
    |> prepend newline
    |> should
        equal
        """
try
    a
with
| b ->
    try
        c
    // inner comment
    with
    | d -> ()
"""

This will still fail, unfortunately. (online tool)

[KarolBajkowski]

I updated the code and test cases. Please take a look when you find some free time 🙏

[nojaf]

@KarolBajkowski I think this is good to go.
I noticed one typo in a unit test name but I can change that myself.
Can we convert the draft to ready PR?

[KarolBajkowski]

@KarolBajkowski I think this is good to go. I noticed one typo in a unit test name but I can change that myself. Can we convert the draft to ready PR?

Yes. I also thought about test naming convention, because I was not sure if the test names are readable. I'm use to Given When Then pattern so it's for me natural to first express all preconditions, then action (sometimes it's obvious so it can be omitted), and then expected outcome.

[nojaf]

Many thanks, @KarolBajkowski for this first contribution!

[KarolBajkowski]

@nojaf I tried to give my best! I thank you for your patience and help :) That was remarkable experience!