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

cannot borrow as mutable because it is also borrowed as immutable (likely regression) #60136

Closed
Dylan-DPC-zz opened this issue Apr 20, 2019 · 8 comments
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Dylan-DPC-zz
Copy link

From a crater run:

 Compiling web-dom v0.0.8
[INFO] [stderr]    Compiling webcomponent v0.3.3 (/opt/crater/workdir)
[INFO] [stderr] error[E0502]: cannot borrow `*controller` as mutable because it is also borrowed as immutable
[INFO] [stderr]   --> src/lib.rs:27:13
[INFO] [stderr]    |
[INFO] [stderr] 27 |             (controller.processor)(controller, &tag, el);
[INFO] [stderr]    |             ----------------------^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stderr]    |             |
[INFO] [stderr]    |             mutable borrow occurs here
[INFO] [stderr]    |             immutable borrow occurs here
[INFO] [stderr]    |             immutable borrow later used by call
[INFO] [stderr] 
[INFO] [stderr] error: aborting due to previous error

crate(s) affected: webcomponent (0.3)
author: @richardanaya
log: https://crater-reports.s3.amazonaws.com/beta-1.35-1/beta-2019-04-11/reg/webcomponent-0.3.3/log.txt

@Dylan-DPC-zz Dylan-DPC-zz added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Apr 20, 2019
@matthewjasper
Copy link
Contributor

cc #59500

This can be fixed by using (*controller.processor) to avoid the new implementation in that PR.

@pietroalbini pietroalbini added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 23, 2019
@nagisa nagisa added P-high High priority and removed I-nominated labels Apr 25, 2019
@nikomatsakis
Copy link
Contributor

@matthewjasper can you elaborate a bit on how #59500 is related here?

@nagisa
Copy link
Member

nagisa commented Apr 25, 2019

Would be great to have a bisection and/or reduction of the reproducer. Cursory look at the contents of the web-dom crate indicates that it should be fairly easy to make a reproducer here.

@lqd
Copy link
Member

lqd commented Apr 25, 2019

Reduction: playground

struct CustomElements {
    processor: Box<fn(&mut CustomElements)>,
}

fn main() {
    move |controller: &mut CustomElements| {
        (controller.processor)(controller);
    };
}

edit Bisection points to nightly-2019-04-06 and acd8dd6 which is indeed #59500

@nikomatsakis
Copy link
Contributor

OK, wait, I didn't read #59500 closely enough. I can see why this regression occurred. We didn't used to consider Box<fn(...)> to implement Fn, but we do now (because of the impls added in #59500). This means that (controller.processor) winds up being desugared as a call through Fn to the Box<..>, instead of inserting an auto-deref. This is the sort of breakage we typically accept (for better or worse) as a result of adding new impls.

@nikomatsakis
Copy link
Contributor

The workaround is to change to (*controller.processor), although I suspect the better fix is to remove the Box altogether and just store the fn(&mut CustomElements) directly. =) (but maybe there's another reason to have the Box)

@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

Closing, as the code breakage described here was an expected consequence of teh impl that were added in PR #59500.

@pnkfelix pnkfelix closed this as completed May 2, 2019
@nikomatsakis
Copy link
Contributor

Discussed at the compiler meeting today. We are closing this as a permissible regression -- as I noted earlier, adding the impl caused the () operator to insert fewer dereferences than is used to, which leads to a compiler error. This is unfortunate but it is the sort of breakage that we current permit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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