Fix for middle::reachable + better comments and tests

In `middle::reachable` mark default impl of a trait method as reachable if this trait method is used from inlinable code

Author: petrochenkov

src/librustc/middle/reachable.rs

@@ -125,16 +125,11 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ReachableContext<'a, 'tcx> {
             hir::ExprMethodCall(..) => {
                 let method_call = ty::MethodCall::expr(expr.id);
                 let def_id = self.tcx.tables.borrow().method_map[&method_call].def_id;
-                match self.tcx.impl_or_trait_item(def_id).container() {
-                    ty::ImplContainer(_) => {
-                        if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
-                            if self.def_id_represents_local_inlined_item(def_id) {
-                                self.worklist.push(node_id)
-                            }
-                            self.reachable_symbols.insert(node_id);
-                        }
+                if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
+                    if self.def_id_represents_local_inlined_item(def_id) {
+                        self.worklist.push(node_id)
                     }
-                    ty::TraitContainer(_) => {}
+                    self.reachable_symbols.insert(node_id);
                 }
             }
             _ => {}

src/librustc_privacy/lib.rs

@@ -183,21 +183,6 @@ struct EmbargoVisitor<'a, 'tcx: 'a> {
 }
 
 impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> {
-    // There are checks inside of privacy which depend on knowing whether a
-    // trait should be exported or not. The two current consumers of this are:
-    //
-    //  1. Should default methods of a trait be exported?
-    //  2. Should the methods of an implementation of a trait be exported?
-    //
-    // The answer to both of these questions partly rely on whether the trait
-    // itself is exported or not. If the trait is somehow exported, then the
-    // answers to both questions must be yes. Right now this question involves
-    // more analysis than is currently done in rustc, so we conservatively
-    // answer "yes" so that all traits need to be exported.
-    fn exported_trait(&self, _id: ast::NodeId) -> bool {
-        true
-    }
-
     // Returns tuple (is_public, is_exported) for a type
     fn is_public_exported_ty(&self, ty: &hir::Ty) -> (bool, bool) {
         if let hir::TyPath(..) = ty.node {
@@ -247,15 +232,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
             // added to public/exported sets based on inherited publicity.
             hir::ItemImpl(..) | hir::ItemDefaultImpl(..) | hir::ItemForeignMod(..) => {}
 
-            // Traits are a little special in that even if they themselves are
-            // not public they may still be exported.
-            hir::ItemTrait(..) => {
-                self.prev_public = self.prev_public && item.vis == hir::Public;
-                self.prev_exported = self.exported_trait(item.id);
-
-                self.maybe_insert_id(item.id);
-            }
-
             // Private by default, hence we only retain the "public chain" if
             // `pub` is explicitly listed.
             _ => {
@@ -284,9 +260,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
                 }
             }
 
-            // * Inherent impls for public types only have public methods exported
-            // * Inherent impls for private types do not need to export their methods
-            // * Inherent impls themselves are not exported, they are nothing more than
+            // Public items in inherent impls for public/exported types are public/exported
+            // Inherent impls themselves are not public/exported, they are nothing more than
             // containers for other items
             hir::ItemImpl(_, _, _, None, ref ty, ref impl_items) => {
                 let (public_ty, exported_ty) = self.is_public_exported_ty(&ty);
@@ -303,42 +278,29 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
                 }
             }
 
-            // Implementations are a little tricky to determine what's exported
-            // out of them. Here's a few cases which are currently defined:
-            //
-            // * Public trait impls for public types must have all methods
-            //   exported.
-            //
-            // * Private trait impls for public types can be ignored
-            //
-            // * Public trait impls for private types have their methods
-            //   exported. I'm not entirely certain that this is the correct
-            //   thing to do, but I have seen use cases of where this will cause
-            //   undefined symbols at linkage time if this case is not handled.
-            //
-            // * Private trait impls for private types can be completely ignored
+            // It's not known until monomorphization if a trait impl item should be reachable
+            // from external crates or not. So, we conservatively mark all of them exported and
+            // the reachability pass (middle::reachable) marks all exported items as reachable.
+            // For example of private trait impl for private type that shoud be reachable see
+            // src/test/auxiliary/issue-11225-3.rs
             hir::ItemImpl(_, _, _, Some(ref trait_ref), ref ty, ref impl_items) => {
                 let (public_ty, _exported_ty) = self.is_public_exported_ty(&ty);
-                let (public_trait, exported_trait) = self.is_public_exported_trait(trait_ref);
+                let (public_trait, _exported_trait) = self.is_public_exported_trait(trait_ref);
 
                 if public_ty && public_trait {
                     self.public_items.insert(item.id);
                 }
-                if exported_trait {
-                    self.exported_items.insert(item.id);
-                }
+                self.exported_items.insert(item.id);
 
                 for impl_item in impl_items {
                     if public_ty && public_trait {
                         self.public_items.insert(impl_item.id);
                     }
-                    if exported_trait {
-                        self.exported_items.insert(impl_item.id);
-                    }
+                    self.exported_items.insert(impl_item.id);
                 }
             }
 
-            // Default trait impls are exported for public traits
+            // Default trait impls are public/exported for public/exported traits
             hir::ItemDefaultImpl(_, ref trait_ref) => {
                 let (public_trait, exported_trait) = self.is_public_exported_trait(trait_ref);
 
@@ -350,8 +312,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
                 }
             }
 
-            // Default methods on traits are all public so long as the trait
-            // is public
+            // Default methods on traits are all public/exported so long as the trait
+            // is public/exported
             hir::ItemTrait(_, _, _, ref trait_items) => {
                 for trait_item in trait_items {
                     self.maybe_insert_id(trait_item.id);

src/test/auxiliary/issue-11225-3.rs

@@ -0,0 +1,29 @@
+// Copyright 2015 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.
+
+trait PrivateTrait {
+    fn private_trait_method(&self);
+}
+
+struct PrivateStruct;
+
+impl PrivateStruct {
+    fn private_inherent_method(&self) { }
+}
+
+impl PrivateTrait for PrivateStruct {
+    fn private_trait_method(&self) { }
+}
+
+#[inline]
+pub fn public_generic_function() {
+    PrivateStruct.private_trait_method();
+    PrivateStruct.private_inherent_method();
+}

src/test/run-pass/issue-11225-3.rs

@@ -0,0 +1,19 @@
+// Copyright 2015 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.
+
+// aux-build:issue-11225-3.rs
+
+// pretty-expanded FIXME #23616
+
+extern crate issue_11225_3;
+
+pub fn main() {
+    issue_11225_3::public_generic_function();
+}