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

record mutation: first field after with should be placed in a new line #457

Closed
yatli opened this issue Aug 4, 2019 · 15 comments
Closed

Comments

@yatli
Copy link

yatli commented Aug 4, 2019

I'm integrating fantomas into coc-fsharp with API FormatDocumentAsync.
I'm using 3.0-beta002 nuget package.

Problem example:

                return { 
                    capabilities = 
                        { defaultServerCapabilities with 
                            hoverProvider = true
                            completionProvider = Some({resolveProvider=true; triggerCharacters=['.']})
                            signatureHelpProvider = Some({triggerCharacters=['('; ',']})
                            documentSymbolProvider = true
                            codeActionProvider = true
                            codeLensProvider = Some({resolveProvider=true})
                            workspaceSymbolProvider = true
                            definitionProvider = true
                            referencesProvider = true
                            renameProvider = true
                            documentFormattingProvider = true
                            documentRangeFormattingProvider = true
                            textDocumentSync = 
                                { defaultTextDocumentSyncOptions with 
                                    openClose = true 
                                    save = Some({ includeText = false })
                                    change = TextDocumentSyncKind.Incremental 
                                } 
                        }
                }

becomes:

                return { capabilities =
                             { defaultServerCapabilities with hoverProvider = true
                               completionProvider =
                                   Some({ resolveProvider = true
                                          triggerCharacters = [ '.' ] })
                               signatureHelpProvider = Some({ triggerCharacters = [ '('; ',' ] })
                               documentSymbolProvider = true
                               codeActionProvider = true
                               codeLensProvider = Some({ resolveProvider = true })
                               workspaceSymbolProvider = true
                               definitionProvider = true
                               referencesProvider = true
                               renameProvider = true
                               documentFormattingProvider = true
                               documentRangeFormattingProvider = true
                               textDocumentSync =
                                   { defaultTextDocumentSyncOptions with openClose = true
                                     save = Some({ includeText = false })
                                     change = TextDocumentSyncKind.Incremental } } }
            }

online fantomas code

Note, this problem does not repro with all record mutations -- not sure about the condition, but I tried a few simpler cases and it works as expected -- this is exactly the repro snippet, extracted from the full source file given in the link above.

@yatli
Copy link
Author

yatli commented Aug 4, 2019

I've also tried to first typecheck with my language server, and send the AST to Fantomas -- the formatting result is mostly correct (see #456), but the comments are gone.

@jindraivanek
Copy link
Contributor

I've also tried to first typecheck with my language server, and send the AST to Fantomas -- the formatting result is mostly correct (see #456), but the comments are gone.

Comments are not included in AST.

@yatli
Copy link
Author

yatli commented Aug 5, 2019

Comments are not included in AST.

I know.

I think the first part is more interesting -- using AST does not repro the problem.

@jindraivanek
Copy link
Contributor

Oh, okay, I misinterpreted it.

That means problem is probably in comments.

This issue is definitely elusive, I tried to find smaller repro but without success (for example whole class Server is formatted correctly).

@jindraivanek
Copy link
Contributor

I was able to find minimized repro.

@nojaf
Copy link
Contributor

nojaf commented Aug 6, 2019

Ok, so fix could be to auto add a newline after the with keyword if there are multiple members?

@yatli
Copy link
Author

yatli commented Aug 6, 2019

@nojaf ideally yes. I haven’t dig into fantomas codebase yet — just wondering: why does it behave correctly in some conditions?

@jindraivanek did you find patterns? That minimal repro definitely helps to simplify things. :)

@yatli
Copy link
Author

yatli commented Aug 6, 2019

In the minimal repro, if we remove the first line (let x = .....), then the record formats correctly.
Semantically it seems irrelevant...?
Even more interesting: changing the first line to:
let x = 1 // no repro
let x = foo.bar(2) //no repro
let x = foo(1).bar(2) // repro!

@yatli
Copy link
Author

yatli commented Aug 6, 2019

Turning on strict mode mutes the repro — this echos the AST formatting approach.

@jindraivanek
Copy link
Contributor

@yatli Yes, also
foo(x).bar() //no repro
foo(1).bar //no repro

@yatli
Copy link
Author

yatli commented Aug 6, 2019

let x = Foo(1).Bar 1 //repro
let x = (Foo 1).Bar 1 // no repro

@yatli
Copy link
Author

yatli commented Aug 6, 2019

ok, the reason "it doesn't repro on simpler cases":

let _ = 
    let x = Foo(1).Bar()
    x

let F x = x

type T = A | B | C of int

let r =
    { s with
        x = 1
        y = 2 }
// repro

It infects following lines

@yatli
Copy link
Author

yatli commented Aug 6, 2019

...and this:

let r =
    { s with
        x = 1
        y = 2 }

let x = (fun () -> Foo(1).Bar())

let r =
    { s with
        x = 1
        y = 2 }

===>

let r =
    { s with
          x = 1
          y = 2 }

let x = (fun () -> Foo(1).Bar())

let r =
    { s with x = 1
      y = 2 }

@yatli
Copy link
Author

yatli commented Aug 6, 2019

It turns on some kind of switch?

@jindraivanek
Copy link
Contributor

I found the bug, it was indeed caused by incorrectly set switch here:

let internal noNln f (ctx : Context) : Context =
ctx.BreakLines <- false
let res = f ctx
ctx.BreakLines <- true
res

Will fix soon.

jindraivanek added a commit to jindraivanek/fantomas that referenced this issue Aug 6, 2019
@nojaf nojaf closed this as completed Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants