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

Expected u32 found usize error on array/slice gives unhelpful help text #47168

Closed
KallDrexx opened this issue Jan 3, 2018 · 36 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-diagnostics Working group: Diagnostics

Comments

@KallDrexx
Copy link

fn main() {
    let array = [1, 2, 3];
    test(array.len());
}

fn test(length: u32) {
    println!("{}", length);
}

Gives the following compile time error on rustc 1.22.1 (stable):

Compiling playground v0.0.1 (file:///playground)
error[E0308]: mismatched types
 --> src/main.rs:3:10
  |
3 |     test(array.len());
  |          ^^^^^^^^^^^ expected u32, found usize
  |
  = help: here are some functions which might fulfill your needs:
          - .count_ones()
          - .count_zeros()
          - .leading_zeros()
          - .trailing_zeros()

error: aborting due to previous error

error: Could not compile `playground`.

I'm assuming there's a lot of nuance in the engine that gives suggestion so I don't know how I would personally codify the rules (and even if casting as a u32 is always appropriate since I guess it could overflow) but I do know that the suggestions definitely do not point me in the right direction.

@estebank
Copy link
Contributor

estebank commented Jan 4, 2018

I believe the output for type errors where the expected and found types are numeric should be:

error[E0308]: mismatched types
 --> src/main.rs:3:10
  |
3 |     test(array.len());
  |          ^^^^^^^^^^^ expected u32, found usize
  |
  = help: you can cast an `usize` to `u32`:
  |
3 |     test(array.len() as u32);
  |          ^^^^^^^^^^^^^^^^^^
  = note: if the value doesn't fit it will be truncated when casting from `usize` to `u32`

The last note should be one of the following caveats as appropriate depending on the types:

  • Casting from a larger integer to a smaller integer (e.g. u32 -> u8) will truncate
  • Casting from a smaller integer to a larger integer (e.g. u8 -> u32) will
    • zero-extend if the source is unsigned
    • sign-extend if the source is signed
  • Casting from a float to an integer will round the float towards zero
    • NOTE: currently this will cause Undefined Behavior if the rounded value cannot be represented by the target integer type. This includes Inf and NaN. This is a bug and will be fixed.
  • Casting from an integer to float will produce the floating point representation of the integer, rounded if necessary (rounding strategy unspecified)
  • Casting from an f32 to an f64 is perfect and lossless
  • Casting from an f64 to an f32 will produce the closest possible value (rounding strategy unspecified)
    • NOTE: currently this will cause Undefined Behavior if the value is finite but larger or smaller than the largest or smallest finite value representable by f32. This is a bug and will be fixed.

The checks can be added to check_ref (modifying it to take &mut err so that multiple notes/labels can be added) or in the else clause before suggesting methods.

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-diagnostics Working group: Diagnostics labels Jan 4, 2018
@estebank
Copy link
Contributor

estebank commented Jan 4, 2018

@leonardo-m, @nikomatsakis what would be the appropriate suggestion in this case?

@leonardo-m
Copy link

leonardo-m commented Jan 4, 2018

The proposed messages seem OK to me.

More generally I think that using naked "as" casts for narrowing is the wrong default to use in Rust. You can't have a nicely designed language that performs overflow tests in debug mode and at the same time encourage the use of "as". In my code I use a to!{} macro that shows the default behaviour that Rust missed to use (I'll try to push for this change in the next Epoch):

#![cfg_attr(debug_assertions, feature(try_from))]

#[cfg(debug_assertions)]
use std::convert::TryFrom;

#[cfg(not(debug_assertions))]
macro_rules! to { ($e:expr, $t:ident) => (($e) as $t)  }

#[cfg(debug_assertions)]
macro_rules! to { ($e:expr, $t:ident) => ($t::try_from($e).unwrap())  }

(Recently the situation is improving for FP<->Int conversions with -Zsaturating-float-casts that should become the default).

@nikomatsakis
Copy link
Contributor

I agree that as is bad, but we've never settled on a "nice" replacement, right? I wish that we had methods like to_u32() or something... (why don't we, anyway?).

Anyway, the messages seem ok.

@GuillaumeGomez
Copy link
Member

It's part of a more global suggestion engine that is quite "simple" (aka stupid) I wrote a few months ago. Maybe we should start thinking about it and how we could improve depending on the situation?

@KallDrexx
Copy link
Author

FWIW, as someone who used rust a year+ ago before stopping and only just now picking it up 95% of the times the suggestion engine works great and has done a good job on pointing me in the right direction. It was only this scenario that stuck out to me.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 21, 2018
@estebank
Copy link
Contributor

On nightly the compiler now only complains about the type mismatch, without any hints as to casting:

error[E0308]: mismatched types
 --> src/main.rs:3:10
  |
3 |     test(array.len());
  |          ^^^^^^^^^^^ expected u32, found usize

The compiler will suggest now if the conversion is lossless to use .into() (modulo the bug I just saw where it the suggestion doesn't add parenthesis around a cast).


The compiler could suggest using a cast when dealing with usize/isize, as I don't think there's any other sane way to deal with conversions from/to them, am I wrong?

@scottmcm
Copy link
Member

That might be fine for now. I'd hesitate to suggest as since it truncates.

Presumably the eventual recommendation will be something like .try_into()?? Dunno if it would ever make sense for the compiler to suggest that, though.

@tvladyslav
Copy link
Contributor

Is there any work to be done in this task?

@estebank
Copy link
Contributor

estebank commented Nov 8, 2018

I do not believe so. I think this is blocked on TryInto stabilization, unless we come to an agreement that suggesting as is a good idea (which I don't think will happen).

@scottmcm we could potentially suggest as with warnings that it truncates... (I believe that was in the original impl.)

@carycodes
Copy link

I think it's worth pointing out that if there is no recommendation, the first thing someone who encounters this problem is going to do is Google their way out of it. For me, that results in a Stackoverflow post advising the use of as. Letting the compiler recommend as with a warning seems like a friendlier path to the same result, and better acclimates people to reading the recommendations for useful info.

@leonardo-m
Copy link

"as" doesn't perform overflow tests at run-time in debug builds, so its usage should be minimized and not advised.

@carycodes
Copy link

carycodes commented Mar 5, 2019

Sorry in advance if this seems argumentative or is rehashing old topics, but I'm new here and don't fully understand the advice. Per @estebank above,

Casting from a larger integer to a smaller integer (e.g. u32 -> u8) will truncate

If as is not the proper way to explicitly truncate a (potentially) larger type to a smaller type, what is? To my eyes, typing out as is an explicit acknowledgement that you might truncate (and want to, in the case where it's necessary). What am I missing?

@estebank
Copy link
Contributor

estebank commented Apr 12, 2019

TryInto is now part of stable, but not part of the prelude. If you attempt to use it without importing, the suggestion engine tells the user the right thing. If we want to suggest calling .try_into(), should we suggest .try_into().unwrap()?

Also, integer <-> float and f32 <-> f64 conversions can still only be accomplished with as. Should we suggest it for those cases in particular (with warnings)?

@estebank
Copy link
Contributor

Sorry for the spam, but I believe we need a ruling here on how to proceed here and this falls under language design/documentation.

@rust-lang/lang you have better visibility onto what the language's long-term view for how to address this case is (particularly {integer} to usize, but also for {float}), so I would like to hear what we should suggest in the future.

@rust-lang/docs you have better insight on what our current documentation actually recommends users, what direction are we pushing people towards?

I believe this to be a pebble in the road with the same intensity as &str + &str, and we 1) made great efforts on that front to fix it and 2) we already have the code to provide as suggestions hidden behind an off-by-default flag and could very easily extend it to suggest try_into().unwrap() when applicable.

@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 17, 2019
@Centril
Copy link
Contributor

Centril commented Apr 17, 2019

Nominating for T-Lang discussion re. @estebank's comment. I'm not really sure myself...

@Centril Centril removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 18, 2019
@scottmcm
Copy link
Member

@estebank We discussed this briefly in the lang team meeting; we have no plans for any new features here, so expect something TryFrom/TryInto-related will be the answer. (We didn't have any strong opinions on exactly what the suggestion should be.)

@estebank
Copy link
Contributor

@scottmcm that should be reasonable for type errors involving usize. What about {integer} <-> {float} and f32 <-> f64?

@cramertj
Copy link
Member

I definitely wouldn't recommend as for {float} -> {integer} due to #10184.

@estebank
Copy link
Contributor

For that we can suggest .trunc().try_into().unwrap(). That leaves only float to integer conversion without a method, right?

@cramertj
Copy link
Member

That leaves only float to integer conversion without a method, right?

Integer to float, you mean? Yeah.

@estebank
Copy link
Contributor

Yes, that's what I meant. I'll implement the suggestions this weekend.

@estebank
Copy link
Contributor

Would adding implementations of TryFrom for all numeric values require an RFC?

@scottmcm
Copy link
Member

@estebank There's a conversation about that going on in https://internals.rust-lang.org/t/tryfrom-for-f64/9793?u=scottmcm -- TLDR people seem unsure about what invertibility semantics the trait has.

Personally I find there's some goodness in rust-lang/rfcs#2484, but I don't know where that's going.

bors added a commit that referenced this issue Apr 30, 2019
@estebank
Copy link
Contributor

estebank commented May 3, 2019

Current output for the original code:

error[E0308]: mismatched types
 --> src/main.rs:3:10
  |
3 |     test(array.len());
  |          ^^^^^^^^^^^ expected u32, found usize
help: you can convert an `usize` to `u32` or panic if it the converted value wouldn't fit
  |
3 |     test(array.len().try_into().unwrap());
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@scottmcm
Copy link
Member

scottmcm commented May 3, 2019

Nice, @estebank! That sounds like issue resolved to me.

@estebank estebank closed this as completed May 3, 2019
@KallDrexx
Copy link
Author

KallDrexx commented May 3, 2019

@estebank Shouldn't that it be removed from the error message to make it gramatically correct? e.g.

help: you can convert an usize to u32 or panic if the converted value wouldn't fit

Also it seems like a better message would be along the lines of

help: you can use .try_into().unwrap() to convert a usize into a u32 and panic if the converted value wouldn't fit.

@estebank
Copy link
Contributor

estebank commented May 3, 2019

@KallDrexx yes that seems like the way to go.

@leonardo-m
Copy link

test(array.len().try_into().unwrap());

The problem with this is that test is performed even in optimized builds, unlike the usual Rust behaviour of performing overflow tests in debug builds only (or on request).

@estebank
Copy link
Contributor

estebank commented May 3, 2019

@leonardo-m that is true, but the aim here is to unblock newcomers who would be stumped by this error and the only two options available at this point to suggest are as and try_into. Anyone with enough knowledge for performance to matter will be able to find out about casting in other ways.

Also, shouldn't we be able to elide the performance hit of turning an usize into a u32 in the generated code when it would be a noop?

@bbugh
Copy link

bbugh commented Aug 12, 2019

Hi, new to rust and ran into this with a vec!. The given advice n.try_into().unwrap() then warns about:

no method named `try_into` found for type `u32` in the current scope

help: items from traits can only be used if the trait is in scope

If the given advice requires something extra to be enabled, I think it would be helpful to add that to the comment. I have no idea how to proceed (not asking for help, just giving message feedback).

@estebank
Copy link
Contributor

estebank commented Aug 12, 2019

What version of rustc are you using? You should be seeing the following:

error[E0599]: no method named `try_into` found for type `usize` in the current scope
 --> src/main.rs:3:22
  |
3 |     test(array.len().try_into().unwrap());
  |                      ^^^^^^^^
  |
  = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope, perhaps add a `use` for it:
  |
1 | use std::convert::TryInto;
  |

@bbugh
Copy link

bbugh commented Aug 12, 2019

Thanks! It looks like the message in Visual Studio Code is truncated:

image

but the compilation is correctly reporting the full message as you said:

error[E0599]: no method named `try_into` found for type `u32` in the current scope
 --> src/lib.rs:2:37
  |
2 |   let mut sieve = vec![true; number.try_into().unwrap()];
  |                                     ^^^^^^^^
  |
  = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope, perhaps add a `use` for it:
  |
1 | use std::convert::TryInto;
  |

I'll look into reporting this to the rust vscode extension. Thank you and sorry for the confusion!

@follower
Copy link
Contributor

Is there a "policy" around whether or not suggestions like this should include the required use invocation too:

help: you can convert an `usize` to `u32` or panic if it the converted value wouldn't fit
  |
3 |     test(array.len().try_into().unwrap());
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

As an early-stage Rust programmer who encountered this issue, it seems a more friendly experience would be to include the use std::convert::TryInto; in the first help message rather than requiring me to make the suggested change & have compilation fail with an error related to something the compiler itself suggested.

(Bonus points: only including the use invocation if it's not already present in the correct location. :) )

@estebank
Copy link
Contributor

@follower many times the error being emitted is in a stage that lacks the necessary context to provide a 100% valid suggestion. As an informal policy, if applying a suggestion gives you a follow up suggestion(s) that if followed land you in working code likely to have been the original intent, that is considered acceptable. Because of this, I feel that the current state is good enough, but do feel free to file a new ticket for this specific case. I can't promise it will be tackled anytime soon, but it is almost assured we won't do anything about it ever if it's not in our backlog :)

@follower
Copy link
Contributor

Thanks for the response & suggestion.

I created #77270 because I'd hate to think that anyone might run out of things to do after the other *checks notes* 5,952 issues are closed. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

No branches or pull requests