Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Kernel#Rational #1438

Conversation

sampersand
Copy link
Contributor

@sampersand sampersand commented Aug 18, 2023

Updated Kernel#Rational to be more accurate.

Phew, this one is a mess. In "normal operation," the signature would look like this:

type num = Complex|Float|int|_ToR|String
def self?.Rational: (num numer, num denom, exception: false) -> Rational?
                  | (num numer, num denom, ?exception: bool) -> Rational

However, Kernel#Rational also tries to be a "division operator" for some reason: If the first argument is Numeric, but defines neither to_int nor to_i nor even to_r, Kernel#Rational will return arg1 / arg2. That is, it'll actually call the / operator on the first argument, with the second.

But it gets worse. If the second argument is exactly 1, then / won't even be called.

In neither case does exception: false actually work—unlike the other conversion methods, exceptions are not caught when Kernel#Rational is trying to act like a division operator.

@sampersand sampersand force-pushed the swesterman/23-08-18/Update-Kernel#Rational branch from 0225acc to 6b12a17 Compare August 18, 2023 22:37
@ParadoxV5
Copy link
Contributor

type num = Complex|Float|int|_ToR|String

Complex, Float and String are already _ToRs. So is the Integer part of int, but not necessarily the _ToInt part. So num can simply be _ToInt|_ToR.

@sampersand sampersand force-pushed the swesterman/23-08-18/Update-Kernel#Rational branch from 6b12a17 to cc8ebac Compare August 18, 2023 23:15
@sampersand
Copy link
Contributor Author

updated to take that into account

@ParadoxV5
Copy link
Contributor

ParadoxV5 commented Aug 18, 2023

that’s quick … I was looking at the changes and it didn’t quite match your desc 😄

EDIT: Why force-push tho

@ParadoxV5
Copy link
Contributor

However, Kernel#Rational also tries to be a "division operator" for some reason: If the first argument is Numeric, but defines neither to_int nor to_i nor even to_r, Kernel#Rational will return arg1 / arg2. That is, it'll actually call the / operator on the first argument, with the second.

Returns x/y or arg as a Rational.
Ruby docs

So this behavoir is kinda but ambiguously defined. I see you have decided to reflect it in your patch.

Comment on lines +619 to +624
| [T] (Numeric&_RationalDiv[T] numer, Numeric denom, ?exception: bool) -> T
| [T < Numeric] (T value, 1, ?exception: bool) -> T
| (untyped, ?untyped, exception: false) -> nil
interface _RationalDiv[T]
def /: (Numeric) -> T
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and confirmed, Kernel#Rational indeed can return non-Rationals, like how the RBS describes. Smells more a Ruby bug than a feature.

core/kernel.rbs Outdated Show resolved Hide resolved
@@ -614,7 +614,15 @@ module Kernel : BasicObject
#
# See also String#to_r.
#
def self?.Rational: (Numeric | String | Object x, ?Numeric | String y, ?exception: bool exception) -> Rational
def self?.Rational: (_ToInt | _ToR numer, ?_ToInt | _ToR denom, exception: false) -> Rational?
| (_ToInt | _ToR numer, ?_ToInt | _ToR denom, ?exception: bool) -> Rational
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With exception: false already covered, can this simply be

Suggested change
| (_ToInt | _ToR numer, ?_ToInt | _ToR denom, ?exception: bool) -> Rational
| (_ToInt | _ToR numer, ?_ToInt | _ToR denom, ?exception: true) -> Rational

?

Also applies to other conversion methods in Kernel.
Does not apply to the [T] ones if they don’t squelch exceptions as you said.

BTW,

Rational 1, 0, exception: false #=! ZeroDivisionError: divided by 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the : false case is for when a typechecker can explicitly determine that it's falsey. the : bool variant is when the typechecker doesn't have enough information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to "default" to having Rational? be the default

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the : false case is for when a typechecker can explicitly determine that it's falsey. the : bool variant is when the typechecker doesn't have enough information.

Not falsey, just false. Ruby rejects exception: nil. It also rejects boolishes, only accepting true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be a world where bool has three values? 💭

@sampersand
Copy link
Contributor Author

Consolidated in #1445

@sampersand sampersand closed this Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants