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

Improve compatibility with MRI's Ripper for JRuby 9.1 #4900

Merged
merged 12 commits into from Dec 18, 2017

Conversation

Projects
None yet
2 participants
@grddev
Contributor

grddev commented Dec 16, 2017

Same as #4898, but targeted on jruby-9.1 branch instead.

The main difference is that it skips 1763e4e that aligned the main parser and Ripper parser, as it seems the misalignment doesn't exist on this branch.

grddev added some commits Dec 16, 2017

Update the MRI ripper spec excludes
First, this ignores the entire file that relies on the MRI file structure, as it seems unlikely that any of those tests will ever be applicable.

Second, it adds the test_sexp.rb file which was absent from the index for some unknown reason. It also introduces a couple of excludes for failing tests in that file.

Third, it clears the excludes for a couple of tests that are currently passing, and updates the description for one of the excludes.
Return false on tokadd_mbchar failure
Otherwise, an invalid byte sequence inside an identifier causes Ripper to continuously parse the same invalid byte sequence over and over.
Call tokadd_mbchar from tokenAddMBC
They are identical (after fixing tokadd_mbchar)
Use current scope in is_id_var
Otherwise, it doesn't recognize variables introduced in the block (parameters), resulting in vcall nodes instead of var_ref
Rework Ripper scope variable analysis
The previous implementation called assignable on a bunch of things that were unlikely to be strings, while only supporting string arguments. This changes the logic so that it relies on getting the identifier value from the lexer instead.

In addition, this inlines the logic from assignable in all cases where the implementation is known, and splits the cases for identifiers and constants, as they have very little to do with each other. Given that the parser has inlined the definition of MRI's user_varaible and keyword variable, it seems to make sense to also inline the assignable actions.

While inlining the assignable implementation, it seamed to make sense to also inline is_id_var (and remove its unused argument).
Add command-args-state begin to Ripper
The corresponding construct was in the main parser, and allowed it correctly parse "m x do; end", where Ripper would fail and assign the block to x rather than m.
Clear command-args-state in brace blocks
It might not be important, but MRI seems to do this.
Rework handling of heredoc end
Instead of ending the heredoc before emitting the final string content part, this instead just emits the final string content part and positions the cursor right before the end marker, such that the next call to the heredoc term should end the heredoc using the early return.

The motivation for doing this was that with ripper the final string content of a heredoc wouldn't be dispatched to any token handler, but instead just propagated.

The old behavior was clearly wrong for this corner-case, but this approach might have other problems.
Dispatch on_magic_comment for Ripper
As an added bonus, the parsing of magic comment key values no longer stops at the first unrecognized key, and thus writing

# -*- warn_past_scope: true; coding: ISO-8859-1 -*-

will now apply the encoding even if JRuby doesn't support the magic comment attribute warn_past_scope.

@enebo enebo added this to the JRuby 9.1.16.0 milestone Dec 18, 2017

@enebo enebo merged commit ba18833 into jruby:jruby-9.1 Dec 18, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@enebo

This comment has been minimized.

Member

enebo commented Dec 18, 2017

@grddev coolio...I will look at regexp PR now!

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