rust icon indicating copy to clipboard operation
rust copied to clipboard

When `.into` used and it doesn't exist but `.try_into()` avaiable, suggest using `try_into()`

Open LeSnake04 opened this issue 3 years ago • 1 comments

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f625bf004abe28d4c42c3c256d8f2aa2

struct Foo {
    bar: u32,
}

#[derive(Debug, Clone)]
struct Error;

impl std::fmt::Display for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str("Number must be positive")
    }
}

impl TryFrom<i32> for Foo {
    type Error = Error;

    fn try_from(value: i32) -> Result<Self, Self::Error> {
        if value < 0 {
            Err(Error)
        } else {
            Ok(Self { bar: value as u32 })
        }
    }
}

fn main() {
    let foobar: Foo = 4.into();
}

The current output is:

error[[E0277]](https://doc.rust-lang.org/stable/error-index.html#E0277): the trait bound `Foo: From<{integer}>` is not satisfied
  --> src/main.rs:27:25
   |
27 |     let foobar: Foo = 4.into();
   |                         ^^^^ the trait `From<{integer}>` is not implemented for `Foo`
   |
   = note: required because of the requirements on the impl of `Into<Foo>` for `{integer}`

For more information about this error, try `rustc --explain E0277`.

Ideally the output should look like:

error[[E0277]](https://doc.rust-lang.org/stable/error-index.html#E0277): the trait bound `Foo: From<{integer}>` is not satisfied
  --> src/main.rs:27:25
   |
27 |     let foobar: Foo = 4.into();
   |                         ^^^^ the trait `From<{integer}>` is not implemented for `Foo`
   |
   = note: required because of the requirements on the impl of `Into<Foo>` for `{integer}`
   = note: `TryFrom<{integer}>` is implemented. Try using `4.try_into()` instead

For more information about this error, try `rustc --explain E0277`.

When an users uses .into() and it doesnt work, they might do an complicated workaround while not realising the libray devs implemented TryFrom<T> instead. So it would be cool if the compiler could check if the struct implements TryFrom instead and suggest try_into() (So it can be automatically fixed by lsp)

It should work the other way around (.try_into() used but .into() avaiable) and Foo::from()/Foo::try_from()

LeSnake04 avatar Oct 17 '22 13:10 LeSnake04

For the code, even compiler suggest use try_into(), the type is also need to be updated: something like:

let foobar: Result<Foo, _> = 4.try_into();

The message is given for Foo because the variable is with a Type of Foo.

chenyukang avatar Oct 17 '22 16:10 chenyukang

@chenyukang

For the code, even compiler suggest use try_into(), the type is also need to be updated: something like:

let foobar: Result<Foo, _> = 4.try_into();

The message is given for Foo because the variable is with a Type of Foo.

I am not sure if and how the other lints implicitly do do error handling when changing T into Result<T, E>.

I personally would just change the call and let the lint of the resulting error do their job and let the user decide how to handle it themselve. I would dislike the compiler also changing the type in this case because I usally prefer returning it or handling the error on the same line instead of storing the result. I can do the error handling myself but still want the lsp to replace the function called.

The "handling the error" step should be a different lint offering multiple ways to handle potential errors.

I tried what happens when doing 1: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=71134db81b153d53739e490e74c0b761 2: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=ee2b35e01438ebc2dd070ca9cf7e509c and got no tip how to handle the Situation. I think there should be suggestions like handling the error in different ways or changing the type. But I would say that should be an seperate discussion.

LeSnake04 avatar Oct 17 '22 21:10 LeSnake04

I guess the problem is. rustc should write you a note of try_into to Result<T, E> if you're writing this

let foo: Result<T, E> = bar.into();

If it is i want to claim it.

TheAwiteb avatar Oct 18 '22 16:10 TheAwiteb

I think it should also work if you don't take a result.

Also, I did a minimal example. Normally you would use something like this: (Consider foo a struct that implements TryInto<f32> but not Into<f32>)

let a: f32 = foo.into()  / 2.0

should be corrected to

let a: f32 = foo.try_into() / 2.0

If you want to handle the error as well, it should rather become this:

let a: 32 = foo.try_into().unwrap() / 2.0

EDIT: Made the example more clear.

LeSnake04 avatar Oct 18 '22 16:10 LeSnake04

Yes i see, something like

impl<T, E, U> const From<T> for Result<U, E>
where
    U: ~const TryFrom<T, Error = E>,
{
    fn from(t: T) -> Self {
        U::try_from(t)
    }
}

in the core.

Can we do this?

TheAwiteb avatar Oct 18 '22 17:10 TheAwiteb

@rustbot claim

TheAwiteb avatar Oct 18 '22 17:10 TheAwiteb

.into() being an alias to .try_from().unwrap() would also be quite useful. I also had that idea. Maybe its best best to do that and show a warning that the call was aliased and that the user might consider switching to .try_from() in order to handle errors properly.

LeSnake04 avatar Oct 18 '22 17:10 LeSnake04

No, i think we cannot use unwrap It's dangerous

TheAwiteb avatar Oct 18 '22 17:10 TheAwiteb

Yes we can't image

I think add a note to use try_into will be clear

TheAwiteb avatar Oct 18 '22 18:10 TheAwiteb

@rustbot release-assignment

TheAwiteb avatar Oct 18 '22 19:10 TheAwiteb

I personally agree that the error message shouldn't suggest to change the type of the result, a note saying that TryFrom (or From) is implemented and suggesting using .try_into() (or .into()) is sufficient because otherwise there are other questions to answer like what to do when the result type is already a Result or what to suggest if it's in an expression composed of other things, etc.

Also I'd be down trying to implement that

Absobel avatar Aug 20 '23 12:08 Absobel

@rustbot claim

Absobel avatar Aug 20 '23 12:08 Absobel