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

Disable some nice region errors in NLL mode. #53115

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

davidtwco
Copy link
Member

Fixes #52739. cc #52742.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2018
@davidtwco
Copy link
Member Author

This disables all but the named_anon_conflict nice region error in NLL mode as NLL's diagnostics are now generally better than the other types of nice region error. There's only one odd case that I can spot in the changes - issue-45983 (in particular with the migrate compare mode).

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Mostly a request to open up some follow-up issues

@@ -65,10 +65,16 @@ impl<'cx, 'gcx, 'tcx> NiceRegionError<'cx, 'gcx, 'tcx> {
}

pub fn try_report(&self) -> Option<ErrorReported> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a try_report_from_nll method instead, and invoke that from the NLL borrow checker, vs checking the flag. Just seems a bit cleaner, but it may also improve the error messages in some cases below

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

| -- -- ^ function was supposed to return data with lifetime `'b` but it is returning data with lifetime `'a`
| | |
| | lifetime `'b` defined here
| lifetime `'a` defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

seems strictly better

LL |
LL | if x > y { x } else { y } //~ ERROR lifetime mismatch
| ^^^^^ ...but data from `x` is returned here
| ^^^^^ requires that `'1` must outlive `'a`
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that we highlight the comparison here -- seems like a problem, though obviously not from this change...maybe we should file a bug? Presumably the actual span we should highlight is the use of x...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mentioned on the .nll.stderr/.stderr comparison here.

LL |
LL | x //~ ERROR lifetime mismatch
| ^ ...but data from `x` is returned here
| ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should file a bug here -- I think it'd be nice to add another note here and use this as an opportunity to teach the elision rules. e.g.,

error: unsatisfied lifetime constraints
  --> $DIR/ex1-return-one-existing-name-return-type-is-anon.rs:18:5
   |
LL |   fn foo<'a>(&self, x: &'a i32) -> &i32 {
   |          --  -                     - defaults to `&'1 i32` per the elision rules
   |          |  |
   |          |  let's call the lifetime of this reference `'1`
   |          lifetime `'a` defined here
LL | 
LL |     x //~ ERROR lifetime mismatch
| ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`

Copy link
Member Author

Choose a reason for hiding this comment

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

Added here.

| ^ ...but data from `value` flows into `data` here
| -- -- lifetime `'b` defined here
| |
| lifetime `'a` defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should file a bug here too -- in general, it's often dissatisfying to highlight the declaration. To do better, though, we'd presumably have to look at the MIR and see if we can identify the parameter. (You could imagine checking if the lifetime only appears in one parameter's type and using that to guess that we should highlight the parameter type, also)

Copy link
Member Author

Choose a reason for hiding this comment

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

issue-52663-lifetimes-not-included-in-span in Zulip and this comment discussed this briefly as a potential part of #52973.

@nikomatsakis
Copy link
Contributor

@bors r+ p=1

Giving p=1 because NLL

@bors
Copy link
Contributor

bors commented Aug 6, 2018

📌 Commit 785b06c36804529092d304426f785b5e31cdcee7 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 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.
[00:51:04] ....................................................................................................
[00:51:06] ....................................................................................................
[00:51:09] ....................................................................................................
[00:51:12] ....................................................................................................
[00:51:15] ..iiiiiiiii.........................................................................................
[00:51:21] ....................................................................................................
[00:51:25] .......i............................................................................................
[00:51:28] ................i...................................................................................
[00:51:31] ....................................................................................................
---
[00:58:12] ............................i.......................................................................
[00:58:16] ....................................................................................................
[00:58:20] ............................................................i.......................................
[00:58:22] .......................................................i..ii........................................
[00:58:26] ............F.F.....................................................................................
[00:58:32] ....................................................................................................
[00:58:35] .......................................i............................................................
[00:58:38] ....................................................................................................
[00:58:41] ....................................................................................................
---
[00:58:46] ---- [compile-fail] compile-fail/nll/where_clauses_in_functions.rs stdout ----
[00:58:46] 
[00:58:46] error: /checkout/src/test/compile-fail/nll/where_clauses_in_functions.rs:23: unexpected error: '23:5: 23:14: unsatisfied lifetime constraints'
[00:58:46] 
[00:58:46] error: /checkout/src/test/compile-fail/nll/where_clauses_in_functions.rs:23: expected error not found: lifetime mismatch [E0623]
[00:58:46] error: 1 unexpected errors found, 1 expected errors not found
[00:58:46] status: exit code: 1
[00:58:46] command: "/checkout/obj/build-fail] compile-fail/nll/where_clauses_in_functions.rs
[00:58:46]     [compile-fail] compile-fail/nll/where_clauses_in_structs.rs
[00:58:46]     [compile-fail] compile-fail/nll/where_clauses_in_structs.rs
[00:58:46] 
[00:58:46] test result: FAILED. 1871 passed; 2 failed; 13 ignored; 0 measured; 0 filtered out
[00:58:46] 
[00:58:46] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:58:46] 
[00:58:46] 
[00:58:46] 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/compile-fail" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "compile-fail" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -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" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
travis_time:end:104f3ca0:start=1533575931278225658,finish=1533579458191671872,duration=3526913446214

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01c4e350

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
Copy link
Member Author

Oops! Fixed those compile-fail tests.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 6, 2018

📌 Commit 37ba9ca has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 6, 2018

⌛ Testing commit 37ba9ca with merge cf84056...

bors added a commit that referenced this pull request Aug 6, 2018
Disable some nice region errors in NLL mode.

Fixes #52739. cc #52742.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 6, 2018

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

@bors bors merged commit 37ba9ca into rust-lang:master Aug 6, 2018
@davidtwco davidtwco deleted the issue-52739 branch August 6, 2018 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants