Skip to content

Commit

Permalink
Auto merge of #55884 - petrochenkov:uni, r=eddyb
Browse files Browse the repository at this point in the history
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
  • Loading branch information
bors committed Nov 18, 2018
2 parents d4de9b1 + b366415 commit d08a0a8
Show file tree
Hide file tree
Showing 187 changed files with 2,224 additions and 1,516 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ introducing `extern crate` items, using keyword `extern`.

For example, `extern::my_crat::a::b` will resolve to path `a::b` in crate `my_crate`.

`feature(extern_absolute_paths)` mode provides the same effect by resolving absolute paths like
`::my_crate::a::b` to paths from extern crates by default.
Absolute paths on 2018 edition (e.g. `::my_crate::a::b`) provide the same effect
and resolve to extern crates (built-in or passed with `--extern`).

```rust,ignore
#![feature(extern_in_paths)]
Expand Down
31 changes: 19 additions & 12 deletions src/librustc/hir/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,6 @@ impl PathResolution {
pub fn unresolved_segments(&self) -> usize {
self.unresolved_segments
}

pub fn kind_name(&self) -> &'static str {
if self.unresolved_segments != 0 {
"associated item"
} else {
self.base_def.kind_name()
}
}
}

/// Different kinds of symbols don't influence each other.
Expand Down Expand Up @@ -269,27 +261,33 @@ impl NonMacroAttrKind {

impl Def {
pub fn def_id(&self) -> DefId {
self.opt_def_id().unwrap_or_else(|| {
bug!("attempted .def_id() on invalid def: {:?}", self)
})
}

pub fn opt_def_id(&self) -> Option<DefId> {
match *self {
Def::Fn(id) | Def::Mod(id) | Def::Static(id, _) |
Def::Variant(id) | Def::VariantCtor(id, ..) | Def::Enum(id) |
Def::TyAlias(id) | Def::TraitAlias(id) |
Def::AssociatedTy(id) | Def::TyParam(id) | Def::Struct(id) | Def::StructCtor(id, ..) |
Def::Union(id) | Def::Trait(id) | Def::Method(id) | Def::Const(id) |
Def::AssociatedConst(id) | Def::Macro(id, ..) |
Def::Existential(id) | Def::AssociatedExistential(id) | Def::ForeignTy(id) |
Def::SelfCtor(id) => {
id
Def::Existential(id) | Def::AssociatedExistential(id) | Def::ForeignTy(id) => {
Some(id)
}

Def::Local(..) |
Def::Upvar(..) |
Def::Label(..) |
Def::PrimTy(..) |
Def::SelfTy(..) |
Def::SelfCtor(..) |
Def::ToolMod |
Def::NonMacroAttr(..) |
Def::Err => {
bug!("attempted .def_id() on invalid def: {:?}", self)
None
}
}
}
Expand Down Expand Up @@ -333,4 +331,13 @@ impl Def {
Def::Err => "unresolved item",
}
}

pub fn article(&self) -> &'static str {
match *self {
Def::AssociatedTy(..) | Def::AssociatedConst(..) | Def::AssociatedExistential(..) |
Def::Enum(..) | Def::Existential(..) | Def::Err => "an",
Def::Macro(.., macro_kind) => macro_kind.article(),
_ => "a",
}
}
}
4 changes: 2 additions & 2 deletions src/librustc/hir/def_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ pub struct DefId {

impl fmt::Debug for DefId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "DefId({:?}/{}:{}",
self.krate.index(),
write!(f, "DefId({}/{}:{}",
self.krate,
self.index.address_space().index(),
self.index.as_array_index())?;

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/dead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
self.check_def_id(def.def_id());
}
_ if self.in_pat => (),
Def::PrimTy(..) | Def::SelfTy(..) |
Def::PrimTy(..) | Def::SelfTy(..) | Def::SelfCtor(..) |
Def::Local(..) | Def::Upvar(..) => {}
Def::Variant(variant_id) | Def::VariantCtor(variant_id, ..) => {
if let Some(enum_id) = self.tcx.parent_def_id(variant_id) {
Expand Down
6 changes: 2 additions & 4 deletions src/librustc/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,10 +781,8 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {

fn visit_path(&mut self, path: &'tcx hir::Path, id: hir::HirId) {
let id = self.tcx.hir.hir_to_node_id(id);
match path.def {
Def::Local(..) | Def::Upvar(..) | Def::SelfCtor(..) |
Def::PrimTy(..) | Def::SelfTy(..) | Def::Err => {}
_ => self.tcx.check_stability(path.def.def_id(), Some(id), path.span)
if let Some(def_id) = path.def.opt_def_id() {
self.tcx.check_stability(def_id, Some(id), path.span)
}
intravisit::walk_path(self, path)
}
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/hair/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,8 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
let label_msg = match pat.node {
PatKind::Path(hir::QPath::Resolved(None, ref path))
if path.segments.len() == 1 && path.segments[0].args.is_none() => {
format!("interpreted as a {} pattern, not new variable", path.def.kind_name())
format!("interpreted as {} {} pattern, not new variable",
path.def.article(), path.def.kind_name())
}
_ => format!("pattern `{}` not covered", pattern_string),
};
Expand Down
8 changes: 5 additions & 3 deletions src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,9 +361,11 @@ impl<'a, 'tcx> Visitor<'tcx> for EmbargoVisitor<'a, 'tcx> {
let def_id = self.tcx.hir.local_def_id(id);
if let Some(exports) = self.tcx.module_exports(def_id) {
for export in exports.iter() {
if let Some(node_id) = self.tcx.hir.as_local_node_id(export.def.def_id()) {
if export.vis == ty::Visibility::Public {
self.update(node_id, Some(AccessLevel::Exported));
if export.vis == ty::Visibility::Public {
if let Some(def_id) = export.def.opt_def_id() {
if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) {
self.update(node_id, Some(AccessLevel::Exported));
}
}
}
}
Expand Down
Loading

0 comments on commit d08a0a8

Please sign in to comment.