Skip to content

Iterator invalidation not caught by borrow-checker #20232

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

Closed
Boddlnagg opened this issue Dec 26, 2014 · 9 comments
Closed

Iterator invalidation not caught by borrow-checker #20232

Boddlnagg opened this issue Dec 26, 2014 · 9 comments
Assignees

Comments

@Boddlnagg
Copy link
Contributor

In the following reduced code the call to do_bad_thing invalidates the iterator in main.

fn do_bad_thing(grid: &mut Vec<Vec<u8>>) {
    grid[0] = vec![];
}

fn main() {
    let mut v: Vec<Vec<u8>> = vec![vec![1,2,3,4,5]];
    for i in v[0].iter() {
        println!("i = {}", i);
        do_bad_thing(&mut v);
    }
}

Running this locally produces (without optimizations):

i = 1
i = 2
i = 32
i = 0
i = 61

On playpen this does not give wrong results, but it does compile (which is the actual problem).

I'm running rustc on Windows:

rustc --version
rustc 0.13.0-nightly (7e11b2271 2014-12-24 20:47:12 +0000)
@pythonesque
Copy link
Contributor

(1) I can replicate the same issue (at least, I'm pretty sure it's the same issue) in Rust 0.12.

(2) No unsafe code need be involved.

(3) It appears to be a combined problem with the interaction of Deref and Index (it's possible that this testcase can be reduced further, but it seems like both are necessary to demonstrate the problem).

Code below (the #![feature(if_let)] is for Rust 0.12).


#![feature(if_let)]

struct Foo<T> { x: T }

impl<T> Index<uint,T> for Foo<T> {
    fn index(&self, _: &uint) -> &T {
        &self.x
    }
}

struct Bar<T> { x: T }

impl<T> Deref<T> for Bar<T> {
    fn deref(&self) -> &T {
        &self.x
    }
}

struct Baz<T> { x: T }

impl<T> Baz<T> {
    fn foo(&self) -> &T {
        &self.x
    }
}

fn main() {
    let u = 1u8;
    let mut v = Foo { x: Bar { x: Baz { x: Some(&u) } } };
    let i = v[0].foo();
    if let Some(ref i) = *i {
        v.x.x.x = None;
        println!("i = {}", i);
    }
}

@nikomatsakis
Copy link
Contributor

I'll check it out (if @eddyb doens't figure it out first!)

@eddyb
Copy link
Member

eddyb commented Dec 30, 2014

What's very interesting is that (v[0]).foo() does cause the error - my initial guess was that overloaded indexing requires adding adjustments on the v[0] expression, and the overloaded autoderef required to call .foo also needs to add adjustments to v[0], and having an intermediary expression keeps them separate.
The solution there would be to add adjustments instead of replacing them - but! - I can't find a place where ExprIndex causes adjustments to be inserted (i.e. the dereference in *v.index(&0) is implicit), which kinda makes sense, however, that invalidates my hypothesis.
There might still be such a conflict - we could debug by changing, e.g. write_adjustment, to assert that there was nothing in the map that is being replaced.
Actually, please make that happen always, it's a such a nasty bug to hide.

@nikomatsakis
Copy link
Contributor

This may be a bug in the for loop handling code. When I do various transforms to the code, such as storing the iterator in a local, I get the expected error messages.

@nikomatsakis nikomatsakis self-assigned this Jan 4, 2015
@eddyb
Copy link
Member

eddyb commented Jan 4, 2015

@nikomatsakis I found some of those a long while ago (#17068), guess there's more?
Wait, no the test case above has no for loops at all!

@pythonesque
Copy link
Contributor

@nikomatsakis It's not related to for.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 8, 2015
… implicit

deref that is associated with an overloaded index, we should not
consult the method lookup table. This deref is *always* a deref of an
`&T` and hence is never overloaded (and is also not present in the
tables; it has no "id" or other associated key).
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 8, 2015
@nikomatsakis
Copy link
Contributor

Fix pending.

@nikomatsakis
Copy link
Contributor

See PR #20751

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 8, 2015
@alexcrichton
Copy link
Member

Fixed in #20751

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants