Broken code after 2017-09-30 nightly

[pedrohjordao]

The following code was working until the 2017-09-30 nightly:

Playground

Maybe it's catching something I'm not seeing. If that's the case maybe someone could explain it to me?

[pedrohjordao]

Notice that if I change the inner try_from call and call the function

fn a_function<'a, R: io::Read>(_: &'a mut EventReader<R>) {}

everything is fine. The way I understand both calls should work the same way.

[pedrohjordao]

Changing a_function to

fn a_function<'a, R: io::Read>(value: &'a mut EventReader<R>) -> Result<Other, ()> {
    Other::try_from(value)
}

makes everything work. So I believe this is actually a bug.

[nikomatsakis]

Most likely caused by #43880

[nikomatsakis]

I think what is happening is that we are no longer inserting the auto-coercion because of the inner details of how method calls are working after #43880 -- i.e., we used to effectively pass &mut *value in the compiler.

@pedrohjordao if you rewrite to <Other as TryFrom<&'a mut EventReader<R>>::try_from(value), that will work too, right?

[nikomatsakis]

triage: P-high

Calling P-high -- we should decide if we plan to fix. cc @arielb1

[pedrohjordao]

@nikomatsakis huum, I think I get the reasoning behind what you're saying about the change, but I do hope you decide to fix it or make a really nice compiler error pointing to the correct call.

Anyway, it might be my sleepy brain making me confuse, but there might still be something wrong. What you said still doesn't work:

https://play.rust-lang.org/?gist=5d5fc92da3270bf04736f943e8f236a5&version=undefined

[arielb1]

@pedrohjordao

That's because you need <Other as TryFrom<&mut EventReader<R>>>::try_from(value), without the 'a.

[arielb1]

I can't get this code as written to compile with any version of rustc.

[pedrohjordao]

Are you sure? Not even the nightly I said? Em qui, 2 de nov de 2017 19:38, Ariel Ben-Yehuda <[email protected]> escreveu:

I can't get this code as written to compile with *any* version of rustc. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFkDZMOaatqwPWvdFA3HDsPD5JgOtBYEks5syjZYgaJpZM4Pyx9Z> .

-- Att. Pedro Jordão

[arielb1]

@pedrohjordao

I tried again and it worked. I'm not sure what was the problem. It was me using nightlies around #43880, from August 30 with TryFrom copied over instead of nightlies from September 30.

[arielb1]

And looking again at the set of commits , I think #44174 (Add blanket TryFrom impl when From is implemented) is actually the PR to blame - it actually landed on Sep 30 (unlike #43880, which landed on Aug 30) and locally adding/removing the impl it added reproduces the problem.

[arielb1]

Minimal example (you can use the actual TryFrom instead of the one implemented here).

struct Test;

struct EventReader<R>(R);

impl<'a, R> TryFrom<&'a mut EventReader<R>> for Test {
    type Error = ();

    fn try_from(value: &'a mut EventReader<R>) -> Result<Self, Self::Error> {
        Other::try_from(value);
        Other::try_from(value);
        Ok(Test {})
    }
}

struct Other;

pub trait TryFrom<T>: Sized {
    /// The type returned in the event of a conversion error.
    type Error;

    /// Performs the conversion.
    fn try_from(value: T) -> Result<Self, Self::Error>;
}

impl<'a, R> TryFrom<&'a mut EventReader<R>> for Other {
    type Error = ();

    fn try_from(value: &'a mut EventReader<R>) -> Result<Self, Self::Error> {
        unimplemented!()
    }
}

pub enum Infallible {}

#[cfg(error)]
impl<T, U> TryFrom<U> for T where T: From<U> {
    type Error = Infallible;

    fn try_from(value: U) -> Result<Self, Self::Error> {
        Ok(T::from(value))
    }
}

#[cfg(not(error))]
impl<'a, T> TryFrom<&'a str> for T where T: std::str::FromStr
{
    type Error = <T as std::str::FromStr>::Err;

    fn try_from(s: &'a str) -> Result<T, Self::Error> {
        std::str::FromStr::from_str(s)
    }
}

fn main() {}

[arielb1]

This looks like a change in an unstable library interface, let's see T-libs opinion on this.

[arielb1]

cc @rust-lang/libs

[arielb1]

@pedrohjordao

Possible fixes are

1. <Other as TryFrom<&mut _>>::try_from(value)
2. Other::try_from(&mut *value)

[pedrohjordao]

Is this final? I get why these solutions would fix the error, but I think the way it worked before is just more intuitive. Also, if it is final, could the compiler give a better message? Em qui, 2 de nov de 2017 20:11, Ariel Ben-Yehuda <[email protected]> escreveu:

@pedrohjordao <https://github.com/pedrohjordao> Possible fixes are 1. <Other as TryFrom<&mut _>>::try_from(value) 2. Other::try_from(&mut *value) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFkDZHjZhNsCI4m0nVe3vvqDkD-J8Xc9ks5syj39gaJpZM4Pyx9Z> .

-- Att. Pedro Jordão

[arielb1]

@pedrohjordao

This is not a regression in the compiler but rather a new impl that was added to TryFrom, therefore there's no simple compiler fix we can write to make this work.

I don't think we'll revert the impl we added to fix this use-case - it's an unstable trait and therefore changes are to be expected, and even if it was stable we very rarely revert new impls because of inference difficulties such as this.

However, I think that NLL (technically, MIR borrowck) will allow us to unify borrows and moves of mutable references and therefore to avoid this problem, so it is not final in that sense. But don't count on that.

[pedrohjordao]

Alright, that's good enough for me. Em qui, 2 de nov de 2017 às 20:29, Ariel Ben-Yehuda < [email protected]> escreveu:

@pedrohjordao <https://github.com/pedrohjordao> This is not a regression in the compiler but rather a new impl that was added to TryFrom, therefore there's no simple compiler fix we can write to make this work. I don't think we'll revert the impl we added to fix this use-case - it's an unstable trait and therefore changes are to be expected, and even if it was stable we *very rarely* revert new impls because of inference difficulties such as this. However, I think that NLL (technically, MIR borrowck) will allow us to unify borrows and moves of mutable references and therefore to avoid this problem, so it is not "final* in that sense. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFkDZIgHoqMX4-898BC-szwGcd_kN9oZks5sykJBgaJpZM4Pyx9Z> .

-- Att. Pedro Jordão

[pnkfelix]

Since @arielb1 brought up MIR-borrowck, let's quickly CC #43234

[ljedrz]

The minimal example compiles fine on current stable.

[pnkfelix]

The aforementioned "minimal example" used cfg blocks to include both working and non-working variants.

Here's a modified version that makes the non-working one the default, rather than vice versa: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=170ae503a6bd8d2cda4081b65fba6e3c

AFAICT, the non-working one still errors.

I haven't read the thread carefully yet, but I think the intention of the "minimal example" as written is that the non-working one should in fact work...

[pnkfelix]

Looking at this (after a long time of it lying fallow), I'm not sure if the "minimal example" that @arielb1 provided above is actually illustrating the bug described here, because (I think) the error from ariel's example happens in every Rust version going back to version 1.0.6

• (Or maybe that is ariel's whole point: Maybe the minimal example being provided there is just showing how this combination of traits and method calls is going to cause an error. That is, maybe it is not a minimal example showing where a regression in behavior was injected; which would mean my interpretation of how it was meant to be used in my previous comment was wrong.)

I'm going to try to go back to the original example filed on this bug to double-check what the story is.

[pnkfelix]

Okay, when I look at the original example from the bug, I get the following diagnostic:

error[E0382]: use of moved value: `value`
  --> src/main.rs:22:37
   |
22 |                     Other::try_from(value);
   |                                     ^^^^^ value moved here in previous iteration of loop
   |
   = note: move occurs because `value` has type `&'a mut xml::EventReader<R>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `*value`
  --> src/main.rs:27:20
   |
22 |                     Other::try_from(value);
   |                                     ----- value moved here
...
27 |             next = value.next();
   |                    ^^^^^ value used here after move
   |
   = note: move occurs because `value` has type `&'a mut xml::EventReader<R>`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

As @nikomatsakis and @arielb1 have previously stated, we are not likely to attempt to start re-inserting the auto-reborrows in this case like we used to before #43880.

However, I find it interesting that niko and ariel were initially suggesting this work-around: <Other as TryFrom<&mut EventReader<R>>>::try_from(value), (which if I understand correctly is a way to force the type-inference engine to know that a &mut is being passed there before it works out method-resolution). The reason I find it interesting is that I find that work-around to be a rather wordy way to ask the compiler to inject the auto-reborrow.

But it seems much more concise to just inject the reborrow ourselves into the source, which was ariel's second suggestion above: Option::try_from(&mut *value)

Furthermore, it seems entirely doable for us to extend the diagnostic above to include a suggestion to try doing just that, as a way to avoid moving a &mut-value into a context that is itself expecting an &mut value.

I'm going to file a new bug (with a reduced example) requesting that change to the diangostic. After that's been filed, I'll close this as not-a-bug.

[pnkfelix]

Filed issue #62112 for extending the diagnostic.

Closing this as not-a-bug.