]> git.lizzy.rs Git - rust.git/commitdiff
resolve: Stop generating uniform path canaries
authorVadim Petrochenkov <vadim.petrochenkov@gmail.com>
Sat, 3 Nov 2018 16:41:44 +0000 (19:41 +0300)
committerVadim Petrochenkov <vadim.petrochenkov@gmail.com>
Sun, 18 Nov 2018 10:49:31 +0000 (13:49 +0300)
src/librustc_resolve/build_reduced_graph.rs
src/librustc_resolve/resolve_imports.rs

index bb451f554d697b6fce15b1cea5dfa9e11d8a8fcd..858e6dc623248db8e7a9f2c8d73d3faf5887803e 100644 (file)
@@ -116,17 +116,14 @@ fn build_reduced_graph_for_use_tree(
         id: NodeId,
         parent_prefix: &[Segment],
         nested: bool,
-        mut uniform_paths_canary_emitted: bool,
         // The whole `use` item
         parent_scope: ParentScope<'a>,
         item: &Item,
         vis: ty::Visibility,
         root_span: Span,
     ) {
-        debug!("build_reduced_graph_for_use_tree(parent_prefix={:?}, \
-                uniform_paths_canary_emitted={}, \
-                use_tree={:?}, nested={})",
-               parent_prefix, uniform_paths_canary_emitted, use_tree, nested);
+        debug!("build_reduced_graph_for_use_tree(parent_prefix={:?}, use_tree={:?}, nested={})",
+               parent_prefix, use_tree, nested);
 
         let uniform_paths =
             self.session.rust_2018() &&
@@ -158,101 +155,6 @@ fn build_reduced_graph_for_use_tree(
         let prefix: Vec<_> = root.into_iter().chain(prefix_iter()).collect();
 
         debug!("build_reduced_graph_for_use_tree: prefix={:?}", prefix);
-
-        // `#[feature(uniform_paths)]` allows an unqualified import path,
-        // e.g. `use x::...;` to resolve not just globally (`use ::x::...;`)
-        // but also relatively (`use self::x::...;`). To catch ambiguities
-        // that might arise from both of these being available and resolution
-        // silently picking one of them, an artificial `use self::x as _;`
-        // import is injected as a "canary", and an error is emitted if it
-        // successfully resolves while an `x` external crate exists.
-        //
-        // For each block scope around the `use` item, one special canary
-        // import of the form `use x as _;` is also injected, having its
-        // parent set to that scope; `resolve_imports` will only resolve
-        // it within its appropriate scope; if any of them successfully
-        // resolve, an ambiguity error is emitted, since the original
-        // import can't see the item in the block scope (`self::x` only
-        // looks in the enclosing module), but a non-`use` path could.
-        //
-        // Additionally, the canary might be able to catch limitations of the
-        // current implementation, where `::x` may be chosen due to `self::x`
-        // not existing, but `self::x` could appear later, from macro expansion.
-        //
-        // NB. The canary currently only errors if the `x::...` path *could*
-        // resolve as a relative path through the extern crate, i.e. `x` is
-        // in `extern_prelude`, *even though* `::x` might still forcefully
-        // load a non-`extern_prelude` crate.
-        // While always producing an ambiguity errors if `self::x` exists and
-        // a crate *could* be loaded, would be more conservative, imports for
-        // local modules named `test` (or less commonly, `syntax` or `log`),
-        // would need to be qualified (e.g. `self::test`), which is considered
-        // ergonomically unacceptable.
-        let emit_uniform_paths_canary =
-            !uniform_paths_canary_emitted &&
-            self.session.rust_2018() &&
-            starts_with_non_keyword;
-        if emit_uniform_paths_canary {
-            let source = prefix_start.unwrap();
-
-            // Helper closure to emit a canary with the given base path.
-            let emit = |this: &mut Self, base: Option<Segment>| {
-                let subclass = SingleImport {
-                    target: Ident {
-                        name: keywords::Underscore.name().gensymed(),
-                        span: source.ident.span,
-                    },
-                    source: source.ident,
-                    result: PerNS {
-                        type_ns: Cell::new(Err(Undetermined)),
-                        value_ns: Cell::new(Err(Undetermined)),
-                        macro_ns: Cell::new(Err(Undetermined)),
-                    },
-                    type_ns_only: false,
-                };
-                this.add_import_directive(
-                    base.into_iter().collect(),
-                    subclass,
-                    source.ident.span,
-                    id,
-                    root_span,
-                    item.id,
-                    ty::Visibility::Invisible,
-                    parent_scope.clone(),
-                    true, // is_uniform_paths_canary
-                );
-            };
-
-            // A single simple `self::x` canary.
-            emit(self, Some(Segment {
-                ident: Ident {
-                    name: keywords::SelfValue.name(),
-                    span: source.ident.span,
-                },
-                id: source.id
-            }));
-
-            // One special unprefixed canary per block scope around
-            // the import, to detect items unreachable by `self::x`.
-            let orig_current_module = self.current_module;
-            let mut span = source.ident.span.modern();
-            loop {
-                match self.current_module.kind {
-                    ModuleKind::Block(..) => emit(self, None),
-                    ModuleKind::Def(..) => break,
-                }
-                match self.hygienic_lexical_parent(self.current_module, &mut span) {
-                    Some(module) => {
-                        self.current_module = module;
-                    }
-                    None => break,
-                }
-            }
-            self.current_module = orig_current_module;
-
-            uniform_paths_canary_emitted = true;
-        }
-
         let empty_for_self = |prefix: &[Segment]| {
             prefix.is_empty() ||
             prefix.len() == 1 && prefix[0].ident.name == keywords::CrateRoot.name()
@@ -350,7 +252,6 @@ fn build_reduced_graph_for_use_tree(
                     item.id,
                     vis,
                     parent_scope,
-                    false, // is_uniform_paths_canary
                 );
             }
             ast::UseTreeKind::Glob => {
@@ -367,7 +268,6 @@ fn build_reduced_graph_for_use_tree(
                     item.id,
                     vis,
                     parent_scope,
-                    false, // is_uniform_paths_canary
                 );
             }
             ast::UseTreeKind::Nested(ref items) => {
@@ -396,7 +296,7 @@ fn build_reduced_graph_for_use_tree(
                 for &(ref tree, id) in items {
                     self.build_reduced_graph_for_use_tree(
                         // This particular use tree
-                        tree, id, &prefix, true, uniform_paths_canary_emitted,
+                        tree, id, &prefix, true,
                         // The whole `use` item
                         parent_scope.clone(), item, vis, root_span,
                     );
@@ -420,7 +320,7 @@ fn build_reduced_graph_for_use_tree(
                     };
                     self.build_reduced_graph_for_use_tree(
                         // This particular use tree
-                        &tree, id, &prefix, true, uniform_paths_canary_emitted,
+                        &tree, id, &prefix, true,
                         // The whole `use` item
                         parent_scope.clone(), item, ty::Visibility::Invisible, root_span,
                     );
@@ -441,7 +341,7 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent_scope: ParentScop
             ItemKind::Use(ref use_tree) => {
                 self.build_reduced_graph_for_use_tree(
                     // This particular use tree
-                    use_tree, item.id, &[], false, false,
+                    use_tree, item.id, &[], false,
                     // The whole `use` item
                     parent_scope, item, vis, use_tree.span,
                 );
@@ -492,7 +392,6 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent_scope: ParentScop
                     module_path: Vec::new(),
                     vis: Cell::new(vis),
                     used: Cell::new(used),
-                    is_uniform_paths_canary: false,
                 });
                 self.potentially_unused_imports.push(directive);
                 let imported_binding = self.import(binding, directive);
@@ -905,7 +804,6 @@ fn process_legacy_macro_imports(&mut self, item: &Item, module: Module<'a>,
             module_path: Vec::new(),
             vis: Cell::new(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))),
             used: Cell::new(false),
-            is_uniform_paths_canary: false,
         });
 
         let allow_shadowing = parent_scope.expansion == Mark::root();
index 5a91b50f6bcc9d1bbad31decd56904f167eb57b1..ece057358e6b52160239bc8f2176117bede29fe3 100644 (file)
@@ -35,7 +35,6 @@
 use syntax_pos::{MultiSpan, Span};
 
 use std::cell::{Cell, RefCell};
-use std::collections::BTreeMap;
 use std::{mem, ptr};
 
 /// Contains data for specific types of import directives.
@@ -95,13 +94,6 @@ pub struct ImportDirective<'a> {
     pub subclass: ImportDirectiveSubclass<'a>,
     pub vis: Cell<ty::Visibility>,
     pub used: Cell<bool>,
-
-    /// Whether this import is a "canary" for the `uniform_paths` feature,
-    /// i.e. `use x::...;` results in an `use self::x as _;` canary.
-    /// This flag affects diagnostics: an error is reported if and only if
-    /// the import resolves successfully and an external crate with the same
-    /// name (`x` above) also exists; any resolution failures are ignored.
-    pub is_uniform_paths_canary: bool,
 }
 
 impl<'a> ImportDirective<'a> {
@@ -188,11 +180,6 @@ pub fn resolve_ident_in_module_unadjusted(&mut self,
                     // But when a crate does exist, it will get chosen even when
                     // macro expansion could result in a success from the lookup
                     // in the `self` module, later on.
-                    //
-                    // NB. This is currently alleviated by the "ambiguity canaries"
-                    // (see `is_uniform_paths_canary`) that get introduced for the
-                    // maybe-relative imports handled here: if the false negative
-                    // case were to arise, it would *also* cause an ambiguity error.
                     if binding.is_ok() {
                         return binding;
                     }
@@ -404,8 +391,7 @@ pub fn add_import_directive(&mut self,
                                 root_span: Span,
                                 root_id: NodeId,
                                 vis: ty::Visibility,
-                                parent_scope: ParentScope<'a>,
-                                is_uniform_paths_canary: bool) {
+                                parent_scope: ParentScope<'a>) {
         let current_module = parent_scope.module;
         let directive = self.arenas.alloc_import_directive(ImportDirective {
             parent_scope,
@@ -418,7 +404,6 @@ pub fn add_import_directive(&mut self,
             root_id,
             vis: Cell::new(vis),
             used: Cell::new(false),
-            is_uniform_paths_canary,
         });
 
         debug!("add_import_directive({:?})", directive);
@@ -648,18 +633,6 @@ pub fn finalize_imports(&mut self) {
             self.finalize_resolutions_in(module);
         }
 
-        struct UniformPathsCanaryResults<'a> {
-            name: Name,
-            module_scope: Option<&'a NameBinding<'a>>,
-            block_scopes: Vec<&'a NameBinding<'a>>,
-        }
-
-        // Collect all tripped `uniform_paths` canaries separately.
-        let mut uniform_paths_canaries: BTreeMap<
-            (Span, NodeId, Namespace),
-            UniformPathsCanaryResults,
-        > = BTreeMap::new();
-
         let mut errors = false;
         let mut seen_spans = FxHashSet::default();
         let mut error_vec = Vec::new();
@@ -667,46 +640,7 @@ struct UniformPathsCanaryResults<'a> {
         for i in 0 .. self.determined_imports.len() {
             let import = self.determined_imports[i];
             let error = self.finalize_import(import);
-
-            // For a `#![feature(uniform_paths)]` `use self::x as _` canary,
-            // failure is ignored, while success may cause an ambiguity error.
-            if import.is_uniform_paths_canary {
-                if error.is_some() {
-                    continue;
-                }
-
-                let (name, result) = match import.subclass {
-                    SingleImport { source, ref result, .. } => (source.name, result),
-                    _ => bug!(),
-                };
-
-                let has_explicit_self =
-                    !import.module_path.is_empty() &&
-                    import.module_path[0].ident.name == keywords::SelfValue.name();
-
-                self.per_ns(|_, ns| {
-                    if let Some(result) = result[ns].get().ok() {
-                        let canary_results =
-                            uniform_paths_canaries.entry((import.span, import.id, ns))
-                                .or_insert(UniformPathsCanaryResults {
-                                    name,
-                                    module_scope: None,
-                                    block_scopes: vec![],
-                                });
-
-                        // All the canaries with the same `id` should have the same `name`.
-                        assert_eq!(canary_results.name, name);
-
-                        if has_explicit_self {
-                            // There should only be one `self::x` (module-scoped) canary.
-                            assert!(canary_results.module_scope.is_none());
-                            canary_results.module_scope = Some(result);
-                        } else {
-                            canary_results.block_scopes.push(result);
-                        }
-                    }
-                });
-            } else if let Some((span, err, note)) = error {
+            if let Some((span, err, note)) = error {
                 errors = true;
 
                 if let SingleImport { source, ref result, .. } = import.subclass {
@@ -743,71 +677,6 @@ struct UniformPathsCanaryResults<'a> {
             }
         }
 
-        let uniform_paths_feature = self.session.features_untracked().uniform_paths;
-        for ((span, _, ns), results) in uniform_paths_canaries {
-            let name = results.name;
-            let external_crate = if ns == TypeNS {
-                self.extern_prelude_get(Ident::with_empty_ctxt(name), true, false)
-                    .map(|binding| binding.def())
-            } else {
-                None
-            };
-
-            // Currently imports can't resolve in non-module scopes,
-            // we only have canaries in them for future-proofing.
-            if external_crate.is_none() && results.module_scope.is_none() {
-                continue;
-            }
-
-            {
-                let mut all_results = external_crate.into_iter().chain(
-                    results.module_scope.iter()
-                        .chain(&results.block_scopes)
-                        .map(|binding| binding.def())
-                );
-                let first = all_results.next().unwrap();
-
-                // An ambiguity requires more than one *distinct* possible resolution.
-                let possible_resultions =
-                    1 + all_results.filter(|&def| def != first).count();
-                if possible_resultions <= 1 {
-                    continue;
-                }
-            }
-
-            errors = true;
-
-            let msg = format!("`{}` import is ambiguous", name);
-            let mut err = self.session.struct_span_err(span, &msg);
-            let mut suggestion_choices = vec![];
-            if external_crate.is_some() {
-                suggestion_choices.push(format!("`::{}`", name));
-                err.span_label(span,
-                    format!("can refer to external crate `::{}`", name));
-            }
-            if let Some(result) = results.module_scope {
-                suggestion_choices.push(format!("`self::{}`", name));
-                if uniform_paths_feature {
-                    err.span_label(result.span,
-                        format!("can refer to `self::{}`", name));
-                } else {
-                    err.span_label(result.span,
-                        format!("may refer to `self::{}` in the future", name));
-                }
-            }
-            for result in results.block_scopes {
-                err.span_label(result.span,
-                    format!("shadowed by block-scoped `{}`", name));
-            }
-            err.help(&format!("write {} explicitly instead", suggestion_choices.join(" or ")));
-            if uniform_paths_feature {
-                err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
-            } else {
-                err.note("in the future, `#![feature(uniform_paths)]` may become the default");
-            }
-            err.emit();
-        }
-
         if !error_vec.is_empty() {
             self.throw_unresolved_import_error(error_vec.clone(), None);
         }
@@ -816,9 +685,6 @@ struct UniformPathsCanaryResults<'a> {
         // to avoid generating multiple errors on the same import.
         if !errors {
             for import in &self.indeterminate_imports {
-                if import.is_uniform_paths_canary {
-                    continue;
-                }
                 self.throw_unresolved_import_error(error_vec, Some(MultiSpan::from(import.span)));
                 break;
             }
@@ -884,11 +750,7 @@ fn resolve_import(&mut self, directive: &'b ImportDirective<'b>) -> bool {
             // while resolving its module path.
             directive.vis.set(ty::Visibility::Invisible);
             let result = self.resolve_path(
-                Some(if directive.is_uniform_paths_canary {
-                    ModuleOrUniformRoot::Module(directive.parent_scope.module)
-                } else {
-                    ModuleOrUniformRoot::UniformRoot(keywords::Invalid.name())
-                }),
+                Some(ModuleOrUniformRoot::UniformRoot(keywords::Invalid.name())),
                 &directive.module_path[..],
                 None,
                 &directive.parent_scope,
@@ -967,11 +829,7 @@ fn finalize_import(
         let ImportDirective { ref module_path, span, .. } = *directive;
 
         let module_result = self.resolve_path(
-            Some(if directive.is_uniform_paths_canary {
-                ModuleOrUniformRoot::Module(directive.parent_scope.module)
-            } else {
-                ModuleOrUniformRoot::UniformRoot(keywords::Invalid.name())
-            }),
+            Some(ModuleOrUniformRoot::UniformRoot(keywords::Invalid.name())),
             &module_path,
             None,
             &directive.parent_scope,
@@ -1038,21 +896,17 @@ fn finalize_import(
             _ => unreachable!(),
         };
 
-        // Do not record uses from canaries, to avoid interfering with other
-        // diagnostics or suggestions that rely on some items not being used.
-        let record_used = !directive.is_uniform_paths_canary;
-
         let mut all_ns_err = true;
         self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS {
             if let Ok(binding) = result[ns].get() {
                 all_ns_err = false;
-                if record_used && this.record_use(ident, ns, binding) {
+                if this.record_use(ident, ns, binding) {
                     if let ModuleOrUniformRoot::Module(module) = module {
                         this.resolution(module, ident, ns).borrow_mut().binding =
                             Some(this.dummy_binding);
                     }
                 }
-                if record_used && ns == TypeNS {
+                if ns == TypeNS {
                     if let ModuleOrUniformRoot::UniformRoot(..) = module {
                         // Make sure single-segment import is resolved non-speculatively
                         // at least once to report the feature error.
@@ -1065,7 +919,7 @@ fn finalize_import(
         if all_ns_err {
             let mut all_ns_failed = true;
             self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS {
-                if this.resolve_ident_in_module(module, ident, ns, record_used, span).is_ok() {
+                if this.resolve_ident_in_module(module, ident, ns, true, span).is_ok() {
                     all_ns_failed = false;
                 }
             });
@@ -1262,15 +1116,7 @@ fn finalize_resolutions_in(&mut self, module: Module<'b>) {
                 None => continue,
             };
 
-            // Don't reexport `uniform_path` canaries.
-            let non_canary_import = match binding.kind {
-                NameBindingKind::Import { directive, .. } => {
-                    !directive.is_uniform_paths_canary
-                }
-                _ => false,
-            };
-
-            if non_canary_import || binding.is_macro_def() {
+            if binding.is_import() || binding.is_macro_def() {
                 let def = binding.def();
                 if def != Def::Err {
                     if !def.def_id().is_local() {