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 ICE with generators and NLL #56460

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@davidtwco
Member

davidtwco commented Dec 3, 2018

Fix #55850.

This PR stops an ICE in #55850 by not panicking when a region cannot be named. However, this PR does not (yet) fix the underlying issue that the correct name for the test case provided for the issue (in this instance, 'a) was not found.

This PR also lays a little bit of groundwork by categorizing yields separately from returns so that region naming can be specialized for this case.

r? @pnkfelix

@davidtwco davidtwco changed the title from ICE with generators and NLL to WIP: ICE with generators and NLL Dec 3, 2018

@pnkfelix pnkfelix changed the title from WIP: ICE with generators and NLL to Fix ICE with generators and NLL Dec 4, 2018

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Dec 4, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Dec 4, 2018

📌 Commit 9acfbb2 has been approved by pnkfelix

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 5, 2018

Rollup merge of rust-lang#56460 - davidtwco:issue-55850, r=pnkfelix
Fix ICE with generators and NLL

Fix rust-lang#55850.

This PR stops an ICE in rust-lang#55850 by not panicking when a region cannot be named. However, this PR does not (yet) fix the underlying issue that the correct name for the test case provided for the issue (in this instance, `'a`) was not found.

This PR also lays a little bit of groundwork by categorizing yields separately from returns so that region naming can be specialized for this case.

r? @pnkfelix
@pietroalbini

This comment was marked as resolved.

Member

pietroalbini commented Dec 5, 2018

@bors r-

Failed in the rollup:

[00:50:01] error: /checkout/src/test/ui/nll/issue-55850.rs:38: unexpected error: '38:16: 38:17: `s` does not live long enough [E0597]'
[00:50:01] 
[00:50:01] error: 1 unexpected errors found, 0 expected errors not found
[00:50:01] status: exit code: 1
[00:50:01] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/nll/issue-55850.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-55850/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-55850/auxiliary" "-A" "unused"
[00:50:01] unexpected errors (from JSON output): [
[00:50:01]     Error {
[00:50:01]         line_num: 38,
[00:50:01]         kind: Some(
[00:50:01]             Error
[00:50:01]         ),
[00:50:01]         msg: "38:16: 38:17: `s` does not live long enough [E0597]"
[00:50:01]     }
[00:50:01] ]
[00:50:01] 
[00:50:01] thread '[ui] ui/nll/issue-55850.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:1349:13

@davidtwco davidtwco force-pushed the davidtwco:issue-55850 branch from 9acfbb2 to 9d25e42 Dec 5, 2018

@davidtwco

This comment was marked as outdated.

Member

davidtwco commented Dec 6, 2018

@bors r=pnkfelix

@bors

This comment was marked as outdated.

Contributor

bors commented Dec 6, 2018

📌 Commit 9d25e42 has been approved by pnkfelix

@bors

This comment was marked as outdated.

Contributor

bors commented Dec 7, 2018

⌛️ Testing commit 9d25e42 with merge ac85c17...

bors added a commit that referenced this pull request Dec 7, 2018

Auto merge of #56460 - davidtwco:issue-55850, r=pnkfelix
Fix ICE with generators and NLL

Fix #55850.

This PR stops an ICE in #55850 by not panicking when a region cannot be named. However, this PR does not (yet) fix the underlying issue that the correct name for the test case provided for the issue (in this instance, `'a`) was not found.

This PR also lays a little bit of groundwork by categorizing yields separately from returns so that region naming can be specialized for this case.

r? @pnkfelix
@bors

This comment was marked as outdated.

Contributor

bors commented Dec 7, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as resolved.

Collaborator

rust-highfive commented Dec 7, 2018

The job x86_64-gnu-nopt of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:07:48] 
[01:07:48] ---- [ui (nll)] ui/nll/issue-55850.rs stdout ----
[01:07:48] diff of stderr:
[01:07:48] 
[01:07:48] 1 error[E0597]: `s` does not live long enough
[01:07:48] 3    |
[01:07:48] 3    |
[01:07:48] - LL |         yield &s[..]
[01:07:48] + LL |         yield &s[..] //~ ERROR `s` does not live long enough [E0597]
[01:07:48] 5    |                ^ borrowed value does not live long enough
[01:07:48] 6 LL |     })
[01:07:48] 7    |     - `s` dropped here while still borrowed
[01:07:48] 
[01:07:48] 9 error[E0626]: borrow may still be in use when generator yields
[01:07:48] 11    |
[01:07:48] 11    |
[01:07:48] - LL |         yield &s[..]
[01:07:48] + LL |         yield &s[..] //~ ERROR `s` does not live long enough [E0597]
[01:07:48] 13    |         -------^---- possible yield occurs here
[01:07:48] 15 error: aborting due to 2 previous errors
[01:07:48] 
[01:07:48] 
[01:07:48] The actual stderr differed from the expected stderr.
[01:07:48] The actual stderr differed from the expected stderr.
[01:07:48] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-55850.nll/issue-55850.nll.stderr
[01:07:48] To update references, rerun the tests and pass the `--bless` flag
[01:07:48] To only update this specific test, also pass `--test-args nll/issue-55850.rs`
[01:07:48] error: 1 errors occurred comparing output.
[01:07:48] status: exit code: 1
[01:07:48] status: exit code: 1
[01:07:48] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/nll/issue-55850.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-55850.nll/a" "-Zborrowck=migrate" "-Ztwo-phase-borrows" "-Crpath" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-55850.nll/auxiliary" "-A" "unused"
[01:07:48] ------------------------------------------
[01:07:48] 
[01:07:48] ------------------------------------------
[01:07:48] stderr:
[01:07:48] stderr:
[01:07:48] ------------------------------------------
[01:07:48] {"message":"`s` does not live long enough","code":{"code":"E0597","explanation":"\nThis error occurs because a borrow was made inside a variable which has a\ngreater lifetime than the borrowed one.\n\nExample of erroneous code:\n\n```compile_fail,E0597\nstruct Foo<'a> {\n    x: Option<&'a u32>,\n}\n\nlet mut x = Foo { x: None };\nlet y = 0;\nx.x = Some(&y); // error: `y` does not live long enough\n```\n\nIn here, `x` is created before `y` and therefore has a greater lifetime. Always\nkeep in mind that values in a scope are dropped in the opposite order they are\ncreated. So to fix the previous example, just make the `y` lifetime greater than\nthe `x`'s one:\n\n```\nstruct Foo<'a> {\n    x: Option<&'a u32>,\n}\n\nlet y = 0;\nlet mut x = Foo { x: None };\nx.x = Some(&y);\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/nll/issue-55850.rs","byte_start":1027,"byte_end":1028,"line_start":38,"line_end":38,"column_start":16,"column_end":17,"is_primary":true,"text":[{"text":"        yield &s[..] //~ ERROR `s` does not live long enough [E0597]","highlight_start":16,"highlight_end":17}],"label":"borrowed value does not live long enough","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/issue-55850.rs","byte_start":1085,"byte_end":1086,"line_start":39,"line_end":39,"column_start":5,"column_end":6,"is_primary":false,"text":[{"text":"    })","highlight_start":5,"highlight_end":6}],"label":"`s` dropped here while still borrowed","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0597]: `s` does not live long enough\n  --> /checkout/src/test/ui/nll/issue-55850.rs:38:16\n   |\nLL |         yield &s[..] //~ ERROR `s` does not live long enough [E0597]\n   |                ^ borrowed value does not live long enough\nLL |     })\n   |     - `s` dropped here while still borrowed\n\n"}
[01:07:48] {"message":"borrow may still be in use when generator yields","code":{"code":"E0626","explanation":"\nThis error occurs because a borrow in a generator persists across a\nyield point.\n\n```compile_fail,E0626\n# #![feature(generators, generator_trait)]\n# use std::ops::Generator;\nlet mut b = || {\n    let a = &String::new(); // <-- This borrow...\n    yield (); // ...is still in scope here, when the yield occurs.\n    println!(\"{}\", a);\n};\nunsafe { b.resume() };\n```\n\nAt present, it is not permitted to have a yield that occurs while a\nborrow is still in scope. To resolve this error, the borrow must\neither be \"contained\" to a smaller scope that does not overlap the\nyield or else eliminated in another way. So, for example, we might\nresolve the previous example by removing the borrow and just storing\nthe integer by value:\n\n```\n# #![feature(generators, generator_trait)]\n# use std::ops::Generator;\nlet mut b = || {\n    let a = 3;\n    yield ();\n    println!(\"{}\", a);\n};\nunsafe { b.resume() };\n```\n\nThis is a very simple case, of course. In more complex cases, we may\nwish to have more than one reference to the value that was borrowed --\nin those cases, something like the `Rc` or `Arc` types may be useful.\n\nThis error also frequently arises with iteration:\n\n```compile_fail,E0626\n# #![feature(generators, generator_trait)]\n# use std::ops::Generator;\nlet mut b = || {\n  let v = vec![1,2,3];\n  for &x in &v { // <-- borrow of `v` is still in scope...\n    yield x; // ...when this yield occurs.\n  }\n};\nunsafe { b.resume() };\n```\n\nSuch cases can sometimes be resolved by iterating \"by value\" (or using\n`into_iter()`) to avoid borrowing:\n\n```\n# #![feature(generators, generator_trait)]\n# use std::ops::Generator;\nlet mut b = || {\n  let v = vec![1,2,3];\n  for x in v { // <-- Take ownership of the values instead!\n    yield x; // <-- Now yield is OK.\n  }\n};\nunsafe { b.resume() };\n```\n\nIf taking ownership is not an option, using indices can work too:\n\n```\n# #![feature(generators, generator_trait)]\n# use std::ops::Generator;\nlet mut b = || {\n  let v = vec![1,2,3];\n  let len = v.len(); // (*)\n  for i in 0..len {\n    let x = v[i]; // (*)\n    yield x; // <-- Now yield is OK.\n  }\n};\nunsafe { b.resume() };\n\n// (*) -- Unfortunately, these temporaries are currently required.\n// See <https://github.com/rust-lang/rust/issues/43122>.\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/nll/issue-55850.rs","byte_start":1020,"byte_end":1032,"line_start":38,"line_end":38,"column_start":9,"column_end":21,"is_primary":false,"text":[{"text":"        yield &s[..] //~ ERROR `s` does not live long enough [E0597]","highlight_start":9,"highlight_end":21}],"label":"possible yield occurs here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/issue-55850.rs","byte_start":1027,"byte_end":1028,"line_start":38,"line_end":38,"column_start":16,"column_end":17,"is_primary":true,"text":[{"text":"        yield &s[..] //~ ERROR `s` does not live long enough [E0597]","highlight_start":16,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0626]: borrow may still be in use when generator yields\n  --> /checkout/src/test/ui/nll/issue-55850.rs:38:16\n   |\nLL |         yield &s[..] //~ ERROR `s` does not live long enough [E0597]\n   |         -------^---- possible yield occurs here\n\n"}
[01:07:48] {"message":"aborting due to 2 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 2 previous errors\n\n"}
[01:07:48] {"message":"Some errors occurred: E0597, E0626.","code":null,"level":"","spans":[],"children":[],"rendered":"Some errors occurred: E0597, E0626.\n"}
[01:07:48] {"message":"For more information about an error, try `rustc --explain E0597`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about an error, try `rustc --explain E0597`.\n"}
[01:07:48] ------------------------------------------
[01:07:48] 
[01:07:48] thread '[ui (nll)] ui/nll/issue-55850.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3284:9
[01:07:48] 
---
[01:07:48] 
[01:07:48] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:503:22
[01:07:48] 
[01:07:48] 
[01:07:48] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--host-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always" "--compare-mode" "nll"
[01:07:48] 
[01:07:48] 
[01:07:48] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:07:48] Build completed unsuccessfully in 0:07:27
[01:07:48] Build completed unsuccessfully in 0:07:27
[01:07:48] make: *** [check] Error 1
[01:07:48] Makefile:58: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0cbe9e59
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Dec  7 12:16:18 UTC 2018
---
travis_time:end:07e0c38a:start=1544184980615017199,finish=1544184980623643880,duration=8626681
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:210d0fa1
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1b06d66e
travis_time:start:1b06d66e
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:01c25ad1
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@davidtwco

This comment has been minimized.

Member

davidtwco commented Dec 7, 2018

Oops, didn’t realise this failed Travis when approving. Will fix shortly.

davidtwco added some commits Dec 3, 2018

Introduce constraint category for yields.
This commit adds a new `ConstraintCategory` for yield points - this
allows for differentiation between a normal return and a yield in the
diagnostics.
Fix ICE in region naming.
This commit puts a fix in place for the ICE in region naming code so
that it doesn't break the compiler. However, this results in the
diagnostic being poorer as the borrow explanation that was causing the
ICE is not being added - this should be fixed as a follow-up.

@davidtwco davidtwco force-pushed the davidtwco:issue-55850 branch from 9d25e42 to 4a286d3 Dec 7, 2018

@davidtwco

This comment has been minimized.

Member

davidtwco commented Dec 7, 2018

@bors r=pnkfelix

@bors

This comment has been minimized.

Contributor

bors commented Dec 7, 2018

📌 Commit 4a286d3 has been approved by pnkfelix

@bors

This comment has been minimized.

Contributor

bors commented Dec 7, 2018

⌛️ Testing commit 4a286d3 with merge a40fded...

bors added a commit that referenced this pull request Dec 7, 2018

Auto merge of #56460 - davidtwco:issue-55850, r=pnkfelix
Fix ICE with generators and NLL

Fix #55850.

This PR stops an ICE in #55850 by not panicking when a region cannot be named. However, this PR does not (yet) fix the underlying issue that the correct name for the test case provided for the issue (in this instance, `'a`) was not found.

This PR also lays a little bit of groundwork by categorizing yields separately from returns so that region naming can be specialized for this case.

r? @pnkfelix
@bors

This comment has been minimized.

Contributor

bors commented Dec 7, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing a40fded to master...

@bors bors merged commit 4a286d3 into rust-lang:master Dec 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-55850 branch Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment