[Feature #20257] Rearchitect Ripper #9923
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yui-knk
force-pushed
the
lrama_after_shift
branch
from
f8c4024
to
97ca269
Compare
|
This PR fixes ripper bugs and makes lex state transition of parser and ripper to be aligned. However it makes one of the Prism test failed. I wrote the detail on the second commit. What do you think about it? |
In this case the parser state of prism is incorrect, as it should be in the ARG state after |
|
Then I want to merge this PR with the second commit, is it ok for you? If you have another option, please let me know. |
|
Please implement a fix yourself or wait until I implement one. I don't think it's a good idea to just comment out tests that break when they are legitimate issues. |
yui-knk
force-pushed
the
lrama_after_shift
branch
from
97ca269
to
1d58295
Compare
yui-knk
added a commit
to yui-knk/ruby
that referenced
this pull request
Feb 16, 2024
…literal/def.txt" See the discussion on ruby#9923
|
@kddnewton I implemented workaround and change to not modify "test/prism/parse_test.rb". What do you think about it? |
yui-knk
added a commit
to yui-knk/ruby
that referenced
this pull request
Feb 16, 2024
…literal/def.txt" See the discussion on ruby#9923
yui-knk
force-pushed
the
lrama_after_shift
branch
from
1d58295
to
102d170
Compare
|
That seems to mask the problem. I'll take care of this first thing Monday, thank you for your patience. |
Introduce another semantic value stack for Ripper so that Ripper can manage both Node and Ruby Object separately. This rearchitectutre of Ripper solves these issues. Therefore adding test cases for them. * [Bug 10436] https://bugs.ruby-lang.org/issues/10436 * [Bug 18988] https://bugs.ruby-lang.org/issues/18988 * [Bug 20055] https://bugs.ruby-lang.org/issues/20055 Checked the differences of `Ripper.sexp` for files under `/test/ruby` are only on test_pattern_matching.rb. The differences comes from the differences between `new_hash_pattern_tail` functions between parser and Ripper. Ripper `new_hash_pattern_tail` didn’t call `assignable` then `kw_rest_arg` wasn’t marked as local variable. This is also fixed by this commit. ``` --- a/./tmp/before/test_pattern_matching.rb +++ b/./tmp/after/test_pattern_matching.rb @@ -3607,7 +3607,7 @@ [:in, [:hshptn, nil, [], [:var_field, [:@Ident, “a”, [984, 13]]]], [[:binary, - [:vcall, [:@Ident, “a”, [985, 10]]], + [:var_ref, [:@Ident, “a”, [985, 10]]], :==, [:hash, nil]]], nil]]], @@ -3662,7 +3662,7 @@ [:in, [:hshptn, nil, [], [:var_field, [:@Ident, “a”, [993, 13]]]], [[:binary, - [:vcall, [:@Ident, “a”, [994, 10]]], + [:var_ref, [:@Ident, “a”, [994, 10]]], :==, [:hash, [:assoclist_from_args, @@ -3813,7 +3813,7 @@ [:command, [:@Ident, “raise”, [1022, 10]], [:args_add_block, - [[:vcall, [:@Ident, “b”, [1022, 16]]]], + [[:var_ref, [:@Ident, “b”, [1022, 16]]]], false]]], [:else, [[:var_ref, [:@kw, “true”, [1024, 10]]]]]]]], nil, @@ -3876,7 +3876,7 @@ [:@int, “0”, [1033, 15]]], :“&&“, [:binary, - [:vcall, [:@Ident, “b”, [1033, 20]]], + [:var_ref, [:@Ident, “b”, [1033, 20]]], :==, [:hash, nil]]]], nil]]], @@ -3946,7 +3946,7 @@ [:@int, “0”, [1042, 15]]], :“&&“, [:binary, - [:vcall, [:@Ident, “b”, [1042, 20]]], + [:var_ref, [:@Ident, “b”, [1042, 20]]], :==, [:hash, [:assoclist_from_args, @@ -5206,7 +5206,7 @@ [[:assoc_new, [:@Label, “c:“, [1352, 22]], [:@int, “0”, [1352, 25]]]]]], - [:vcall, [:@Ident, “r”, [1352, 29]]]], + [:var_ref, [:@Ident, “r”, [1352, 29]]]], false]]], [:binary, [:call, @@ -5299,7 +5299,7 @@ [:assoc_new, [:@Label, “c:“, [1367, 34]], [:@int, “0”, [1367, 37]]]]]], - [:vcall, [:@Ident, “r”, [1367, 41]]]], + [:var_ref, [:@Ident, “r”, [1367, 41]]]], false]]], [:binary, [:call, @@ -5931,7 +5931,7 @@ [:in, [:hshptn, nil, [], [:var_field, [:@Ident, “r”, [1533, 11]]]], [[:binary, - [:vcall, [:@Ident, “r”, [1534, 8]]], + [:var_ref, [:@Ident, “r”, [1534, 8]]], :==, [:hash, [:assoclist_from_args, ```
The previous commit changes Ripper lex state behavior of `tLABEL`. For example, Ripper lex state for the second `bar` was changed by the previous commit. ``` def foo(bar: bar()) end ``` Before the commit, Ripper didn’t add label `bar` id to `vtable` because `formal_argument` function for Ripper didn’t return id but returned `VALUE lhs`. Therefore Ripper didn’t `SET_LEX_STATE(EXPR_END|EXPR_LABEL)` for following `bar` in `parse_ident` because `lvar_defined` for following `bar` was not true. Currently Ripper does `SET_LEX_STATE(EXPR_END|EXPR_LABEL)` then the result of this test case is changed like below. ``` Prism::ParseTest#test_filepath_unparser/corpus/literal/def.txt [ruby/test/prism/parse_test.rb:280]: <[[80, 13], :on_ident, “bar”, END|LABEL]> expected but was <[[80, 13], :on_ident, “bar”, ARG]>. ``` Parser sets lex state to `END|LABEL` on master branch. Therefore previous commit makes Ripper’s behavior aligned with parser’s behavior. ``` $ ruby -v -y -e "def foo(bar: bar())" -e "end" ruby 3.4.0dev (2024-02-11T23:52:05Z master 697ade7) [arm64-darwin21] ... Reading a token parser_dispatch_scan_event:11210 (1: 12|1|7) lex_state: ARG|LABELED -> ARG at line 11113 lex_state: ARG -> END|LABEL at line 11128 parser_dispatch_scan_event:11877 (1: 13|3|4) Next token is token “local variable or method” (1.13-1.16: bar) Shifting token “local variable or method” (1.13-1.16: bar) ```
…literal/def.txt" See the discussion on ruby#9923
yui-knk
force-pushed
the
lrama_after_shift
branch
from
102d170
to
98c901e
Compare
|
This PR changes a lot of line of parse.y and my other commits are created based on this change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.ruby-lang.org/issues/20257
Introduce another semantic value stack for Ripper so that Ripper can manage both Node and Ruby Object separately.
This rearchitectutre of Ripper solves these issues.
Therefore adding test cases for them.