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

Add information to higher-ranked lifetimes conflicts error messages #57901

Merged
merged 16 commits into from Jan 29, 2019

Conversation

Projects
None yet
6 participants
@lqd
Copy link
Contributor

lqd commented Jan 25, 2019

Make these errors go through the new "placeholder error" code path, to have self tys displayed and make them hopefully less confusing.

Should fix #57362.

r? @nikomatsakis — so we can iterate on the specific wording you wanted.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 25, 2019

The job x86_64-gnu-llvm-6.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.
travis_time:end:10d9727c:start=1548442033018742450,finish=1548442035633104723,duration=2614362273
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:04:34] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:34] tidy error: binary checked into source: /checkout/src/test/ui/issues/issue-57362.rs
[00:04:35] tidy error: /checkout/src/test/ui/issues/issue-57362.rs: missing trailing newline
[00:04:36] some tidy checks failed
[00:04:36] 
[00:04:36] 
[00:04:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:36] 
[00:04:36] 
[00:04:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:36] Build completed unsuccessfully in 0:00:46
[00:04:36] Build completed unsuccessfully in 0:00:46
[00:04:36] Makefile:68: recipe for target 'tidy' failed
[00:04:36] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:12976398
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Jan 25 18:52:03 UTC 2019
---
travis_time:end:06cef247:start=1548442323575264000,finish=1548442323579840139,duration=4576139
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0a69b642
$ 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:0142bd95
travis_time:start:0142bd95
$ 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:12ab855c
$ 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)

fn f(self);
}

impl<T> Trait for fn(&T) {

This comment has been minimized.

@Centril

Centril Jan 25, 2019

Contributor

I'd write this out:

Suggested change Beta
impl<T> Trait for fn(&T) {
impl<T> Trait for for<'a> fn(&'a T) {

to make what's tested clearer.

Show resolved Hide resolved src/test/ui/issues/issue-57362.stderr Outdated
Show resolved Hide resolved src/test/ui/issues/issue-57362.stderr Outdated
LL | a.f(); //~ ERROR not general enough
| ^
|
= note: `Trait` would have to be implemented for the type `fn(&u8)`

This comment has been minimized.

@Centril

Centril Jan 25, 2019

Contributor
Suggested change Beta
= note: `Trait` would have to be implemented for the type `fn(&u8)`
= note: `Trait` must be implemented for the type `fn(&u8)`

Shorter and more direct :)
(alternatively s/must/needs to/)

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 25, 2019

Contributor

I'm not so sure. I find the word "must" more confusing somehow. "would have to be" makes it clearer to me that it is not the case.

This comment has been minimized.

@Centril

Centril Jan 25, 2019

Contributor

In that case: "Trait isn't, but must be..."; or "Trait needed to be..." -- I just find the current text wordy, but its not a big deal or anything. :)

Show resolved Hide resolved src/librustc/infer/error_reporting/nice_region_error/placeholder_error.rs
if self_ty_has_vid {
err.note(&format!(
"but `{}` is actually implemented for the type `{}`, \
for the specific lifetime `'{}`",

This comment has been minimized.

@Centril

Centril Jan 25, 2019

Contributor

Nothing to do about here... but it would have been nice to "switch" on the format in some way since the only difference is the format and not the arguments... I suspect this contributes to duplication in many places...

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 25, 2019

Contributor

I guess you mean moving "the specific" and "some" into some string? I'm of mixed minds. I often prefer to read the full text. It's also more friendly to i18n if we ever do that (ha, that's a long way away). But OTOH, it is duplication indeed. No strong opinion.

This comment has been minimized.

@Centril

Centril Jan 25, 2019

Contributor

I guess you mean moving "the specific" and "some" into some string?

Right; I was mainly thinking of switching on the "string literal"; but that doesn't work with format! afaik so its probably not implementable? (which is why it mostly was a general comment about the future... ^^)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Jan 25, 2019

The job x86_64-gnu-llvm-6.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.
travis_time:end:1b13a4f2:start=1548449110584964937,finish=1548449113397924191,duration=2812959254
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:55] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:55] tidy error: binary checked into source: /checkout/src/test/ui/issues/issue-57362-1.rs
[00:03:57] some tidy checks failed
[00:03:57] 
[00:03:57] 
[00:03:57] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:57] 
[00:03:57] 
[00:03:57] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:57] Build completed unsuccessfully in 0:00:48
[00:03:57] Build completed unsuccessfully in 0:00:48
[00:03:57] make: *** [tidy] Error 1
[00:03:57] Makefile:68: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05c9da89
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Jan 25 20:49:21 UTC 2019
---
travis_time:end:0bd9c108:start=1548449362512961133,finish=1548449362518531927,duration=5570794
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1114b31b
$ 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:009dbce8
travis_time:start:009dbce8
$ 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:059da0f8
$ 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)

@lqd

This comment has been minimized.

Copy link
Contributor Author

lqd commented Jan 26, 2019

Updated to address the review comments here and on Zulip:

  • The code handling all the notes has been extracted to a helper function
  • All the cases needed to correctly print either the trait or the self type where the sub/sup placeholders appear next to these placeholders, and not at opposite ends of the error message, have been added to this helper function.
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Jan 27, 2019

☔️ The latest upstream changes (presumably #57907) made this pull request unmergeable. Please resolve the merge conflicts.

lqd and others added some commits Jan 25, 2019

Make NiceRegionError use the `InferCtxt` instead of its `TyCtxt`
Some errors (e.g placeholder errors) have unresolved type vars so this will allow to use `resolve_type_vars_if_possible` when needed.
Handle higher-ranked lifetime conflict errors where the subtype is th…
…e `sup` region

These are happening since the switch to universes, and will now go through the "placeholder error" path, instead of the current fallback of E308 "mismatched types" errors.
Try to resolve type vars in the placeholder errors trait refs
These can sometimes be unresolved: some of the rustc UI tests show this.
When mentioning lifetimes, put either the trait ref or the self type …
…closer to the lifetimes

When mentioning lifetimes, only invert wording between the expected trait and the self type when the self type has the vid.
This way, the lifetimes always stay close to the self type or trait ref that actually contains them.

@lqd lqd force-pushed the lqd:issue_57362 branch from d01879e to e077501 Jan 27, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 29, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 29, 2019

📌 Commit c97d135 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 29, 2019

@bors p=1 -- fairly severe, if limited, diagnostic regression

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 29, 2019

⌛️ Testing commit c97d135 with merge 7425663...

bors added a commit that referenced this pull request Jan 29, 2019

Auto merge of #57901 - lqd:issue_57362, r=nikomatsakis
Add information to higher-ranked lifetimes conflicts error messages

Make these errors go through the new "placeholder error" code path, to have self tys displayed and make them hopefully less confusing.

Should fix #57362.

r? @nikomatsakis — so we can iterate on the specific wording you wanted.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 29, 2019

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 29, 2019

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

@bors bors merged commit c97d135 into rust-lang:master Jan 29, 2019

1 check passed

homu Test successful
Details

@lqd lqd deleted the lqd:issue_57362 branch Jan 29, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 30, 2019

beta nominating so that we discuss at T-compiler meeting. (But to be clear, I personally am skeptical about trying to backport this. We can probably live with a diagnostic regression, even one this severe, in the stable channel for a release cycle.)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 31, 2019

discussed at T-compiler meeting. declining for beta backport.

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