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🙏
This commit is contained in:
parent
27622f3eb9
commit
f5e387a300
25
ast.c
25
ast.c
@ -183,29 +183,6 @@ node_find(VALUE self, const int node_id)
|
|||||||
|
|
||||||
extern VALUE rb_e_script;
|
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
|
static VALUE
|
||||||
node_id_for_backtrace_location(rb_execution_context_t *ec, VALUE module, VALUE location)
|
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");
|
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);
|
node = rb_ast_parse_array(lines, keep_script_lines, error_tolerant, keep_tokens);
|
||||||
}
|
}
|
||||||
else if (e_option) {
|
else if (e_option) {
|
||||||
|
4
ruby.c
4
ruby.c
@ -2581,7 +2581,7 @@ struct load_file_arg {
|
|||||||
VALUE f;
|
VALUE f;
|
||||||
};
|
};
|
||||||
|
|
||||||
VALUE rb_script_lines_for(VALUE path, bool add);
|
VALUE rb_script_lines_for(VALUE path);
|
||||||
|
|
||||||
static VALUE
|
static VALUE
|
||||||
load_file_internal(VALUE argp_v)
|
load_file_internal(VALUE argp_v)
|
||||||
@ -2686,7 +2686,7 @@ load_file_internal(VALUE argp_v)
|
|||||||
rb_parser_set_options(parser, opt->do_print, opt->do_loop,
|
rb_parser_set_options(parser, opt->do_print, opt->do_loop,
|
||||||
opt->do_line, opt->do_split);
|
opt->do_line, opt->do_split);
|
||||||
|
|
||||||
VALUE lines = rb_script_lines_for(orig_fname, true);
|
VALUE lines = rb_script_lines_for(orig_fname);
|
||||||
if (!NIL_P(lines)) {
|
if (!NIL_P(lines)) {
|
||||||
rb_parser_set_script_lines(parser, lines);
|
rb_parser_set_script_lines(parser, lines);
|
||||||
}
|
}
|
||||||
|
@ -994,3 +994,16 @@ rb_node_encoding_val(const NODE *node)
|
|||||||
{
|
{
|
||||||
return rb_enc_from_encoding(RNODE_ENCODING(node)->enc);
|
return rb_enc_from_encoding(RNODE_ENCODING(node)->enc);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
VALUE
|
||||||
|
rb_script_lines_for(VALUE path)
|
||||||
|
{
|
||||||
|
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;
|
||||||
|
rb_hash_aset(hash, path, lines = rb_ary_new());
|
||||||
|
return lines;
|
||||||
|
}
|
||||||
|
@ -746,6 +746,19 @@ dummy
|
|||||||
assert_equal("def test_keep_script_lines_for_of\n", node_method.source.lines.first)
|
assert_equal("def test_keep_script_lines_for_of\n", node_method.source.lines.first)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def test_keep_script_lines_for_of_with_existing_SCRIPT_LINES__that_has__FILE__as_a_key
|
||||||
|
# This test confirms that the bug that previously occurred because of
|
||||||
|
# `AbstractSyntaxTree.of`s unnecessary dependence on SCRIPT_LINES__ does not reproduce.
|
||||||
|
# The bug occurred only if SCRIPT_LINES__ included __FILE__ as a key.
|
||||||
|
lines = [
|
||||||
|
"SCRIPT_LINES__ = {__FILE__ => []}",
|
||||||
|
"puts RubyVM::AbstractSyntaxTree.of(->{ 1 + 2 }, keep_script_lines: true).script_lines",
|
||||||
|
"p SCRIPT_LINES__"
|
||||||
|
]
|
||||||
|
test_stdout = lines + ['{"-e"=>[]}']
|
||||||
|
assert_in_out_err(["-e", lines.join("\n")], "", test_stdout, [])
|
||||||
|
end
|
||||||
|
|
||||||
def test_source_with_multibyte_characters
|
def test_source_with_multibyte_characters
|
||||||
ast = RubyVM::AbstractSyntaxTree.parse(%{a("\u00a7");b("\u00a9")}, keep_script_lines: true)
|
ast = RubyVM::AbstractSyntaxTree.parse(%{a("\u00a7");b("\u00a9")}, keep_script_lines: true)
|
||||||
a_fcall, b_fcall = ast.children[2].children
|
a_fcall, b_fcall = ast.children[2].children
|
||||||
|
Loading…
x
Reference in New Issue
Block a user