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

enum type instead of variant suggestion unification #56188

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zackmdavis
Member

zackmdavis commented Nov 23, 2018

Fixes #56028.

Weirdly, we were deciding between a help note and a structured suggestion based on whether the import candidate span was a dummy—but we weren't using that span in any case! The dummy-ness of the span (which appears to be a matter of this-crate vs. other-crate definition) isn't the right criterion by which we should decide whether it's germane to mention that "there is an enum variant"; instead, let's use the someness of def (which is used as the has_unexpected_resolution argument to error_code).

Since import_candidate_to_paths has no other callers, we are free to stop returning the span and rename the function. By using span_suggestions_, we leverage the max-suggestions output limit already built in to the emitter, thus resolving #56028.

In the matter of message wording, "you can" is redundant (and perhaps too informal); prefer the imperative.

In a second commit, we do some unprincipled special-casing to correct and beautify suggestions for Option and Result (where the existing code was being confused by their being reexported in the prelude).

r? @davidtwco

@davidtwco

Overall, this looks good to me, a few small nits.

Show resolved Hide resolved src/librustc_resolve/lib.rs
Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Nov 26, 2018

(tight schedule this week; expect revision and/or reply Saturday the first)

@bors

This comment was marked as resolved.

Contributor

bors commented Dec 8, 2018

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

enum type instead of variant suggestion unification
Weirdly, we were deciding between a help note and a structured
suggestion based on whether the import candidate span was a dummy—but
we weren't using that span in any case! The dummy-ness of the span
(which appears to be a matter of this-crate vs. other-crate
definition) isn't the right criterion by which we should decide
whether it's germane to mention that "there is an enum variant";
instead, let's use the someness of `def` (which is used as the
`has_unexpected_resolution` argument to `error_code`).

Since `import_candidate_to_paths` has no other callers, we are free to
stop returning the span and rename the function. By using
`span_suggestions_`, we leverage the max-suggestions output limit
already built in to the emitter, thus resolving #56028.

In the matter of message wording, "you can" is redundant (and perhaps
too informal); prefer the imperative.

@zackmdavis zackmdavis force-pushed the zackmdavis:if_i_may_suggest branch from 56027e9 to 373417a Dec 15, 2018

@zackmdavis zackmdavis referenced this pull request Dec 15, 2018

Open

prelude path should not appear in error messages #56861

0 of 2 tasks complete
@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Dec 15, 2018

expect revision and/or reply Saturday the first

Unfortunately, I fear that my estimates are systemically biased. 😰 Rebased and replied to comments above @davidtwco

@rust-highfive

This comment was marked as resolved.

Collaborator

rust-highfive commented Dec 15, 2018

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.
travis_time:end:1f0aa660:start=1544907861092915829,finish=1544907862150113883,duration=1057198054
$ 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-5.0
---
[00:30:44]    Compiling rustc_privacy v0.0.0 (/checkout/src/librustc_privacy)
[00:30:45] error: use of deprecated item 'bitflags::core::str::<impl str>::trim_left_matches': superseded by `trim_start_matches`
[00:30:45]     --> src/librustc_resolve/lib.rs:3252:46
[00:30:45]      |
[00:30:45] 3252 |                                 enum_ty_path.trim_left_matches("std::prelude::v1::").to_owned()
[00:30:45]      |
[00:30:45]      = note: `-D deprecated` implied by `-D warnings`
[00:30:45] 
[00:30:46] error: aborting due to previous error
---
187088 ./obj/build/cache/2018-12-09
160528 ./obj/build/bootstrap/debug/incremental
153272 ./src/tools/clang
144428 ./obj/build/bootstrap/debug/incremental/bootstrap-2x7szixskz2uj
144424 ./obj/build/bootstrap/debug/incremental/bootstrap-2x7szixskz2uj/s-f7mgxceffg-1384krn-4zt1yysir9st
115340 ./src/llvm/test/CodeGen
114584 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
114580 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
112368 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps
---
travis_time:end:0f26066d:start=1544909730942015489,finish=1544909730946641482,duration=4625993
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0196cd20
$ 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:002e068c
travis_time:start:002e068c
$ 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-

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)

adjust enum type instead of variant suggestions for prelude enums
The present author regrets not thinking of a more eloquent way to do
this.

@zackmdavis zackmdavis force-pushed the zackmdavis:if_i_may_suggest branch from 373417a to 6fabc24 Dec 15, 2018

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Dec 15, 2018

(force-pushed again to cope with trim_left_matches deprecation having hit the beta compiler)

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 15, 2018

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.
travis_time:end:257ea7f7:start=1544910026187639262,finish=1544910027244849955,duration=1057210693
$ 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-5.0
---
[00:43:45] .................................................................................................... 500/5177
[00:43:48] ..............................i..................................................................... 600/5177
[00:43:52] .................................................................................................... 700/5177
[00:43:57] .................................................................................................... 800/5177
[00:44:01] .i...............i.................................................................................. 900/5177
[00:44:04] ....................F....iiiii...................................................................... 1000/5177
[00:44:09] .................................................................................................... 1200/5177
[00:44:12] .................................................................................................... 1300/5177
[00:44:14] .................................................................................................... 1400/5177
[00:44:16] .................................................................................................... 1500/5177
---
[00:46:09] ................................i................................................................... 4800/5177
[00:46:12] .................................................................................................... 4900/5177
[00:46:15] .................................................................................................... 5000/5177
[00:46:17] .................................................................................................... 5100/5177
rc/test/ui/did_you_mean/issue-56028-there-is-an-enum-variant.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/did_you_mean/issue-56028-there-is-an-enum-variant/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/did_you_mean/issue-56028-there-is-an-enum-variant/auxiliary" "-A" "unused"
[00:46:19]     Error {
[00:46:19]         line_num: 9,
[00:46:19]         kind: Some(
[00:46:19]             Error
[00:46:19]             Error
[00:46:19]         ),
[00:46:19]         msg: "9:15: 9:18: cannot find type `Set` in this scope [E0412]"
[00:46:19]     Error {
[00:46:19]         line_num: 9,
[00:46:19]         kind: Some(
[00:46:19]             Error
[00:46:19]             Error
[00:46:19]         ),
[00:46:19]         msg: "9:21: 9:24: cannot find value `Set` in this scope [E0425]"
[00:46:19] ]
[00:46:19] 
[00:46:19] thread '[ui] ui/did_you_mean/issue-56028-there-is-an-enum-variant.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:1349:13
[00:46:19] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:46:19] 
[00:46:19] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:503:22
[00:46:19] 
[00:46:19] 
[00:46:19] 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" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--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"
[00:46:19] 
[00:46:19] 
[00:46:19] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:46:19] Build completed unsuccessfully in 0:03:46
[00:46:19] Build completed unsuccessfully in 0:03:46
[00:46:19] make: *** [check] Error 1
[00:46:19] Makefile:58: recipe for target 'check' failed
124332 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
118648 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps
115352 ./src/llvm/test/CodeGen
111164 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends

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)

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Dec 15, 2018

(most recent Travis error) Ugh, I don't know how that E0412 changed to a E0425: almost certainly just recent development on master (and really, the E0425 is probably better); can't cope with task-switching now but will cursorily investigate and re-bless soon

@davidtwco

This comment has been minimized.

Member

davidtwco commented Dec 17, 2018

r=me when Travis is happy.
Update: Added another small nit.

@davidtwco

One last nit then r=me.

.filter(|enum_ty_path| enum_ty_path != "std::prelude::v1")
// also say `Option` rather than `std::prelude::v1::Option`
.map(|enum_ty_path| {
enum_ty_path.trim_start_matches("std::prelude::v1::").to_owned()

This comment has been minimized.

@davidtwco

davidtwco Dec 17, 2018

Member

Could you add a // FIXME(#56861) comment above this line?

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