diff options
| author | HASUMI Hitoshi <[email protected]> | 2024-03-29 16:18:14 +0900 |
|---|---|---|
| committer | Nobuyoshi Nakada <[email protected]> | 2024-04-04 18:29:16 +0900 |
| commit | f5e387a30010fe68f8073a6eb5a5b80bb834cd72 (patch) | |
| tree | 2482ec341e8dabbd2dafa34c92bbad9926f3bb9e /ast.c | |
| parent | 27622f3eb92287b3a22256609a05eba39d493923 (diff) | |
Separate SCRIPT_LINES__ from ast.c
This patch suggests relocating the code dealing with `SCRIPT_LINES__` from ast.c to ruby_parser.c.
## Background
- I guess `AbstractSyntaxTree.of` method used to use `SCRIPT_LINES__` internally for some reason before
- However, now it appears `SCRIPT_LINES__` is no longer used meaningfully by the method
- As evidence of this, (and as my patch shows,) removing the function call of `rb_script_lines_for()` from `ast_s_of()` does not affect the result of `test/ruby/test_ast.rb`
Given the above, I think two possibilities can be considered:
- (A) `AbstractSyntaxTree.of` has not needed `SCRIPT_LINES__` already (I pick this)
- (B) We lack a test case of `AbstractSyntaxTree.of` that needs to use `SCRIPT_LINES__`
## Besides,
The current implementation causes strange behavior:
```console
ruby -e"SCRIPT_LINES__ = {__FILE__ => []}; puts RubyVM::AbstractSyntaxTree.of(->{ 1 + 2 }, keep_script_lines: true).script_lines"
=> `-e:1:in '<main>': undefined method 'script_lines' for nil (NoMethodError)`
```
I think this is a bug because `AbstractSyntaxTree.of` is not supposed to return `nil` even in this case.
This happens due to the ast.c's dependence on `SCRIPT_LINES__`.
And at the end of the `ast_s_of()`, `node_find()` can not find the target child node obviously because it doesn't make sense to look for a corresponding node made from the parameter of `AbstractSyntaxTree.of` in the AST tree made from the value of `{__FILE__ => []}`
## Solution
Since I think it's good enough `SCRIPT_LINES__` to be only referred by ruby.c, I chose the possibility "(A)" and wrote this patch which moves `rb_script_lines_for()` from ast.c to ruby_parser.c.
So as the result:
- `ast_s_of()` function no longer look up `SCRIPT_LINES__`
- Even so, this patched code passes the existing tests
- The strange behavior above no longer happens (I also added a test for it)
Please correct me if I miss something🙏
Diffstat (limited to 'ast.c')
| -rw-r--r-- | ast.c | 25 |
1 files changed, 1 insertions, 24 deletions
@@ -183,29 +183,6 @@ node_find(VALUE self, const int node_id) extern VALUE rb_e_script; -VALUE -rb_script_lines_for(VALUE path, bool add) -{ - VALUE hash, lines; - ID script_lines; - CONST_ID(script_lines, "SCRIPT_LINES__"); - if (!rb_const_defined_at(rb_cObject, script_lines)) return Qnil; - hash = rb_const_get_at(rb_cObject, script_lines); - if (!RB_TYPE_P(hash, T_HASH)) return Qnil; - if (add) { - rb_hash_aset(hash, path, lines = rb_ary_new()); - } - else if (!RB_TYPE_P((lines = rb_hash_lookup(hash, path)), T_ARRAY)) { - return Qnil; - } - return lines; -} -static VALUE -script_lines(VALUE path) -{ - return rb_script_lines_for(path, false); -} - static VALUE node_id_for_backtrace_location(rb_execution_context_t *ec, VALUE module, VALUE location) { @@ -267,7 +244,7 @@ ast_s_of(rb_execution_context_t *ec, VALUE module, VALUE body, VALUE keep_script rb_raise(rb_eArgError, "cannot get AST for method defined in eval"); } - if (!NIL_P(lines) || !NIL_P(lines = script_lines(path))) { + if (!NIL_P(lines)) { node = rb_ast_parse_array(lines, keep_script_lines, error_tolerant, keep_tokens); } else if (e_option) { |