February 2024 - ruby-core - ml.ruby-lang.org

[ruby-core:116542] [Ruby master Misc#20232] Document Kernel#require and Module#autoload concurrency guarantees
by fxn (Xavier Noria) 04 Mar '24

04 Mar '24

Issue #20232 has been reported by fxn (Xavier Noria). ---------------------------------------- Misc #20232: Document Kernel#require and Module#autoload concurrency guarantees https://bugs.ruby-lang.org/issues/20232 * Author: fxn (Xavier Noria) * Status: Open * Priority: Normal ---------------------------------------- I'd like to document `Kernel#require` and `Module#autoload` concurrency guarantees. In the case of multiple threads loading the same file concurrently, `Kernel#require` will succeed in just one of them and the rest will wait and return false. If a constant that has an autoload is concurrently referenced, the same can be said. Assuming no errors, only one thread will succeed, and the rest wait. There will be no context switching in the middle of an autoload that will result in a `NameError` in other threads waiting for that constant. Now, I'd like to have a discussion about those guarantees with fibers. In the case of manually managed fibers, users can enter a deadlock by hand: ```ruby # This produces a deadlock. File.write('/tmp/bar.rb', <<~RUBY) Fiber.yield RUBY Fiber.new { require '/tmp/bar.rb' }.resume Fiber.new { require '/tmp/bar.rb' }.resume ``` If this is expected, I guess users should be told this is a possibility in the API dosc? Because from a user perspective, you don't really have elements to anticipate a deadlock there if the docs don't warn you. A similar deadlock can be triggered with an `autoload` instead of a `require`: ```ruby # This produces a deadlock. File.write('/tmp/bar.rb', <<~RUBY) Fiber.yield Bar = 1 RUBY autoload :Bar, '/tmp/bar.rb' Fiber.new { Bar }.resume Fiber.new { Bar }.resume ``` A different matter is fibers managed by fiber schedulers. I have not been able to enter a deadlock with the fiber schedulers I have tried, but from the point of view of the user, doing something like I/O or sleeping at the top-level is not unlike that manual `Fiber.yield` above. The contract for fiber schedulers is mostly an interface, but it does not address this, at least in an explicit way. Do fiber schedulers guarantee anything about this with the current contract? I'd be glad to volunteer docs with the conclusions of this thread. -- https://bugs.ruby-lang.org/

1 1

[ruby-core:115892] [Ruby master Bug#20085] Fiber.new{ }.resume causes Segmentation fault for Ruby 3.3.0 on aarch64-linux
by oleksii (Oleksii Leonov) 29 Feb '24

29 Feb '24

Issue #20085 has been reported by oleksii (Oleksii Leonov). ---------------------------------------- Bug #20085: Fiber.new{ }.resume causes Segmentation fault for Ruby 3.3.0 on aarch64-linux https://bugs.ruby-lang.org/issues/20085 * Author: oleksii (Oleksii Leonov) * Status: Open * Priority: Normal * ruby -v: ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [aarch64-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- ``` ruby -e "Fiber.new{}.resume" 0.170 -e:1: [BUG] Segmentation fault at 0x0036ffffb4f110f0 0.170 ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [aarch64-linux] 0.170 0.170 -- Control frame information ----------------------------------------------- 0.170 c:0003 p:---- s:0010 e:000009 CFUNC :resume 0.170 c:0002 p:0007 s:0006 E:0005e0 EVAL -e:1 [FINISH] 0.170 c:0001 p:0000 s:0003 E:000b50 DUMMY [FINISH] 0.170 0.170 -- Ruby level backtrace information ---------------------------------------- 0.170 -e:1:in `<main>' 0.170 -e:1:in `resume' 0.170 0.170 -- Threading information --------------------------------------------------- 0.170 Total ractor count: 1 0.170 Ruby thread count for this ractor: 1 0.170 0.170 -- Machine register context ------------------------------------------------ 0.170 x0: 0x0000aaab11b4f570 x1: 0x0000aaab11de4a40 x2: 0x0000ffffe5571990 0.170 x3: 0x0000ffff9ac5ff60 x4: 0x0000ffff9ac60018 x5: 0x0000ffff9ac80000 0.170 x6: 0x0000ffffb5458b88 x7: 0x0000000000000000 x18: 0x00000000007fffff 0.170 x19: 0x0000000000000000 x20: 0x0000000000000000 x21: 0x0000000000000000 0.170 x22: 0x0000000000000000 x23: 0x0000000000000000 x24: 0x0000000000000000 0.170 x25: 0x0000000000000000 x26: 0x0000000000000000 x27: 0x0000000000000000 0.170 x28: 0x0000000000000000 x29: 0x0000000000000000 sp: 0x0000ffff9ac60000 0.170 fau: 0x0036ffffb4f110f0 0.170 0.170 -- C level backtrace information ------------------------------------------- 0.171 Segmentation fault ``` - Ruby 3.3.0-rc1 and 3.2.2 work without Segmentation fault on both amd64 and arm64. - Ruby 3.3.0 works on amd64, but fails with Segmentation fault on arm64 (aarch64-linux) on Ubuntu 22.04 and Debian Bookworm. Dockerfile to reproduce: ``` FROM debian:bookworm # Set UTF-8 locale by default. ENV RBENV_ROOT=/usr/local/rbenv \ PATH=/usr/local/rbenv/bin:/usr/local/rbenv/shims:$PATH RUN ( \ export DEBIAN_FRONTEND=noninteractive \ && apt-get update \ && apt-get install -y --no-install-recommends \ ca-certificates curl git \ # standard dependencies for rbenv autoconf \ patch \ build-essential \ rustc \ libssl-dev \ libyaml-dev \ libreadline6-dev \ zlib1g-dev \ libgmp-dev \ libncurses5-dev \ libffi-dev \ libgdbm6 \ libgdbm-dev \ libdb-dev \ uuid-dev \ ) ENV RBENV_VERSION=3.3.0 RUN ( \ export DEBIAN_FRONTEND=noninteractive \ # Install rbenv & ruby-build && git clone https://github.com/rbenv/rbenv.git /usr/local/rbenv \ && git clone https://github.com/rbenv/ruby-build.git /usr/local/rbenv/plugins/ruby-build \ && /usr/local/rbenv/plugins/ruby-build/install.sh \ && echo 'export RBENV_ROOT=/usr/local/rbenv' >> /etc/profile.d/rbenv.sh \ && echo 'export PATH=/usr/local/rbenv/bin:$PATH' >> /etc/profile.d/rbenv.sh \ && echo 'eval "$(rbenv init -)"' >> /etc/profile.d/rbenv.sh \ && echo 'export RBENV_ROOT=/usr/local/rbenv' >> /root/.bashrc \ && echo 'export PATH=/usr/local/rbenv/bin:$PATH' >> /root/.bashrc \ && echo 'eval "$(rbenv init -)"' >> /root/.bashrc \ && eval "$(rbenv init -)"; rbenv install $RBENV_VERSION \ && eval "$(rbenv init -)"; rbenv global $RBENV_VERSION \ ) RUN RUN ruby -e "Fiber.new{}.resume" ``` `docker build --platform linux/arm64 .` ---Files-------------------------------- Dockerfile (1.53 KB) -- https://bugs.ruby-lang.org/

13 26

[ruby-core:117014] [Ruby master Bug#20316] Regexp quantifier behavior changes in different syntactic context.
by jirkamarsik (Jirka Marsik) 29 Feb '24

29 Feb '24

Issue #20316 has been reported by jirkamarsik (Jirka Marsik). ---------------------------------------- Bug #20316: Regexp quantifier behavior changes in different syntactic context. https://bugs.ruby-lang.org/issues/20316 * Author: jirkamarsik (Jirka Marsik) * Status: Open * ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux-gnu] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- In the example below, adding a vertical bar to the end of a regular expression changes what is being matched by the preceding regular expression. ``` irb(main):001:0> /(|a){3}b/.match("aab") => #<MatchData "aab" 1:""> irb(main):002:0> /(|a){3}b|/.match("aab") => #<MatchData "aab" 1:"a"> ``` This is because the `{3}` quantifier is compiled into a `repeat` loop which uses the `OP_NULL_CHECK_END_MEMST` operation to perform a capture-group sensitive null-check after every loop iteration. The logic behind the `OP_NULL_CHECK_END_MEMST` operation is implemented using the `STACK_NULL_CHECK_MEMST` macro in `regexec.c`. The `STACK_NULL_CHECK_MEMST` macro checks whether any capture groups have been updated inside the last loop iteration and it does so by searching the stack for `STK_MEM_START` entries. However, such entries are not used for all capture groups. They are only used by capture groups which are listed in `bt_mem_start`. A capture group is marked as needing such bookkeeping only when it is either referenced by a back-reference or it appears in certain syntactic contexts (see e.g. around line 4096 of `regcomp.c`). This looks like an optimization that tries to avoid polluting the stack with `STK_MEM_START` entries in cases in which they are not needed. However, in this case, not putting `STK_MEM_START` entries on the stack leads to a different match result. In the example above, by adding a vertical bar to the end of the regexp, we have placed the group `(|a)` inside an alternation. This means that a different operation for tracking the state of the capture group is emitted in the compiled bytecode and this leads to a different result. Incidentally, this result should be the correct result, as the null-check ends up respecting the state of capture groups, as it tries to do in Ruby. This is the compilation and execution of `/(|a){3}b/.match("aab")` with ONIG_DEBUG_PARSE_TREE, ONIG_DEBUG_COMPILE and ONIG_DEBUG_MATCH. Note that `mem-start:1` is used for tracking the state of capture group 1. ``` PATTERN: /(|a){3}b/ (US-ASCII) <list:55db929991f0> <quantifier:55db92999230>{3,3} <enclose:55db92999330> memory:1 <alt:55db929991b0> <string:55db929992f0> <string:55db929992b0>a <string:55db929993b0>b optimize: EXACT anchor: [] sub anchor: [] exact: [b]: length: 1 code length: 37 0:[repeat:0:27] 7:[null-check-start:0] 10:[mem-start:1] 13:[push:(+5)] 18:[jump:(+2)] 23:[exact1:a] 25:[mem-end:1] 28:[null-check-end-memst:0] 31:[repeat-inc:0] 34:[exact1:b] 36:[end] match_at: str: 139809364021904 (0x7f27e77a9290), end: 139809364021906 (0x7f27e77a9292), start: 139809364021904 (0x7f27e77a9290), sprev: 0 ((nil)) size: 2, start offset: 0 ofs> str stk:type addr:opcode 0> "ab" 0:Alt 0:[repeat:0:27] 0> "ab" 1:Rep 7:[null-check-start:0] 0> "ab" 2:NulChS 10:[mem-start:1] 0> "ab" 2:NulChS 13:[push:(+5)] 0> "ab" 3:Alt 18:[jump:(+2)] 0> "ab" 3:Alt 25:[mem-end:1] 0> "ab" 3:Alt 28:[null-check-end-memst:0] NULL_CHECK_END_MEMST: skip id:0, s:139809364021904 (0x7f27e77a9290) 0> "ab" 3:Alt 34:[exact1:b] 0> "ab" 2:NulChS 23:[exact1:a] 1> "b" 2:NulChS 25:[mem-end:1] 1> "b" 2:NulChS 28:[null-check-end-memst:0] 1> "b" 2:NulChS 31:[repeat-inc:0] 1> "b" 3:RepInc 7:[null-check-start:0] 1> "b" 4:NulChS 10:[mem-start:1] 1> "b" 4:NulChS 13:[push:(+5)] 1> "b" 5:Alt 18:[jump:(+2)] 1> "b" 5:Alt 25:[mem-end:1] 1> "b" 5:Alt 28:[null-check-end-memst:0] NULL_CHECK_END_MEMST: skip id:0, s:139809364021905 (0x7f27e77a9291) 1> "b" 5:Alt 34:[exact1:b] 2> "" 5:Alt 36:[end] ``` This is the compilation and execution of `/(|a){3}b|/.match("aab")` (where the group `(|a)` appears inside an alternation) with ONIG_DEBUG_PARSE_TREE, ONIG_DEBUG_COMPILE and ONIG_DEBUG_MATCH. Note that `mem-start-push:1` is used for tracking the state of capture group 1, not `mem-start:1`. ``` PATTERN: /(|a){3}b|/ (US-ASCII) <alt:55d9a91cc3c0> <list:55d9a91cc200> <quantifier:55d9a91cc240>{3,3} <enclose:55d9a91cc340> memory:1 <alt:55d9a91cc1c0> <string:55d9a91cc300> <string:55d9a91cc2c0>a <string:55d9a91cc480>b <string:55d9a91cc400> optimize: NONE anchor: [] code length: 47 0:[push:(+41)] 5:[repeat:0:27] 12:[null-check-start:0] 15:[mem-start-push:1] 18:[push:(+5)] 23:[jump:(+2)] 28:[exact1:a] 30:[mem-end:1] 33:[null-check-end-memst:0] 36:[repeat-inc:0] 39:[exact1:b] 41:[jump:(+0)] 46:[end] match_at: str: 140072854131304 (0x7f6540b69268), end: 140072854131306 (0x7f6540b6926a), start: 140072854131304 (0x7f6540b69268), sprev: 0 ((nil)) size: 2, start offset: 0 ofs> str stk:type addr:opcode 0> "ab" 0:Alt 0:[push:(+41)] 0> "ab" 1:Alt 5:[repeat:0:27] 0> "ab" 2:Rep 12:[null-check-start:0] 0> "ab" 3:NulChS 15:[mem-start-push:1] 0> "ab" 4:MemS 18:[push:(+5)] 0> "ab" 5:Alt 23:[jump:(+2)] 0> "ab" 5:Alt 30:[mem-end:1] 0> "ab" 5:Alt 33:[null-check-end-memst:0] 0> "ab" 5:Alt 36:[repeat-inc:0] 0> "ab" 6:RepInc 12:[null-check-start:0] 0> "ab" 7:NulChS 15:[mem-start-push:1] 0> "ab" 8:MemS 18:[push:(+5)] 0> "ab" 9:Alt 23:[jump:(+2)] 0> "ab" 9:Alt 30:[mem-end:1] 0> "ab" 9:Alt 33:[null-check-end-memst:0] NULL_CHECK_END_MEMST: skip id:0, s:140072854131304 (0x7f6540b69268) 0> "ab" 9:Alt 39:[exact1:b] 0> "ab" 8:MemS 28:[exact1:a] 1> "b" 8:MemS 30:[mem-end:1] 1> "b" 8:MemS 33:[null-check-end-memst:0] 1> "b" 8:MemS 36:[repeat-inc:0] 1> "b" 9:RepInc 12:[null-check-start:0] 1> "b" 10:NulChS 15:[mem-start-push:1] 1> "b" 11:MemS 18:[push:(+5)] 1> "b" 12:Alt 23:[jump:(+2)] 1> "b" 12:Alt 30:[mem-end:1] 1> "b" 12:Alt 33:[null-check-end-memst:0] 1> "b" 12:Alt 36:[repeat-inc:0] 1> "b" 13:RepInc 39:[exact1:b] 2> "" 13:RepInc 41:[jump:(+0)] 2> "" 13:RepInc 46:[end] ``` -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117013] [Ruby master Bug#20315] Quantifier expansion leads to different results in Regexp.
by jirkamarsik (Jirka Marsik) 29 Feb '24

29 Feb '24

Issue #20315 has been reported by jirkamarsik (Jirka Marsik). ---------------------------------------- Bug #20315: Quantifier expansion leads to different results in Regexp. https://bugs.ruby-lang.org/issues/20315 * Author: jirkamarsik (Jirka Marsik) * Status: Open * ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux-gnu] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- Consider the following series of regular expression matches: ``` irb(main):001:0> /(|a)(|a)(|a)(|a)(|a)b/.match("aaaab") => #<MatchData "aaaab" 1:"" 2:"a" 3:"a" 4:"a" 5:"a"> irb(main):002:0> /(|a)(|a)(|a)(|a)b/.match("aaab") => #<MatchData "aaab" 1:"" 2:"a" 3:"a" 4:"a"> irb(main):003:0> /(|a)(|a)(|a)b/.match("aab") => #<MatchData "aab" 1:"" 2:"a" 3:"a"> irb(main):004:0> /(|a)(|a)b/.match("ab") => #<MatchData "ab" 1:"" 2:"a"> ``` Let `X^{N}` mean `N` concatenated repetitions of `X`. When matching the pattern `/(|a)^{N}b/` against `a^{N-1}b`, the first group will match the empty string and the last `N-1` groups will match `a`. Now, let's look at this series of similar expressions, in which `(|a)^{N}` is replaced with `(|a){N}`, i.e. a counted quantifier. ``` irb(main):001:0> /(|a){5}b/.match("aaaab") => #<MatchData "aaaab" 1:""> irb(main):002:0> /(|a){4}b/.match("aaab") => #<MatchData "aaab" 1:""> irb(main):003:0> /(|a){3}b/.match("aab") => #<MatchData "aab" 1:""> irb(main):004:0> /(|a){2}b/.match("ab") => #<MatchData "ab" 1:"a"> ``` When matching the pattern `/(|a){N}b/` against `a^{N-1}b`, the first `N-1` iterations will match `a` and the `N`-th iteration will match the empty string (compare this with the behavior of the first series of expressions). However, something strange happens when `N` is 2. We end up getting a result which is not consistent with this series, but looks like a result that belongs to the first series discussed above. This is due to quantifier expansion done by the regexp compiler (see usages of `QUANTIFIER_EXPAND_LIMIT_SIZE` in `regcomp.c`). This is an optimization that tries to remove the overhead of managing a counted repetition during regex execution at the cost of increasing the size of the compiled regex bytecode. The source of the inconsistency is caused by the fact that this optimization can actually change the semantics of the regular expression, because as we have seen above, `X^{N}` (`X` repeated `N` times) can have different semantics from `X{N}` (a single `X` with an `{N}` quantifier). Compilation and execution of `/(|a){3}b/.match("aab")` with `ONIG_DEBUG_PARSE_TREE`, `ONIG_DEBUG_COMPILE` and `ONIG_DEBUG_MATCH`: ``` PATTERN: /(|a){3}b/ (US-ASCII) <list:556161bb11f0> <quantifier:556161bb1230>{3,3} <enclose:556161bb1330> memory:1 <alt:556161bb11b0> <string:556161bb12f0> <string:556161bb12b0>a <string:556161bb13b0>b optimize: EXACT anchor: [] sub anchor: [] exact: [b]: length: 1 code length: 37 0:[repeat:0:27] 7:[null-check-start:0] 10:[mem-start:1] 13:[push:(+5)] 18:[jump:(+2)] 23:[exact1:a] 25:[mem-end:1] 28:[null-check-end-memst:0] 31:[repeat-inc:0] 34:[exact1:b] 36:[end] match_at: str: 140607416406704 (0x7fe1b71b92b0), end: 140607416406706 (0x7fe1b71b92b2), start: 140607416406704 (0x7fe1b71b92b0), sprev: 0 ((nil)) size: 2, start offset: 0 ofs> str stk:type addr:opcode 0> "ab" 0:Alt 0:[repeat:0:27] 0> "ab" 1:Rep 7:[null-check-start:0] 0> "ab" 2:NulChS 10:[mem-start:1] 0> "ab" 2:NulChS 13:[push:(+5)] 0> "ab" 3:Alt 18:[jump:(+2)] 0> "ab" 3:Alt 25:[mem-end:1] 0> "ab" 3:Alt 28:[null-check-end-memst:0] NULL_CHECK_END_MEMST: skip id:0, s:140607416406704 (0x7fe1b71b92b0) 0> "ab" 3:Alt 34:[exact1:b] 0> "ab" 2:NulChS 23:[exact1:a] 1> "b" 2:NulChS 25:[mem-end:1] 1> "b" 2:NulChS 28:[null-check-end-memst:0] 1> "b" 2:NulChS 31:[repeat-inc:0] 1> "b" 3:RepInc 7:[null-check-start:0] 1> "b" 4:NulChS 10:[mem-start:1] 1> "b" 4:NulChS 13:[push:(+5)] 1> "b" 5:Alt 18:[jump:(+2)] 1> "b" 5:Alt 25:[mem-end:1] 1> "b" 5:Alt 28:[null-check-end-memst:0] NULL_CHECK_END_MEMST: skip id:0, s:140607416406705 (0x7fe1b71b92b1) 1> "b" 5:Alt 34:[exact1:b] 2> "" 5:Alt 36:[end] ``` Compilation and execution of `/(|a){2}b/.match("ab")` with `ONIG_DEBUG_PARSE_TREE`, `ONIG_DEBUG_COMPILE` and `ONIG_DEBUG_MATCH`: ``` PATTERN: /(|a){2}b/ (US-ASCII) <list:55bbc826c1f0> <quantifier:55bbc826c230>{2,2} <enclose:55bbc826c330> memory:1 <alt:55bbc826c1b0> <string:55bbc826c2f0> <string:55bbc826c2b0>a <string:55bbc826c3b0>b optimize: EXACT anchor: [] sub anchor: [] exact: [b]: length: 1 code length: 39 0:[mem-start:1] 3:[push:(+5)] 8:[jump:(+2)] 13:[exact1:a] 15:[mem-end:1] 18:[mem-start:1] 21:[push:(+5)] 26:[jump:(+2)] 31:[exact1:a] 33:[mem-end:1] 36:[exact1:b] 38:[end] match_at: str: 140139875963504 (0x7f74db869270), end: 140139875963506 (0x7f74db869272), start: 140139875963504 (0x7f74db869270), sprev: 0 ((nil)) size: 2, start offset: 0 ofs> str stk:type addr:opcode 0> "ab" 0:Alt 0:[mem-start:1] 0> "ab" 0:Alt 3:[push:(+5)] 0> "ab" 1:Alt 8:[jump:(+2)] 0> "ab" 1:Alt 15:[mem-end:1] 0> "ab" 1:Alt 18:[mem-start:1] 0> "ab" 1:Alt 21:[push:(+5)] 0> "ab" 2:Alt 26:[jump:(+2)] 0> "ab" 2:Alt 33:[mem-end:1] 0> "ab" 2:Alt 36:[exact1:b] 0> "ab" 1:Alt 31:[exact1:a] 1> "b" 1:Alt 33:[mem-end:1] 1> "b" 1:Alt 36:[exact1:b] 2> "" 1:Alt 38:[end] ``` -- https://bugs.ruby-lang.org/

1 0

[ruby-core:116995] [Ruby master Bug#20312] Memory leak with BigDecimal::EXCEPTION_OVERFLOW
by MaxLap (Maxime Lapointe) 29 Feb '24

29 Feb '24

Issue #20312 has been reported by MaxLap (Maxime Lapointe). ---------------------------------------- Bug #20312: Memory leak with BigDecimal::EXCEPTION_OVERFLOW https://bugs.ruby-lang.org/issues/20312 * Author: MaxLap (Maxime Lapointe) * Status: Open * ruby -v: 3.3.0 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- The following causes a memory leak. I left a commented line, both trigger an memory leak. I assume it's the same bug ``` require 'bigdecimal' BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW, true) 10.times do |j| 100000.times do |i| begin BigDecimal("1E11111111111111111111") #BigDecimal("1E1000000000000000000")**10 # Also leaks rescue end end GC.start puts `ps -o rss= -p #{$$}`.to_i end ``` ``` $ ruby local.rb 28652 34988 41060 47396 53732 59804 66140 72476 78548 84884 ``` Happens in 3.2.3, 3.3.0, 3.3-head, ruby-head (of a few days ago). -- https://bugs.ruby-lang.org/

2 2

[ruby-core:117011] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by Edwing123 (Edwin Garcia) 29 Feb '24

29 Feb '24

Issue #15554 has been updated by Edwing123 (Edwin Garcia). > Regarding the warning message: +1, these messages are clearer. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-107072 * Author: ko1 (Koichi Sasada) * Status: Open * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117009] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by Eregon (Benoit Daloze) 29 Feb '24

29 Feb '24

Issue #15554 has been updated by Eregon (Benoit Daloze). These results look encouraging to me. The number of "false positives" seems reasonable. Also they are not really false positives, because they are cases where a block is given and ignored by a method, but with the same call site also potentially using that block when calling another method. It seems useful to have some way to suppress this warning at a call site for cases where it's not possible to modify the called method immediately (e.g. because it's in some other gem and so need a fix + release of that gem). It's probably very rare though, as it would need to be such an intentional case for duck typing (same call site calls 2 methods, one accepts a block, one does not). So maybe it's fine to not have such a way because rare enough. Best way to know seems to experiment by merging this change. Not sure what form to suppress the warning would be best. I can think of a magic comment (e.g. ignore block warnings for all call sites between `unused_block_warning: false` and `unused_block_warning: true`). Or it could be a way to mark a Method/UnboundMethod object (would mark the callee as "accepting a block"). But it feels quite wrong to mutate another gem's methods like that. > I found a test that the passed block will be ignored, but I'm not sure it is needed. It's fine to remove that spec. Regarding the warning message: > file.rb:line: warning: a block is given for a block unused method 'ObjectSpace.garbage_collect' I think this would be better: > file.rb:line: warning: the passed block is ignored because 'ObjectSpace.garbage_collect' does not accept a block parameter and for a block given to a Proc: > file.rb:line: warning: the passed block is ignored because #<Proc:0x... file:line> does not accept a block parameter ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-107070 * Author: ko1 (Koichi Sasada) * Status: Open * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117007] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by ko1 (Koichi Sasada) 29 Feb '24

29 Feb '24

Issue #15554 has been updated by ko1 (Koichi Sasada). We found issues with this warning system, like: https://github.com/ruby/ruby/commit/8316cb213c ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-107066 * Author: ko1 (Koichi Sasada) * Status: Open * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117006] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by ko1 (Koichi Sasada) 29 Feb '24

29 Feb '24

Issue #15554 has been updated by ko1 (Koichi Sasada). Duck typing case: ``` class C1 def foo = nil end class C2 def foo = yield end [C1.new, C2.new].each{ _1.foo{ ... } } ``` RDoc's `each_ancestor` method(s) is. ---- Mock cases: ```patch diff --git a/spec/ruby/security/cve_2019_8322_spec.rb b/spec/ruby/security/cve_2019_8322_spec.rb index b70d78c033..7a25b68268 100644 --- a/spec/ruby/security/cve_2019_8322_spec.rb +++ b/spec/ruby/security/cve_2019_8322_spec.rb @@ -8,7 +8,7 @@ describe "CVE-2019-8322 is resisted by" do it "sanitising owner names" do command = Gem::Commands::OwnerCommand.new - def command.rubygems_api_request(*args) + def command.rubygems_api_request(*args, &_) Struct.new(:body).new("---\n- email: \"\e]2;nyan\a\"\n handle: handle\n id: id\n") end def command.with_response(response) ``` In this case, it provides `command.rubygems_api_request` method for mock which ignores the given block. I think this kind of code can be many places. (using `...` is one solutuon) ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-107065 * Author: ko1 (Koichi Sasada) * Status: Open * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117005] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by ko1 (Koichi Sasada) 29 Feb '24

29 Feb '24

Issue #15554 has been updated by ko1 (Koichi Sasada). Retry it on current master 2024. Summary: * false positive on several cases: * intentional cases for duck typing * making mock cases they want to ignore arguments * found a few issues by warnings ---- https://gist.github.com/ko1/804a0034fb0ec0bdcb67f492932dc976 is a patch to make `unused block` warning. ---- on `make test-all` I need to add some patches to remove warnings: https://gist.github.com/ko1/ee47ff7ba1eb8d2beaee74f4124a348d Most of cases they are mock/wrapper methods for tests. ---- on `make test-spec` I need to add some patches to remove warnings: https://gist.github.com/ko1/ba24938ee810793d52e462961056190c Most of cases they are mock/wrapper methods for tests. We got several issues on cgi. Passing blocks for CGI related methods can be bugs (not checked details). https://gist.github.com/ko1/3aafaf775e77074108bb0058ab47cd10 I found a test that the passed block will be ignored, but I'm not sure it is needed. ```ruby it "ignores the supplied block" do -> { ObjectSpace.garbage_collect {} }.should_not raise_error end ``` The warning is the following: ``` /home/ko1/ruby/src/trunk/spec/ruby/core/objectspace/garbage_collect_spec.rb:14: warning: a block is given for a block unused method 'ObjectSpace.garbage_collect' ``` ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-107064 * Author: ko1 (Koichi Sasada) * Status: Open * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0