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

Inconsistent evaluation order for assignment operations #27868

Open
eefriedman opened this issue Aug 17, 2015 · 10 comments

Comments

Projects
None yet
10 participants
@eefriedman
Copy link
Contributor

commented Aug 17, 2015

UPDATE: This is fixed by the MIR-based borrow checker and just needs a test. See the mentoring instructions below.


Currently, the borrow checker thinks that the RHS of += is evaluated before the LHS. trans thinks that the LHS is evaluated before the RHS. The disagreement leads to bad results.

fn main() {
    let x = Box::new(0);
    let mut y = 0;
    *{ drop(x); &mut y } += *x;
    assert_eq!(y, 0);
}
@nrc

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

triage: P-medium

Also fixed by MIR (almost a dup of #28160, but not quite)/

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

@steveklabnik steveklabnik added T-lang and removed A-lang labels Mar 24, 2017

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented May 6, 2017

So the internals thread says this: "before MIR, we were actually quite inconsistent here, in that borrowck and trans disagreed on some of the particulars. MIR eliminated that inconsistency, but there are still some matters that we should try to resolve -- and quick!" If that's the case, then this should be closed. @nikomatsakis Is this the case?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

Well, we're not yet running the borrowck on MIR, so I'm not sure.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

But certainly the example seems to work ok now.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

Assignment operators are still wrong, which can lead to use-after-free:

use std::ops::AddAssign;

struct MyVec<T>(Vec<T>);

impl <T> Drop for MyVec<T> {
    fn drop(&mut self) {
        println!("Being dropped.");
    }
}

impl<T> AddAssign<T> for MyVec<T> {
    fn add_assign(&mut self, _elem: T) {
        println!("In add_assign.");
    }
}

fn main() {
    let mut vec = MyVec(vec![0]);
    let mut vecvec = vec![vec];

    vecvec[0] += {
        vecvec = vec![];
        0
    };
}

Prints

Being dropped.
In add_assign.

Credit to @xfix for bringing this up in rust-lang/rfcs#2025 (comment), I just weaponized it.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

@RalfJung's example gets an error with MIR borrowck enabled (as expected):

https://play.rust-lang.org/?gist=761478ea89b3cd1df50ee274cb7a909d&version=nightly

I am filing this under "bugs fixed by MIR borrowck" #47366

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Marking as E-needstest and E-mentor:

Since this code works now, what we need to do is add it to our test-suite. To do that, you would create a file in src/test/ui/nll named issue-27868.rs containing the example I linked to here. There are general instructions for adding new tests here.

memoryruins added a commit to memoryruins/rust that referenced this issue Aug 14, 2018

frewsxcv added a commit to frewsxcv/rust that referenced this issue Aug 17, 2018

Rollup merge of rust-lang#53326 - memoryruins:issue-27868-test, r=nik…
…omatsakis

[nll] add regression test for issue rust-lang#27868

Adds a test for rust-lang#27868 ``Inconsistent evaluation order for assignment operations``

apart of rust-lang#47366 ``tracking issue for bugs fixed by the MIR borrow checker or NLL``

r? @nikomatsakis

bors added a commit that referenced this issue Aug 17, 2018

Auto merge of #53449 - frewsxcv:rollup, r=frewsxcv
Rollup of 11 pull requests

Successful merges:

 - #52858 (Implement Iterator::size_hint for Elaborator.)
 - #53321 (Fix usage of `wasm_target_feature`)
 - #53326 ([nll] add regression test for issue #27868)
 - #53347 (rustc_resolve: don't allow paths starting with `::crate`.)
 - #53349 ([nll] add tests for #48697 and #30104)
 - #53357 (Pretty print btreemap for GDB)
 - #53358 (`{to,from}_{ne,le,be}_bytes` for unsigned integer types)
 - #53406 (Do not suggest conversion method that is already there)
 - #53407 (make more ported compile fail tests more robust w.r.t. NLL)
 - #53413 (Warn that `#![feature(rust_2018_preview)]` is implied when the edition is set to Rust 2018.)
 - #53434 (wasm: Remove --strip-debug argument to LLD)

Failed merges:

r? @ghost
@pnkfelix

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Apparently PR #53326 added the desired test here. So this is NLL-fixed-by-NLL now, I think.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

NLL (migrate mode) is enabled in all editions as of PR #59114.

The only policy question is that, under migrate mode, we only emit a warning on this (unsound) test case. Therefore, I am not 100% sure whether we should close this until that warning has been turned into a hard-error under our (still in development) future-compatibility lint policy.

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.