diff --git a/lib/open3.rb b/lib/open3.rb index 74d00b86d93f55..d212c4a440af50 100644 --- a/lib/open3.rb +++ b/lib/open3.rb @@ -520,13 +520,6 @@ def popen2e(*cmd, &block) opts[[:out, :err]] = out_w popen_run(cmd, opts, [in_r, out_w], [in_w, out_r], &block) - ensure - if block - in_r.close - in_w.close - out_r.close - out_w.close - end end module_function :popen2e @@ -534,16 +527,23 @@ def popen_run(cmd, opts, child_io, parent_io) # :nodoc: pid = spawn(*cmd, opts) wait_thr = Process.detach(pid) child_io.each(&:close) + child_io = nil result = [*parent_io, wait_thr] if defined? yield - begin - return yield(*result) - ensure + yield(*result) + else + result + end + ensure + if result + if defined? yield parent_io.each(&:close) wait_thr.join end + else + child_io&.each(&:close) + parent_io.each(&:close) end - result end module_function :popen_run class << self @@ -1354,7 +1354,7 @@ def pipeline_run(cmds, pipeline_opts, child_io, parent_io) # :nodoc: opts_base.delete :out wait_thrs = [] - r = nil + r = r2 = w2 = nil cmds.each_with_index {|cmd, i| cmd_opts = opts_base.dup if String === cmd @@ -1387,17 +1387,25 @@ def pipeline_run(cmds, pipeline_opts, child_io, parent_io) # :nodoc: w2&.close r = r2 } - result = parent_io + [wait_thrs] child_io.each(&:close) + child_io = nil + result = parent_io + [wait_thrs] if defined? yield - begin - return yield(*result) - ensure + yield(*result) + else + result + end + ensure + if result + if defined? yield parent_io.each(&:close) wait_thrs.each(&:join) end + else + [r, r2, w2, *child_io, *parent_io].each do |io| + io&.close + end end - result end module_function :pipeline_run class << self diff --git a/prism/internal/parser.h b/prism/internal/parser.h index 4320cf40294867..0c071b1375d177 100644 --- a/prism/internal/parser.h +++ b/prism/internal/parser.h @@ -689,15 +689,6 @@ struct pm_parser_t { /* The current parsing context. */ pm_context_node_t *current_context; - /* - * The hash keys for the hash that is currently being parsed. This is not - * usually necessary because it can pass it down the various call chains, - * but in the event that you're parsing a hash that is being directly - * pushed into another hash with **, we need to share the hash keys so that - * we can warn for the nested hash as well. - */ - pm_static_literals_t *current_hash_keys; - /* * The encoding functions for the current file is attached to the parser as * it's parsing so that it can change with a magic comment. diff --git a/prism/prism.c b/prism/prism.c index 51980155e4a0f8..ecb329bef6a142 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -13751,6 +13751,28 @@ parse_statements(pm_parser_t *parser, pm_context_t context, uint16_t depth) { return statements; } +/** + * Append the warning for a hash key that is overwritten by a later occurrence. + */ +static void +pm_hash_key_duplicated_warn(pm_parser_t *parser, const pm_node_t *duplicated, const pm_node_t *node) { + pm_buffer_t buffer = { 0 }; + pm_static_literal_inspect(&buffer, &parser->line_offsets, parser->start, parser->start_line, parser->encoding, duplicated); + + pm_diagnostic_list_append_format( + &parser->metadata_arena, + &parser->warning_list, + duplicated->location.start, + duplicated->location.length, + PM_WARN_DUPLICATED_HASH_KEY, + (int) pm_buffer_length(&buffer), + pm_buffer_value(&buffer), + pm_line_offset_list_line_column(&parser->line_offsets, PM_NODE_START(node), parser->start_line).line + ); + + pm_buffer_cleanup(&buffer); +} + /** * Add a node to a set of static literals that holds a set of hash keys. If the * node is a duplicate, then add an appropriate warning. @@ -13760,21 +13782,47 @@ pm_hash_key_static_literals_add(pm_parser_t *parser, pm_static_literals_t *liter const pm_node_t *duplicated = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, parser->encoding, literals, node, true); if (duplicated != NULL) { - pm_buffer_t buffer = { 0 }; - pm_static_literal_inspect(&buffer, &parser->line_offsets, parser->start, parser->start_line, parser->encoding, duplicated); + pm_hash_key_duplicated_warn(parser, duplicated, node); + } +} - pm_diagnostic_list_append_format( - &parser->metadata_arena, - &parser->warning_list, - duplicated->location.start, - duplicated->location.length, - PM_WARN_DUPLICATED_HASH_KEY, - (int) pm_buffer_length(&buffer), - pm_buffer_value(&buffer), - pm_line_offset_list_line_column(&parser->line_offsets, PM_NODE_START(node), parser->start_line).line - ); +/** + * Add the keys of a hash literal splatted directly into another hash with ** + * to the outer hash's set of keys, as if they were written in place. A key + * whose previous occurrence starts at or past boundary (the start of the + * splatted hash) is still replaced but not warned about again: that pair was + * already warned about when the splatted hash was parsed. + */ +static void +pm_hash_key_static_literals_merge(pm_parser_t *parser, pm_static_literals_t *literals, const pm_hash_node_t *hash, uint32_t boundary) { + const pm_node_list_t *elements = &hash->elements; - pm_buffer_cleanup(&buffer); + for (size_t index = 0; index < elements->size; index++) { + pm_node_t *element = elements->nodes[index]; + + switch (PM_NODE_TYPE(element)) { + case PM_ASSOC_NODE: { + pm_node_t *key = ((pm_assoc_node_t *) element)->key; + const pm_node_t *duplicated = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, parser->encoding, literals, key, true); + + if (duplicated != NULL && PM_NODE_START(duplicated) < boundary) { + pm_hash_key_duplicated_warn(parser, duplicated, key); + } + + break; + } + case PM_ASSOC_SPLAT_NODE: { + const pm_node_t *value = ((pm_assoc_splat_node_t *) element)->value; + + if (value != NULL && PM_NODE_TYPE_P(value, PM_HASH_NODE)) { + pm_hash_key_static_literals_merge(parser, literals, (const pm_hash_node_t *) value, boundary); + } + + break; + } + default: + break; + } } } @@ -13816,15 +13864,14 @@ parse_assocs(pm_parser_t *parser, pm_static_literals_t *literals, pm_node_t *nod pm_token_t operator = parser->previous; pm_node_t *value = NULL; - if (match1(parser, PM_TOKEN_BRACE_LEFT_HASH)) { - // If we're about to parse a nested hash that is being - // pushed into this hash directly with **, then we want the - // inner hash to share the static literals with the outer - // hash. - parser->current_hash_keys = literals; - value = parse_value_expression(parser, PM_BINDING_POWER_DEFINED, PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_SPLAT_HASH, (uint16_t) (depth + 1)); - } else if (token_begins_expression_p(parser->current.type)) { + if (token_begins_expression_p(parser->current.type)) { value = parse_value_expression(parser, PM_BINDING_POWER_DEFINED, PM_PARSE_ACCEPTS_DO_BLOCK, PM_ERR_EXPECT_EXPRESSION_AFTER_SPLAT_HASH, (uint16_t) (depth + 1)); + + /* If the splatted value is itself a hash literal, its keys + * become part of this hash for the duplicate key warning. */ + if (value != NULL && PM_NODE_TYPE_P(value, PM_HASH_NODE)) { + pm_hash_key_static_literals_merge(parser, literals, (const pm_hash_node_t *) value, PM_NODE_START(value)); + } } else { pm_parser_scope_forwarding_keywords_check(parser, &operator); } @@ -19551,29 +19598,15 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, u case PM_TOKEN_PARENTHESIS_LEFT_PARENTHESES: return parse_parentheses(parser, binding_power, flags, depth); case PM_TOKEN_BRACE_LEFT_HASH: { - // If we were passed a current_hash_keys via the parser, then that - // means we're already parsing a hash and we want to share the set - // of hash keys with this inner hash we're about to parse for the - // sake of warnings. We'll set it to NULL after we grab it to make - // sure subsequent expressions don't use it. Effectively this is a - // way of getting around passing it to every call to - // parse_expression. - pm_static_literals_t *current_hash_keys = parser->current_hash_keys; - parser->current_hash_keys = NULL; - parser_lex(parser); pm_token_t opening = parser->previous; pm_hash_node_t *node = pm_hash_node_create(parser, &opening); if (!match2(parser, PM_TOKEN_BRACE_RIGHT, PM_TOKEN_EOF)) { - if (current_hash_keys != NULL) { - parse_assocs(parser, current_hash_keys, UP(node), (uint16_t) (depth + 1)); - } else { - pm_static_literals_t hash_keys = { 0 }; - parse_assocs(parser, &hash_keys, UP(node), (uint16_t) (depth + 1)); - pm_static_literals_free(&hash_keys); - } + pm_static_literals_t hash_keys = { 0 }; + parse_assocs(parser, &hash_keys, UP(node), (uint16_t) (depth + 1)); + pm_static_literals_free(&hash_keys); accept1(parser, PM_TOKEN_NEWLINE); } diff --git a/test/prism/result/warnings_test.rb b/test/prism/result/warnings_test.rb index 34c4684c0433a6..e77f109161b014 100644 --- a/test/prism/result/warnings_test.rb +++ b/test/prism/result/warnings_test.rb @@ -73,6 +73,24 @@ def test_duplicated_hash_key assert_warning("{ a: 1, **{ a: 2 } }", "duplicated and overwritten") end + def test_duplicated_hash_key_splatted_literal_position + assert_warning("{ **{ a: 1 }, a: 2 }", "duplicated and overwritten", compare: false) + assert_warning("{ **{}, a: 1, **{ a: 2 } }", "duplicated and overwritten", compare: false) + assert_warning("{ **z, a: 1, **{ a: 1 } }", "duplicated and overwritten", compare: false) + assert_warning("foo(**{ a: 1 }, a: 2)", "duplicated and overwritten", compare: false) + end + + def test_duplicated_hash_key_splatted_literal_nested + assert_warning("{ a: 1, **{ **{ a: 2 } } }", "duplicated and overwritten", compare: false) + assert_warning("{ a: 1,\n**{ a: 2,\n**{ a: 3 } } }", "on line 3", "on line 2", compare: false) + end + + def test_duplicated_hash_key_splatted_expression + refute_warning("{ a: 1, **{ a: 2 }.dup }", compare: false) + refute_warning("{ a: 1, **({ a: 2 }) }", compare: false) + refute_warning("foo(a: 1, **{ a: 2 }.dup)", compare: false) + end + def test_duplicated_when_clause assert_warning("case 1; when 1, 1; end", "when' clause") end diff --git a/test/test_open3.rb b/test/test_open3.rb index 19277c8a66eca6..0fb3e0e0deae85 100644 --- a/test/test_open3.rb +++ b/test/test_open3.rb @@ -159,6 +159,67 @@ def test_popen2e_noblock t.join end + def test_popen_spawn_failure_closes_pipes + [:popen3, :popen2, :popen2e].each do |method| + assert_no_fd_leak(method) do + assert_raise(Errno::ENOENT) do + Open3.public_send(method, "/open3-command-does-not-exist") + end + end + end + end + + def test_popen_spawn_throw_closes_pipes + tag = Object.new + assert_no_fd_leak(:popen3) do + stub_open3_spawn(->(*) {throw tag}) do + assert_throw(tag) do + Open3.popen3("unused") + end + end + end + end + + def test_pipeline_spawn_failure_closes_pipes + first = [RUBY, '-e', ''] + missing = ["/open3-command-does-not-exist"] + + [:pipeline_rw, :pipeline_r, :pipeline_w, + :pipeline_start, :pipeline].each do |method| + threads = Thread.list + assert_no_fd_leak(method) do + assert_raise(Errno::ENOENT) do + Open3.public_send(method, first, missing) + end + end + (Thread.list - threads).each do |thread| + thread.join if Process::Waiter === thread + end + end + end + + def test_pipeline_spawn_throw_closes_pipes + tag = Object.new + spawn = Open3.method(:spawn) + calls = 0 + threads = Thread.list + + assert_no_fd_leak(:pipeline_rw) do + stub_open3_spawn(->(*args) { + calls += 1 + throw tag if calls == 2 + spawn.call(*args) + }) do + assert_throw(tag) do + Open3.pipeline_rw([RUBY, '-e', ''], ["unused"]) + end + end + end + (Thread.list - threads).each do |thread| + thread.join if Process::Waiter === thread + end + end + def test_capture3 o, e, s = Open3.capture3(RUBY, '-e', 'i=STDIN.read; print i+"o"; STDOUT.flush; STDERR.print i+"e"', :stdin_data=>"i") assert_equal("io", o) @@ -332,4 +393,33 @@ def test_integer_and_symbol_key assert_equal("test_integer_and_symbol_key\n", out) assert_predicate(status, :success?) end + + private + + def stub_open3_spawn(spawn) + Open3.define_singleton_method(:spawn, spawn) + yield + ensure + Open3.singleton_class.send(:remove_method, :spawn) + end + + def assert_no_fd_leak(method) + fd_dir = ["/proc/self/fd", "/dev/fd"].find {|dir| File.directory?(dir) } + omit "cannot inspect open file descriptors" unless fd_dir + + gc_was_disabled = GC.disable + before = open_fds(fd_dir) + yield + assert_equal(before, open_fds(fd_dir), + "#{method} leaked file descriptors") + ensure + GC.enable unless gc_was_disabled + end + + def open_fds(fd_dir) + Dir.open(fd_dir) do |dir| + fds = dir.children(&:to_i).sort + fds -= [dir.fileno] if dir.respond_to? :fileno + end + end end