Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Nov 19, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
293f101
fix WIP draft
KarolBajkowski Nov 17, 2021
e21b02b
fix test name to avoid confusion
KarolBajkowski Nov 17, 2021
3ac9799
resolve some review comments
KarolBajkowski Nov 18, 2021
85080df
cleanup
KarolBajkowski Nov 18, 2021
2f7a0af
nits
KarolBajkowski Nov 18, 2021
8496983
reformat using 'dotnet fantomas src -r'
KarolBajkowski Nov 18, 2021
f7adb1b
follow test code conventions
KarolBajkowski Nov 18, 2021
67d8038
cover one more case
KarolBajkowski Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions src/Fantomas.Tests/CommentTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,3 +1599,89 @@ type TorDirectory =
}
}
"""

[<Test>]
let ``double try-with, comment before inner 'with' not duplicated, 1969`` () =
formatSourceString
false
"""
try
try
()
// xxx
with
| _ -> ()
with
| _ -> ()
"""
config
|> prepend newline
|> should
equal
"""
try
try
()
// xxx
with
| _ -> ()
with
| _ -> ()
"""

[<Test>]
let ``comment shold not be lost`` () =
formatSourceString
false
"""
try
a
// comment
with


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


| b -> c
"""

[<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 -> ()
"""
33 changes: 33 additions & 0 deletions src/Fantomas.Tests/CompilerDirectivesTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2659,3 +2659,36 @@ let ``indented #if directive inside another non-indented #if directive should fo
#endif
#endif
"""

[<Test>]
let ``double try-with, inner #if directive should not throw error, 1969`` () =
formatSourceString
false
"""
try
try
()
#if FOO
()
#endif
with
| _ -> ()
with
| _ -> ()
"""
config
|> prepend newline
|> should
equal
"""
try
try
()
#if FOO
()
#endif
with
| _ -> ()
with
| _ -> ()
"""
10 changes: 8 additions & 2 deletions src/Fantomas/CodePrinter.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2023,14 +2023,20 @@ and genExpr astContext synExpr ctx =

atCurrentColumn (colWithNlnWhenItemIsMultilineUsingConfig items) ctx
// Could customize a bit if e is single line
| TryWith (e, cs) ->
| TryWith (e, mWithToLast, cs) ->
atCurrentColumn (
kw TRY !- "try "
+> indent
+> sepNln
+> genExpr astContext e
+> unindent
+> kw WITH !+~ "with"
+> (fun ctx ->
let lookupRange =
ctx.MkRangeWith
Copy link
Contributor

@nojaf nojaf Nov 17, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@KarolBajkowski KarolBajkowski Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@KarolBajkowski KarolBajkowski Nov 18, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nojaf nojaf Nov 18, 2021

Choose a reason for hiding this comment

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

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

image

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.

Copy link
Contributor Author

@KarolBajkowski KarolBajkowski Nov 18, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@nojaf nojaf Nov 19, 2021

Choose a reason for hiding this comment

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

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.

(mWithToLast.StartLine, mWithToLast.StartColumn)
(mWithToLast.StartLine, mWithToLast.StartColumn + 3)

tokN lookupRange WITH !+~ "with" ctx)
+> indentOnWith
+> sepNln
+> col sepNln cs (genClause astContext true)
Expand Down
2 changes: 1 addition & 1 deletion src/Fantomas/SourceParser.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,7 +1073,7 @@ let (|LongIdentSet|_|) =

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

let (|TryFinally|_|) =
Expand Down