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

Wrong anon module formatting when filename starts with a digit #408

Closed
auduchinok opened this issue Feb 1, 2019 · 12 comments
Closed

Wrong anon module formatting when filename starts with a digit #408

auduchinok opened this issue Feb 1, 2019 · 12 comments

Comments

@auduchinok
Copy link
Contributor

Consider file 60Seconds.fsx with following content:

let simplePatternMatch =
    let x = "a"
    match x with
    | "a" -> printfn "x is a"
    | "b" -> printfn "x is b"
    | _ -> printfn "x is %s" x

It's being formatted to

     |
     |
     |
module ``60Seconds``
let simplePatternMatch =
    let x = "a"
    match x with
    | "a" -> printfn "x is a"
    | "b" -> printfn "x is b"
    | _ -> printfn "x is %s" x
@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Hmm, not getting this by formatting with command line.
Any idea how you are doing the formatting? What code do you call?

@auduchinok
Copy link
Contributor Author

auduchinok commented Feb 1, 2019

@nojaf Sorry I forgot to add the initial issue link.
https://youtrack.jetbrains.com/issue/RIDER-24086
I suppose it's default Rider formatting settings, i.e. have Preserve end of line option turned on.

@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Ok, I'm able to reproduce this in Rider. I think there are two problems:

  • A module statement is added where none existed.
  • Preserve end of line adds pipes and messes up the code.

For the first problem I'd like to know how Fantomas is called when pressing Reformated Code.
@auduchinok could you point me to the code in https://github.com/JetBrains/fsharp-support?

As for the second problem, --preserve-eol will go away in the next major so I'm not that eager to solve that problem.

@auduchinok
Copy link
Contributor Author

auduchinok commented Feb 1, 2019

@nojaf Sure, look at ReformatCode.fs.

The first problem looks like a repro for #264.

As for the second problem and that option going away I'd be happy to discuss it when the next version is being designed as I think it's quite important to try to preserve user formatting for line endings and that's why we have this option turned on by default.

@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Well as might know by now I'm not sure we should preserve user formatting.

  • Other formatters (based on AST) don't have this. F.ex prettier.
  • Fantomas didn't have this option for most of its existence. I feel like we merged it in without fully understanding all the consequences. And the current implementation has a lot of issues, even more than logged. And the original writer of --preserveEOL has moved on.
  • preserve user formatting is also a very broad spectrum. Where do we draw the line?

Feel free to further discuss in #390.

@auduchinok
Copy link
Contributor Author

auduchinok commented Feb 1, 2019

Other formatters (based on AST) don't have this. F.ex prettier.

R# and IntelliJ do it for other languages. I believe as a user I'd expect it to do it for F# code as well. :)

Thanks, I'll try to add some meaningful input there.

@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Is there any way of knowing the value of parsedInput ?
What would the exact AST be? Could you show me how the AST was created?

@auduchinok
Copy link
Contributor Author

It's coming from FSharpChecker.ParseFile, called inside FSharpCheckerService.fs. It should be the same result as when using FSharp.Compiler.Sevice.dll from NuGet inside a script, at least before the recently (yesterday?) released FCS, due to changes in dotnet/fsharp#6027 which help with dealing with anon modules.
Otherwise, If you want to debug our code in Rider, you can build the F# plugin and attach to R# process calling everything discussed above.

@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Yeah I knew it would have to come from FSharpChecker but was more wondering what FSharpParsingOptions are passed.

@auduchinok
Copy link
Contributor Author

It's default options with file name set and IsExe = true for scripts (i.e. this case).

let getParsingOptionsForSingleFile (file: IPsiSourceFile) =
        { FSharpParsingOptions.Default with SourceFiles = [| file.GetLocation().FullPath |] }
member x.GetParsingOptions(file) =
    if isScriptLike file then { getParsingOptionsForSingleFile file with IsExe = true } else

    getOrCreateFSharpProject file
    |> Option.map (fun fsProject -> fsProject.ParsingOptions)
    |> Option.defaultWith (fun _ -> getParsingOptionsForSingleFile file)

@nojaf
Copy link
Contributor

nojaf commented Feb 1, 2019

Ok, thanks for all the pointers. I can try and write a unit test now.

nojaf added a commit that referenced this issue Jul 12, 2019
@nojaf
Copy link
Contributor

nojaf commented Jul 12, 2019

I can't reproduce this anymore in Rider.
And I've added a regression test.
Should be ok, please re-open if needed.

@nojaf nojaf closed this as completed Jul 12, 2019
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