- ruby-core - ml.ruby-lang.org

[ruby-core:117644] [Ruby master Feature#20443] Allow Major GC's to be disabled

by eightbitraptor (Matthew Valentine-House)

Issue #20443 has been reported by eightbitraptor (Matthew Valentine-House). ---------------------------------------- Feature #20443: Allow Major GC's to be disabled https://bugs.ruby-lang.org/issues/20443 * Author: eightbitraptor (Matthew Valentine-House) * Status: Open ---------------------------------------- [[Github PR #10598]](https://github.com/ruby/ruby/pull/10598) ## Background Ruby's GC running during Rails requests can have negative impacts on currently running requests, causing applications to have high tail-latency. A technique to mitigate this high tail-latency is Out-of-band GC (OOBGC). This is basically where the application is run with GC disabled, and then GC is explicitly started after each request, or when no requests are in progress. This can reduce the tail latency, but also introduces problems of its own. Long GC pauses after each request reduce throughput. This is more pronounced on threading servers like Puma because all the threads have to finish processing user requests and be "paused" before OOBGC can be triggered. This throughput decrease happens for a couple of reasons: 1. There are few heuristics available for users to determine when GC should run, this means that in OOBGC scenarios, it's possible that major GC's are being run more than necessary. 2. The lack of any GC during a request means that lots of garbage objects have been created and not cleaned up, so the process is using more memory than it should - requiring major GC's run as part of OOBGC to do more work and therefore take more time. This ticket attempts to address these issues by: 1. Provide `GC.disable_major` and its antonym `GC.enable_major` to disable and enable only major GC 2. Provide `GC.needs_major?` as a basic heuristic allowing users to tell when Ruby should run a Major GC. These ideas were originally proposed by @ko1 and @byroot in [this rails issue](https://github.com/rails/rails/issues/50449) Disabling GC major's would still allow minor GC's to run during the request, avoiding the ballooning memory usage caused by not running GC at all, and reducing the time that a major takes when we do run it, because the nursery objects have been cleaned up during the request already so there is less work for a major GC to do. This can be used in combination with `GC.needs_major?` to selectively run an OOBGC only when necessary ## Implementation This PR adds 3 new methods to the `GC` module - `GC.disable_major` This prevents major GC's from running automatically. It does not restrict minors. When `objspace->rgengc.need_major_gc` is set and a GC is run, instead of running a major, new heap pages will be allocated and a minor run instead. `objspace->rgengc.need_major_gc` will remain set until a major is manually run. If a major is not manually run then the process will eventually run out of memory. When major GC's are disabled, object promotion is disabled. That is, no objects will increment their ages during a minor GC. This is to attempt to minimise heap growth during the period between major GC's, by restricting the number of old-gen objects that will remain unconsidered by the GC until the next major. When `GC.start` is run, then major GC's will be enabled, a GC triggered with the options passed to `GC.start`, and then `disable_major` will be set to the state it was in before `GC.start` was called. - `GC.enable_major` This simply unsets the bit preventing major GC's. This will revert the GC to normal generational behaviour. Everything behaves as default again. - `GC.needs_major?` This exposes the value of `objspace->rgengc.need_major_gc` to the user level API. This is already exposed in `GC.latest_gc_info[:need_major_by]` but I felt that a simpler interface would make this easier to use and result in more readable code. eg. ```ruby out_of_band do GC.start if GC.needs_major? end ``` Because object aging is disabled when majors are disabled it is recommended to use this in conjunction with `Process.warmup`, which will prepare the heap by running a major GC, compacting the heap, and promoting every remaining object to old-gen. This ensures that minor GC's are running over the smallets possible set of young objects when `GC.disable_major` is true. ## Benchmarks We ran some tests in production on Shopify's core monolith over a weekend and found that: **Mean time spent in GC, as well as p99.9 and p99.99 GC times are all improved.** <img width="1000" alt="Screenshot 2024-04-22 at 16 41 49" src="https://github.com/ruby/ruby/assets/31869/6cff5b11-2e21-40c1-bb84-d994e0e17…"> **p99 GC time is slightly higher.** <img width="1000" alt="Screenshot 2024-04-22 at 16 44 55" src="https://github.com/ruby/ruby/assets/31869/dc645cbe-9495-46f0-8485-24e790c42…"> We're running far fewer OOBGC major GC's now that we have `GC.needs_major?` than we were before, and we believe that this is contributing to a slightly increased number of minor GC's. raising the p99 slightly. **App response times are all improved** We see a 9% reduction in average and p99 response times when compared against standard GC (4% p99.9 and p99.99). <img width="1000" alt="Screenshot 2024-04-22 at 16 55 53" src="https://github.com/ruby/ruby/assets/31869/8a80c102-1564-4bc9-ba44-6e9a8b85f…"> This drops slightly to an 8% reduction in average and p99 response times when compared against standard OOBGC (3.59 p99.9 and 4% p99.99) <img width="1000" alt="Screenshot 2024-04-22 at 16 56 10" src="https://github.com/ruby/ruby/assets/31869/1baef7ea-0155-4ff9-8ba4-a967b7574…"> -- https://bugs.ruby-lang.org/

11 minutes

4
5
0 0

[ruby-core:117684] [Ruby master Bug#20452] Ruby 3.3 on Alpine Linux results in a relatively shallow SystemStackError exception

by Earlopain (A S)

Issue #20452 has been reported by Earlopain (A S). ---------------------------------------- Bug #20452: Ruby 3.3 on Alpine Linux results in a relatively shallow SystemStackError exception https://bugs.ruby-lang.org/issues/20452 * Author: Earlopain (A S) * Status: Open * ruby -v: 3.3.1 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- This is a redo of https://bugs.ruby-lang.org/issues/14387, reported against a non-eol version. The same issue still applies on recent rubies, though I personally only have tested 3.3 and 3.2: ```rb n = 100000 res = {} 1.upto(n).to_a.inject(res) do |r, i| r[i] = {} end def f(x) x.each_value { |v| f(v) } end f(res) ``` The patch from https://bugs.ruby-lang.org/issues/14387#note-13 still works, though as per https://bugs.ruby-lang.org/issues/14387#note-16 it can't be accepted as is. With the patch applied: ``` (irb):8:in `block in f': stack level too deep (SystemStackError) from (irb):8:in `each_value' from (irb):8:in `f' from (irb):8:in `block in f' from (irb):8:in `each_value' from (irb):8:in `f' from (irb):8:in `block in f' from (irb):8:in `each_value' from (irb):8:in `f' ... 11549 levels... from /usr/local/lib/ruby/gems/3.3.0/gems/irb-1.11.0/exe/irb:9:in `<top (required)>' from /usr/local/bin/irb:25:in `load' from /usr/local/bin/irb:25:in `<main>' ``` Without the patch: ``` (irb):8:in `each_value': stack level too deep (SystemStackError) from (irb):8:in `f' from (irb):8:in `block in f' from (irb):8:in `each_value' from (irb):8:in `f' from (irb):8:in `block in f' from (irb):8:in `each_value' from (irb):8:in `f' from (irb):8:in `block in f' ... 286 levels... from /usr/local/lib/ruby/gems/3.3.0/gems/irb-1.11.0/exe/irb:9:in `<top (required)>' from /usr/local/bin/irb:25:in `load' from /usr/local/bin/irb:25:in `<main>' ``` To reproduce, I have removed lines 102-105 from https://github.com/docker-library/ruby/blob/f5753434bb23041dd9913bb7b650e7b…. Check out the previous ticket for more information, there are a few conversations about possible solutions. -- https://bugs.ruby-lang.org/

42 minutes

2
2
0 0

[ruby-core:117658] [Ruby master Feature#20448] Make coverage event hooking C API public

by ms-tob (Matt S)

Issue #20448 has been reported by ms-tob (Matt S). ---------------------------------------- Feature #20448: Make coverage event hooking C API public https://bugs.ruby-lang.org/issues/20448 * Author: ms-tob (Matt S) * Status: Open ---------------------------------------- # Abstract Gathering code coverage information is a well-known goal within software engineering. It is most commonly used to assess code coverage during automated testing. A lesser known use-case is coverage-guided fuzz testing, which will be the primary use-case presented in this issue. This issue exists to request that Ruby coverage event hooking be made part of its official, public C API. # Background Ruby currently provides a number of avenues for hooking events *or* gathering coverage information: 1. The [Coverage](https://ruby-doc.org/3.3.0/exts/coverage/Coverage.html) module 2. The [TracePoint](https://ruby-doc.org/3.3.0/TracePoint.html) module 3. The [rb_add_event_hook](https://ruby-doc.org/3.3.0/extension_rdoc.html#label-Hoo… extension function Unfortunately, none of these pieces of functionality solve this issue's specific use-case. The `Coverage` module is not a great fit for real-time coverage analysis with an unknown start and stop point. Coverage-guided fuzz testing requires this. The `TracePoint` module and `rb_add_event_hook` are not able to hook branch and line coverage events. Coverage-guided fuzz testing typically tracks branch events. # Proposal The ultimate goal is to enable Ruby C extensions to process coverage events in real-time. I did some cursory investigation into the Ruby C internals to determine what it would take to achieve this, but I'm by no means an expert, so my list may be incomplete. The good news is that much of this functionality already exists, but it's part of the private, internal-only C API. 1. Make `RUBY_EVENT_COVERAGE_LINE` and `RUBY_EVENT_COVERAGE_BRANCH` public: https://github.com/ruby/ruby/blob/v3_3_0/vm_core.h#L2182-L2184 a. This would be an addition to the current public event types: https://github.com/ruby/ruby/blob/v3_3_0/include/ruby/internal/event.h#L32-… 2. Allow initializing global coverage state so that coverage tracking can be fully enabled a. Currently, if `Coverage.setup` or `Coverage.start` is not called, then coverage events cannot be hooked. I do not fully understand why this is, but I believe it has something to do with `rb_get_coverages` and `rb_set_coverages`. If calls to `rb_get_coverages` return `NULL` (https://github.com/ruby/ruby/blob/v3_3_0/iseq.c#L641-L647, https://github.com/ruby/ruby/blob/v3_3_0/iseq.c#L864-L868), then coverage hooking will not be enabled. I believe the `Coverage` module initializes that state via a `rb_set_coverages` call here: https://github.com/ruby/ruby/blob/v3_3_0/ext/coverage/coverage.c#L112-L120. b. So, to achieve this goal, a C extension would need to be able to call `rb_set_coverages` or somehow initialize the global coverage state. I've actually been able to achieve this functionality by calling undocumented features and defining `RUBY_EVENT_COVERAGE_BRANCH`: ```c #include <ruby.h> #include <ruby/debug.h> #define RUBY_EVENT_COVERAGE_BRANCH 0x020000 // ... rb_event_flag_t events = RUBY_EVENT_COVERAGE_BRANCH; rb_event_hook_flag_t flags = ( RUBY_EVENT_HOOK_FLAG_SAFE | RUBY_EVENT_HOOK_FLAG_RAW_ARG ); rb_add_event_hook2( (rb_event_hook_func_t) event_hook_branch, events, counter_hash, flags ); ``` If I call `Coverage.setup(branches: true)`, and add this event hook, then branch hooking works as expected. `rb_add_event_hook2` will still respect the `RUBY_EVENT_COVERAGE_BRANCH` value if its passed. But it would be better if I could rely on official functionality rather than undocumented features. The above two points would be requirements for this functionality, but there's an additional nice-to-have: 3. Extend the public `tracearg` functionality to include additional coverage information a. Currently, `tracearg` offers information like `rb_tracearg_lineno` and `rb_tracearg_path`. It would be helpful if it also provided additional coverage information like `coverage.c`'s column information and a unique identifier for each branch. Currently, I can only use `(path, lineno)` as a unique identifier for a branch because that's what's offered by the public API, but more information like column number would be helpful for uniquely identify branches. Since there can be multiple `if` statements on a single line, this can provide ambiguous identification for a branch event. # Use cases This use-case was born out of a new coverage-guided Ruby fuzzer: https://github.com/trailofbits/ruzzy. You can read more about its implementation details here: https://blog.trailofbits.com/2024/03/29/introducing-ruzzy-a-coverage-guided…. You can also find the Ruby C extension code behind its implementation here: https://github.com/trailofbits/ruzzy/blob/v0.7.0/ext/cruzzy/cruzzy.c#L220-L…. So, the primary use-case here is enabling real-time, coverage-guided fuzz testing of Ruby code. However, as mentioned in the abstract, gathering code coverage information is useful in many domains. For example, it could enable new workflows in standard unit/integration test coverage. It could also enable gathering coverage information in real-time as an application is running. I see this as the most generalized form of gathering code coverage information, and something like the `Coverage` module as a specialized implementation. Another example, https://bugs.ruby-lang.org/issues/20282 may be solved by this more generalized solution. We are tracking this request downstream here: https://github.com/trailofbits/ruzzy/issues/9 # Discussion Fuzz testing is another tool in a testers toolbelt. It is an increasingly common way to improve software's robustness. Go has it built in to the standard library, Python has Atheris, Java has Jazzer, JavaScript has Jazzer.js, etc. OSS-Fuzz has helped identify and fix over 10,000 vulnerabilities and 36,000 bugs [using fuzzing](https://google.github.io/oss-fuzz/#trophies). Ruby deserves a good fuzzer, and improving coverage gathering would help achieve that goal. The `Coverage` module, `TracePoint` module, and `rb_add_event_hook` function seem like they could fulfill this goal. However, after deeper investigation, none of them fit the exact requirements for this use-case. # See also - https://bugs.ruby-lang.org/issues/20282 - https://github.com/google/atheris - https://security.googleblog.com/2020/12/how-atheris-python-fuzzer-works.html - https://github.com/CodeIntelligenceTesting/jazzer/ - https://www.code-intelligence.com/blog/java-fuzzing-with-jazzer - https://go.dev/doc/security/fuzz/ -- https://bugs.ruby-lang.org/

2 hours, 59 minutes

2
2
0 0

[ruby-core:117674] [Ruby master Bug#20450] Ruby 3.1.1 broken with bootsnap

by philippe.bs.noel＠tutanota.com (Philippe Noel)

Issue #20450 has been reported by philippe.bs.noel(a)tutanota.com (Philippe Noel). ---------------------------------------- Bug #20450: Ruby 3.1.1 broken with bootsnap https://bugs.ruby-lang.org/issues/20450 * Author: philippe.bs.noel(a)tutanota.com (Philippe Noel) * Status: Open * ruby -v: ruby 3.3.1 (2024-04-23 revision c56cd86388) [x86_64-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- The issue looks like https://bugs.ruby-lang.org/issues/20060 With the new release of ruby 3.1.1, bootsnap does not work anymore. ``` bin/rails aborted! ArgumentError: comparison of String with nil failed (ArgumentError) msg = " #{RUBY_VERSION < SINCE[gem] ? "will no longer be" : "is not"} part of the default gems since Ruby #{SINCE[gem]}." ^^^^^^^^^^ /usr/local/bundle/gems/bootsnap-1.18.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require' ``` It's working with DISABLE_BOOTSNAP=1 with the following message : `warning: parser/current is loading parser/ruby33, which recognizes 3.3.0-compliant syntax, but you are running 3.3.1.` -- https://bugs.ruby-lang.org/

3 hours, 53 minutes

3
3
0 0

[ruby-core:117680] [Ruby master Bug#20451] Bad Ruby 3.1.5 backport causes fiddle to fail to build

by Bo98 (Bo Anderson)

Issue #20451 has been reported by Bo98 (Bo Anderson). ---------------------------------------- Bug #20451: Bad Ruby 3.1.5 backport causes fiddle to fail to build https://bugs.ruby-lang.org/issues/20451 * Author: Bo98 (Bo Anderson) * Status: Open * ruby -v: 3.1.5p252 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- Ruby 3.1.5 shipped with the following backport: https://github.com/ruby/ruby/commit/84f2aabd272a54e79979795d2d405090704a1d07 However this backport directly breaks the build: ``` closure.c:279:60: error: use of undeclared identifier 'data' result = ffi_prep_closure(pcl, cif, callback, (void *)(data->self)); ^ ``` The original commit (https://github.com/ruby/fiddle/commit/2530496602) was updating the second branch to match the change in the first branch a couple lines up. However that change in the other branch does not exist in Ruby 3.1. The commit in question requires a previous commit of https://github.com/ruby/fiddle/commit/81a8a56239973ab7559229830a449d201955b…. The backport should either be reverted or an other commit should also be backported. Note that these commits were in a series of many commits made to fix an upstream issue https://github.com/ruby/fiddle/issues/102 so I cannot vouch whether or not the two commits are sufficient to fix the originally reported issue. -- https://bugs.ruby-lang.org/

3 hours, 55 minutes

1
0
0 0

[ruby-core:117646] [Ruby master Bug#20444] Kernel#loop: returning the "result" value of StopIteration doesn't work when raised directly

by esad (Esad Hajdarevic)

Issue #20444 has been reported by esad (Esad Hajdarevic). ---------------------------------------- Bug #20444: Kernel#loop: returning the "result" value of StopIteration doesn't work when raised directly https://bugs.ruby-lang.org/issues/20444 * Author: esad (Esad Hajdarevic) * Status: Open * ruby -v: ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin20] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- There was a https://bugs.ruby-lang.org/issues/11498 a while ago which was merged in, but I was surprised to find out that raising `StopIteration` in a loop like `loop { raise StopIteration.new(3) }` returns nil and not 3. -- https://bugs.ruby-lang.org/

4 hours, 7 minutes

3
6
0 0

[ruby-core:116687] [Ruby master Bug#20262] Regex mismatch between Ruby 3.2.2 and 3.3.0

by weilandia (Nick Weiland)

Issue #20262 has been reported by weilandia (Nick Weiland). ---------------------------------------- Bug #20262: Regex mismatch between Ruby 3.2.2 and 3.3.0 https://bugs.ruby-lang.org/issues/20262 * Author: weilandia (Nick Weiland) * Status: Open * Priority: Normal * ruby -v: 3.3.0 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- This might be a duplicate of https://bugs.ruby-lang.org/issues/20098, but I cannot make it match with the backref so maybe not. Below example matches in 3.2.2 but not in 3.3.0 ``` ruby str = "------------abcdefg------------#3895912" re = /()\1\b\w*[a-zA-Z-]*\d+[\w-]{3,}\w+\b/ re.match?(str) ``` -- https://bugs.ruby-lang.org/

6 hours, 54 minutes

2
2
0 0

[ruby-core:117657] [Ruby master Bug#20447] Ruby 3.3.1 broken on i686

by vo.x (Vit Ondruch)

Issue #20447 has been reported by vo.x (Vit Ondruch). ---------------------------------------- Bug #20447: Ruby 3.3.1 broken on i686 https://bugs.ruby-lang.org/issues/20447 * Author: vo.x (Vit Ondruch) * Status: Open * ruby -v: ruby 3.3.1 (2024-04-23 revision c56cd86388) [i686-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- ~~~ gcc -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -fPIC -m32 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-strict-overflow -fvisibility=hidden -fexcess-precision=standard -DRUBY_EXPORT -I. -I.ext/include/i386-linux -I/builddir/build/BUILD/ruby-3.3.1/include -I/builddir/build/BUILD/ruby-3.3.1 -I/builddir/build/BUILD/ruby-3.3.1/prism -I/builddir/build/BUILD/ruby-3.3.1/enc/unicode/15.0.0 -o time.o -c /builddir/build/BUILD/ruby-3.3.1/time.c make: Leaving directory '/builddir/build/BUILD/ruby-3.3.1/redhat-linux-build' /builddir/build/BUILD/ruby-3.3.1/time.c: In function ‘zone_timelocal’: /builddir/build/BUILD/ruby-3.3.1/time.c:2345:35: error: passing argument 3 of ‘split_second’ from incompatible pointer type [-Wincompatible-pointer-types] 2345 | split_second(tobj->timew, &t, &s); | ^~ | | | wideval_t * {aka long long unsigned int *} /builddir/build/BUILD/ruby-3.3.1/time.c:616:58: note: expected ‘VALUE *’ {aka ‘long unsigned int *’} but argument is of type ‘wideval_t *’ {aka ‘long long unsigned int *’} 616 | split_second(wideval_t timew, wideval_t *timew_p, VALUE *subsecx_p) | ~~~~~~~^~~~~~~~~ make: *** [Makefile:447: time.o] Error 1 make: *** Waiting for unfinished jobs.... make: Entering directory '/builddir/build/BUILD/ruby-3.3.1/redhat-linux-build' ~~~ This is presumably due to #20327 / https://github.com/ruby/ruby/pull/10332. There seems to be medicine in master commit:git|055613fd868a8c94e43893f8c58a00cdd2a81f6d or maybe the whole https://github.com/ruby/ruby/pull/10332, not sure -- https://bugs.ruby-lang.org/

7 hours, 54 minutes

2
2
0 0

[ruby-core:117670] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block

by ko1 (Koichi Sasada)

Issue #15554 has been updated by ko1 (Koichi Sasada). Status changed from Closed to Assigned BTW on our application I found the following issue on RSpec tests on strict mode. ``` it ... do subject do ... end # this subject ignores the block end ``` This kind of issue can not be checked with current relax mode (because `subject{}` works outside of `it` block). I feel this kind of information is useful if developers have (1) effective warning filtering mechanism and (2) a tool to list the suspicious methods by analyzing the acquired warnings. For (1), at least application authors doesn't need gem's warning. For (2), false positive will be ignored easily (as https://bugs.ruby-lang.org/issues/15554#note-57). Also I'm afraid that if we introduce strict mode option (like environment variable), people can believe duck typing methods should make explicit for accepting block or not. It is what Matz does not like (*now*). ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108080 * Author: ko1 (Koichi Sasada) * Status: Assigned * 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/

8 hours, 31 minutes

1
0
0 0

[ruby-core:117669] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block

by byroot (Jean Boussier)

Issue #15554 has been updated by byroot (Jean Boussier). > Acceptable for a codebase as big as Rails? IMO: - Rails isn't a big code base. Perhaps when compared to the average gem, but not when compared to the average **application** people work on. - Adding a warnings that most people will disregard because it throw too many false positive isn't helping. - Also consider all dependencies will throw warnings too, so when running this on an application with a substantial amount of dependencies, this will generate a TON of noise. I'm not hostile to declare ignored block, I think it's a pretty good idea, and to be honest it's even a bit weird that block parameters are implicit, when others aren't. But if that's the argument, then it would make more sense to me to just always require declaring it, regardless of whether `yield` is used or not. ```ruby >> def foo = yield >> method(:foo).parameters => [] ``` If you are trying to generate API docs or something along these lines, needed to parse the source or bytecode to figure out if a `yield` exist is a lot of extra work. But regardless of what is decided, I think doing it in stages would be preferable. As it stand I'm pretty happy with this new warnings, and I think if it's to be made more verbose, it could wait for Ruby 3.5. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108079 * Author: ko1 (Koichi Sasada) * Status: Closed * 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/

8 hours, 47 minutes

1
0
0 0

2024

2023

2022

ruby-core ----- 2024 ----- April 2024 March 2024 February 2024 January 2024 ----- 2023 ----- December 2023 November 2023 October 2023 September 2023 August 2023 July 2023 June 2023 May 2023 April 2023 March 2023 February 2023 January 2023 ----- 2022 ----- December 2022 November 2022

ruby-core