Skip to content

Commit

Permalink
Rollup merge of rust-lang#63400 - petrochenkov:resplit, r=eddyb
Browse files Browse the repository at this point in the history
Try to break resolve into more isolated parts

Some small step towards resolve librarification.

"Late resolution" is the pass that resolves most of names in a crate beside imports and macros.
It runs when the crate is fully expanded and its module structure is fully built.
So we just walk through the crate and resolve all the expressions, types, etc.

This pass is pretty self-contained, but it was previously done by implementing `Visitor` on the whole `Resolver` (which is used for many other tasks), and fields specific to this pass were indiscernible from the global `Resolver` state.

This PR moves the late resolution pass into a separate visitor and a separate file, fields specific to this visitor are moved from `Resolver` as well.

I'm especially happy about `current_module` being removed from `Resolver`.
It was used even for operations not related to visiting and changing the `current_module` position in process.
It was also used as an implicit argument for some functions used in this style
```rust
let orig_current_module = mem::replace(&mut self.current_module, module);
self.resolve_ident_somewhere();
self.current_module = orig_current_module;
```
and having effects on e.g. privacy checking somewhere deeply inside `resolve_ident_somewhere`.
Now we explicitly pass a `ParentScope` to those functions instead, which includes the module and some other data describing our position in the crate relatively to which we resolve names.

Rustdoc was one of the users of `current_module`, it set it for resolving intra-doc links.
Now it passes it explicitly as an argument as well (I also supported resolving paths from rustdoc in unnamed blocks as a drive-by fix).

Visibility resolution is also changed to use early resolution (which is correct because it's used during the work of `BuildReducedGraphVisitor`, i.e. integration of a new AST fragment into the existing partially built module structures.) instead of untimely late resolution (which worked only due to restrictions on paths in visibilities like inability to refer to anything except ancestor modules).
This slightly regresses its diagnostics because late resolution has a more systematic error detection and recovery currently.
Due to changes in `current_module` and visibilities `BuildReducedGraphVisitor` ended up almost as heavily affected by this refactoring as late resolution.

Fixes rust-lang#63223 (due to visibility resolution changes).
  • Loading branch information
Centril authored Aug 10, 2019
2 parents d19a359 + 319f0de commit 9d76a93
Show file tree
Hide file tree
Showing 18 changed files with 3,982 additions and 3,884 deletions.
15 changes: 4 additions & 11 deletions src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use crate::hir::{self, ParamName};
use crate::hir::HirVec;
use crate::hir::map::{DefKey, DefPathData, Definitions};
use crate::hir::def_id::{DefId, DefIndex, CRATE_DEF_INDEX};
use crate::hir::def::{Res, DefKind, PartialRes, PerNS};
use crate::hir::def::{Namespace, Res, DefKind, PartialRes, PerNS};
use crate::hir::{GenericArg, ConstArg};
use crate::hir::ptr::P;
use crate::lint::builtin::{self, PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,
Expand Down Expand Up @@ -148,13 +148,6 @@ pub struct LoweringContext<'a> {
}

pub trait Resolver {
/// Resolve a path generated by the lowerer when expanding `for`, `if let`, etc.
fn resolve_ast_path(
&mut self,
path: &ast::Path,
is_value: bool,
) -> Res<NodeId>;

/// Obtain resolution for a `NodeId` with a single resolution.
fn get_partial_res(&mut self, id: NodeId) -> Option<PartialRes>;

Expand All @@ -175,7 +168,7 @@ pub trait Resolver {
span: Span,
crate_root: Option<Symbol>,
components: &[Symbol],
is_value: bool,
ns: Namespace,
) -> (ast::Path, Res<NodeId>);

fn has_derives(&self, node_id: NodeId, derives: SpecialDerives) -> bool;
Expand Down Expand Up @@ -5717,8 +5710,8 @@ impl<'a> LoweringContext<'a> {
params: Option<P<hir::GenericArgs>>,
is_value: bool,
) -> hir::Path {
let (path, res) = self.resolver
.resolve_str_path(span, self.crate_root, components, is_value);
let ns = if is_value { Namespace::ValueNS } else { Namespace::TypeNS };
let (path, res) = self.resolver.resolve_str_path(span, self.crate_root, components, ns);

let mut segments: Vec<_> = path.segments.iter().map(|segment| {
let res = self.expect_full_res(segment.id);
Expand Down
750 changes: 480 additions & 270 deletions src/librustc_resolve/build_reduced_graph.rs

Large diffs are not rendered by default.

201 changes: 91 additions & 110 deletions src/librustc_resolve/check_unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
// - `check_crate` finally emits the diagnostics based on the data generated
// in the last step

use std::ops::{Deref, DerefMut};

use crate::Resolver;
use crate::resolve_imports::ImportDirectiveSubclass;

Expand All @@ -49,45 +47,30 @@ impl<'a> UnusedImport<'a> {
}

struct UnusedImportCheckVisitor<'a, 'b> {
resolver: &'a mut Resolver<'b>,
r: &'a mut Resolver<'b>,
/// All the (so far) unused imports, grouped path list
unused_imports: NodeMap<UnusedImport<'a>>,
base_use_tree: Option<&'a ast::UseTree>,
base_id: ast::NodeId,
item_span: Span,
}

// Deref and DerefMut impls allow treating UnusedImportCheckVisitor as Resolver.
impl<'a, 'b> Deref for UnusedImportCheckVisitor<'a, 'b> {
type Target = Resolver<'b>;

fn deref<'c>(&'c self) -> &'c Resolver<'b> {
&*self.resolver
}
}

impl<'a, 'b> DerefMut for UnusedImportCheckVisitor<'a, 'b> {
fn deref_mut<'c>(&'c mut self) -> &'c mut Resolver<'b> {
&mut *self.resolver
}
}

impl<'a, 'b> UnusedImportCheckVisitor<'a, 'b> {
// We have information about whether `use` (import) directives are actually
// used now. If an import is not used at all, we signal a lint error.
fn check_import(&mut self, id: ast::NodeId) {
let mut used = false;
self.per_ns(|this, ns| used |= this.used_imports.contains(&(id, ns)));
self.r.per_ns(|this, ns| used |= this.used_imports.contains(&(id, ns)));
if !used {
if self.maybe_unused_trait_imports.contains(&id) {
if self.r.maybe_unused_trait_imports.contains(&id) {
// Check later.
return;
}
self.unused_import(self.base_id).add(id);
} else {
// This trait import is definitely used, in a way other than
// method resolution.
self.maybe_unused_trait_imports.remove(&id);
self.r.maybe_unused_trait_imports.remove(&id);
if let Some(i) = self.unused_imports.get_mut(&self.base_id) {
i.unused.remove(&id);
}
Expand Down Expand Up @@ -238,104 +221,102 @@ fn calc_unused_spans(
}
}

pub fn check_crate(resolver: &mut Resolver<'_>, krate: &ast::Crate) {
for directive in resolver.potentially_unused_imports.iter() {
match directive.subclass {
_ if directive.used.get() ||
directive.vis.get() == ty::Visibility::Public ||
directive.span.is_dummy() => {
if let ImportDirectiveSubclass::MacroUse = directive.subclass {
if !directive.span.is_dummy() {
resolver.session.buffer_lint(
lint::builtin::MACRO_USE_EXTERN_CRATE,
directive.id,
directive.span,
"deprecated `#[macro_use]` directive used to \
import macros should be replaced at use sites \
with a `use` statement to import the macro \
instead",
);
impl Resolver<'_> {
crate fn check_unused(&mut self, krate: &ast::Crate) {
for directive in self.potentially_unused_imports.iter() {
match directive.subclass {
_ if directive.used.get() ||
directive.vis.get() == ty::Visibility::Public ||
directive.span.is_dummy() => {
if let ImportDirectiveSubclass::MacroUse = directive.subclass {
if !directive.span.is_dummy() {
self.session.buffer_lint(
lint::builtin::MACRO_USE_EXTERN_CRATE,
directive.id,
directive.span,
"deprecated `#[macro_use]` directive used to \
import macros should be replaced at use sites \
with a `use` statement to import the macro \
instead",
);
}
}
}
ImportDirectiveSubclass::ExternCrate { .. } => {
self.maybe_unused_extern_crates.push((directive.id, directive.span));
}
ImportDirectiveSubclass::MacroUse => {
let lint = lint::builtin::UNUSED_IMPORTS;
let msg = "unused `#[macro_use]` import";
self.session.buffer_lint(lint, directive.id, directive.span, msg);
}
_ => {}
}
ImportDirectiveSubclass::ExternCrate { .. } => {
resolver.maybe_unused_extern_crates.push((directive.id, directive.span));
}
ImportDirectiveSubclass::MacroUse => {
let lint = lint::builtin::UNUSED_IMPORTS;
let msg = "unused `#[macro_use]` import";
resolver.session.buffer_lint(lint, directive.id, directive.span, msg);
}
_ => {}
}
}

for (id, span) in resolver.unused_labels.iter() {
resolver.session.buffer_lint(lint::builtin::UNUSED_LABELS, *id, *span, "unused label");
}

let mut visitor = UnusedImportCheckVisitor {
resolver,
unused_imports: Default::default(),
base_use_tree: None,
base_id: ast::DUMMY_NODE_ID,
item_span: DUMMY_SP,
};
visit::walk_crate(&mut visitor, krate);

for unused in visitor.unused_imports.values() {
let mut fixes = Vec::new();
let mut spans = match calc_unused_spans(unused, unused.use_tree, unused.use_tree_id) {
UnusedSpanResult::Used => continue,
UnusedSpanResult::FlatUnused(span, remove) => {
fixes.push((remove, String::new()));
vec![span]
}
UnusedSpanResult::NestedFullUnused(spans, remove) => {
fixes.push((remove, String::new()));
spans
}
UnusedSpanResult::NestedPartialUnused(spans, remove) => {
for fix in &remove {
fixes.push((*fix, String::new()));
}
spans
}
let mut visitor = UnusedImportCheckVisitor {
r: self,
unused_imports: Default::default(),
base_use_tree: None,
base_id: ast::DUMMY_NODE_ID,
item_span: DUMMY_SP,
};
visit::walk_crate(&mut visitor, krate);

let len = spans.len();
spans.sort();
let ms = MultiSpan::from_spans(spans.clone());
let mut span_snippets = spans.iter()
.filter_map(|s| {
match visitor.session.source_map().span_to_snippet(*s) {
Ok(s) => Some(format!("`{}`", s)),
_ => None,
for unused in visitor.unused_imports.values() {
let mut fixes = Vec::new();
let mut spans = match calc_unused_spans(unused, unused.use_tree, unused.use_tree_id) {
UnusedSpanResult::Used => continue,
UnusedSpanResult::FlatUnused(span, remove) => {
fixes.push((remove, String::new()));
vec![span]
}
}).collect::<Vec<String>>();
span_snippets.sort();
let msg = format!("unused import{}{}",
if len > 1 { "s" } else { "" },
if !span_snippets.is_empty() {
format!(": {}", span_snippets.join(", "))
} else {
String::new()
});
UnusedSpanResult::NestedFullUnused(spans, remove) => {
fixes.push((remove, String::new()));
spans
}
UnusedSpanResult::NestedPartialUnused(spans, remove) => {
for fix in &remove {
fixes.push((*fix, String::new()));
}
spans
}
};

let fix_msg = if fixes.len() == 1 && fixes[0].0 == unused.item_span {
"remove the whole `use` item"
} else if spans.len() > 1 {
"remove the unused imports"
} else {
"remove the unused import"
};
let len = spans.len();
spans.sort();
let ms = MultiSpan::from_spans(spans.clone());
let mut span_snippets = spans.iter()
.filter_map(|s| {
match visitor.r.session.source_map().span_to_snippet(*s) {
Ok(s) => Some(format!("`{}`", s)),
_ => None,
}
}).collect::<Vec<String>>();
span_snippets.sort();
let msg = format!("unused import{}{}",
if len > 1 { "s" } else { "" },
if !span_snippets.is_empty() {
format!(": {}", span_snippets.join(", "))
} else {
String::new()
});

visitor.session.buffer_lint_with_diagnostic(
lint::builtin::UNUSED_IMPORTS,
unused.use_tree_id,
ms,
&msg,
lint::builtin::BuiltinLintDiagnostics::UnusedImports(fix_msg.into(), fixes),
);
let fix_msg = if fixes.len() == 1 && fixes[0].0 == unused.item_span {
"remove the whole `use` item"
} else if spans.len() > 1 {
"remove the unused imports"
} else {
"remove the unused import"
};

visitor.r.session.buffer_lint_with_diagnostic(
lint::builtin::UNUSED_IMPORTS,
unused.use_tree_id,
ms,
&msg,
lint::builtin::BuiltinLintDiagnostics::UnusedImports(fix_msg.into(), fixes),
);
}
}
}
Loading

0 comments on commit 9d76a93

Please sign in to comment.