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

fallback for diverging expressions leaks into the `?` operator #39297

Open
nikomatsakis opened this issue Jan 25, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@nikomatsakis
Copy link
Contributor

commented Jan 25, 2017

This curious example was found by @canndrew:

trait Deserialize: Sized {
    fn deserialize() -> Result<Self, String>;
}

impl Deserialize for i32 {
    fn deserialize() -> Result<i32, String> {
        Ok(22)
    }
}

fn doit() -> Result<(), String> {
    let _ = <_ as Deserialize>::deserialize()?;
    Ok(())
}

fn main() {
    let _ = doit();
}

This fails to compile. In particular, the _ is inferred to () (at present) rather than i32. This is because of the interaction of two things:

  • impl selection refuses to infer the Self type, for some sort of arbitrary reason;
  • the ? desugars into a match where one of the arms has a return; the type of this return thus has a diverging default.

Since there are no other constraints on the type of _, this winds up defaulting to (). Once the never-type work completes, it will default to !.

It's not entirely clear that this is a bug -- each part sort of makes sense -- but the result is pretty confounding. Some of the work on improving the trait system I've been doing would lead to this example compiling, because the _ would be inferred to i32.

Note that there are variants of this which do compile (because of the fallback to ()) -- i.e., if you changed the impl to be implemented for (). In this case, changing the fallback (to !) without improving the trait system's inference leads to a regression, since we fail to infer that () was the right answer all along.

I think that improving the trait system's inference does not lead to any breakage (since the default never kicks in). The basic reasoning is that, if the code compiled with a defualt before, but now compiles with improved inference, then the trait system must infer the same thing as the default, since otherwise there'd be ambiguity and it should not have done any inference (put another way, if it found another answer, then the default should have led to a compilation error).

That said, I think we should stop desugaring ? when we lower to HIR, and instead do it when we lower to MIR. This would be helpful for implementing catch, and would also give us more control over how the typing works. I think it's quite surprising the way the "divergence" is hidden in this example.

cc @eddyb @aturon, with whom I've discussed related issues

@canndrew

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2017

I think that improving the trait system's inference does not lead to any breakage (since the default never kicks in). The basic reasoning is that, if the code compiled with a defualt before, but now compiles with improved inference, then the trait system must infer the same thing as the default, since otherwise there'd be ambiguity and it should not have done any inference (put another way, if it found another answer, then the default should have led to a compilation error).

Maybe I'm not sure what you're saying here but as far as I can tell there'll still be some breakage. What about this for instance:

trait Deserialize: Sized {
    fn deserialize() -> Result<Self, String>;
}

impl Deserialize for () {
    fn deserialize() -> Result<(), String> {
        Ok(())
    }
}

impl Deserialize for ! {
    fn deserialize() -> Result<!, String> {
        Err("Failed to deserialize a `!`")
    }
}

fn doit() -> Result<(), String> {
    let _ = <_ as Deserialize>::deserialize()?;
    Ok(())
}

fn main() {
    let _ = doit();
}

Pre-feature(never_type) the type of _ will get inferred to () and the code compiles. Post-feature(never_type) it gets inferred to ! and the code still compiles - but it does something different at run time. This is one thing that the new lint checks for. Do you mean for this to become an error?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

So @eddyb and I were talking on IRC and we had come to the conclusion that something wacky is going in the LUB code. In particular, if this type-checks (and it does):

fn main() {
  let x = if true { 22 } else { return; 'a' };
}

then we ought not to be considering the type of diverging arm at all.

(Oddly, that code fails if you add let x: i32, which I also consider a bug.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

@eddyb notes that we could remove these lines to correct the behavior with an explicit type annotation.

@canndrew

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

then we ought not to be considering the type of diverging arm at all.

I'm pretty sure the whole else block will be typed ! there won't it? So we're still unifying the type that the ! coerces into with the type of 22.

canndrew added a commit to canndrew/rust that referenced this issue Feb 3, 2017

@canndrew

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

@eddyb notes that we could remove these lines ...

I've made the change and tested it.

Note that these compile-fail tests now compile (not that I see a problem with that):

fn main() {
    &panic!()
}
fn f() -> isize {
    (return 1, return 2)
}

fn main {}

canndrew added a commit to canndrew/rust that referenced this issue Feb 9, 2017

bors added a commit that referenced this issue Feb 16, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)

bors added a commit that referenced this issue Feb 17, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)

bors added a commit that referenced this issue Feb 17, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.