#53359: putting multiple unresolved import on single line #53565

PramodBisht · 2018-08-21T15:30:22Z

r? @estebank
Here is WIP implementation of #53359
this PR have clubbed multiple unresolved imports into a single line.
I think still two things need to improve like giving specific label message for each span of multi_span(how we can do this?) and second we are getting a warning while compiling, stating something like E0432 have been passed before.

estebank · 2018-08-21T17:19:16Z

this PR have clubbed multiple unresolved imports into a single line.

I feel it might be slightly less disruptive to third party tools to only unify those unresolved imports that are part of the same path (baz and qux in foo::bar::{baz, qux}).

I think still two things need to improve like giving specific label message for each span of
multi_span(how we can do this?)

You need to give one label to each span and it should work.

and second we are getting a warning while compiling, stating something like E0432 have been passed before.

That is because a span_err with a given number can only be used once, and you're reusing that error code here. You should look into where that error is being emitted now and unify them.

rust-highfive · 2018-08-31T17:30:33Z

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:17:09]    Compiling rustc_lint v0.0.0 (file:///checkout/src/librustc_lint)
[00:17:09]    Compiling rustc_passes v0.0.0 (file:///checkout/src/librustc_passes)
[00:17:13]    Compiling rustc_plugin v0.0.0 (file:///checkout/src/librustc_plugin)
[00:17:23]    Compiling rustc_resolve v0.0.0 (file:///checkout/src/librustc_resolve)
[00:17:24] warning: diagnostic code E0432 already used
[00:17:24]     |
[00:17:24]     |
[00:17:24] 365 |             let mut err = struct_span_err!(resolver.session, span, E0432, "{}", msg);
[00:17:24]     |
[00:17:24] note: previous invocation
[00:17:24]    --> librustc_resolve/resolve_imports.rs:799:23
[00:17:24]     |
[00:17:24]     |
[00:17:24] 799 |           let mut err = struct_span_err!(self.resolver.session,
[00:17:24]     |  _______________________^
[00:17:24] 800 | |                                        multi_span.clone(), E0432, "{}", &msg);
[00:17:24]     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:17:24] 
[00:18:37]    Compiling rustc-main v0.0.0 (file:///checkout/src/rustc)
[00:18:37]     Finished release [optimized] target(s) in 13m 16s
---
[00:31:53]    Compiling rustc_privacy v0.0.0 (file:///checkout/src/librustc_privacy)
[00:32:09]    Compiling rustc_save_analysis v0.0.0 (file:///checkout/src/librustc_save_analysis)
[00:32:58]    Compiling rustc_resolve v0.0.0 (file:///checkout/src/librustc_resolve)
[00:32:58]    Compiling rustc_plugin v0.0.0 (file:///checkout/src/librustc_plugin)
[00:32:59] warning: diagnostic code E0432 already used
[00:32:59]     |
[00:32:59]     |
[00:32:59] 365 |             let mut err = struct_span_err!(resolver.session, span, E0432, "{}", msg);
[00:32:59]     |
[00:32:59] note: previous invocation
[00:32:59]    --> librustc_resolve/resolve_imports.rs:799:23
[00:32:59]     |
[00:32:59]     |
[00:32:59] 799 |           let mut err = struct_span_err!(self.resolver.session,
[00:32:59]     |  _______________________^
[00:32:59] 800 | |                                        multi_span.clone(), E0432, "{}", &msg);
[00:32:59]     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:32:59] 
[00:33:04]    Compiling rustc_codegen_utils v0.0.0 (file:///checkout/src/librustc_codegen_utils)
[00:33:04]    Compiling rustc_passes v0.0.0 (file:///checkout/src/librustc_passes)
---
[00:44:22] ....................................................................................................
[00:44:24] ....................................................................................................
[00:44:27] i..................................................................i................................
[00:44:30] ....................................................................................................
[00:44:33] ........................................F...........................................................
[00:44:38] ....................................................................................................
[00:44:41] ....................................................................................................
[00:44:44] ....................................................................................................
[00:44:47] ....................................................................................................
---
[00:45:46] 
[00:45:46] ---- [ui] ui/issue-53565.rs stdout ----
[00:45:46] diff of stderr:
[00:45:46] 
[00:45:46] 1 error[E0432]: unresolved imports `std::time::foo`, `std::time::bar`, `std::time::buzz`
[00:45:46] +   --> $DIR/issue-53565.rs:10:17
[00:45:46] 3    |
[00:45:46] 3    |
[00:45:46] 4 LL | use std::time::{foo, bar, buzz};
[00:45:46] 5    |                 ^^^  ^^^  ^^^^ no `buzz` in `time`
[00:45:46] 
[00:45:46] 8    |                 no `foo` in `time`
[00:45:46] 9 
[00:45:46] 10 error[E0432]: unresolved imports `std::time::abc`, `std::time::def`
[00:45:46] +   --> $DIR/issue-53565.rs:11:17
[00:45:46] 12    |
[00:45:46] 12    |
[00:45:46] 13 LL | use std::time::{abc, def};
[00:45:46] 14    |                 ^^^  ^^^ no `def` in `time`
[00:45:46] 
[00:45:46] The actual stderr differed from the expected stderr.
[00:45:46] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issue-53565/issue-53565.stderr
[00:45:46] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issue-53565/issue-53565.stderr
[00:45:46] To update references, rerun the tests and pass the `--bless` flag
[00:45:46] To only update this specific test, also pass `--test-args issue-53565.rs`
[00:45:46] error: 1 errors occurred comparing output.
[00:45:46] status: exit code: 1
[00:45:46] status: exit code: 1
[00:45:46] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/issue-53565.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/issue-53565/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/issue-53565/auxiliary" "-A" "unused"
[00:45:46] ------------------------------------------
[00:45:46] 
[00:45:46] ------------------------------------------
[00:45:46] stderr:
[00:45:46] stderr:
[00:45:46] ------------------------------------------
[00:45:46] {"message":"unresolved imports `std::time::foo`, `std::time::bar`, `std::time::buzz`","code":{"code":"E0432","explanation":"\nAn import was unresolved.\n\nErroneous code example:\n\n```compile_fail,E0432\nuse something::Foo; // error: unresolved import `something::Foo`.\n```\n\nPaths in `use` statements are relative to the crate root. To import items\nrelative to the current and parent modules, use the `self::` and `super::`\nprefixes, respectively. Also verify that you didn't misspell the import\nname and that the import exists in the module from where you tried to\nimport it. Example:\n\n```\nuse self::something::Foo; // ok!\n\nmod something {\n    pub struct Foo;\n}\n# fn main() {}\n```\n\nOr, if you tried to use a module from an external crate, you may have missed\nthe `extern crate` declaration (which is usually placed in the crate root):\n\n```\nextern crate core; // Required to use the `core` crate\n\nuse core::any;\n# fn main() {}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issue-53565.rs","byte_start":482,"byte_end":485,"line_start":10,"line_end":10,"column_start":17,"column_end":20,"is_primary":true,"text":[{"text":"use std::time::{foo, bar, buzz};","highlight_start":17,"highlight_end":20}],"label":"no `foo` in `time`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/issue-53565.rs","byte_start":487,"byte_end":490,"line_start":10,"line_end":10,"column_start":22,"column_end":25,"is_primary":true,"text":[{"text":"use std::time::{foo, bar, buzz};","highlight_start":22,"highlight_end":25}],"label":"no `bar` in `time`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/issue-53565.rs","byte_start":492,"byte_end":496,"line_start":10,"line_end":10,"column_start":27,"column_end":31,"is_primary":true,"text":[{"text":"use std::tessfully in 0:03:03
[00:45:46] make: *** [check] Error 1
[00:45:46] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:23adbb39
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
151200 ./src/tools/clang
149120 ./src/llvm-emscripten/test
148964 ./obj/build/bootstrap/debug/incremental
134532 ./obj/build/bootstrap/debug/incremental/bootstrap-11nz4fw202v9g
134528 ./obj/build/bootstrap/debug/incremental/bootstrap-11nz4fw202v9g/s-f4dehdm17m-1ighvpz-3aquzm2prkry8
129812 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu
1or 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 -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:009c58e8
travis_time:start:009c58e8
$ 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:10ff9758
$ 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)

estebank · 2018-08-31T17:42:26Z

src/librustc_resolve/resolve_imports.rs

+        let msg = format!("unresolved import{} {}",
+                        if path_vec.len() > 1 { "s" } else { "" },  path);
+        let mut err = struct_span_err!(self.resolver.session,
+                                       multi_span.clone(), E0432, "{}", &msg);


To avoid the error you're seeing in travis you need to extract this to a method called from both here and the other occurrence of E0432.

@estebank Sure!

@estebank Challenge here is to create a generic function which can take any one of Span or MultiSpan as a type of argument. I think creating a traits and implementing it both for Span and MultiSpan is one way but I am bit reluctant toward this approach. Let me know what are your thoughts on that.

I think I have found other way for this.

You can use where S: Into<MultiSpan> and have your method accept a sp: S. Span implements it, so your method can take either and inside your method you operate with a MultiSpan always.

estebank · 2018-08-31T17:44:29Z

src/test/ui/issue-53565.stderr

+  --> $DIR/issue-53565.rs:1:17
+   |
+LL | use std::time::{foo, bar, buzz};
+   |                 ^^^  ^^^  ^^^^ no `buzz` in `time`


It might be nice for this to be

no `buzz` in `std::time`

but I'm not too worried one way or another.

estebank · 2018-09-04T11:41:26Z

src/librustc_resolve/resolve_imports.rs

+                    // in case of new import line, throw diagnostic message
+                    // for previous line.
+                    self.throw_unresolved_import_error(error_vec.clone(), None);
+                    error_vec.clear();


Here I feel using std::mem::swap would be a better idea:

let mut empty_vec = vec![]; swap(empty_vec, error_vec); self.throw_unresolved_import_error(empty_vec, None);

That way you're not cloning error_vec just to discard its contents immediately after.

estebank

Other than the nitpicks inline, r=me.

estebank · 2018-09-04T11:42:38Z

src/librustc_resolve/resolve_imports.rs

+        let (span,msg) = match error_vec.is_empty() {
+            true => (span.unwrap(), "unresolved import".to_string()),
+            false => {
+                let span = MultiSpan::from_spans(error_vec.clone().into_iter()


Clean up indentation in this block.

I don't get it, what's wrong with indentation here.

estebank · 2018-09-04T11:43:55Z

src/librustc_resolve/resolve_imports.rs

+                let path_vec: Vec<String> = error_vec.clone().into_iter()
+                                .map(|elem: (Span, String, String)| { format!("`{}`", elem.1) }
+                                ).collect();
+                let path = path_vec.join(", ");


It'd be a good idea to bound this list to a maximum size, to avoid having a ridiculous amount of text. The limit should be high enough that it is rarely hit, but must be present in case somebody is writing non-stylistic code.

@estebank What should be upper limit of path_vec here ?

Any number would be arbitrary, but let's say... 10?

@estebank: or should we simply emit message once vector size reaches 10 or new import line? However, in that case we would not be able to avoid ridiculous amount of data.

I think the ideal would be something similar to the unexpected token error, where the error message lists all of the tokens that could have applied (theoretically unbounded list), while the label after a certain number (don't recall the threshold) is just "expected one of N tokens here".

This is what I'm thinking of

error: expected one of `)`, `,`, `-`, `.`, `<`, `?`, `break`, `continue`, `false`, `for`, `if`, `loop`, `match`, `move`, `return`, `static`, `true`, `unsafe`, `while`, `yield`, or an operator, found `;` --> $DIR/token-error-correct.rs:14:13 | LL | foo(bar(; | -- | || | |expected one of 21 possible tokens here | |help: ...the missing `)` may belong here | if you meant to close this...

In this case you'd have multiple spans, so the output would be a bit more cluttered, which makes me think we might want to remove the labels...

@estebank done! now I have set an upper limit on a number of span_label for each diagnostic msg.

…one diagnostic Addressed estebank's comments for 53359

PramodBisht · 2018-09-09T03:02:03Z

r? @estebank

estebank · 2018-09-09T13:17:02Z

@bors r+

❤️

bors · 2018-09-09T13:17:03Z

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

PramodBisht · 2018-09-09T13:47:53Z

@estebank you may have to r+ it again :)

estebank · 2018-09-09T14:19:16Z

@bors r+

bors · 2018-09-09T14:19:17Z

📌 Commit 21ba03e has been approved by estebank

bors · 2018-09-09T23:26:07Z

⌛ Testing commit 21ba03e with merge 4fdc118a89e0acbf5e753e627845ba2e310657d3...

bors · 2018-09-10T01:42:05Z

💔 Test failed - status-travis

rust-highfive · 2018-09-10T01:42:07Z

The job arm-android 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:42:07] test time::tests::system_time_elapsed ... ok
[01:42:07] test time::tests::system_time_math ... ok
[01:42:13] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:42:14] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:42:50] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

kennytm · 2018-09-10T06:14:01Z

@bors retry #43283

bors · 2018-09-10T07:48:56Z

⌛ Testing commit 21ba03e with merge b8d45da...

@estebank

#53359: putting multiple unresolved import on single line r? @estebank Here is WIP implementation of #53359 this PR have clubbed multiple unresolved imports into a single line. I think still two things need to improve like giving specific `label message` for each span of multi_span(how we can do this?) and second we are getting a warning while compiling, stating something like `E0432` have been passed before.

bors · 2018-09-10T10:28:47Z

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

rust-highfive assigned estebank Aug 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2018

This comment has been minimized.

Sign in to view

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2018

PramodBisht force-pushed the issue/53359_b branch 2 times, most recently from 18b7296 to e8dda23 Compare August 31, 2018 15:50

This comment has been minimized.

Sign in to view

PramodBisht force-pushed the issue/53359_b branch 2 times, most recently from b3aaae6 to 9006adc Compare August 31, 2018 16:26

estebank reviewed Aug 31, 2018

View reviewed changes

PramodBisht force-pushed the issue/53359_b branch from 451c01d to 62850f6 Compare August 31, 2018 18:05

This comment has been minimized.

Sign in to view

PramodBisht force-pushed the issue/53359_b branch 3 times, most recently from fc471ba to 527ad26 Compare September 3, 2018 17:59

estebank reviewed Sep 4, 2018

View reviewed changes

estebank approved these changes Sep 4, 2018

View reviewed changes

Fixed 53359: E0432 unresolved import on the same line is now emiting …

21ba03e

…one diagnostic Addressed estebank's comments for 53359

PramodBisht force-pushed the issue/53359_b branch from 527ad26 to 21ba03e Compare September 8, 2018 15:12

PramodBisht changed the title ~~WIP - #53359: putting multiple unresolved import on single line~~ #53359: putting multiple unresolved import on single line Sep 9, 2018

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2018

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 Sep 10, 2018

bors merged commit 21ba03e into rust-lang:master Sep 10, 2018

PramodBisht deleted the issue/53359_b branch September 10, 2018 13:20

#53359: putting multiple unresolved import on single line #53565

#53359: putting multiple unresolved import on single line #53565

Conversation

PramodBisht commented Aug 21, 2018

estebank commented Aug 21, 2018

This comment has been minimized.

This comment has been minimized.

rust-highfive commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PramodBisht Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PramodBisht commented Sep 9, 2018

estebank commented Sep 9, 2018

bors commented Sep 9, 2018

PramodBisht commented Sep 9, 2018

estebank commented Sep 9, 2018

bors commented Sep 9, 2018

bors commented Sep 9, 2018

bors commented Sep 10, 2018

rust-highfive commented Sep 10, 2018

kennytm commented Sep 10, 2018

bors commented Sep 10, 2018

bors commented Sep 10, 2018

PramodBisht Sep 7, 2018 •

edited

Loading