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

Rustc coerces smart pointer to unsized too eagerly #36007

Closed
plietar opened this issue Aug 26, 2016 · 8 comments · Fixed by #103631
Closed

Rustc coerces smart pointer to unsized too eagerly #36007

plietar opened this issue Aug 26, 2016 · 8 comments · Fixed by #103631
Assignees
Labels
A-type-system Area: Type system C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@plietar
Copy link
Contributor

plietar commented Aug 26, 2016

Full code: https://is.gd/hZ9nqK

Given a smart pointer, which only works on types implementing the trait Bar or on Bar trait objects, such that Pointer<T> is allowed to coerce to Pointer<Bar>, and the corresponding PartialEq implementation :

trait Bar {}
struct Foo;
impl Bar for Foo {}

struct Pointer<T: ?Sized>
        where T: Bar + Unsize<Bar> {

    value: *mut T,
}

impl <T: ?Sized> CoerceUnsized<Pointer<Bar>> for Pointer<T>
        where T: Bar + Unsize<Bar> {}

impl <T: ?Sized, U: ?Sized> PartialEq<Pointer<U>> for Pointer<T>
        where T: Bar + Unsize<Bar>,
              U: Bar + Unsize<Bar> {

    fn eq(&self, _other: &Pointer<U>) -> bool {
        true
    }
}

Applying the == operator on two Pointer<Foo> will try to coerce them to Pointer<Bar>, even though this isn't needed. This is an issue since coercion moves the original pointer.

fn cmp(x: Pointer<Foo>, y: Pointer<Foo>) {
    x == y;
    x == y; // use of moved value: `y`
}

Removing the CoerceUnsized implementation lets the code build, which means that the coercion is not needed.

As @Diggsey pointed out on reddit, if the PartialEq is restricted to pointers with identical types (PartialEq for Pointer<T>), then no coercion occurs.

@bluss
Copy link
Member

bluss commented Aug 26, 2016

Seems related to #31740

@abonander
Copy link
Contributor

abonander commented Jan 27, 2017

I've discovered a simpler reproduction: https://is.gd/0OpDXV

#![feature(coerce_unsized, unsize)]

use std::marker::Unsize;
use std::ops::CoerceUnsized;

struct Foo<T: ?Sized>(Box<T>);

impl<T> CoerceUnsized<Foo<Baz>> for Foo<T> where T: Unsize<Baz> {}

struct Bar;

trait Baz {}

impl Baz for Bar {}

fn main() {
    let foo = Foo(Box::new(Bar));
    let foobar: Foo<Bar> = foo;
}

Curiously, when you just do let foo: Foo<Bar> = Foo(Box::new(Bar));, there's no problem. The overeager coercion can only occur in the presence of type inference.

It also only occurs when the impl CoerceUnsized is non-parametric, i.e. not written like this:

impl<T, U: ?Sized> CoerceUnsized<Foo<U>> for Foo<T> where T: Unsize<U> {}

Likely because this impl allows Foo<Bar> to coerce to itself, hiding the issue. This also explains why it hasn't come up in the stdlib, because no type tries to coerce to a specific unsized type; it's all parametric.

@Mark-Simulacrum
Copy link
Member

The original code compiles with no errors today, so seems like E-needstest.

@Mark-Simulacrum
Copy link
Member

I think I tested the wrong thing; not E-needstest.

@Mark-Simulacrum Mark-Simulacrum added A-type-system Area: Type system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 26, 2017
@alice-i-cecile
Copy link

The linked reproduction from @abonander no longer fails to compile; is this fixed?

@compiler-errors
Copy link
Member

This is fixed but does need a test, though I think it could also be closed without a test, since it's been open for so long there's probably already coverage 🤔

@jackh726 jackh726 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 24, 2022
@jackh726
Copy link
Member

I think we should add the test just to be sure.

@Rageking8
Copy link
Contributor

@rustbot claim

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 28, 2022
…, r=compiler-errors

Add test for issue 36007

Fixes rust-lang#36007

r? `@compiler-errors`
@bors bors closed this as completed in 19b406d Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants