From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from neon.ruby-lang.org (neon.ruby-lang.org [221.186.184.75]) by dcvr.yhbt.net (Postfix) with ESMTP id 19B571F5AE for ; Sun, 19 Jul 2020 12:39:04 +0000 (UTC) Received: from neon.ruby-lang.org (localhost [IPv6:::1]) by neon.ruby-lang.org (Postfix) with ESMTP id 61D7E12099A; Sun, 19 Jul 2020 21:38:28 +0900 (JST) Received: from xtrwkhkc.outbound-mail.sendgrid.net (xtrwkhkc.outbound-mail.sendgrid.net [167.89.16.28]) by neon.ruby-lang.org (Postfix) with ESMTPS id A194312097C for ; Sun, 19 Jul 2020 21:38:25 +0900 (JST) Received: by filterdrecv-p3mdw1-75c584b9c6-phwzs with SMTP id filterdrecv-p3mdw1-75c584b9c6-phwzs-21-5F143EDC-6 2020-07-19 12:38:52.107445874 +0000 UTC m=+1970956.705120514 Received: from herokuapp.com (unknown) by ismtpd0018p1iad2.sendgrid.net (SG) with ESMTP id SjblRKsWSFaKv3CFD0FjBg for ; Sun, 19 Jul 2020 12:38:52.001 +0000 (UTC) Date: Sun, 19 Jul 2020 12:38:52 +0000 (UTC) From: merch-redmine@jeremyevans.net Message-ID: References: Mime-Version: 1.0 X-Redmine-MailingListIntegration-Message-Ids: 75014 X-Redmine-Project: ruby-master X-Redmine-Issue-Tracker: Bug X-Redmine-Issue-Id: 17017 X-Redmine-Issue-Author: sambostock X-Redmine-Sender: jeremyevans0 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-SG-EID: =?us-ascii?Q?RVE3t853K5scBhbmJHUzZTFFeVC=2FZSUmHZ0Dc+26wcEi2CTgsF1oz0wTSSxGGN?= =?us-ascii?Q?BI7yV2VQC2uwQoGhuxx=2FByL0EAC4BKCPxq7ebtd?= =?us-ascii?Q?Hp20Yxl9j6I0HO+cR+mkxfu0lwaOtkes0eHlzXk?= =?us-ascii?Q?bp84yJFlL1oKAE2SB41u3L=2FigwPwmIquDCeb9vR?= =?us-ascii?Q?G6hSgMxSbgtdRiWcFbAOQIOQgaQXwR2JMz+lMLT?= =?us-ascii?Q?cRpQtXxVKrDAP=2F83w=3D?= To: ruby-core@ruby-lang.org X-ML-Name: ruby-core X-Mail-Count: 99225 Subject: [ruby-core:99225] [Ruby master Bug#17017] Range#max & Range#minmax incorrectly use Float end as max X-BeenThere: ruby-core@ruby-lang.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: Ruby developers List-Id: Ruby developers List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ruby-core-bounces@ruby-lang.org Sender: "ruby-core" Issue #17017 has been updated by jeremyevans0 (Jeremy Evans). marcandre (Marc-Andre Lafortune) wrote in #note-15: > jeremyevans0 (Jeremy Evans) wrote in #note-13: > > I think this pull request should require matz approval because it is a deliberate tradeoff of correctness in order to keep compatibility. The correct behavior should be raising RangeError, as that is what an endless range returns. > > Well, `(1.0..).max` raises a `RangeError` while `(1.0..Float::INFINITY).max` returns `Float::INFINITY`; which is "correct"? Range#max behavior for integer ranges is different than behavior for float ranges. `(1..2.1).max` and `(1.0..2.1).max` have different result even though `1 == 1.0`. However, I see your point, that there is precedent for behavior to differ between endless ranges and ranges with infinite end. > Unless I'm mistaken, this behavior change was not approved by Matz (or anybody else), changes a behavior that dates back to Ruby 1.8, breaks (that we know of) `activemodel` and `rubocop`, doesn't even raise the right error and isn't tested, and is a change that I and others disapprove of. Yet when you propose that that commit remains as is and that someone else has to bring up the situation with Matz, no other committer seems confused by this. The behavior change in this issue is to fix an obvious bug, which is that `(1..2.1).max` returned `2.1` instead of `2`. In general we don't require Matz's approval to fix obvious bugs. Were you against fixing that bug? After the change was committed, that is when there were reports that the `(1..Float::Infinity).max` case broke. The only reason that case worked previously is it was relying on the previous bug (that Range#max returned a float end for an integer range). It's not like I removed a special case for it. I even worked on a pull request to add a special case for it for compatibility. However, since I think it makes Range#max behavior inconsistent for integer ranges, I think it is something that should be approved by Matz. Now, I may be wrong here. As committers go, I'm fairly junior. If another committer with more experience thinks it something that can be committed without approval from Matz, they can merge my pull request. ---------------------------------------- Bug #17017: Range#max & Range#minmax incorrectly use Float end as max https://bugs.ruby-lang.org/issues/17017#change-86606 * Author: sambostock (Sam Bostock) * Status: Open * Priority: Normal * ruby -v: ruby 2.8.0dev (2020-07-14T04:19:55Z master e60cd14d85) [x86_64-darwin17] * Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN ---------------------------------------- While continuing to add edge cases to [`Range#minmax` specs](https://github.com/ruby/spec/pull/777), I discovered the following bug: ```ruby (1..3.1).to_a == [1, 2, 3] # As expected (1..3.1).to_a.max == 3 # As expected (1..3.1).to_a.minmax == [1, 3] # As expected (1..3.1).max == 3.1 # Should be 3, as above (1..3.1).minmax == [1, 3.1] # Should be [1, 3], as above ``` One way to detect this scenario might be to do (whatever the C equivalent is of) ```ruby range_end.is_a?(Numeric) // Is this a numeric range? && (range_end - range_begin).modulo(1) == 0 // Can we reach the range_end using the standard step size (1) ``` As for how to handle it, a couple options come to mind: - We could error out and do something similar to what we do for exclusive ranges ```ruby raise TypeError, 'cannot exclude non Integer end value' ``` - We might be able to calculate the range end by doing something like ```ruby num_steps = (range_end / range_beg).to_i - 1 # one fewer steps than would exceed the range_end max = range_beg + num_steps # take that many steps all at once ``` - We could delegate to `super` and enumerate the range to find the max ```ruby super ``` - We could update the documentation to define the max for this case as the `range_end`, similarly to how the documentation for `include?` says it behaves like `cover?` for numeric ranges. -- https://bugs.ruby-lang.org/