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

olsp can keep incorrect document src in its state because of encoding issues #934

Closed
ulugbekna opened this issue Nov 21, 2022 · 6 comments · Fixed by #966
Closed

olsp can keep incorrect document src in its state because of encoding issues #934

ulugbekna opened this issue Nov 21, 2022 · 6 comments · Fixed by #966
Labels
bug Something isn't working
Milestone

Comments

@ulugbekna
Copy link
Collaborator

olsp sometimes formats code into illegal state, e.g., given file

let foo ~i ~j = i + j
let bar = 10
let y = foo ~i:5

let lst = []

let map = List.m

let map = List.m []

let map = List.m

olsp formats it to

let foo ~i ~j = i + j
let bar = 10
let y = foo ~i:5
let lst = []
let map = List.mlet map = List.m []
let map = List.m

trace (notice how ofmt says it's disabled but still formats code)

[Trace - 10:35:22 PM] Sending request 'textDocument/formatting - (374)'.
Params: {
    "textDocument": {
        "uri": "file:///Users/ulugbek/code/merlin/tests/test-dirs/completion/application_context.t/application_context.ml"
    },
    "options": {
        "tabSize": 2,
        "insertSpaces": true
    }
}


File tests/test-dirs/completion/application_context.t/application_context.ml
Warning: Ocamlformat disabled because [--enable-outside-detected-project] is not set and no [.ocamlformat] was found within the project (root: ../merlin)
[Trace - 10:35:22 PM] Received response 'textDocument/formatting - (374)' in 3ms.
Result: [
    {
        "newText": "",
        "range": {
            "end": {
                "character": 0,
                "line": 4
            },
            "start": {
                "character": 0,
                "line": 3
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 0,
                "line": 10
            },
            "start": {
                "character": 0,
                "line": 5
            }
        }
    },
    {
        "newText": "let map = List.m []\nlet map = List.m\n",
        "range": {
            "end": {
                "character": 0,
                "line": 11
            },
            "start": {
                "character": 0,
                "line": 11
            }
        }
    }
]

ofmt also sometimes gets stuck and can't get format; restarting olsp enables it to format again.


this must be some ofmt-rpc issue

@rgrinberg
Copy link
Member

Shall we disable ocamlformat-rpc if it's not stable enough?

@ulugbekna
Copy link
Collaborator Author

I think we should disable it for formatting files. Seems fine enough for code snippets on hover, etc

@ulugbekna
Copy link
Collaborator Author

The rpc also doesn't respect a missing .ocamlformat in the project and formats files with default config, which is unacceptable for us

@ulugbekna
Copy link
Collaborator Author

ulugbekna commented Nov 29, 2022

I have a suspicion that legal code not being formatted is actually caused by our representation of document source code because I think I see "phantom" syntax errors (ie source is legit in the editor but there's a syntax error shown for strange position within the source). It could be the encoding or batching document updates PR at fault. #940 would make it easier to debug.

@rgrinberg
Copy link
Member

I have a feeling that this bug was here all along. In fact, since the encoding PR, I haven't been able to reproduce it. Perhaps because neovim allows for utf8. It might be the bug is in the utf16 decoding?

@ulugbekna
Copy link
Collaborator Author

have a feeling that this bug was here all along.

interesting. I never had this problem before, at least not past year.

Indeed, vscode uses utf16 position encoding, so that may be the problem

@ulugbekna ulugbekna changed the title formatter can't format legal code & formats code into illegal states olsp can keep incorrect document src in its state because of encoding issues Dec 5, 2022
@ulugbekna ulugbekna added the bug Something isn't working label Dec 5, 2022
@rgrinberg rgrinberg linked a pull request Dec 24, 2022 that will close this issue
@rgrinberg rgrinberg added this to the 1.15.0 milestone Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants