Suggest casting on numeric type error#47247

estebank · 2018-01-07T05:13:56Z

rust-highfive · 2018-01-07T05:14:00Z

(rust_highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2018-01-07T10:56:56Z

Shouldn't we suggest to use from instead of casting?

GuillaumeGomez · 2018-01-07T10:59:03Z

More globally, I'm not sure talking about size fit/shortage is a good idea. Just talking about from should be enough for most cases, don't you think?

varkor · 2018-01-07T14:06:40Z

src/librustc_typeck/check/demand.rs

Should "undefined behaviour" be capitalised like this? It doesn't seem to be typical in error messages.

estebank · 2018-01-07T18:17:40Z

More globally, I'm not sure talking about size fit/shortage is a good idea.

I find the possibility of using the compiler errors to teach the language appealing. Presenting some of the possible trade offs inline like this seems like a great opportunity, even though completely I agree with the problems presented by as.

Just talking about from should be enough for most cases, don't you think?

Part of what gave me pause is that From is not implemented for all conversions, only for the ones that cannot fail, and TryFrom is confined to nightly behind a feature flag. Also, the suggestion becomes much more verbose in that case (and only applies to nightly):

fn foo<T>(x: T) {}
fn main() {
    foo::<u32>(3_i32);
}

after:

#![feature(try_from)]
use std::convert::TryFrom;

fn foo<T>(x: T) {}
fn main() {
    foo::<u32>(u32::try_from(3_i32).unwrap());
}

That being said, the way I wrote the code was to make it possible to only show a subset of these suggestions depending on the case. I could suggest into() when it is supported (growing an type, u16 to u32), and remain silent in the other cases.

estebank · 2018-01-07T20:56:16Z

Updated the PR to only suggest .into() conversions. The code for suggesting casts is still there, just never triggered (it is gated on the type of expression, right now not enabled for any).

GuillaumeGomez · 2018-01-08T09:22:32Z

That's problematic that TryFrom is still nightly only... And where is From implementation missing?

estebank · 2018-01-08T18:20:49Z

@GuillaumeGomez in https://github.com/rust-lang/rust/pull/47247/files#diff-1743dc5c92f33d15fb1e903d7ad3ce58 (which is admittedly very verbose, as it has every single possible numeric type mismatch) you can see that the only suggestions provided are for the types that have a From implementation:

within the same numeric type, being signed or unsigned, there's a From implementation from the type with a lesser bit width to the wider type (u16 -> u32, i32 -> i64),
from f32 to f64,
from 32 bit integers and lower to f64
from 16 bit integers and lower to f32.

petrochenkov · 2018-01-09T21:36:47Z

src/librustc_typeck/check/demand.rs

I think you can reuse syntax::util::parser::expr_precedence here and get more precise result.
See fn print_expr_method_call in pprust.rs for an example.

petrochenkov · 2018-01-09T21:49:04Z

src/librustc_typeck/check/demand.rs

float <-> float conversions are buggy?
I don't think this is true. There's -Z saturating-float-casts for correcting float -> int casts, but everything else is already correct, if I remember correctly.

This warning is taken from https://doc.rust-lang.org/book/first-edition/casting-between-types.html#numeric-casts. Is it out of date?

Oh, indeed, #15536 is still open.
Ping @rkruppe just in case.

The default behavior is indeed still UB, saturating casts are still opt-in.

petrochenkov · 2018-01-09T21:51:04Z

src/librustc_typeck/check/demand.rs

If into is available, then the conversion will be lossless and no rounding will happen.
Also into is not available for i/usize and floats.

Updated message.

The suggestion is not given for isize/usize (that's what the unwrap(256) is doing). Added comment to that effect.

petrochenkov · 2018-01-09T21:51:56Z

src/librustc_typeck/check/demand.rs

Same as https://github.com/rust-lang/rust/pull/47247/files#r160534951

petrochenkov · 2018-01-09T21:54:34Z

Looks pretty useful.
I'm also okay with dead code from can_cast = false because the implementation doesn't look "complete" without it, something can be done with it later.
I left some comments inline.

petrochenkov · 2018-01-10T09:17:19Z

src/librustc_typeck/check/demand.rs

Argh, this expr_precedence works on AST.
There's another one in rustc::hir::print.
Also, AssocOp::As.precedence() -> PREC_POSTFIX.

I'm gonna add a commit to merge both into one place (new enum that only holds the type to precedence mapping, and an into impl for {hir, ast}::Expr so that there's only one source of truth for this. Started that yesterday but went to bed before I could complete it.

bors · 2018-01-13T05:05:06Z

☔ The latest upstream changes (presumably #46461) made this pull request unmergeable. Please resolve the merge conflicts.

- Use `syntax::util::parser::expr_precedence` to determine wether parenthesis are needed around the casting target. - Update message to not incorrectly mention rounding on `.into()` suggestions, as those types that do have that implemented will never round.

Introduce a new unified type that holds the expression precedence for both AST and HIR nodes.

estebank · 2018-01-15T12:50:23Z

Fixed.

petrochenkov · 2018-01-15T20:56:23Z

src/libsyntax/ast.rs

 }

+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ExprPrecedence {


Could you move all this stuff from ast.rs somewhere, e.g. into util/parser.rs where it was previously.
This is not a part of AST or HIR, really, it's a parsing-specific detail.

Done. Waiting for the tests to run to make sure nothing broke.

petrochenkov · 2018-01-16T21:03:53Z

@bors r+

bors · 2018-01-16T21:03:54Z

📌 Commit 71c0873 has been approved by petrochenkov

Suggest casting on numeric type error Re rust-lang#47168.

Rollup of 9 pull requests - Successful merges: #47247, #47334, #47512, #47582, #47595, #47625, #47632, #47633, #47637 - Failed merges:

rust-highfive assigned petrochenkov Jan 7, 2018

varkor reviewed Jan 7, 2018

View reviewed changes

estebank force-pushed the suggest-cast branch from 03d1402 to 09bba0c Compare January 7, 2018 22:28

estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2018

petrochenkov reviewed Jan 9, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2018

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2018

estebank force-pushed the suggest-cast branch from 85696b5 to 59cc9f7 Compare January 10, 2018 03:12

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2018

petrochenkov reviewed Jan 10, 2018

View reviewed changes

estebank added 4 commits January 14, 2018 22:46

Suggest casting on numeric type error

aec1623

Use into for casting when possible

af91d99

Only suggest casting numeric types using into()

509ea8e

estebank force-pushed the suggest-cast branch from 59cc9f7 to 27e9689 Compare January 15, 2018 09:38

Use single source of truth for expr precedence

afe8d13

Introduce a new unified type that holds the expression precedence for both AST and HIR nodes.

estebank force-pushed the suggest-cast branch from 27e9689 to afe8d13 Compare January 15, 2018 10:02

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2018

petrochenkov reviewed Jan 15, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2018

Move ExprPrecedence to libsyntax/util/parser.rs

71c0873

petrochenkov added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2018

GuillaumeGomez mentioned this pull request Jan 21, 2018

Rollup of 7 pull requests #47636

Closed

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 21, 2018

Rollup merge of rust-lang#47247 - estebank:suggest-cast, r=petrochenkov

6bb1b0d

Suggest casting on numeric type error Re rust-lang#47168.

GuillaumeGomez mentioned this pull request Jan 21, 2018

Rollup of 9 pull requests #47644

Merged

bors added a commit that referenced this pull request Jan 21, 2018

Auto merge of #47644 - GuillaumeGomez:rollup, r=GuillaumeGomez

ff2a7c8

Rollup of 9 pull requests - Successful merges: #47247, #47334, #47512, #47582, #47595, #47625, #47632, #47633, #47637 - Failed merges:

bors merged commit 71c0873 into rust-lang:master Jan 22, 2018

estebank mentioned this pull request Jan 24, 2018

Add parenthesis around suggestions to use .into() on cast expr #47699

Closed

durka mentioned this pull request Mar 17, 2018

.into() suggested in const context #49100

Closed

estebank deleted the suggest-cast branch November 9, 2023 05:25

Uh oh!

Conversation

estebank commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 7, 2018

Uh oh!

GuillaumeGomez commented Jan 7, 2018

Uh oh!

GuillaumeGomez commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estebank commented Jan 7, 2018

Uh oh!

estebank commented Jan 7, 2018

Uh oh!

GuillaumeGomez commented Jan 8, 2018

Uh oh!

estebank commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 13, 2018

Uh oh!

estebank commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jan 16, 2018

Uh oh!

bors commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

estebank commented Jan 7, 2018 •

edited

Loading