-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[refurb
] Mark int
and bool
cases for Decimal.from_float
as safe fixes in FURB164
tests
#19468
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
Conversation
} | ||
replacement_arg = chars.into_iter().collect(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for float("\x2dnan")
or float("\N{HYPHEN-MINUS}nan")
? Cf. #16771.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it covers both of those cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add tests for those cases? I also think we could use the return value of as_non_finite_float_string_literal
just above this instead of doing some of the manual manipulation ourselves. That should normalize the result so that we can just compare against "-nan" directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple of suggestions
@@ -180,18 +180,18 @@ fn is_valid_argument_type( | |||
typing::is_float(binding, semantic), | |||
) | |||
}) | |||
.unwrap_or_default() | |||
.unwrap_or((false, false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be equivalent, but it's probably more readable, even though I think I suggested the old version 😆
} | ||
replacement_arg = chars.into_iter().collect(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add tests for those cases? I also think we could use the return value of as_non_finite_float_string_literal
just above this instead of doing some of the manual manipulation ourselves. That should normalize the result so that we can just compare against "-nan" directly.
} else { | ||
(false, false) | ||
}; | ||
|
||
match (method_name, constructor) { | ||
// Decimal.from_float accepts int, bool, float | ||
// Decimal.from_float: Only int or bool are safe (float is unsafe due to FloatOperation trap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to update the name of this function and also the comment where it's called. is_valid
made me think we would bail out without a diagnostic or without a fix if the result is invalid, but it's really just checking the fix safety.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just one more tiny nit that I'll probably just apply and then merge this.
Summary
Fixes #19460