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

Error on deriving PartialEq on Foo and then implementing it for dyn Foo #78808

Open
frostblooded opened this issue Nov 6, 2020 · 6 comments
Open
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@frostblooded
Copy link

I tried this code:

trait Foo {}

impl PartialEq for dyn Foo {
    fn eq (self: &'_ Self, _: &'_ Self) -> bool { false }
}

#[derive(PartialEq)]
struct Bar {
    __: ::std::rc::Rc<dyn Foo>,
}
fn main () {}

I expected to see this happen:

A working PartialEq derived implementation for Bar.

Instead, this happened:
An error was thrown:

error[E0507]: cannot move out of `*__self_1_0` which is behind a shared reference
 --> src/main.rs:9:5
  |
9 |     __: ::std::rc::Rc<dyn Foo>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because `*__self_1_0` has type `Rc<dyn Foo>`, which does not implement the `Copy` trait
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0507`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

Meta

The bug is present on nightly and stable in the two versions I tried:
rustc --version --verbose:

rustc 1.49.0-nightly (ffa2e7ae8 2020-10-24)
binary: rustc
commit-hash: ffa2e7ae8fbf9badc035740db949b9dae271c29f
commit-date: 2020-10-24
host: x86_64-pc-windows-msvc
release: 1.49.0-nightly
LLVM version: 11.0

rustc --version --verbose:

rustc 1.46.0 (04488afe3 2020-08-24)
binary: rustc
commit-hash: 04488afe34512aa4c33566eb16d8c912a3ae04f9
commit-date: 2020-08-24
host: x86_64-pc-windows-msvc
release: 1.46.0
LLVM version: 10.0

Backtrace doesn't show anything different than the error.
Here is the reproducable code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a4d7bd689ed0687e24533f956f7a5971
@danielhenrymantilla seemed interested in the issue.

@frostblooded frostblooded added the C-bug Category: This is a bug. label Nov 6, 2020
@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Nov 6, 2020
@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Nov 6, 2020

I have minimized the problem a bit more:

trait T {}
impl PartialEq for dyn T + '_ {
    fn eq (self: &'_ Self, _: &'_ Self) -> bool
    {
        loop {}
    }
}

fn _check (it: &'_ Box<dyn T>)
{
    *it == *it;
}

fails with the same error message:

error[E0507]: cannot move out of `*it` which is behind a shared reference
  --> src/main.rs:11:12
   |
11 |     *it == *it;
   |            ^^^ move occurs because `*it` has type `Box<dyn T>`, which does not implement the `Copy` trait

error: aborting due to previous error

So, either that is a valid error, and the macro should be unsugaring to PartialEq::eq(& <left>, & <right>) instead of <left> == <right>, or that is the actual bug, and the derive is fine since it assumes such code to be always valid.

@rustbot rustbot added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html and removed A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Nov 6, 2020
@danielhenrymantilla
Copy link
Contributor

FWIW, I have implemented a fix for the "the macro should be unsugaring to …" case, although I think that it's rather == *it not working that should be fixed:

diff --git a/compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs b/compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs
index 8e9f15743cc..faaba4bf4ef 100644
--- a/compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs
+++ b/compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs
@@ -5,7 +5,7 @@ use crate::deriving::{path_local, path_std};
 use rustc_ast::ptr::P;
 use rustc_ast::{BinOpKind, Expr, MetaItem};
 use rustc_expand::base::{Annotatable, ExtCtxt};
-use rustc_span::symbol::sym;
+use rustc_span::symbol::{sym, Symbol};
 use rustc_span::Span;
 
 pub fn expand_deriving_partial_eq(
@@ -21,7 +21,7 @@ pub fn expand_deriving_partial_eq(
         cx: &mut ExtCtxt<'_>,
         span: Span,
         substr: &Substructure<'_>,
-        op: BinOpKind,
+        op: Symbol,
         combiner: BinOpKind,
         base: bool,
     ) -> P<Expr> {
@@ -31,7 +31,15 @@ pub fn expand_deriving_partial_eq(
                 _ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialEq)`"),
             };
 
-            cx.expr_binary(span, op, self_f, other_f.clone())
+            // Instead of `*at_field OP *at_other_field`, do:
+            // `::core::cmp::PartialEq::op(&*at_field, &*at_other_field)`
+            // so as to avoid a bug when `*at_field` is of type
+            // `impl !Copy + Deref<Target = impl !Sized + PartialEq>` (_e.g._,
+            // `Box<dyn Trait>` where `dyn Trait : PartialEq`. See #78808).
+            let self_f_ref = cx.expr_addr_of(span, self_f);
+            let other_f_ref = cx.expr_addr_of(span, other_f.clone());
+            let partial_eq_op = cx.std_path(&[sym::cmp, sym::PartialEq, op]);
+            cx.expr_call_global(span, partial_eq_op, vec![self_f_ref, other_f_ref])
         };
 
         cs_fold1(
@@ -57,10 +65,10 @@ pub fn expand_deriving_partial_eq(
     }
 
     fn cs_eq(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
-        cs_op(cx, span, substr, BinOpKind::Eq, BinOpKind::And, true)
+        cs_op(cx, span, substr, sym::eq, BinOpKind::And, true)
     }
     fn cs_ne(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
-        cs_op(cx, span, substr, BinOpKind::Ne, BinOpKind::Or, false)
+        cs_op(cx, span, substr, sym::ne, BinOpKind::Or, false)
     }
 
     macro_rules! md {

@RustyYato
Copy link
Contributor

I ran into the same bug today

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@Dylan-DPC
Copy link
Member

Current output:

error[[E0507]](https://doc.rust-lang.org/stable/error_codes/E0507.html): cannot move out of `other.__` which is behind a shared reference
 --> src/main.rs:9:5
  |
7 | #[derive(PartialEq)]
  |          --------- in this derive macro expansion
8 | struct Bar {
9 |     __: ::std::rc::Rc<dyn Foo>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because `other.__` has type `Rc<dyn Foo>`, which does not implement the `Copy` trait

acl-cqc added a commit to CQCL/hugr that referenced this issue Jul 31, 2023
Add extra tuple to workaround rust-lang/rust#78808

Rename CustomConst::(eq => equal_consts) to disambiguate
github-merge-queue bot pushed a commit to CQCL/hugr that referenced this issue Jul 31, 2023
Add extra tuple to workaround rust-lang/rust#78808

Rename CustomConst::(eq => equal_consts) to disambiguate
@dnut
Copy link

dnut commented Aug 3, 2023

I ran into the same problem today, and used this workaround. It's not pretty, but it's easy. Double wrap the trait object.

This:

#[derive(PartialEq)]
pub struct Foo(Box<dyn MyTrait>);

... becomes this:

#[derive(PartialEq)]
pub struct Foo(Box<Box<dyn MyTrait>>);

It should work with any combination of smart pointers, not just Box and Box.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Aug 3, 2023

@dnut FWIW a less expensive workaround:

trait Trait {}

impl PartialEq for dyn '_ + Trait { ... }

#[derive(PartialEq)]
struct Foo<DynTrait : ?Sized + PartialEq = dyn Trait>(
    Box<DynTrait>,
);
  • (or just manually implement PartialEq)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants