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

Fix bug where the outer num_ignored variable was ignored. #44

Merged

Conversation

ltratt
Copy link
Member

@ltratt ltratt commented Nov 26, 2019

The basic problem is that Rust allows move closure to capture variables if they
have a Copyable type, leading to the "outer" and "inner" variables to diverge.
For example, this simple program:

  fn main() {
    let mut x = 0;
    let mut y = move |z: u32| {
      x += z;
      println!("inner x {}", x);
    };

    println!("outer x {}", x);
    y(2);
    println!("outer x {}", x);
  }

outputs:

  outer x 0
  inner x 2
  outer x 0

Note that the above program gives no warnings (or errors) in rustc or clippy.

In this case, the num_ignored variable had started life in single-threaded
code and later been moved to multi-threaded code. It compiled, even though the
outer variable could never be mutated. In other words, lang_tester always said
that 0 tests were ignored -- even if some were ignored!

This commit fixes this problem using an AtomicUsize so that ignored tests now
count as can be seen here:

  $ cargo run --example rust_lang_tester
      Finished dev [unoptimized + debuginfo] target(s) in 0.12s
       Running `target/debug/examples/rust_lang_tester`

  running 7 tests
  test lang_tests::no_main ... ok
  test lang_tests::ignore ... ignored
  test lang_tests::unknown_var ... ok
  test lang_tests::exit_code ... ok
  test lang_tests::unused_var ... ok
  test lang_tests::sig_caught ... ok
  test lang_tests::custom_cla ... ok

  test result: ok. 7 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

The basic problem is that Rust allows move closure to capture variables if they
have a `Copy`able type, leading to the "outer" and "inner" variables to diverge.
For example, this simple program:

  fn main() {
    let mut x = 0;
    let mut y = move |z: u32| {
      x += z;
      println!("inner x {}", x);
    };

    println!("outer x {}", x);
    y(2);
    println!("outer x {}", x);
  }

outputs:

  outer x 0
  inner x 2
  outer x 0

Note that the above program gives no warnings (or errors) in rustc or clippy.

In this case, the `num_ignored` variable had started life in single-threaded
code and later been moved to multi-threaded code. It compiled, even though the
outer variable could never be mutated. In other words, lang_tester always said
that 0 tests were ignored -- even if some were ignored!

This commit fixes this problem using an `AtomicUsize` so that ignored tests now
count as can be seen here:

  $ cargo run --example rust_lang_tester
      Finished dev [unoptimized + debuginfo] target(s) in 0.12s
       Running `target/debug/examples/rust_lang_tester`

  running 7 tests
  test lang_tests::no_main ... ok
  test lang_tests::ignore ... ignored
  test lang_tests::unknown_var ... ok
  test lang_tests::exit_code ... ok
  test lang_tests::unused_var ... ok
  test lang_tests::sig_caught ... ok
  test lang_tests::custom_cla ... ok

  test result: ok. 7 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out
Copy link
Contributor

@jacob-hughes jacob-hughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jacob-hughes jacob-hughes merged commit d9fe4b1 into softdevteam:master Nov 26, 2019
@ltratt ltratt deleted the fix_incorrect_capture_of_mut_variable branch November 26, 2019 15:37
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.

2 participants