-
Notifications
You must be signed in to change notification settings - Fork 54
Add a FRC about implicit numeric widening #356
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,3 +217,30 @@ Cross-referencing to other discussions: | |
| * https://github.com/rust-lang/rfcs/issues/1397 | ||
| * https://github.com/rust-lang/rust/issues/17027 | ||
| * https://github.com/rust-lang/unsafe-code-guidelines/issues/176 | ||
|
|
||
| ## Implicit Widening | ||
|
|
||
| Often there are requests to no longer need to manually perform lossless numeric conversions, | ||
| such as from `f32` to `f64`, from `u8` to `i32`, etc. | ||
|
|
||
| While that's convenient in trivial cases, it's not necessarily good in more complex cases. | ||
| If code was perfect then yes it'd be convenient, but Rust would rather ask which thing you | ||
| want between different options when the types don't match. | ||
|
|
||
| Take something like this: | ||
| ```rust | ||
| let x: u16 = …; | ||
| let y: u16 = …; | ||
| takes_u32(x + y); | ||
| ``` | ||
|
|
||
| It would certainly be *possible* to have that just compile, as though you'd written | ||
| ```rust | ||
| takes_u32((x+y).into()) | ||
| ``` | ||
|
|
||
| But having an error there is a good opportunity to ask whether perhaps you wanted | ||
| ```rust | ||
| takes_u32(u32::from(x) + u32::from(y)) | ||
| ``` | ||
| so that it can't overflow in the addition. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the desire for users to consider overflow and think about whether upcasting early is the right choice. However, I don't think it's inherently more problematic to add two It's possibly useful to consider what changes we'd like to make if Rust allowed this coercion today. Personally, I don't think this meets the bar for a warn-by-default-lint: false positives would be extremely common. I would expect we'd instead suggest that this should be a clippy lint, one which could be a bit "fuzzier" and capture more information about the context in which the upcasting occurs (for example, is the value a result of an addition or multiplication, or some other operation which could result in an undesirable overflow?). I think nearly every Rust user has at some point encountered Personally, I think implicit numeric widening + a thoughtful and comprehensive clippy lint is the correct balance.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree that that's a problem, but it's not obvious to me that widening is the answer. What if we had something like rustc's Or even without that, over an edition (because of inference changes) we could just allow indexing with any integer type but still not do widening. (And widening to usize also adds all kinds of annoying complications around whether u32 can widen to usize and whether we add target-specific type system rules like that.) |
||
Uh oh!
There was an error while loading. Please reload this page.
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.
The strongest version of this argument to me goes like this. A user might not realize that the following code has a risk of overflow:
In this case it is clear that the argument type to
offset()is unlikely to wrap for normal offset values, but for theoffsetfield this is not so. Requiring a cast forces the user to consider this fact and put the cast in the right place.The issue though is that detecting this problematic case happens downstream of the actual problematic operation, at the coercion site instead of at the add, and as a result it is very much an overapproximation. This leads to cases where the need to cast is purely a nuisance, like
foo[sm1.offset as usize].I think the places where the current behavior is useful would be better served by a custom integer type that requires more explicit syntax for operations that risk overflow. There is always an opportunity for the user to consider whether they ought to use an integer type like this. Usually multiple, e.g. in this case when writing the
HasSmallOffsetstruct and when performing a narrowing cast from an isize to i8.A family of types could be provided by the standard library that don't implement traits like
Add<Self>but require explicit wrapping methods. Separately, we should consider a feature that allows assigning integer literals to custom integer types to make them more convenient.