Auto merge of #31920 - jseyfried:fix_spurious_privacy_error, r=nikomatsakis

This PR allows using methods from traits that are visible but are defined in an inaccessible module (fixes #18241). For example,
```rust
mod foo {
    pub use foo::bar::Tr;
    mod bar { // This module is inaccessible from `g`
        pub trait Tr { fn f(&self) {} }
    }
}
fn g<T: foo::Tr>(t: T) {
    t.f(); // Currently, this is a privacy error even though `foo::Tr` is visible
}
```

After this PR, it will continue to be a privacy error to use a method from a trait that is not visible. This can happen when a public trait inherits from a private trait (in violation of the `public_in_private` lint) -- see @petrochenkov's example in #28504.
r? @nikomatsakis

Author: bors

src/librustc_privacy/lib.rs

@@ -492,11 +492,6 @@ enum FieldName {
 }
 
 impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
-    // used when debugging
-    fn nodestr(&self, id: ast::NodeId) -> String {
-        self.tcx.map.node_to_string(id).to_string()
-    }
-
     // Determines whether the given definition is public from the point of view
     // of the current item.
     fn def_privacy(&self, did: DefId) -> PrivacyResult {
@@ -604,75 +599,44 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
             return Allowable;
         }
 
-        // We now know that there is at least one private member between the
-        // destination and the root.
-        let mut closest_private_id = node_id;
-        loop {
-            debug!("privacy - examining {}", self.nodestr(closest_private_id));
-            let vis = match self.tcx.map.find(closest_private_id) {
-                // If this item is a method, then we know for sure that it's an
-                // actual method and not a static method. The reason for this is
-                // that these cases are only hit in the ExprMethodCall
-                // expression, and ExprCall will have its path checked later
-                // (the path of the trait/impl) if it's a static method.
-                //
-                // With this information, then we can completely ignore all
-                // trait methods. The privacy violation would be if the trait
-                // couldn't get imported, not if the method couldn't be used
-                // (all trait methods are public).
-                //
-                // However, if this is an impl method, then we dictate this
-                // decision solely based on the privacy of the method
-                // invocation.
-                // FIXME(#10573) is this the right behavior? Why not consider
-                //               where the method was defined?
-                Some(ast_map::NodeImplItem(ii)) => {
-                    match ii.node {
-                        hir::ImplItemKind::Const(..) |
-                        hir::ImplItemKind::Method(..) => {
-                            let imp = self.tcx.map
-                                          .get_parent_did(closest_private_id);
-                            match self.tcx.impl_trait_ref(imp) {
-                                Some(..) => return Allowable,
-                                _ if ii.vis == hir::Public => {
-                                    return Allowable
-                                }
-                                _ => ii.vis
-                            }
-                        }
-                        hir::ImplItemKind::Type(_) => return Allowable,
-                    }
-                }
-                Some(ast_map::NodeTraitItem(_)) => {
-                    return Allowable;
+        let vis = match self.tcx.map.find(node_id) {
+            // If this item is a method, then we know for sure that it's an
+            // actual method and not a static method. The reason for this is
+            // that these cases are only hit in the ExprMethodCall
+            // expression, and ExprCall will have its path checked later
+            // (the path of the trait/impl) if it's a static method.
+            //
+            // With this information, then we can completely ignore all
+            // trait methods. The privacy violation would be if the trait
+            // couldn't get imported, not if the method couldn't be used
+            // (all trait methods are public).
+            //
+            // However, if this is an impl method, then we dictate this
+            // decision solely based on the privacy of the method
+            // invocation.
+            Some(ast_map::NodeImplItem(ii)) => {
+                let imp = self.tcx.map.get_parent_did(node_id);
+                match self.tcx.impl_trait_ref(imp) {
+                    Some(..) => hir::Public,
+                    _ => ii.vis,
                 }
+            }
+            Some(ast_map::NodeTraitItem(_)) => hir::Public,
 
-                // This is not a method call, extract the visibility as one
-                // would normally look at it
-                Some(ast_map::NodeItem(it)) => it.vis,
-                Some(ast_map::NodeForeignItem(_)) => {
-                    self.tcx.map.get_foreign_vis(closest_private_id)
-                }
-                Some(ast_map::NodeVariant(..)) => {
-                    hir::Public // need to move up a level (to the enum)
-                }
-                _ => hir::Public,
-            };
-            if vis != hir::Public { break }
-            // if we've reached the root, then everything was allowable and this
-            // access is public.
-            if closest_private_id == ast::CRATE_NODE_ID { return Allowable }
-            closest_private_id = *self.parents.get(&closest_private_id).unwrap();
-
-            // If we reached the top, then we were public all the way down and
-            // we can allow this access.
-            if closest_private_id == ast::DUMMY_NODE_ID { return Allowable }
-        }
-        debug!("privacy - closest priv {}", self.nodestr(closest_private_id));
-        if self.private_accessible(closest_private_id) {
+            // This is not a method call, extract the visibility as one
+            // would normally look at it
+            Some(ast_map::NodeItem(it)) => it.vis,
+            Some(ast_map::NodeForeignItem(_)) => {
+                self.tcx.map.get_foreign_vis(node_id)
+            }
+            _ => hir::Public,
+        };
+        if vis == hir::Public { return Allowable }
+
+        if self.private_accessible(node_id) {
             Allowable
         } else {
-            DisallowedBy(closest_private_id)
+            DisallowedBy(node_id)
         }
     }
 
@@ -834,8 +798,8 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
             // Trait methods are always all public. The only controlling factor
             // is whether the trait itself is accessible or not.
             ty::TraitContainer(trait_def_id) => {
-                self.report_error(self.ensure_public(span, trait_def_id,
-                                                     None, "source trait"));
+                let msg = format!("source trait `{}`", self.tcx.item_path_str(trait_def_id));
+                self.report_error(self.ensure_public(span, trait_def_id, None, &msg));
             }
         }
     }

src/test/compile-fail/trait-not-accessible.rs

@@ -20,7 +20,7 @@ struct S;
 impl m::Pub for S {}
 
 fn g<T: m::Pub>(arg: T) {
-    arg.f(); //~ ERROR: source trait is private
+    arg.f(); //~ ERROR: source trait `m::Priv` is private
 }
 
 fn main() {

src/test/compile-fail/trait-privacy.rs

@@ -0,0 +1,30 @@
+// Copyright 2016 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.
+
+#![feature(rustc_attrs)]
+#![allow(dead_code)]
+
+mod foo {
+    pub use self::bar::T;
+    mod bar {
+        pub trait T {
+            fn f(&self) {}
+        }
+        impl T for () {}
+    }
+}
+
+fn g() {
+    use foo::T;
+    ().f(); // Check that this does not trigger a privacy error
+}
+
+#[rustc_error]
+fn main() {} //~ ERROR compilation successful