Merge #1258

1258: Report privacy errors on explicit types r=CohenArthur a=CohenArthur

~~Needs #1255, so skip reviewing the first commit or wait for it to be merged~~ Done and rebased :)

Co-authored-by: Arthur Cohen <arthur.cohen@embecosm.com>

8 files changed, 240 insertions, 28 deletions

diff --git a/gcc/rust/hir/tree/rust-hir.h b/gcc/rust/hir/tree/rust-hir.h
index 67cfb3a..cab46b2 100644
--- a/gcc/rust/hir/tree/rust-hir.h
+++ b/gcc/rust/hir/tree/rust-hir.h
@@ -486,6 +486,8 @@ protected:
   virtual Type *clone_type_impl () const = 0;
 
   Analysis::NodeMapping mappings;
+
+  // FIXME: How do we get the location here for each type?
 };
 
 // A type without parentheses? - abstract
diff --git a/gcc/rust/privacy/rust-privacy-check.cc b/gcc/rust/privacy/rust-privacy-check.cc
index dca5235..9664d62 100644
--- a/gcc/rust/privacy/rust-privacy-check.cc
+++ b/gcc/rust/privacy/rust-privacy-check.cc
@@ -41,7 +41,7 @@ Resolver::resolve (HIR::Crate &crate)
 
   VisibilityResolver (*mappings, *resolver).go (crate);
   PubRestrictedVisitor (*mappings).go (crate);
-  PrivacyReporter (*mappings, *resolver).go (crate);
+  PrivacyReporter (*mappings, *resolver, *ty_ctx).go (crate);
 
   auto visitor = ReachabilityVisitor (ctx, *ty_ctx);
 
diff --git a/gcc/rust/privacy/rust-privacy-reporter.cc b/gcc/rust/privacy/rust-privacy-reporter.cc
index 377e861..4b010a0 100644
--- a/gcc/rust/privacy/rust-privacy-reporter.cc
+++ b/gcc/rust/privacy/rust-privacy-reporter.cc
@@ -6,9 +6,10 @@
 namespace Rust {
 namespace Privacy {
 
-PrivacyReporter::PrivacyReporter (Analysis::Mappings &mappings,
-				  Resolver::Resolver &resolver)
-  : mappings (mappings), resolver (resolver),
+PrivacyReporter::PrivacyReporter (
+  Analysis::Mappings &mappings, Resolver::Resolver &resolver,
+  const Rust::Resolver::TypeCheckContext &ty_ctx)
+  : mappings (mappings), resolver (resolver), ty_ctx (ty_ctx),
     current_module (Optional<NodeId>::none ())
 {}
 
@@ -52,7 +53,11 @@ PrivacyReporter::check_for_privacy_violation (const NodeId &use_id,
   if (!resolver.lookup_resolved_name (use_id, &ref_node_id))
     resolver.lookup_resolved_type (use_id, &ref_node_id);
 
-  rust_assert (ref_node_id != UNKNOWN_NODEID);
+  // FIXME: Assert here. For now, we return since this causes issues when
+  // checking inferred types (#1260)
+  // rust_assert (ref_node_id != UNKNOWN_NODEID);
+  if (ref_node_id == UNKNOWN_NODEID)
+    return;
 
   ModuleVisibility vis;
 
@@ -98,6 +103,106 @@ PrivacyReporter::check_for_privacy_violation (const NodeId &use_id,
 }
 
 void
+PrivacyReporter::check_base_type_privacy (Analysis::NodeMapping &node_mappings,
+					  const TyTy::BaseType *ty,
+					  const Location &locus)
+{
+  // Avoids repeating commong argument such as `use_id` or `locus` since we're
+  // doing a lot of recursive calls here
+  auto recursive_check
+    = [this, &node_mappings, &locus] (const TyTy::BaseType *ty) {
+	return check_base_type_privacy (node_mappings, ty, locus);
+      };
+
+  switch (ty->get_kind ())
+    {
+      // These "simple" types are our stop condition
+    case TyTy::BOOL:
+    case TyTy::CHAR:
+    case TyTy::INT:
+    case TyTy::UINT:
+    case TyTy::FLOAT:
+    case TyTy::USIZE:
+    case TyTy::ISIZE:
+    case TyTy::ADT:
+      case TyTy::STR: {
+	auto ref_id = ty->get_ref ();
+	NodeId lookup_id;
+
+	mappings.lookup_hir_to_node (node_mappings.get_crate_num (), ref_id,
+				     &lookup_id);
+
+	return check_for_privacy_violation (lookup_id, locus);
+      }
+    case TyTy::REF:
+      return recursive_check (
+	static_cast<const TyTy::ReferenceType *> (ty)->get_base ());
+    case TyTy::POINTER:
+      return recursive_check (
+	static_cast<const TyTy::PointerType *> (ty)->get_base ());
+    case TyTy::ARRAY:
+      return recursive_check (
+	static_cast<const TyTy::ArrayType *> (ty)->get_element_type ());
+    case TyTy::SLICE:
+      return recursive_check (
+	static_cast<const TyTy::SliceType *> (ty)->get_element_type ());
+    case TyTy::FNPTR:
+      for (auto &param : static_cast<const TyTy::FnPtr *> (ty)->get_params ())
+	recursive_check (param.get_tyty ());
+      return recursive_check (
+	static_cast<const TyTy::FnPtr *> (ty)->get_return_type ());
+    case TyTy::TUPLE:
+      for (auto &param :
+	   static_cast<const TyTy::TupleType *> (ty)->get_fields ())
+	recursive_check (param.get_tyty ());
+      return;
+    case TyTy::PLACEHOLDER:
+      return recursive_check (
+	// FIXME: Can we use `resolve` here? Is that what we should do?
+	static_cast<const TyTy::PlaceholderType *> (ty)->resolve ());
+    case TyTy::PROJECTION:
+      return recursive_check (
+	static_cast<const TyTy::ProjectionType *> (ty)->get ());
+    case TyTy::CLOSURE:
+      sorry_at (locus.gcc_location (),
+		"privacy pass for closures is not handled yet");
+      break;
+
+      // If we're dealing with a generic param, there's nothing we should be
+      // doing here
+    case TyTy::PARAM:
+      // We are dealing with a function definition that has been assigned
+      // somewhere else. Nothing to resolve privacy-wise other than the actual
+      // function, which is resolved as an expression
+    case TyTy::FNDEF:
+      // FIXME: Can we really not resolve Dynamic types here? Shouldn't we have
+      // a look at the path and perform proper privacy analysis?
+    case TyTy::DYNAMIC:
+      // The never type is builtin and always available
+    case TyTy::NEVER:
+      // We shouldn't have inference types here, ever
+    case TyTy::INFER:
+      return;
+    case TyTy::ERROR:
+      rust_unreachable ();
+    }
+}
+
+void
+PrivacyReporter::check_type_privacy (const HIR::Type *type,
+				     const Location &locus)
+{
+  rust_assert (type);
+
+  TyTy::BaseType *lookup = nullptr;
+  rust_assert (
+    ty_ctx.lookup_type (type->get_mappings ().get_hirid (), &lookup));
+
+  auto node_mappings = type->get_mappings ();
+  return check_base_type_privacy (node_mappings, lookup, locus);
+}
+
+void
 PrivacyReporter::visit (HIR::IdentifierExpr &ident_expr)
 {}
 
@@ -529,6 +634,11 @@ PrivacyReporter::visit (HIR::UseDeclaration &use_decl)
 void
 PrivacyReporter::visit (HIR::Function &function)
 {
+  for (auto &param : function.get_function_params ())
+    check_type_privacy (param.get_type (), param.get_locus ());
+
+  // FIXME: It would be better if it was specifically the type's locus (#1256)
+
   function.get_definition ()->accept_vis (*this);
 }
 
@@ -626,7 +736,11 @@ PrivacyReporter::visit (HIR::EmptyStmt &stmt)
 void
 PrivacyReporter::visit (HIR::LetStmt &stmt)
 {
-  // FIXME: We probably have to check the type as well
+  auto type = stmt.get_type ();
+  if (type)
+    check_type_privacy (type, stmt.get_locus ());
+  // FIXME: #1256
+
   auto init_expr = stmt.get_init_expr ();
   if (init_expr)
     init_expr->accept_vis (*this);
diff --git a/gcc/rust/privacy/rust-privacy-reporter.h b/gcc/rust/privacy/rust-privacy-reporter.h
index 868428a..234bea7 100644
--- a/gcc/rust/privacy/rust-privacy-reporter.h
+++ b/gcc/rust/privacy/rust-privacy-reporter.h
@@ -37,7 +37,8 @@ class PrivacyReporter : public HIR::HIRExpressionVisitor,
 {
 public:
   PrivacyReporter (Analysis::Mappings &mappings,
-		   Rust::Resolver::Resolver &resolver);
+		   Rust::Resolver::Resolver &resolver,
+		   const Rust::Resolver::TypeCheckContext &ty_ctx);
 
   /**
    * Perform privacy error reporting on an entire crate
@@ -57,6 +58,26 @@ private:
   void check_for_privacy_violation (const NodeId &use_id,
 				    const Location &locus);
 
+  /**
+   * Internal function used by `check_type_privacy` when dealing with complex
+types
+   * such as references or arrays
+   */
+  void check_base_type_privacy (Analysis::NodeMapping &node_mappings,
+				const TyTy::BaseType *ty,
+				const Location &locus);
+
+  /**
+   * Check the privacy of an explicit type.
+   *
+   * This function reports the errors it finds.
+   *
+   * @param type Reference to an explicit type used in a statement, expression
+   * 		or parameter
+   * @param locus Location of said type
+   */
+  void check_type_privacy (const HIR::Type *type, const Location &locus);
+
   virtual void visit (HIR::StructExprFieldIdentifier &field);
   virtual void visit (HIR::StructExprFieldIdentifierValue &field);
   virtual void visit (HIR::StructExprFieldIndexValue &field);
@@ -142,6 +163,7 @@ private:
 
   Analysis::Mappings &mappings;
   Rust::Resolver::Resolver &resolver;
+  const Rust::Resolver::TypeCheckContext &ty_ctx;
 
   // `None` means we're in the root module - the crate
   Optional<NodeId> current_module;
diff --git a/gcc/rust/util/rust-hir-map.cc b/gcc/rust/util/rust-hir-map.cc
index 934331b..8672c8a 100644
--- a/gcc/rust/util/rust-hir-map.cc
+++ b/gcc/rust/util/rust-hir-map.cc
@@ -239,7 +239,7 @@ Mappings::insert_hir_item (CrateNum crateNum, HirId id, HIR::Item *item)
   rust_assert (lookup_hir_item (crateNum, id) == nullptr);
 
   hirItemMappings[crateNum][id] = item;
-  nodeIdToHirMappings[crateNum][item->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, item->get_mappings ().get_nodeid (), id);
 }
 
 HIR::Item *
@@ -263,7 +263,7 @@ Mappings::insert_hir_trait_item (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_trait_item (crateNum, id) == nullptr);
 
   hirTraitItemMappings[crateNum][id] = item;
-  nodeIdToHirMappings[crateNum][item->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, item->get_mappings ().get_nodeid (), id);
 }
 
 HIR::TraitItem *
@@ -287,7 +287,7 @@ Mappings::insert_hir_extern_item (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_extern_item (crateNum, id) == nullptr);
 
   hirExternItemMappings[crateNum][id] = item;
-  nodeIdToHirMappings[crateNum][item->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, item->get_mappings ().get_nodeid (), id);
 }
 
 HIR::ExternalItem *
@@ -311,7 +311,7 @@ Mappings::insert_hir_impl_block (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_impl_block (crateNum, id) == nullptr);
 
   hirImplBlockMappings[crateNum][id] = item;
-  nodeIdToHirMappings[crateNum][item->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, item->get_mappings ().get_nodeid (), id);
 }
 
 HIR::ImplBlock *
@@ -357,7 +357,7 @@ Mappings::insert_hir_implitem (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_implitem (crateNum, id, nullptr) == nullptr);
   hirImplItemMappings[crateNum][id]
     = std::pair<HirId, HIR::ImplItem *> (parent_impl_id, item);
-  nodeIdToHirMappings[crateNum][item->get_impl_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, item->get_impl_mappings ().get_nodeid (), id);
 }
 
 HIR::ImplItem *
@@ -383,7 +383,7 @@ void
 Mappings::insert_hir_expr (CrateNum crateNum, HirId id, HIR::Expr *expr)
 {
   hirExprMappings[crateNum][id] = expr;
-  nodeIdToHirMappings[crateNum][expr->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, expr->get_mappings ().get_nodeid (), id);
   insert_location (crateNum, id, expr->get_locus ());
 }
 
@@ -408,7 +408,7 @@ Mappings::insert_hir_path_expr_seg (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_path_expr_seg (crateNum, id) == nullptr);
 
   hirPathSegMappings[crateNum][id] = expr;
-  nodeIdToHirMappings[crateNum][expr->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, expr->get_mappings ().get_nodeid (), id);
   insert_location (crateNum, id, expr->get_locus ());
 }
 
@@ -433,7 +433,7 @@ Mappings::insert_simple_path_segment (CrateNum crateNum, HirId id,
   rust_assert (lookup_simple_path_segment (crateNum, id) == nullptr);
 
   astSimplePathSegmentMappings[crateNum][id] = path;
-  nodeIdToHirMappings[crateNum][path->get_node_id ()] = id;
+  insert_node_to_hir (crateNum, path->get_node_id (), id);
   insert_location (crateNum, id, path->get_locus ());
 }
 
@@ -458,7 +458,7 @@ Mappings::insert_simple_path (CrateNum crateNum, HirId id,
   rust_assert (lookup_simple_path (crateNum, id) == nullptr);
 
   astSimplePathMappings[crateNum][id] = path;
-  nodeIdToHirMappings[crateNum][path->get_node_id ()] = id;
+  insert_node_to_hir (crateNum, path->get_node_id (), id);
   insert_location (crateNum, id, path->get_locus ());
 }
 
@@ -483,7 +483,7 @@ Mappings::insert_hir_generic_param (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_generic_param (crateNum, id) == nullptr);
 
   hirGenericParamMappings[crateNum][id] = param;
-  nodeIdToHirMappings[crateNum][param->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, param->get_mappings ().get_nodeid (), id);
   insert_location (crateNum, id, param->get_locus ());
 }
 
@@ -507,7 +507,7 @@ Mappings::insert_hir_type (CrateNum crateNum, HirId id, HIR::Type *type)
   rust_assert (lookup_hir_type (crateNum, id) == nullptr);
 
   hirTypeMappings[crateNum][id] = type;
-  nodeIdToHirMappings[crateNum][type->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, type->get_mappings ().get_nodeid (), id);
 }
 
 HIR::Type *
@@ -525,12 +525,12 @@ Mappings::lookup_hir_type (CrateNum crateNum, HirId id)
 }
 
 void
-Mappings::insert_hir_stmt (CrateNum crateNum, HirId id, HIR::Stmt *type)
+Mappings::insert_hir_stmt (CrateNum crateNum, HirId id, HIR::Stmt *stmt)
 {
   rust_assert (lookup_hir_stmt (crateNum, id) == nullptr);
 
-  hirStmtMappings[crateNum][id] = type;
-  nodeIdToHirMappings[crateNum][type->get_mappings ().get_nodeid ()] = id;
+  hirStmtMappings[crateNum][id] = stmt;
+  insert_node_to_hir (crateNum, stmt->get_mappings ().get_nodeid (), id);
 }
 
 HIR::Stmt *
@@ -554,7 +554,7 @@ Mappings::insert_hir_param (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_stmt (crateNum, id) == nullptr);
 
   hirParamMappings[crateNum][id] = param;
-  nodeIdToHirMappings[crateNum][param->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, param->get_mappings ().get_nodeid (), id);
 }
 
 HIR::FunctionParam *
@@ -578,7 +578,7 @@ Mappings::insert_hir_self_param (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_stmt (crateNum, id) == nullptr);
 
   hirSelfParamMappings[crateNum][id] = param;
-  nodeIdToHirMappings[crateNum][param->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, param->get_mappings ().get_nodeid (), id);
 }
 
 HIR::SelfParam *
@@ -602,7 +602,7 @@ Mappings::insert_hir_struct_field (CrateNum crateNum, HirId id,
   rust_assert (lookup_hir_stmt (crateNum, id) == nullptr);
 
   hirStructFieldMappings[crateNum][id] = field;
-  nodeIdToHirMappings[crateNum][field->get_mappings ().get_nodeid ()] = id;
+  insert_node_to_hir (crateNum, field->get_mappings ().get_nodeid (), id);
 }
 
 HIR::StructExprField *
@@ -624,8 +624,8 @@ Mappings::insert_hir_pattern (CrateNum crateNum, HirId id,
 			      HIR::Pattern *pattern)
 {
   hirPatternMappings[crateNum][id] = pattern;
-  nodeIdToHirMappings[crateNum][pattern->get_pattern_mappings ().get_nodeid ()]
-    = id;
+  insert_node_to_hir (crateNum, pattern->get_pattern_mappings ().get_nodeid (),
+		      id);
 }
 
 HIR::Pattern *
@@ -684,6 +684,7 @@ void
 Mappings::insert_node_to_hir (CrateNum crate, NodeId id, HirId ref)
 {
   nodeIdToHirMappings[crate][id] = ref;
+  hirIdToNodeMappings[crate][ref] = id;
 }
 
 bool
@@ -701,6 +702,21 @@ Mappings::lookup_node_to_hir (CrateNum crate, NodeId id, HirId *ref)
   return true;
 }
 
+bool
+Mappings::lookup_hir_to_node (CrateNum crate, HirId id, NodeId *ref)
+{
+  auto it = hirIdToNodeMappings.find (crate);
+  if (it == hirIdToNodeMappings.end ())
+    return false;
+
+  auto iy = it->second.find (id);
+  if (iy == it->second.end ())
+    return false;
+
+  *ref = iy->second;
+  return true;
+}
+
 void
 Mappings::insert_location (CrateNum crate, HirId id, Location locus)
 {
diff --git a/gcc/rust/util/rust-hir-map.h b/gcc/rust/util/rust-hir-map.h
index 03bfd5f..65f8661 100644
--- a/gcc/rust/util/rust-hir-map.h
+++ b/gcc/rust/util/rust-hir-map.h
@@ -177,7 +177,7 @@ public:
   void insert_hir_type (CrateNum crateNum, HirId id, HIR::Type *type);
   HIR::Type *lookup_hir_type (CrateNum crateNum, HirId id);
 
-  void insert_hir_stmt (CrateNum crateNum, HirId id, HIR::Stmt *type);
+  void insert_hir_stmt (CrateNum crateNum, HirId id, HIR::Stmt *stmt);
   HIR::Stmt *lookup_hir_stmt (CrateNum crateNum, HirId id);
 
   void insert_hir_param (CrateNum crateNum, HirId id, HIR::FunctionParam *type);
@@ -199,6 +199,7 @@ public:
 
   void insert_node_to_hir (CrateNum crate, NodeId id, HirId ref);
   bool lookup_node_to_hir (CrateNum crate, NodeId id, HirId *ref);
+  bool lookup_hir_to_node (CrateNum crate, HirId id, NodeId *ref);
 
   void insert_location (CrateNum crate, HirId id, Location locus);
   Location lookup_location (CrateNum crate, HirId id);
@@ -376,8 +377,8 @@ private:
   // location info
   std::map<CrateNum, std::map<NodeId, Location>> locations;
 
-  // reverse mappings
   std::map<CrateNum, std::map<NodeId, HirId>> nodeIdToHirMappings;
+  std::map<CrateNum, std::map<HirId, NodeId>> hirIdToNodeMappings;
 
   // all hirid nodes
   std::map<CrateNum, std::set<HirId>> hirNodesWithinCrate;
diff --git a/gcc/testsuite/rust/compile/privacy5.rs b/gcc/testsuite/rust/compile/privacy5.rs
new file mode 100644
index 0000000..ad552c7
--- /dev/null
+++ b/gcc/testsuite/rust/compile/privacy5.rs
@@ -0,0 +1,18 @@
+mod orange {
+    mod green {
+        struct Foo;
+        pub(in orange) struct Bar;
+        pub struct Baz;
+    }
+
+    fn brown() {
+        let _ = green::Foo; // { dg-error "definition is private in this context" }
+        let _ = green::Bar;
+        let _ = green::Baz;
+
+        let _: green::Foo; // { dg-error "definition is private in this context" }
+
+        fn any(a0: green::Foo, a1: green::Bar) {}
+        // { dg-error "definition is private in this context" "" { target *-*-* } .-1 }
+    }
+}
diff --git a/gcc/testsuite/rust/compile/privacy6.rs b/gcc/testsuite/rust/compile/privacy6.rs
new file mode 100644
index 0000000..487ed02
--- /dev/null
+++ b/gcc/testsuite/rust/compile/privacy6.rs
@@ -0,0 +1,39 @@
+// { dg-additional-options "-w" }
+
+struct Adt;
+enum EAdt {
+    V0,
+    V1,
+}
+struct Registers {
+    r0: i64,
+    r1: i64,
+    r2: i64,
+    r3: i64,
+}
+trait Foo {}
+
+fn foo1(value: bool) {}
+fn foo2(value: char) {}
+fn foo3(value: i32) {}
+fn foo4(value: u16) {}
+fn foo5(value: f64) {}
+fn foo6(value: usize) {}
+fn foo7(value: isize) {}
+fn foo8(value: Adt) {}
+fn foo9(value: EAdt) {}
+fn foo10(value: &str) {}
+fn foo11(value: *const i8) {}
+fn foo12<T>(value: T) {}
+fn foo13(value: [i32; 5]) {}
+fn foo14(value: [Adt]) {}
+fn foo15(value: fn(i32) -> i32) {}
+fn foo16(value: (i32, Adt)) {}
+fn foo17(value: (i32, [f64; 5])) {}
+fn foo18(value: Registers) {}
+fn foo19(value: &dyn Foo) {}
+fn foo20(value: &[Adt]) {}
+// FIXME: Uncomment once #1257 is fixed
+// fn foo21(value: fn(i32)) {}
+// fn foo22(value: fn()) {}
+fn foo23(value: fn() -> i32) {}