From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on starla X-Spam-Level: X-Spam-Status: No, score=0.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_BL_SPAMCOP_NET,SPF_HELO_PASS, SPF_PASS autolearn=no autolearn_force=no version=3.4.6 Received: from nue.mailmanlists.eu (nue.mailmanlists.eu [IPv6:2a01:4f8:1c0c:6b10::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id CEC1A1F44D for ; Fri, 19 Apr 2024 08:07:17 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (1024-bit key; secure) header.d=ml.ruby-lang.org header.i=@ml.ruby-lang.org header.a=rsa-sha256 header.s=mail header.b=Cer/I9wL; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=ruby-lang.org header.i=@ruby-lang.org header.a=rsa-sha256 header.s=s1 header.b=T7ZNVJKh; dkim-atps=neutral Received: from nue.mailmanlists.eu (localhost [127.0.0.1]) by nue.mailmanlists.eu (Postfix) with ESMTP id BCF9B84385; Fri, 19 Apr 2024 08:07:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ml.ruby-lang.org; s=mail; t=1713514028; bh=ZwrjQi6yt9w4BtIzbdNzaL5HmCGk6hrscVPgU4ZJzTo=; h=Date:References:To:Reply-To:Subject:List-Id:List-Archive: List-Help:List-Owner:List-Post:List-Subscribe:List-Unsubscribe: From:Cc:From; b=Cer/I9wLBKfwmrqVS0hzbYmXJcVwMcpKHzPXhgrwYejNk9hUSww6ahwKRRtiDcu3n apB0sj6vlDN5LQSBXMuI7VE4TxN9sFHFYF4DMIYGI/tAFbegIhjgW1SvMSukZ8dhO5 Iiuaqx0Cj4aFN4puYxiidsdMNGLnl9lnFGU9/HjM= Received: from s.wrqvtbkv.outbound-mail.sendgrid.net (s.wrqvtbkv.outbound-mail.sendgrid.net [149.72.123.24]) by nue.mailmanlists.eu (Postfix) with ESMTPS id 09DBE8432A for ; Fri, 19 Apr 2024 08:07:04 +0000 (UTC) Authentication-Results: nue.mailmanlists.eu; dkim=pass (2048-bit key; unprotected) header.d=ruby-lang.org header.i=@ruby-lang.org header.a=rsa-sha256 header.s=s1 header.b=T7ZNVJKh; dkim-atps=neutral DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ruby-lang.org; h=from:references:subject:mime-version:content-type: content-transfer-encoding:list-id:to:cc:content-type:from:subject:to; s=s1; bh=o8YZdVqaocxy3ys85Rw1QI0pV6mESLVQItcE6PheY8E=; b=T7ZNVJKhx+aIlYjQk7CMH/3E5FedXfaXkRFY3SdK4ufHLlq92ovLSGHGgIrExMsLbAEp XUb2zxUh/sMnIqOEobbzlpcXdQmlE8ZVmx154O1JQz51aUYVTks9T5IbhmJR01ZtVYiAzG Ed/ll8qC+khIN1fCNwm6O0yECJ2g6dBTE9vD+VpoQlWGDeiPrdLoxDiygDdbTqLtWS7Ldl UdeZT0HYcl+do/ozOhpu0gvmK0XNVSBry7lnZ4pxJcAYVd2cnpHyeAN4cYMDRHP9GoBykG 9+grLMBYze9uJxjdk1biSwXl+wuNTqu1ilWTDALTzTzQXCjNXvh/lKecX7Pbhl9Q== Received: by recvd-547d99795c-kgks6 with SMTP id recvd-547d99795c-kgks6-1-66222627-1E 2024-04-19 08:07:03.829074014 +0000 UTC m=+554803.869285469 Received: from herokuapp.com (unknown) by geopod-ismtpd-5 (SG) with ESMTP id ZOhcMN-YSb2DLN0ZjGmYSg for ; Fri, 19 Apr 2024 08:07:03.811 +0000 (UTC) Date: Fri, 19 Apr 2024 08:07:03 +0000 (UTC) Message-ID: References: Mime-Version: 1.0 X-Redmine-Project: ruby-master X-Redmine-Issue-Tracker: Feature X-Redmine-Issue-Id: 15554 X-Redmine-Issue-Author: ko1 X-Redmine-Issue-Assignee: matz X-Redmine-Issue-Priority: Normal X-Redmine-Sender: byroot X-Mailer: Redmine X-Redmine-Host: bugs.ruby-lang.org X-Redmine-Site: Ruby Issue Tracking System X-Auto-Response-Suppress: All Auto-Submitted: auto-generated X-Redmine-MailingListIntegration-Message-Ids: 94204 X-SG-EID: =?us-ascii?Q?u001=2EKmNZ1u3n1vIpO8NNTdp+Q9c0ai7potxbEDLMO7SOJO=2F4KkRUz0d23466m?= =?us-ascii?Q?naiq=2F5fmA4hb60MdRMUAwHZnjIWVFu=2FrqiBOz5c?= =?us-ascii?Q?nOvkBudsSTj8eOPdkuMngBs5LxUP6YsbuAaKftj?= =?us-ascii?Q?bH506MPRGUyGtS8=2F6YdO2=2FlESqWQrHXIhUyLMva?= =?us-ascii?Q?Xu3J72tbzGh4YW3aiG4aTNEpsiTvL1=2Ft7tBnK0G?= =?us-ascii?Q?nX4Lsobv3z+xPvWesWIFl+w7e4MIGtALVMIgbAk?= =?us-ascii?Q?AX3NYnFr+G5A4wLc5uR8zHvkqA=3D=3D?= To: ruby-core@ml.ruby-lang.org X-Entity-ID: u001.I8uzylDtAfgbeCOeLBYDww== Message-ID-Hash: OJABNPRXGFKZRZITPTCY5I7HHK2ZWQMI X-Message-ID-Hash: OJABNPRXGFKZRZITPTCY5I7HHK2ZWQMI X-MailFrom: bounces+313651-b711-ruby-core=ml.ruby-lang.org@em5188.ruby-lang.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.3 Precedence: list Reply-To: Ruby developers Subject: [ruby-core:117608] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block List-Id: Ruby developers Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: "byroot (Jean Boussier) via ruby-core" Cc: "byroot (Jean Boussier)" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Issue #15554 has been updated by byroot (Jean Boussier). So I went over Rails warnings after the last patch, I may have missed some, but there is now about 5 unique warnings left: ``` /rails/activejob/lib/active_job/enqueuing.rb:69: warning: the block passed to 'job_or_instantiate' defined at /rails/activejob/lib/active_job/enqueuing.rb:78 may be ignored ``` This one is the one I mentioned above with `...`. It's technically correct, I'll likely fix it in Rails. ``` /rails/actionpack/test/controller/resources_test.rb:592: warning: the block passed to 'ResourcesTest#assert_singleton_named_routes_for' defined at /rails/actionpack/test/controller/resources_test.rb:1308 may be ignored ``` Totally legit, allowed to find a couple assertions that were ignored in the test suite. ``` /rails/activesupport/lib/active_support/callbacks.rb:130: warning: the block passed to 'FilterTest::NonYieldingAroundFilterController#non_yielding_action' defined at /rails/actionpack/test/controller/filters_test.rb:517 may be ignored ``` `non_yielding_action` is essentially a stub, so technically a false positive, but I think it's acceptable. ``` /rails/activerecord/test/cases/serialized_attribute_test.rb:505: warning: the block passed to 'attribute' defined at /rails/activemodel/lib/active_model/attribute_registration.rb:12 may be ignored ``` Legit, uncovered a problem in a test. ``` /usr/local/lib/ruby/gems/3.4.0+0/gems/actionview-7.1.3.2/lib/action_view/base.rb:264: warning: the block passed to '_inline_template___4411181644816080575_6440' defined at inline template:0 may be ignored ``` Semi-legit, the Rails "template" interface allow for a block, so we always pass one, but not all implementation use it. My overall impression after the latest change: - It is much much less noisy - I don't know how many true positive are now silenced, but the interesting thing is that the noise reduction allowed me to find 2-3 bugs I couldn't before because it was hidden in the noise. - There is still things that aren't strictly false positives, but that are somewhat intended, but I think it's acceptable. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108017 * 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/lib/optparse.rb#L698 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-patch-L61 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#L81 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-patch-L86 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/ ______________________________________________ ruby-core mailing list -- ruby-core@ml.ruby-lang.org To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/