6016: Emit diagnostics for unresolved imports and extern crates r=jonas-schievink a=jonas-schievink

AFAIK, we don't have any major bugs in name resolution that would cause a lot of false positives here (except procedural attribute macro support and some rare issues around `#[path]` on module files), so these are *not* marked as experimental diagnostics right now.

I noticed that diagnostics in a file sometimes don't get displayed after opening, but require some edit to be performed. This seems like a preexisting issue though.

Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>
This commit is contained in:
bors[bot] 2020-09-17 13:00:25 +00:00 committed by GitHub
commit 933fc1eb18
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 445 additions and 77 deletions

View file

@ -28,3 +28,45 @@ impl Diagnostic for UnresolvedModule {
self
}
}
#[derive(Debug)]
pub struct UnresolvedExternCrate {
pub file: HirFileId,
pub item: AstPtr<ast::ExternCrate>,
}
impl Diagnostic for UnresolvedExternCrate {
fn code(&self) -> DiagnosticCode {
DiagnosticCode("unresolved-extern-crate")
}
fn message(&self) -> String {
"unresolved extern crate".to_string()
}
fn display_source(&self) -> InFile<SyntaxNodePtr> {
InFile::new(self.file, self.item.clone().into())
}
fn as_any(&self) -> &(dyn Any + Send + 'static) {
self
}
}
#[derive(Debug)]
pub struct UnresolvedImport {
pub file: HirFileId,
pub node: AstPtr<ast::UseTree>,
}
impl Diagnostic for UnresolvedImport {
fn code(&self) -> DiagnosticCode {
DiagnosticCode("unresolved-import")
}
fn message(&self) -> String {
"unresolved import".to_string()
}
fn display_source(&self) -> InFile<SyntaxNodePtr> {
InFile::new(self.file, self.node.clone().into())
}
fn as_any(&self) -> &(dyn Any + Send + 'static) {
self
}
}

View file

@ -291,7 +291,6 @@ pub enum AttrOwner {
Variant(Idx<Variant>),
Field(Idx<Field>),
// FIXME: Store variant and field attrs, and stop reparsing them in `attrs_query`.
}
macro_rules! from_attrs {
@ -483,6 +482,11 @@ pub struct Import {
/// AST ID of the `use` or `extern crate` item this import was derived from. Note that many
/// `Import`s can map to the same `use` item.
pub ast_id: FileAstId<ast::Use>,
/// Index of this `Import` when the containing `Use` is visited via `ModPath::expand_use_item`.
///
/// This can be used to get the `UseTree` this `Import` corresponds to and allows emitting
/// precise diagnostics.
pub index: usize,
}
#[derive(Debug, Clone, Eq, PartialEq)]

View file

@ -483,7 +483,7 @@ impl Ctx {
ModPath::expand_use_item(
InFile::new(self.file, use_item.clone()),
&self.hygiene,
|path, _tree, is_glob, alias| {
|path, _use_tree, is_glob, alias| {
imports.push(id(tree.imports.alloc(Import {
path,
alias,
@ -491,6 +491,7 @@ impl Ctx {
is_glob,
is_prelude,
ast_id,
index: imports.len(),
})));
},
);

View file

@ -228,9 +228,9 @@ fn smoke() {
top-level items:
#[Attrs { entries: Some([Attr { path: ModPath { kind: Plain, segments: [Name(Text("attr_on_use"))] }, input: None }]) }]
Import { path: ModPath { kind: Plain, segments: [Name(Text("a"))] }, alias: None, visibility: RawVisibilityId("pub(self)"), is_glob: false, is_prelude: false, ast_id: FileAstId::<syntax::ast::generated::nodes::Use>(0) }
Import { path: ModPath { kind: Plain, segments: [Name(Text("a"))] }, alias: None, visibility: RawVisibilityId("pub(self)"), is_glob: false, is_prelude: false, ast_id: FileAstId::<syntax::ast::generated::nodes::Use>(0), index: 0 }
#[Attrs { entries: Some([Attr { path: ModPath { kind: Plain, segments: [Name(Text("attr_on_use"))] }, input: None }]) }]
Import { path: ModPath { kind: Plain, segments: [Name(Text("b"))] }, alias: None, visibility: RawVisibilityId("pub(self)"), is_glob: true, is_prelude: false, ast_id: FileAstId::<syntax::ast::generated::nodes::Use>(0) }
Import { path: ModPath { kind: Plain, segments: [Name(Text("b"))] }, alias: None, visibility: RawVisibilityId("pub(self)"), is_glob: true, is_prelude: false, ast_id: FileAstId::<syntax::ast::generated::nodes::Use>(0), index: 1 }
#[Attrs { entries: Some([Attr { path: ModPath { kind: Plain, segments: [Name(Text("ext_crate"))] }, input: None }]) }]
ExternCrate { path: ModPath { kind: Plain, segments: [Name(Text("krate"))] }, alias: None, visibility: RawVisibilityId("pub(self)"), is_macro_use: false, ast_id: FileAstId::<syntax::ast::generated::nodes::ExternCrate>(1) }
#[Attrs { entries: Some([Attr { path: ModPath { kind: Plain, segments: [Name(Text("on_trait"))] }, input: None }]) }]

View file

@ -288,31 +288,70 @@ pub enum ModuleSource {
mod diagnostics {
use hir_expand::diagnostics::DiagnosticSink;
use hir_expand::hygiene::Hygiene;
use hir_expand::InFile;
use syntax::{ast, AstPtr};
use crate::{db::DefDatabase, diagnostics::UnresolvedModule, nameres::LocalModuleId, AstId};
use crate::path::ModPath;
use crate::{db::DefDatabase, diagnostics::*, nameres::LocalModuleId, AstId};
#[derive(Debug, PartialEq, Eq)]
pub(super) enum DefDiagnostic {
UnresolvedModule {
module: LocalModuleId,
declaration: AstId<ast::Module>,
candidate: String,
},
enum DiagnosticKind {
UnresolvedModule { declaration: AstId<ast::Module>, candidate: String },
UnresolvedExternCrate { ast: AstId<ast::ExternCrate> },
UnresolvedImport { ast: AstId<ast::Use>, index: usize },
}
#[derive(Debug, PartialEq, Eq)]
pub(super) struct DefDiagnostic {
in_module: LocalModuleId,
kind: DiagnosticKind,
}
impl DefDiagnostic {
pub(super) fn unresolved_module(
container: LocalModuleId,
declaration: AstId<ast::Module>,
candidate: String,
) -> Self {
Self {
in_module: container,
kind: DiagnosticKind::UnresolvedModule { declaration, candidate },
}
}
pub(super) fn unresolved_extern_crate(
container: LocalModuleId,
declaration: AstId<ast::ExternCrate>,
) -> Self {
Self {
in_module: container,
kind: DiagnosticKind::UnresolvedExternCrate { ast: declaration },
}
}
pub(super) fn unresolved_import(
container: LocalModuleId,
ast: AstId<ast::Use>,
index: usize,
) -> Self {
Self { in_module: container, kind: DiagnosticKind::UnresolvedImport { ast, index } }
}
pub(super) fn add_to(
&self,
db: &dyn DefDatabase,
target_module: LocalModuleId,
sink: &mut DiagnosticSink,
) {
match self {
DefDiagnostic::UnresolvedModule { module, declaration, candidate } => {
if *module != target_module {
return;
}
if self.in_module != target_module {
return;
}
match &self.kind {
DiagnosticKind::UnresolvedModule { declaration, candidate } => {
let decl = declaration.to_node(db.upcast());
sink.push(UnresolvedModule {
file: declaration.file_id,
@ -320,6 +359,36 @@ mod diagnostics {
candidate: candidate.clone(),
})
}
DiagnosticKind::UnresolvedExternCrate { ast } => {
let item = ast.to_node(db.upcast());
sink.push(UnresolvedExternCrate {
file: ast.file_id,
item: AstPtr::new(&item),
});
}
DiagnosticKind::UnresolvedImport { ast, index } => {
let use_item = ast.to_node(db.upcast());
let hygiene = Hygiene::new(db.upcast(), ast.file_id);
let mut cur = 0;
let mut tree = None;
ModPath::expand_use_item(
InFile::new(ast.file_id, use_item),
&hygiene,
|_mod_path, use_tree, _is_glob, _alias| {
if cur == *index {
tree = Some(use_tree.clone());
}
cur += 1;
},
);
if let Some(tree) = tree {
sink.push(UnresolvedImport { file: ast.file_id, node: AstPtr::new(&tree) });
}
}
}
}
}

View file

@ -5,6 +5,7 @@
use base_db::{CrateId, FileId, ProcMacroId};
use cfg::CfgOptions;
use hir_expand::InFile;
use hir_expand::{
ast_id_map::FileAstId,
builtin_derive::find_builtin_derive,
@ -14,6 +15,7 @@ use hir_expand::{
HirFileId, MacroCallId, MacroDefId, MacroDefKind,
};
use rustc_hash::FxHashMap;
use rustc_hash::FxHashSet;
use syntax::ast;
use test_utils::mark;
@ -21,9 +23,7 @@ use crate::{
attr::Attrs,
db::DefDatabase,
item_scope::{ImportType, PerNsGlobImports},
item_tree::{
self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind,
},
item_tree::{self, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind},
nameres::{
diagnostics::DefDiagnostic, mod_resolution::ModDir, path_resolution::ReachedFixedPoint,
BuiltinShadowMode, CrateDefMap, ModuleData, ModuleOrigin, ResolveMode,
@ -111,6 +111,12 @@ impl PartialResolvedImport {
}
}
#[derive(Clone, Debug, Eq, PartialEq)]
enum ImportSource {
Import(ItemTreeId<item_tree::Import>),
ExternCrate(ItemTreeId<item_tree::ExternCrate>),
}
#[derive(Clone, Debug, Eq, PartialEq)]
struct Import {
pub path: ModPath,
@ -120,11 +126,12 @@ struct Import {
pub is_prelude: bool,
pub is_extern_crate: bool,
pub is_macro_use: bool,
source: ImportSource,
}
impl Import {
fn from_use(tree: &ItemTree, id: FileItemTreeId<item_tree::Import>) -> Self {
let it = &tree[id];
fn from_use(tree: &ItemTree, id: ItemTreeId<item_tree::Import>) -> Self {
let it = &tree[id.value];
let visibility = &tree[it.visibility];
Self {
path: it.path.clone(),
@ -134,11 +141,12 @@ impl Import {
is_prelude: it.is_prelude,
is_extern_crate: false,
is_macro_use: false,
source: ImportSource::Import(id),
}
}
fn from_extern_crate(tree: &ItemTree, id: FileItemTreeId<item_tree::ExternCrate>) -> Self {
let it = &tree[id];
fn from_extern_crate(tree: &ItemTree, id: ItemTreeId<item_tree::ExternCrate>) -> Self {
let it = &tree[id.value];
let visibility = &tree[it.visibility];
Self {
path: it.path.clone(),
@ -148,6 +156,7 @@ impl Import {
is_prelude: false,
is_extern_crate: true,
is_macro_use: it.is_macro_use,
source: ImportSource::ExternCrate(id),
}
}
}
@ -245,9 +254,10 @@ impl DefCollector<'_> {
let unresolved_imports = std::mem::replace(&mut self.unresolved_imports, Vec::new());
// show unresolved imports in completion, etc
for directive in unresolved_imports {
self.record_resolved_import(&directive)
for directive in &unresolved_imports {
self.record_resolved_import(directive)
}
self.unresolved_imports = unresolved_imports;
// Record proc-macros
self.collect_proc_macro();
@ -420,7 +430,11 @@ impl DefCollector<'_> {
.as_ident()
.expect("extern crate should have been desugared to one-element path"),
);
PartialResolvedImport::Resolved(res)
if res.is_none() {
PartialResolvedImport::Unresolved
} else {
PartialResolvedImport::Resolved(res)
}
} else {
let res = self.def_map.resolve_path_fp_with_macro(
self.db,
@ -774,7 +788,51 @@ impl DefCollector<'_> {
.collect(item_tree.top_level_items());
}
fn finish(self) -> CrateDefMap {
fn finish(mut self) -> CrateDefMap {
// Emit diagnostics for all remaining unresolved imports.
// We'd like to avoid emitting a diagnostics avalanche when some `extern crate` doesn't
// resolve. We first emit diagnostics for unresolved extern crates and collect the missing
// crate names. Then we emit diagnostics for unresolved imports, but only if the import
// doesn't start with an unresolved crate's name. Due to renaming and reexports, this is a
// heuristic, but it works in practice.
let mut diagnosed_extern_crates = FxHashSet::default();
for directive in &self.unresolved_imports {
if let ImportSource::ExternCrate(krate) = directive.import.source {
let item_tree = self.db.item_tree(krate.file_id);
let extern_crate = &item_tree[krate.value];
diagnosed_extern_crates.insert(extern_crate.path.segments[0].clone());
self.def_map.diagnostics.push(DefDiagnostic::unresolved_extern_crate(
directive.module_id,
InFile::new(krate.file_id, extern_crate.ast_id),
));
}
}
for directive in &self.unresolved_imports {
if let ImportSource::Import(import) = &directive.import.source {
let item_tree = self.db.item_tree(import.file_id);
let import_data = &item_tree[import.value];
match (import_data.path.segments.first(), &import_data.path.kind) {
(Some(krate), PathKind::Plain) | (Some(krate), PathKind::Abs) => {
if diagnosed_extern_crates.contains(krate) {
continue;
}
}
_ => {}
}
self.def_map.diagnostics.push(DefDiagnostic::unresolved_import(
directive.module_id,
InFile::new(import.file_id, import_data.ast_id),
import_data.index,
));
}
}
self.def_map
}
}
@ -830,14 +888,20 @@ impl ModCollector<'_, '_> {
ModItem::Import(import_id) => {
self.def_collector.unresolved_imports.push(ImportDirective {
module_id: self.module_id,
import: Import::from_use(&self.item_tree, import_id),
import: Import::from_use(
&self.item_tree,
InFile::new(self.file_id, import_id),
),
status: PartialResolvedImport::Unresolved,
})
}
ModItem::ExternCrate(import_id) => {
self.def_collector.unresolved_imports.push(ImportDirective {
module_id: self.module_id,
import: Import::from_extern_crate(&self.item_tree, import_id),
import: Import::from_extern_crate(
&self.item_tree,
InFile::new(self.file_id, import_id),
),
status: PartialResolvedImport::Unresolved,
})
}
@ -1051,13 +1115,11 @@ impl ModCollector<'_, '_> {
self.import_all_legacy_macros(module_id);
}
}
Err(candidate) => self.def_collector.def_map.diagnostics.push(
DefDiagnostic::UnresolvedModule {
module: self.module_id,
declaration: ast_id,
candidate,
},
),
Err(candidate) => {
self.def_collector.def_map.diagnostics.push(
DefDiagnostic::unresolved_module(self.module_id, ast_id, candidate),
);
}
};
}
}

View file

@ -2,6 +2,7 @@ mod globs;
mod incremental;
mod macros;
mod mod_resolution;
mod diagnostics;
mod primitives;
use std::sync::Arc;

View file

@ -0,0 +1,131 @@
use base_db::fixture::WithFixture;
use base_db::FileId;
use base_db::SourceDatabaseExt;
use hir_expand::db::AstDatabase;
use rustc_hash::FxHashMap;
use syntax::TextRange;
use syntax::TextSize;
use crate::test_db::TestDB;
fn check_diagnostics(ra_fixture: &str) {
let db: TestDB = TestDB::with_files(ra_fixture);
let annotations = db.extract_annotations();
assert!(!annotations.is_empty());
let mut actual: FxHashMap<FileId, Vec<(TextRange, String)>> = FxHashMap::default();
db.diagnostics(|d| {
let src = d.display_source();
let root = db.parse_or_expand(src.file_id).unwrap();
// FIXME: macros...
let file_id = src.file_id.original_file(&db);
let range = src.value.to_node(&root).text_range();
let message = d.message().to_owned();
actual.entry(file_id).or_default().push((range, message));
});
for (file_id, diags) in actual.iter_mut() {
diags.sort_by_key(|it| it.0.start());
let text = db.file_text(*file_id);
// For multiline spans, place them on line start
for (range, content) in diags {
if text[*range].contains('\n') {
*range = TextRange::new(range.start(), range.start() + TextSize::from(1));
*content = format!("... {}", content);
}
}
}
assert_eq!(annotations, actual);
}
#[test]
fn unresolved_import() {
check_diagnostics(
r"
use does_exist;
use does_not_exist;
//^^^^^^^^^^^^^^ unresolved import
mod does_exist {}
",
);
}
#[test]
fn unresolved_import_in_use_tree() {
// Only the relevant part of a nested `use` item should be highlighted.
check_diagnostics(
r"
use does_exist::{Exists, DoesntExist};
//^^^^^^^^^^^ unresolved import
use {does_not_exist::*, does_exist};
//^^^^^^^^^^^^^^^^^ unresolved import
use does_not_exist::{
a,
//^ unresolved import
b,
//^ unresolved import
c,
//^ unresolved import
};
mod does_exist {
pub struct Exists;
}
",
);
}
#[test]
fn unresolved_extern_crate() {
check_diagnostics(
r"
//- /main.rs crate:main deps:core
extern crate core;
extern crate doesnotexist;
//^^^^^^^^^^^^^^^^^^^^^^^^^^ unresolved extern crate
//- /lib.rs crate:core
",
);
}
#[test]
fn dedup_unresolved_import_from_unresolved_crate() {
check_diagnostics(
r"
//- /main.rs crate:main
mod a {
extern crate doesnotexist;
//^^^^^^^^^^^^^^^^^^^^^^^^^^ unresolved extern crate
// Should not error, since we already errored for the missing crate.
use doesnotexist::{self, bla, *};
use crate::doesnotexist;
//^^^^^^^^^^^^^^^^^^^ unresolved import
}
mod m {
use super::doesnotexist;
//^^^^^^^^^^^^^^^^^^^ unresolved import
}
",
);
}
#[test]
fn unresolved_module() {
check_diagnostics(
r"
//- /lib.rs
mod foo;
mod bar;
//^^^^^^^^ unresolved module
mod baz {}
//- /foo.rs
",
);
}

View file

@ -671,42 +671,6 @@ pub struct Baz;
);
}
#[test]
fn unresolved_module_diagnostics() {
let db = TestDB::with_files(
r"
//- /lib.rs
mod foo;
mod bar;
mod baz {}
//- /foo.rs
",
);
let krate = db.test_crate();
let crate_def_map = db.crate_def_map(krate);
expect![[r#"
[
UnresolvedModule {
module: Idx::<ModuleData>(0),
declaration: InFile {
file_id: HirFileId(
FileId(
FileId(
0,
),
),
),
value: FileAstId::<syntax::ast::generated::nodes::Module>(1),
},
candidate: "bar.rs",
},
]
"#]]
.assert_debug_eq(&crate_def_map.diagnostics);
}
#[test]
fn module_resolution_decl_inside_module_in_non_crate_root_2() {
check(

View file

@ -5,9 +5,15 @@ use std::{
sync::{Arc, Mutex},
};
use base_db::SourceDatabase;
use base_db::{salsa, CrateId, FileId, FileLoader, FileLoaderDelegate, Upcast};
use hir_expand::db::AstDatabase;
use hir_expand::diagnostics::Diagnostic;
use hir_expand::diagnostics::DiagnosticSinkBuilder;
use rustc_hash::FxHashMap;
use rustc_hash::FxHashSet;
use syntax::TextRange;
use test_utils::extract_annotations;
use crate::db::DefDatabase;
@ -98,4 +104,40 @@ impl TestDB {
})
.collect()
}
pub fn extract_annotations(&self) -> FxHashMap<FileId, Vec<(TextRange, String)>> {
let mut files = Vec::new();
let crate_graph = self.crate_graph();
for krate in crate_graph.iter() {
let crate_def_map = self.crate_def_map(krate);
for (module_id, _) in crate_def_map.modules.iter() {
let file_id = crate_def_map[module_id].origin.file_id();
files.extend(file_id)
}
}
assert!(!files.is_empty());
files
.into_iter()
.filter_map(|file_id| {
let text = self.file_text(file_id);
let annotations = extract_annotations(&text);
if annotations.is_empty() {
return None;
}
Some((file_id, annotations))
})
.collect()
}
pub fn diagnostics<F: FnMut(&dyn Diagnostic)>(&self, mut cb: F) {
let crate_graph = self.crate_graph();
for krate in crate_graph.iter() {
let crate_def_map = self.crate_def_map(krate);
let mut sink = DiagnosticSinkBuilder::new().build(&mut cb);
for (module_id, _) in crate_def_map.modules.iter() {
crate_def_map.add_diagnostics(self, module_id, &mut sink);
}
}
}
}

View file

@ -622,13 +622,65 @@ pub struct Foo { pub a: i32, pub b: i32 }
r#"
use a;
use a::{c, d::e};
mod a {
mod c {}
mod d {
mod e {}
}
}
"#,
);
check_fix(r#"use {<|>b};"#, r#"use b;"#);
check_fix(r#"use {b<|>};"#, r#"use b;"#);
check_fix(r#"use a::{c<|>};"#, r#"use a::c;"#);
check_fix(r#"use a::{self<|>};"#, r#"use a;"#);
check_fix(r#"use a::{c, d::{e<|>}};"#, r#"use a::{c, d::e};"#);
check_fix(
r"
mod b {}
use {<|>b};
",
r"
mod b {}
use b;
",
);
check_fix(
r"
mod b {}
use {b<|>};
",
r"
mod b {}
use b;
",
);
check_fix(
r"
mod a { mod c {} }
use a::{c<|>};
",
r"
mod a { mod c {} }
use a::c;
",
);
check_fix(
r"
mod a {}
use a::{self<|>};
",
r"
mod a {}
use a;
",
);
check_fix(
r"
mod a { mod c {} mod d { mod e {} } }
use a::{c, d::{e<|>}};
",
r"
mod a { mod c {} mod d { mod e {} } }
use a::{c, d::e};
",
);
}
#[test]