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

Idempotency problem when no space between getter and parameter #2291

Closed
3 tasks
lkluc opened this issue Jun 6, 2022 · 7 comments
Closed
3 tasks

Idempotency problem when no space between getter and parameter #2291

lkluc opened this issue Jun 6, 2022 · 7 comments

Comments

@lkluc
Copy link

lkluc commented Jun 6, 2022

Issue created from fantomas-online

Formatted code

module FsAstExample

type internal Foo() =
    member this.get(l: int list) = 42

type internal Bar() =
    member this.bar() = new Foo()

    member this.bar(idx1, idx2) =
        this.bar().get[idx1
        idx2]

Reformatted code

module FsAstExample

type internal Foo() =
    member this.get(l: int list) = 42

type internal Bar() =
    member this.bar() = new Foo()

    member this.bar(idx1, idx2) = this.bar().get[idx1 idx2]

Problem description

Fantomas was not able to produce the same code after reformatting the result.

Extra information

  • The formatted result breaks my code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.

I stumble upon this problem when trying to generate code with FsAst, I simplified the code to show the problem.

Now I don't know if Visual Studio is too permissive, but it accepts when the code is specified without the space :
"this.bar().get[idx1; idx2]"

after Fantomas reformat, the list is split in two lines without the appropriate indentation, and it seems the first parameter is inferred as a ('a -> int) type...

Options

Fantomas master branch at 2022-05-30T18:11:41Z - 5e92752

Default Fantomas configuration

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

@nojaf
Copy link
Contributor

nojaf commented Jun 7, 2022

Hello, thank you for reporting this issue.
This is a duplicate of #2242.
Please read that thread to discover the relevance of that space.
I've added a hint here to solve this, are you interested in submitting a PR?

@lkluc
Copy link
Author

lkluc commented Jun 7, 2022

Hello Florian, thanks for the prompt reply! :)

I have unfortunately no fix for this problem. I'm currently working on a code generating project and I thought I was missing important part in my generated AST, and came to the conclusion after some comparative tests that Ranges are involved and would be necessary (not only dummy range0) to generate the appropriate code.

After 2-3 days fighting this problem, I finally thought about a simple workaround, and blame myself under a huge pile of rocks of shame for not having thought about it earlier : parenthesis around the list! doh!

Once I have finished with my code generation project, I might be able to push some improvement on the FsAst project though, but I have to figure out first if my workplace would allow it.

@nojaf
Copy link
Contributor

nojaf commented Jun 7, 2022

One thing comes to mind here:
We check if the expression is an indexer expression via this active pattern:

let rec (|IndexWithoutDotExpr|NestedIndexWithoutDotExpr|NonAppExpr|) e =
match e with
| SynExpr.App (ExprAtomicFlag.Atomic, false, identifierExpr, SynExpr.ArrayOrListComputed (false, indexExpr, _), _) ->
IndexWithoutDotExpr(identifierExpr, indexExpr)
| SynExpr.App (ExprAtomicFlag.NonAtomic,
false,
identifierExpr,
(SynExpr.ArrayOrListComputed (isArray = false; expr = indexExpr) as argExpr),
_) when (RangeHelpers.isAdjacentTo identifierExpr.Range argExpr.Range) ->
IndexWithoutDotExpr(identifierExpr, indexExpr)
| SynExpr.App (ExprAtomicFlag.NonAtomic, false, IndexWithoutDotExpr (identifier, indexExpr), argExpr, _) ->
NestedIndexWithoutDotExpr(identifier, indexExpr, argExpr)
| _ -> NonAppExpr

Where I'm wondering what

let isAdjacentTo (r1: Range) (r2: Range) : bool =
r1.FileName = r2.FileName
&& r1.End.Line = r2.Start.Line
&& r1.EndColumn = r2.StartColumn

would return if both ranges are range0. Maybe faking one of the ranges, so that it is further away might resolve your problem.

@lkluc
Copy link
Author

lkluc commented Jun 7, 2022

Yep, it crossed my mind to fake a +1 range just to see if it could work. But I thought it might been more contrived than this, in fact I was a bit surprised and discouraged at first to realize that ranges could be an important part of the AST tree, but when you think about it 2 seconds, I guess we could say that F# code have part of its semantic based on spaces, and relying on ranges might be unavoidable.

But yes, in deed, thanks for the code excerpt, it was on my TODO list eventually to look at Fantomas's interns, and I agree, it might do the trick! :)

@nojaf
Copy link
Contributor

nojaf commented Jun 7, 2022

Yes, a part of me also wished that we had a separate SynExpr case for the new indexer syntax without a dot. I'm not entirely sure what the rationale is there.

When it comes to code generation in general, you really need to follow as closely as possible what the parser produces. That is the tree that Fantomas can format, any deviation might not end well.

@lkluc
Copy link
Author

lkluc commented Jun 16, 2022

Hello nojaf,

Where I'm wondering what
(...)
would return if both ranges are range0. Maybe faking one of the ranges, so that it is further away might resolve your problem.

Yup, that did the trick, as you expected with a simple range of +1 :

let range0 = Range.range0
let pos1 = Position.mkPos 0 1
let range1 = Range.mkRange range0.FileName pos1 pos1

Thanks!

@nojaf
Copy link
Contributor

nojaf commented Jun 21, 2022

Alright, I'll close this issue as there are many duplicates of this one already.

@nojaf nojaf closed this as completed Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants