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

Using std::borrow::Borrow has sneaky consequences #66630

Open
joIivier opened this issue Nov 22, 2019 · 3 comments
Open

Using std::borrow::Borrow has sneaky consequences #66630

joIivier opened this issue Nov 22, 2019 · 3 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@joIivier
Copy link

joIivier commented Nov 22, 2019

Consider the following playground that compiles fine:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d735c0950cb1b25e8ff19e6a5c9d0445

use std::sync::Mutex;
use std::cell::RefCell;
//use std::borrow::Borrow;

struct A {
    child: Mutex<RefCell<i32>>,
}

impl A {
    fn is_good(&self) -> bool {
        let cell = self.child.lock().unwrap();
        let opt_target = cell.borrow();
        *opt_target > 1
    }
}



Uncommenting the use std::borrow::Borrow; breaks it quite badly:

error[E0369]: binary operation `>` cannot be applied to type `std::sync::MutexGuard<'_, std::cell::RefCell<i32>>`
  --> src/lib.rs:13:21
   |
13 |         *opt_target > 1
   |         ----------- ^ - {integer}
   |         |
   |         std::sync::MutexGuard<'_, std::cell::RefCell<i32>>
   |
   = note: an implementation of `std::cmp::PartialOrd` might be missing for `std::sync::MutexGuard<'_, std::cell::RefCell<i32>>`

This problem does not appear when there is no RefCell inside the Mutex.
This creates confusion when the same function works in a module and not in another.
Especially when Borrow is not implemented directly by Mutex or MutexGuard it is quite hard to understand what happens here, I guess it is due to a blanket implementation since RefCell<i32> is Sized.

My main issue in this situation is that I spent a lot of time facing this compilation issue in a module whereas the same kind of code worked in multiple other modules. I had to comment/remove every other struct/impls/use in the module until I found the culprit.
Maybe the error message could be improved to hint at traits in scope that could alter the expected behavior ?

The issue appeared on stable and nightly (rustc 1.41.0-nightly (53712f8 2019-11-21)).

@jonas-schievink jonas-schievink added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2019
@joIivier
Copy link
Author

On a side note, in my situation this was triggered by the IntelliJ rust plugin that added the use std::borrow::Borrow automatically in some situation to help me, which had clearly the opposite effect.

@csmoe csmoe added the C-bug Category: This is a bug. label Nov 22, 2019
@Veykril
Copy link
Member

Veykril commented Nov 22, 2019

To my understanding the snippet without the std::borrow::Borrow should compile, since the borrow call there is the borrow method of std::cell::RefCell which is being accessed through the deref of the MutexGuard.

@csmoe
Copy link
Member

csmoe commented Nov 22, 2019

To my understanding the snippet without the std::borrow::Borrow should compile, since the borrow call there is the borrow method of std::cell::RefCell.

yep, my bad. borrow came from RefCell's inherent method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. 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

4 participants