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

NLL promotes arbitrary code to be evaluated by miri if the result is unused #53606

Closed
oli-obk opened this issue Aug 22, 2018 · 7 comments · Fixed by #58479
Closed

NLL promotes arbitrary code to be evaluated by miri if the result is unused #53606

oli-obk opened this issue Aug 22, 2018 · 7 comments · Fixed by #58479
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2018

This takes a while to compile. I presume because loop {} needs to hit the loop checker in miri. Changing the _ to _foo or foo will immediately bail out due to &(loop{}, 1).1 not living for the 'static lifetime.

#![feature(nll)]

fn main() {

    let _: &'static usize = &(loop {}, 1).1;
}

(Playground)

Errors:

   Compiling playground v0.0.1 (file:///playground)
warning: unreachable expression
 --> src/main.rs:4:40
  |
4 |     let _: &'static usize = &(loop {}, 1).1;
  |                                        ^
  |
  = note: #[warn(unreachable_code)] on by default

    Finished dev [unoptimized + debuginfo] target(s) in 1.09s
     Running `target/debug/playground`
/root/entrypoint.sh: line 8:     7 Killed                  timeout --signal=KILL ${timeout} "$@"

@oli-obk oli-obk added the A-NLL Area: Non-lexical lifetimes (NLL) label Aug 22, 2018
@matthewjasper
Copy link
Contributor

The following code times out with either borrow checker:

fn main() {

    let _ = &(loop {}, 1).1;
}

@nikomatsakis
Copy link
Contributor

The reason that _ vs _foo matters is because of #47184

@nikomatsakis
Copy link
Contributor

I don't think this is an NLL issue in particular?

@pnkfelix
Copy link
Member

Based on @matthewjasper 's identification of a test that reproduces the issue on either borrow-checker, I am going to remove the A-NLL label.

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: Constant evaluation (MIR interpretation) and removed A-NLL Area: Non-lexical lifetimes (NLL) labels Aug 28, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 28, 2018

That example is not problematic, because nothing is promoted at all.

I now don't even think my example is problematic anymore, because it seems to be perfectly alright to promote anything if we diverge before we give the user access to the value.

I guess this just needs a test now (just a ui test with a //compile-pass comment, not a run-pass test, as that would hang forever)

@oli-obk oli-obk added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Aug 28, 2018
@oli-obk oli-obk added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 28, 2019
@saleemjaffer
Copy link
Contributor

@oli-obk Can I work on this? I'm a newbie to rust.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2019

Sure!

You can take the example from the issue message and create a new file in src/test/ui/consts/ and add the //compile-pass comment at the top.

Then, when running ./x.py test --stage 1 src/test/ui --test-args const --bless it should succeed to run the tests.

saleemjaffer pushed a commit to saleemjaffer/rust that referenced this issue Feb 15, 2019
kennytm added a commit to kennytm/rust that referenced this issue Feb 16, 2019
bors added a commit that referenced this issue Feb 16, 2019
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58477 (Fix the syntax error in publish_toolstate.py)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
kennytm added a commit to kennytm/rust that referenced this issue Feb 17, 2019
bors added a commit that referenced this issue Feb 17, 2019
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
 - #58521 (Fix tracking issue for error iterators)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants