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

Recover from files with initial syntax errors #224

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

vinistock
Copy link
Member

Motivation

We are getting some errors on user's machines of the document instance being nil. The only possibility for this, is if we weren't able to parse the first version of the file due to syntax errors and never recovered.

Implementation

This PR attempts to make this process more robust by always inserting the document into the store regardless of syntax errors. If a syntax error occurs, the tree will be nil, which we have to handle in requests.

When edits are made to the file with syntax errors, push_edits will take care of re-parsing.

Automated Tests

Added a couple of examples and changed some existing ones.

Manual Tests

If you wanna try this manually

  1. Start the LSP on this branch
  2. Created a new file and save it with a syntax error (e.g.: def Foo)
  3. Close the file to clear its state from the store
  4. Re-open the file (this re-open will fail to parse)
  5. Fix the syntax error by adding end
  6. Verify that the LSP recovered and features are now available

@vinistock vinistock added the bug Something isn't working label Jul 25, 2022
@vinistock vinistock requested a review from a team as a code owner July 25, 2022 13:24
@vinistock vinistock self-assigned this Jul 25, 2022
lib/ruby_lsp/document.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should let the exception bubble up to to handle and catch it there rather than making all those types nilable?

def parse
@tree = SyntaxTree.parse(@source)
rescue SyntaxTree::Parser::ParseError
# Do not raise if we failed to parse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log something in the console in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's valuable, but I'll suggest doing this in a follow up PR. If we're going to log something, we should include the URI, which is currently not passed to Document. I can create a follow up as soon as this one goes in.

@vinistock
Copy link
Member Author

Would we be able to recover from the errors if we let the handler take care of it? The complication is that we only receive the full document source once, when opening the file and then only receive incremental text changes to it.

So we need to save the initial document, regardless of whether there are errors or not.

@Morriar
Copy link
Contributor

Morriar commented Jul 25, 2022

So we need to save the initial document, regardless of whether there are errors or not.

Ah I see. Ok then 👍

@vinistock vinistock force-pushed the vs/recover_from_initial_syntax_errors branch from 41a74d9 to 1aeaef3 Compare July 25, 2022 17:44
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may not be the best solution for the problem. Currently, successful document parsing and document processing logic (including caching) go hand-in-hand. And that's why it's safe for the latter because it saves us the need to handle nil in a lot of places.

If we want to make parsing part failable, we shouldn't compromise the safety we have on document processing. Instead, we should probably consider handling requests in stages, like:

1. Document parsing + storing
2. Parsing result check + early return
3. Document processing

It'll likely require structural change on the Handler class but I think it'd be better.

Another concern is that VSCode will hang if it doesn't receive a request's response (see #149). So we may need to pay more attention on the early return's implementation and make sure it still returns something?

Update: I gave the crossed part a try and it doesn't work because requests like didOpen do need to handle the text -> document conversion. So if we separate a request into stages, it'll just make those requests hard to understand. Also, a lot of nilable change will disappear because #221 has been merged so the code will look less confusing.

lib/ruby_lsp/handler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/store.rb Show resolved Hide resolved
lib/ruby_lsp/store.rb Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs/recover_from_initial_syntax_errors branch from 1aeaef3 to cea52c6 Compare July 26, 2022 14:03
@vinistock vinistock force-pushed the vs/recover_from_initial_syntax_errors branch from cea52c6 to fd919d7 Compare July 26, 2022 14:08
@vinistock
Copy link
Member Author

vinistock commented Jul 26, 2022

@st0012 the void vs nil returns has been addressed in #100. Returning nil for all of our current requests are valid responses and VS Code handles them properly.

The reason it was hanging in #149 is because RuboCop would print argument errors to STDOUT. Printing anything to STDOUT from inside the LSP will always put it in an endless loop.

@vinistock vinistock merged commit 09a5cfe into main Jul 26, 2022
@vinistock vinistock deleted the vs/recover_from_initial_syntax_errors branch July 26, 2022 17:55
vinistock added a commit that referenced this pull request Jul 27, 2022
…rrors

Recover from files with initial syntax errors
@shopify-shipit shopify-shipit bot temporarily deployed to production August 26, 2022 20:47 Inactive
andyw8 pushed a commit to andyw8/ruby-lsp that referenced this pull request Mar 2, 2024
…ce_to_error_telemetry

Add params and backtrace to error telemetry
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 this pull request may close these issues.

3 participants