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

Fix LSP crash parsing scripts with no lines [3.x] #53261

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented Sep 30, 2021

Certain LSP clients like Vim occasionally, erroneously, or for whatever reason report documents as being size 0, which breaks the LSP due to a fix in parsing documentation that is above multi-line functions parameters.

Fixes #53238

@Razoric480 Razoric480 requested a review from a team as a code owner September 30, 2021 15:14
@Razoric480 Razoric480 marked this pull request as draft September 30, 2021 15:15
@Calinou Calinou added this to the 3.4 milestone Sep 30, 2021
@Razoric480 Razoric480 changed the title Fix LSP crash parsing scripts of temp size 0 [3.x] Fix LSP crash parsing scripts with no lines [3.x] Sep 30, 2021
@Rubonnek
Copy link
Member

Nice! No crashes! But I've stumbled into a minor issue.

When I save, now I'm getting the following message: Unexpected token: Identifier:Null

Here's the LSP interaction: unexpected_token_identifier_null.txt.

You'll find the unexpected token message at the bottom of that log.

@Razoric480
Copy link
Contributor Author

That looks like a client issue. It's reporting that your script became:

Null

which even in Godot itself would trigger Unexpected Token: Identifier: Null
image

Could the LSP client code in Vim be reporting some temporary file it's using for saving a file, instead of the saved file end result?

@Rubonnek
Copy link
Member

That's quite possible. As far as I can tell, this seems to be a bug within ALE which might be referencing some temporary file.

I'll look into it.

@Razoric480 Razoric480 marked this pull request as ready for review September 30, 2021 15:56
@Razoric480
Copy link
Contributor Author

Razoric480 commented Sep 30, 2021

For now, this will at least keep the LSP itself from dying and keep Godot from crashing, so it's good for merging

@akien-mga akien-mga merged commit 23f21ac into godotengine:3.x Sep 30, 2021
@akien-mga
Copy link
Member

Thanks!

@Rubonnek
Copy link
Member

Welp, I've confirmed that the Unexpected token: Identifier:Null issue is indeed a bug in ALE.

The textDocument/didSave message is not including the contents of the file at all, which it should according to the official LSP specification. I'll submit a PR to ALE sometime this week.

@Razoric480 thanks for the quick turnaround on this!

@Razoric480
Copy link
Contributor Author

♥ Glad it worked out

@Razoric480 Razoric480 deleted the lsp_crash_vim_ale branch September 30, 2021 18:02
@akien-mga
Copy link
Member

Cherry-picked for 3.3.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants