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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 10, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,6 @@ enum ResolutionError<'a> {
SelfImportCanOnlyAppearOnceInTheList,
/// error E0431: `self` import can only appear in an import list with a non-empty prefix
SelfImportOnlyInImportListWithNonEmptyPrefix,
/// error E0432: unresolved import
UnresolvedImport(Option<(Span, &'a str, &'a str)>),
/// error E0433: failed to resolve
FailedToResolve(&'a str),
/// error E0434: can't capture dynamic environment in a fn item
Expand Down Expand Up @@ -357,17 +355,6 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver,
err.span_label(span, "can only appear in an import list with a non-empty prefix");
err
}
ResolutionError::UnresolvedImport(name) => {
let (span, msg) = match name {
Some((sp, n, _)) => (sp, format!("unresolved import `{}`", n)),
None => (span, "unresolved import".to_owned()),
};
let mut err = struct_span_err!(resolver.session, span, E0432, "{}", msg);
if let Some((_, _, p)) = name {
err.span_label(span, p);
}
err
}
ResolutionError::FailedToResolve(msg) => {
let mut err = struct_span_err!(resolver.session, span, E0433,
"failed to resolve. {}", msg);
Expand Down
50 changes: 44 additions & 6 deletions src/librustc_resolve/resolve_imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use syntax::ext::base::Determinacy::{self, Determined, Undetermined};
use syntax::ext::hygiene::Mark;
use syntax::symbol::keywords;
use syntax::util::lev_distance::find_best_match_for_name;
use syntax_pos::Span;
use syntax_pos::{MultiSpan, Span};

use std::cell::{Cell, RefCell};
use std::collections::BTreeMap;
Expand Down Expand Up @@ -635,6 +635,8 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {

let mut errors = false;
let mut seen_spans = FxHashSet();
let mut error_vec = Vec::new();
let mut prev_root_id: NodeId = NodeId::new(0);
for i in 0 .. self.determined_imports.len() {
let import = self.determined_imports[i];
let error = self.finalize_import(import);
Expand Down Expand Up @@ -689,13 +691,22 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
// If the error is a single failed import then create a "fake" import
// resolution for it so that later resolve stages won't complain.
self.import_dummy_binding(import);
if prev_root_id.as_u32() != 0 &&
prev_root_id.as_u32() != import.root_id.as_u32() &&
!error_vec.is_empty(){
// in case of new import line, throw diagnostic message
// for previous line.
let mut empty_vec = vec![];
mem::swap(&mut empty_vec, &mut error_vec);
self.throw_unresolved_import_error(empty_vec, None);
}
if !seen_spans.contains(&span) {
let path = import_path_to_string(&import.module_path[..],
&import.subclass,
span);
let error = ResolutionError::UnresolvedImport(Some((span, &path, &err)));
resolve_error(self.resolver, span, error);
error_vec.push((span, path, err));
seen_spans.insert(span);
prev_root_id = import.root_id;
}
}
}
Expand Down Expand Up @@ -760,21 +771,48 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
});
}

if !error_vec.is_empty() {
self.throw_unresolved_import_error(error_vec.clone(), None);
}

// Report unresolved imports only if no hard error was already reported
// to avoid generating multiple errors on the same import.
if !errors {
for import in &self.indeterminate_imports {
if import.is_uniform_paths_canary {
continue;
}

let error = ResolutionError::UnresolvedImport(None);
resolve_error(self.resolver, import.span, error);
self.throw_unresolved_import_error(error_vec, Some(MultiSpan::from(import.span)));
break;
}
}
}

fn throw_unresolved_import_error(&self, error_vec: Vec<(Span, String, String)>,
span: Option<MultiSpan>) {
let max_span_label_msg_count = 10; // upper limit on number of span_label message.
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()
.map(|elem: (Span, String, String)| { elem.0 }
).collect());
let path_vec: Vec<String> = error_vec.clone().into_iter()
.map(|elem: (Span, String, String)| { format!("`{}`", elem.1) }
).collect();
let path = path_vec.join(", ");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estebank What should be upper limit of path_vec here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PramodBisht PramodBisht Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

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 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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let msg = format!("unresolved import{} {}",
if path_vec.len() > 1 { "s" } else { "" }, path);
(span, msg)
}
};
let mut err = struct_span_err!(self.resolver.session, span, E0432, "{}", &msg);
for span_error in error_vec.into_iter().take(max_span_label_msg_count) {
err.span_label(span_error.0, span_error.2);
}
err.emit();
}

/// Attempts to resolve the given import, returning true if its resolution is determined.
/// If successful, the resolved bindings are written into the module.
fn resolve_import(&mut self, directive: &'b ImportDirective<'b>) -> bool {
Expand Down
16 changes: 8 additions & 8 deletions src/test/ui/dollar-crate/dollar-crate-is-keyword-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
error[E0432]: unresolved import `a::$crate`
--> $DIR/dollar-crate-is-keyword-2.rs:15:13
error[E0433]: failed to resolve. `$crate` in paths can only be used in start position
--> $DIR/dollar-crate-is-keyword-2.rs:16:16
|
LL | use a::$crate; //~ ERROR unresolved import `a::$crate`
| ^^^^^^^^^ no `$crate` in `a`
LL | use a::$crate::b; //~ ERROR `$crate` in paths can only be used in start position
| ^^^^^^ `$crate` in paths can only be used in start position
...
LL | m!();
| ----- in this macro invocation

error[E0433]: failed to resolve. `$crate` in paths can only be used in start position
--> $DIR/dollar-crate-is-keyword-2.rs:16:16
error[E0432]: unresolved import `a::$crate`
--> $DIR/dollar-crate-is-keyword-2.rs:15:13
|
LL | use a::$crate::b; //~ ERROR `$crate` in paths can only be used in start position
| ^^^^^^ `$crate` in paths can only be used in start position
LL | use a::$crate; //~ ERROR unresolved import `a::$crate`
| ^^^^^^^^^ no `$crate` in `a`
...
LL | m!();
| ----- in this macro invocation
Expand Down
14 changes: 14 additions & 0 deletions src/test/ui/issue-53565.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
use std::time::{foo, bar, buzz};
use std::time::{abc, def};
fn main(){
println!("Hello World!");
}
20 changes: 20 additions & 0 deletions src/test/ui/issue-53565.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error[E0432]: unresolved imports `std::time::foo`, `std::time::bar`, `std::time::buzz`
--> $DIR/issue-53565.rs:10:17
|
LL | use std::time::{foo, bar, buzz};
| ^^^ ^^^ ^^^^ no `buzz` in `time`
| | |
| | no `bar` in `time`
| no `foo` in `time`

error[E0432]: unresolved imports `std::time::abc`, `std::time::def`
--> $DIR/issue-53565.rs:11:17
|
LL | use std::time::{abc, def};
| ^^^ ^^^ no `def` in `time`
| |
| no `abc` in `time`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0432`.