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

- Parser#parse returns nil instead of false if error is thrown #722

Merged
merged 1 commit into from Jul 22, 2020

Conversation

marcandre
Copy link
Contributor

In some circumstances (when reaching the EOF too early), racc erroneously returns false instead of nil.

Even though it's an upstream issue, the fix is so simple that I thought it was worthwhile to circumvent it.

This PR also improves the documentation for non-fatal cases. There are no other tests with all_errors_are_fatal set to false.

@marcandre marcandre changed the title - Have Parser#parse and dependents return nil instead of false for non-fatal errors - Have Parser#parse and dependents return nil instead of false for all non-fatal errors Jul 22, 2020
marcandre referenced this pull request in rubocop/rubocop-ast Jul 22, 2020
Fixes #6945.

By convention, a year after Ruby became an EOL, it seems that the old
Ruby version support has been dropped. I'm not sure if it's good to drop
Ruby 2.3 support, but it's good time to drop Ruby 2.2 support.
Copy link
Collaborator

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -210,8 +210,9 @@ def parse_with_comments(source_buffer)

##
# Parses a source buffer and returns the AST, the source code comments,
# and the tokens emitted by the lexer. If `recover` is true and a fatal
# {SyntaxError} is encountered, `nil` is returned instead of the AST, and
# and the tokens emitted by the lexer. In case of a fatal error, a {SyntaxError}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ha, it took me a while to understand that recover here is responsible for "swallowing the error", but not "error recovery". Not sure if it's worth renaming it, current name sounds good as a part of the public API (you "recover" from VM error, not from the parsing error). Thanks for changing this doc too 👍

@iliabylich iliabylich changed the title - Have Parser#parse and dependents return nil instead of false for all non-fatal errors - Parser#parse returns nil instead of false if error is thrown Jul 22, 2020
@iliabylich iliabylich merged commit e88d47b into whitequark:master Jul 22, 2020
@iliabylich
Copy link
Collaborator

@marcandre Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants