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

Also compare panicking behavior #4

Closed
wants to merge 3 commits into from

Conversation

Shnatsel
Copy link

@Shnatsel Shnatsel commented Mar 4, 2020

I've attempted to implement #2 but ended up in a weird lifetime hell, and could not resolve it by myself. I'm opening a WIP PR so that someone else might get a better starting point if they try to implement this.

The error is:

error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
   --> src/tests/index_map.rs:168:16
    |
168 |             fn values_mut(&mut self) -> impl Iterator<Item = &mut V>;
    |                ^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the lifetime `'_` as defined on the body at 135:1...
   --> src/tests/index_map.rs:135:1
    |
135 |   arbitrary_stateful_operations! {
    |  _-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
136 | |     model = ModelHashMap<K, V>,
137 | |     tested = IndexMap<K, V>,
138 | |
...   |
170 | |     }
171 | | }
    | |_- in this macro invocation
note: ...so that closure can access `model`
   --> src/tests/index_map.rs:135:1
    |
135 | / arbitrary_stateful_operations! {
136 | |     model = ModelHashMap<K, V>,
137 | |     tested = IndexMap<K, V>,
138 | |
...   |
170 | |     }
171 | | }
    | | ^ in this macro invocation
    | |_|
    | 
note: but, the lifetime must be valid for the call at 135:1...
   --> src/tests/index_map.rs:135:1
    |
135 | / arbitrary_stateful_operations! {
136 | |     model = ModelHashMap<K, V>,
137 | |     tested = IndexMap<K, V>,
138 | |
...   |
170 | |     }
171 | | }
    | | ^ in this macro invocation
    | |_|
    | 
note: ...so type `std::result::Result<impl std::iter::Iterator, std::boxed::Box<(dyn std::any::Any + std::marker::Send + 'static)>>` of expression is valid during the expression
   --> src/tests/index_map.rs:135:1
    |
135 | / arbitrary_stateful_operations! {
136 | |     model = ModelHashMap<K, V>,
137 | |     tested = IndexMap<K, V>,
138 | |
...   |
170 | |     }
171 | | }
    | | ^ in this macro invocation
    | |_|
    | 

@jakubadamw
Copy link
Owner

jakubadamw commented Jul 20, 2020

@Shnatsel, I'm sorry for not getting back to you about this PR. ☹️ I had looked at it briefly. The problem is the way the PR is written you're moving the instance of the structure being tested into the closure, which we can't do. I think we'd have to store it in a RefCell to make it work but I might be wrong.

@Shnatsel
Copy link
Author

After merging the latest master I'm getting a different error:

error: captured variable cannot escape `FnMut` closure body
  --> examples/src/binary_heap.rs:62:1
   |
62 | / arbitrary_stateful_operations! {
63 | |     model = ModelBinaryHeap<T>,
64 | |     tested = BinaryHeap<T>,
65 | |
...  |
82 | |     }
83 | | }
   | | ^
   | | |
   | |_inferred to be a `FnMut` closure
   |   returns a reference to a captured variable which escapes the closure body
   |
   = note: `FnMut` closures only have access to their captured variables while they are executing...
   = note: ...therefore, they cannot allow references to captured variables to escape
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

error: could not compile `rutenspitz-examples`.

I feel a strategically placed .clone() would resolve this but could't figure out how to do that yet. Any help would be appreciated as I feel I'm in over my head here.

@Shnatsel
Copy link
Author

@jakubadamw
Copy link
Owner

@Shnatsel, well, adding .cloned() to the v.get_mut(999) call does the trick. 🙂 Sorry, I of course talked gibberish about the moving, I think I may have forgotten the gist of the issue by now. 🙂 So cloning return values of all operations and moving them out of the closure is one plausible solution. It'd require adding a special trait because we can't just Clone: for Option values, for example, we need the inner value cloned (via the cloned() method, indeed). Then the're the special handling of iterators… And it'll make the whole fuzzing process slower. But it's doable.

I can take a look. 🙂

@Shnatsel
Copy link
Author

I think we can get rid of the clones if one closure is nested inside the other, something like

let model_panic_res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
    model.#method_name(#(#args),*)
    let tested_panic_res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        tested.#method_name(#(#args),*)
        // TODO: comparison code here while both references are still alive
    }));
}));

@Plecra
Copy link

Plecra commented Jul 20, 2020

You can pass a mutable reference through a catch_unwind closure like this:

fn main() {
    use std::panic;
    let mut v = vec![64u64; 64];
    let v = panic::AssertUnwindSafe(&mut v);
    let x = panic::catch_unwind(move || {
        v.0.get_mut(999)
    });
    
    println!("{:?}", x);
}

I couldn't explain why, but it seems like Rust won't treat the closure as an FnOnce otherwise. This solution should be able to be used in the implementation: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4caef01237a7c4462d08acbf6de8ec00

@Plecra Plecra mentioned this pull request Jul 21, 2020
@jakubadamw
Copy link
Owner

@Shnatsel, thanks again! ❤️ I'll close this now that #6 and #7 are merged in.

@jakubadamw jakubadamw closed this Jul 21, 2020
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

Successfully merging this pull request may close these issues.

3 participants