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,T_SCC_BODY_TEXT_LINE 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 4E9C91F44D for ; Thu, 29 Feb 2024 09:15:59 +0000 (UTC) Received: from nue.mailmanlists.eu (localhost [127.0.0.1]) by nue.mailmanlists.eu (Postfix) with ESMTP id F3EB37EFA5; Thu, 29 Feb 2024 09:15:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ml.ruby-lang.org; s=mail; t=1709198149; bh=yuar0kuaTxCITGojlw80XTTtu8a51gIpJPpayOMiSkQ=; 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=2K+g7+Si+JsCVCvPBoRoyX9cDWrM1SA5xujQSaxGl6WqIftU4s6m683CnX+OGJToa v4djCw0Qj+oACDZdQ6spN/uepgIOmzuNBOfirCFuP6sku8Ey/7mKAYtR3GSJp9OsLR DF5008BWmoRMaog2sNJOIofP1rN6nWgeobWV4efc= Received: from s.wrqvwxzv.outbound-mail.sendgrid.net (s.wrqvwxzv.outbound-mail.sendgrid.net [149.72.154.232]) by nue.mailmanlists.eu (Postfix) with ESMTPS id 08BA97EF32 for ; Thu, 29 Feb 2024 09:15:44 +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=K7pX9vGN; 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=BKH2THupuw7KAwR0fNoHnonYXPyv8JOqBBv2Kh/DXus=; b=K7pX9vGNoO3xeMGu+VdXeZbgV4TmpPh6poGQkrxtVeZPpoAPMdkfpbWWObIC1w7C/B6v kW4tZ6LYkDXsit1vvHQCTLERbjJg34xPjGAYtP3GWqCbCrQtL/kvJ2vP0g3yoMblKwNBDY qEavB1gtMnekmy/CfQt6epzIaqxpU3CpxMcqkBzyMMkDfOZHHTB5IOQNwYBiSLO/Hfdnpv OrqqnQiA2D2fVJbDpkX5Af8GDIJOUg6Vop5hnZPIebeaFvtQDeZSRrxMApoIVFdAoQEyip IMkpwMcmedkjjlc7PfVu3WA3E+EP1a6ab31Kg8chp3aArT2Tl+gCeoNB/0Sn9+sA== Received: by recvd-7ffbd89f59-7fr2m with SMTP id recvd-7ffbd89f59-7fr2m-1-65E04B3F-E 2024-02-29 09:15:43.404084121 +0000 UTC m=+41090.565716633 Received: from herokuapp.com (unknown) by geopod-ismtpd-26 (SG) with ESMTP id P4QLMyP-Re-UqO6oC38khA for ; Thu, 29 Feb 2024 09:15:43.280 +0000 (UTC) Date: Thu, 29 Feb 2024 09:15:43 +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: Eregon 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: 93568 X-SG-EID: =?us-ascii?Q?DvL3W2Xo+Vk=2FeUn3F50L=2FNc8u9NqZCnbE0mXZHiyye4R1YZg=2FtAFc0SAFzpcS=2F?= =?us-ascii?Q?Zcr7BqPN=2FYCcMZBMITNeIci9STxYay0JAJWPbek?= =?us-ascii?Q?rFR+qXd6HFG2XmHPE3ktrkzK+BM73adXQcIRdtx?= =?us-ascii?Q?s66AQ+XxuMY+0iigU8AAbRqzKoq9J8nfT5ZIH4k?= =?us-ascii?Q?EmsuTW7b=2FIiK4o5V=2F3g9fb34aznNIDbdB6ybt9W?= =?us-ascii?Q?1JYTbLc+i0OcXfKtBX8fEAkEc5iTLTz0bzoBXqC?= =?us-ascii?Q?QEUr71IeourUWFaq5eeeQ=3D=3D?= To: ruby-core@ml.ruby-lang.org X-Entity-ID: b/2+PoftWZ6GuOu3b0IycA== Message-ID-Hash: REAZHOTDJGPED5BZIHUZLI6MULRVOXI6 X-Message-ID-Hash: REAZHOTDJGPED5BZIHUZLI6MULRVOXI6 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:117009] [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: "Eregon (Benoit Daloze) via ruby-core" Cc: "Eregon (Benoit Daloze)" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 # 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/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/