Merge #1202

1202: Resolve module visibility properly r=CohenArthur a=CohenArthur

Closes #1159 

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

15 files changed, 230 insertions, 46 deletions

diff --git a/gcc/rust/Make-lang.in b/gcc/rust/Make-lang.in
index ae385e0..2aec8a9 100644
--- a/gcc/rust/Make-lang.in
+++ b/gcc/rust/Make-lang.in
@@ -85,6 +85,7 @@ GRS_OBJS = \
     rust/rust-ast-lower.o \
     rust/rust-ast-lower-base.o \
     rust/rust-ast-lower-pattern.o \
+    rust/rust-ast-lower-item.o \
     rust/rust-name-resolver.o \
     rust/rust-ast-resolve.o \
     rust/rust-ast-resolve-base.o \
diff --git a/gcc/rust/hir/rust-ast-lower-item.cc b/gcc/rust/hir/rust-ast-lower-item.cc
new file mode 100644
index 0000000..0d59d7f
--- /dev/null
+++ b/gcc/rust/hir/rust-ast-lower-item.cc
@@ -0,0 +1,74 @@
+// Copyright (C) 2020-2022 Free Software Foundation, Inc.
+
+// This file is part of GCC.
+
+// GCC is free software; you can redistribute it and/or modify it under
+// the terms of the GNU General Public License as published by the Free
+// Software Foundation; either version 3, or (at your option) any later
+// version.
+
+// GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+// WARRANTY; without even the implied warranty of MERCHANTABILITY or
+// FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+// for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with GCC; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include "rust-ast-lower-item.h"
+
+namespace Rust {
+namespace HIR {
+
+HIR::SimplePath
+ASTLoweringSimplePath::translate (const AST::SimplePath &path)
+{
+  ASTLoweringSimplePath resolver;
+
+  return resolver.lower (path);
+}
+
+HIR::SimplePathSegment
+ASTLoweringSimplePath::lower (const AST::SimplePathSegment &segment)
+{
+  auto crate_num = mappings->get_current_crate ();
+  auto node_id = segment.get_node_id ();
+
+  auto mapping = Analysis::NodeMapping (crate_num, node_id,
+					mappings->get_next_hir_id (crate_num),
+					UNKNOWN_LOCAL_DEFID);
+
+  auto hir_seg = HIR::SimplePathSegment (mapping);
+
+  mappings->insert_node_to_hir (crate_num, node_id, mapping.get_hirid ());
+  mappings->insert_simple_path_segment (crate_num, node_id, &segment);
+
+  return hir_seg;
+}
+
+HIR::SimplePath
+ASTLoweringSimplePath::lower (const AST::SimplePath &path)
+{
+  auto segments = std::vector<HIR::SimplePathSegment> ();
+  for (auto &segment : path.get_segments ())
+    segments.emplace_back (lower (segment));
+
+  auto crate_num = mappings->get_current_crate ();
+  auto node_id = path.get_node_id ();
+
+  auto mapping = Analysis::NodeMapping (crate_num, node_id,
+					mappings->get_next_hir_id (crate_num),
+					UNKNOWN_LOCAL_DEFID);
+
+  auto lowered
+    = HIR::SimplePath (std::move (segments), mapping, path.get_locus ());
+
+  mappings->insert_node_to_hir (crate_num, node_id, mapping.get_hirid ());
+  mappings->insert_simple_path (crate_num, node_id, &path);
+
+  return lowered;
+}
+
+} // namespace HIR
+} // namespace Rust
diff --git a/gcc/rust/hir/rust-ast-lower-item.h b/gcc/rust/hir/rust-ast-lower-item.h
index c926926..6da50fe 100644
--- a/gcc/rust/hir/rust-ast-lower-item.h
+++ b/gcc/rust/hir/rust-ast-lower-item.h
@@ -826,6 +826,15 @@ private:
   HIR::Item *translated;
 };
 
+class ASTLoweringSimplePath : public ASTLoweringBase
+{
+public:
+  static HIR::SimplePath translate (const AST::SimplePath &path);
+
+  HIR::SimplePathSegment lower (const AST::SimplePathSegment &segment);
+  HIR::SimplePath lower (const AST::SimplePath &path);
+};
+
 } // namespace HIR
 } // namespace Rust
 
diff --git a/gcc/rust/hir/rust-ast-lower.cc b/gcc/rust/hir/rust-ast-lower.cc
index bc613e1..d861207 100644
--- a/gcc/rust/hir/rust-ast-lower.cc
+++ b/gcc/rust/hir/rust-ast-lower.cc
@@ -46,7 +46,8 @@ translate_visibility (const AST::Visibility &vis)
     case AST::Visibility::PUB_CRATE:
     case AST::Visibility::PUB_SUPER:
     case AST::Visibility::PUB_IN_PATH:
-      return Visibility (Visibility::VisType::PUBLIC, vis.get_path ());
+      return Visibility (Visibility::VisType::RESTRICTED,
+			 ASTLoweringSimplePath::translate (vis.get_path ()));
       break;
     }
 
diff --git a/gcc/rust/hir/tree/rust-hir-full-test.cc b/gcc/rust/hir/tree/rust-hir-full-test.cc
index a53210b..8ef1194 100644
--- a/gcc/rust/hir/tree/rust-hir-full-test.cc
+++ b/gcc/rust/hir/tree/rust-hir-full-test.cc
@@ -123,7 +123,8 @@ Visibility::as_string () const
     case PRIVATE:
       return std::string ("private");
     case PUBLIC:
-      return std::string ("pub(in ") + path.as_string () + std::string (")");
+      return std::string ("pub(in ") + path.get_mappings ().as_string ()
+	     + std::string (")");
     default:
       gcc_unreachable ();
     }
diff --git a/gcc/rust/hir/tree/rust-hir-item.h b/gcc/rust/hir/tree/rust-hir-item.h
index b3dc025..109a5c2 100644
--- a/gcc/rust/hir/tree/rust-hir-item.h
+++ b/gcc/rust/hir/tree/rust-hir-item.h
@@ -557,19 +557,19 @@ public:
   {
     PRIVATE,
     PUBLIC,
+    RESTRICTED,
     ERROR,
   };
 
 private:
   VisType vis_type;
-  AST::SimplePath path;
+  HIR::SimplePath path;
 
   // should this store location info?
 
 public:
-  // Creates a Visibility - TODO make constructor protected or private?
   Visibility (VisType vis_type,
-	      AST::SimplePath path = AST::SimplePath::create_empty ())
+	      HIR::SimplePath path = HIR::SimplePath::create_empty ())
     : vis_type (vis_type), path (std::move (path))
   {}
 
@@ -579,14 +579,23 @@ public:
   // Does the current visibility refer to a simple `pub <item>` entirely public
   bool is_public () const { return vis_type == PUBLIC; }
 
+  // Is the current visibility public restricted to a certain path
+  bool is_restricted () const { return vis_type == RESTRICTED; }
+
   // Creates an error visibility.
   static Visibility create_error ()
   {
-    return Visibility (ERROR, AST::SimplePath::create_empty ());
+    return Visibility (ERROR, HIR::SimplePath::create_empty ());
   }
 
   VisType get_vis_type () const { return vis_type; }
 
+  const HIR::SimplePath &get_path () const
+  {
+    rust_assert (!is_error ());
+    return path;
+  }
+
   std::string as_string () const;
 };
 
diff --git a/gcc/rust/hir/tree/rust-hir-path.h b/gcc/rust/hir/tree/rust-hir-path.h
index aa3e29a..b18bf1d 100644
--- a/gcc/rust/hir/tree/rust-hir-path.h
+++ b/gcc/rust/hir/tree/rust-hir-path.h
@@ -946,6 +946,39 @@ public:
 
   Location get_locus () { return locus; }
 };
+
+class SimplePathSegment
+{
+  Analysis::NodeMapping mappings;
+
+public:
+  SimplePathSegment (Analysis::NodeMapping mappings) : mappings (mappings) {}
+
+  const Analysis::NodeMapping &get_mappings () const { return mappings; }
+};
+
+class SimplePath
+{
+  std::vector<SimplePathSegment> segments;
+  Analysis::NodeMapping mappings;
+  Location locus;
+
+public:
+  SimplePath (std::vector<SimplePathSegment> segments,
+	      Analysis::NodeMapping mappings, Location locus)
+    : segments (std::move (segments)), mappings (mappings), locus (locus)
+  {}
+
+  static HIR::SimplePath create_empty ()
+  {
+    return HIR::SimplePath ({}, Analysis::NodeMapping::get_error (),
+			    Location ());
+  }
+
+  const Analysis::NodeMapping &get_mappings () const { return mappings; }
+  const Location &get_locus () const { return locus; }
+};
+
 } // namespace HIR
 } // namespace Rust
 
diff --git a/gcc/rust/parse/rust-parse-impl.h b/gcc/rust/parse/rust-parse-impl.h
index 23ab32c..e76bdd8 100644
--- a/gcc/rust/parse/rust-parse-impl.h
+++ b/gcc/rust/parse/rust-parse-impl.h
@@ -5402,7 +5402,7 @@ Parser<ManagedTokenSource>::parse_inherent_impl_item ()
     case FN_TOK:
       // function or method
       return parse_inherent_impl_function_or_method (
-	AST::Visibility::create_error (), std::move (outer_attrs));
+	AST::Visibility::create_private (), std::move (outer_attrs));
     case CONST:
       /* lookahead to resolve production - could be function/method or const
        * item */
@@ -5412,13 +5412,13 @@ Parser<ManagedTokenSource>::parse_inherent_impl_item ()
 	{
 	case IDENTIFIER:
 	case UNDERSCORE:
-	  return parse_const_item (AST::Visibility::create_error (),
+	  return parse_const_item (AST::Visibility::create_private (),
 				   std::move (outer_attrs));
 	case UNSAFE:
 	case EXTERN_TOK:
 	case FN_TOK:
 	  return parse_inherent_impl_function_or_method (
-	    AST::Visibility::create_error (), std::move (outer_attrs));
+	    AST::Visibility::create_private (), std::move (outer_attrs));
 	default:
 	  add_error (Error (t->get_locus (),
 			    "unexpected token %qs in some sort of const item "
@@ -5572,7 +5572,7 @@ Parser<ManagedTokenSource>::parse_trait_impl_item ()
       // these seem to be SimplePath tokens, so this is a macro invocation semi
       return parse_macro_invocation_semi (std::move (outer_attrs));
     case TYPE:
-      return parse_type_alias (AST::Visibility::create_error (),
+      return parse_type_alias (AST::Visibility::create_private (),
 			       std::move (outer_attrs));
       case PUB: {
 	// visibility, so not a macro invocation semi - must be constant,
@@ -5631,7 +5631,7 @@ Parser<ManagedTokenSource>::parse_trait_impl_item ()
     case FN_TOK:
       // function or method
       return parse_trait_impl_function_or_method (
-	AST::Visibility::create_error (), std::move (outer_attrs));
+	AST::Visibility::create_private (), std::move (outer_attrs));
     case CONST:
       // lookahead to resolve production - could be function/method or const
       // item
@@ -5641,13 +5641,13 @@ Parser<ManagedTokenSource>::parse_trait_impl_item ()
 	{
 	case IDENTIFIER:
 	case UNDERSCORE:
-	  return parse_const_item (AST::Visibility::create_error (),
+	  return parse_const_item (AST::Visibility::create_private (),
 				   std::move (outer_attrs));
 	case UNSAFE:
 	case EXTERN_TOK:
 	case FN_TOK:
 	  return parse_trait_impl_function_or_method (
-	    AST::Visibility::create_error (), std::move (outer_attrs));
+	    AST::Visibility::create_private (), std::move (outer_attrs));
 	default:
 	  add_error (Error (
 	    t->get_locus (),
diff --git a/gcc/rust/privacy/rust-privacy-check.cc b/gcc/rust/privacy/rust-privacy-check.cc
index ccfed2c..ed90c7c 100644
--- a/gcc/rust/privacy/rust-privacy-check.cc
+++ b/gcc/rust/privacy/rust-privacy-check.cc
@@ -20,6 +20,7 @@
 #include "rust-reachability.h"
 #include "rust-hir-type-check.h"
 #include "rust-hir-map.h"
+#include "rust-name-resolver.h"
 #include "rust-visibility-resolver.h"
 
 extern bool
@@ -32,9 +33,10 @@ Resolver::resolve (HIR::Crate &crate)
 {
   PrivacyContext ctx;
   auto mappings = Analysis::Mappings::get ();
+  auto resolver = Rust::Resolver::Resolver::get ();
 
-  auto resolver = VisibilityResolver (*mappings);
-  resolver.go (crate);
+  auto visibility_resolver = VisibilityResolver (*mappings, *resolver);
+  visibility_resolver.go (crate);
 
   auto ty_ctx = ::Rust::Resolver::TypeCheckContext::get ();
   auto visitor = ReachabilityVisitor (ctx, *ty_ctx);
diff --git a/gcc/rust/privacy/rust-privacy-common.h b/gcc/rust/privacy/rust-privacy-common.h
index 145eac7..a47992f 100644
--- a/gcc/rust/privacy/rust-privacy-common.h
+++ b/gcc/rust/privacy/rust-privacy-common.h
@@ -51,6 +51,7 @@ public:
   Type get_kind () const { return kind; }
 
   const DefId &get_module_id () const { return module_id; }
+  DefId &get_module_id () { return module_id; }
 
 private:
   ModuleVisibility (Type kind, DefId module_id)
diff --git a/gcc/rust/privacy/rust-visibility-resolver.cc b/gcc/rust/privacy/rust-visibility-resolver.cc
index 3ab6085..d2e75c2 100644
--- a/gcc/rust/privacy/rust-visibility-resolver.cc
+++ b/gcc/rust/privacy/rust-visibility-resolver.cc
@@ -24,8 +24,9 @@
 namespace Rust {
 namespace Privacy {
 
-VisibilityResolver::VisibilityResolver (Analysis::Mappings &mappings)
-  : mappings (mappings)
+VisibilityResolver::VisibilityResolver (Analysis::Mappings &mappings,
+					Resolver::Resolver &resolver)
+  : mappings (mappings), resolver (resolver)
 {}
 
 void
@@ -45,16 +46,56 @@ VisibilityResolver::go (HIR::Crate &crate)
     }
 }
 
-// FIXME: At this point in the pipeline, we should not be dealing with
-// `AST::SimplePath`s anymore! We need to be dealing with their "resolved
-// counterpart", so probably a NodeId/HirId/DefId.
+bool
+VisibilityResolver::resolve_module_path (const HIR::SimplePath &restriction,
+					 DefId &id)
+{
+  // We need, from the restriction, to figure out the actual Module it
+  // belongs to.
+
+  NodeId ast_node_id = restriction.get_mappings ().get_nodeid ();
+
+  auto invalid_path
+    = Error (restriction.get_locus (),
+	     "cannot use non-module path as privacy restrictor");
+
+  NodeId ref_node_id = UNKNOWN_NODEID;
+  if (!resolver.lookup_resolved_name (ast_node_id, &ref_node_id))
+    {
+      invalid_path.emit_error ();
+      return false;
+    }
+  // FIXME: Add a hint here if we can find the path in another scope, such as
+  // a type or something else
+  // TODO: For the hint, can we point to the original item's definition if
+  // present?
+
+  Resolver::Definition def;
+  rust_assert (resolver.lookup_definition (ref_node_id, &def));
+
+  // FIXME: Is that what we want?
+  ref_node_id = def.parent;
 
-// static bool
-// resolve_module_path (std::vector<HIR::Module> &module_stack,
-// 		     const AST::SimplePath &restriction, DefId &id)
-// {
-//   return false;
-// }
+  HirId ref;
+  rust_assert (
+    mappings.lookup_node_to_hir (restriction.get_mappings ().get_crate_num (),
+				 ref_node_id, &ref));
+
+  auto module
+    = mappings.lookup_module (restriction.get_mappings ().get_crate_num (),
+			      ref);
+
+  if (!module)
+    {
+      invalid_path.emit_error ();
+      return false;
+    }
+
+  // Fill in the resolved `DefId`
+  id = module->get_mappings ().get_defid ();
+
+  return true;
+}
 
 bool
 VisibilityResolver::resolve_visibility (const HIR::Visibility &visibility,
@@ -66,11 +107,14 @@ VisibilityResolver::resolve_visibility (const HIR::Visibility &visibility,
       to_resolve = ModuleVisibility::create_restricted (peek_module ());
       return true;
     case HIR::Visibility::PUBLIC:
-      // FIXME: We need to handle the restricted path here
-      // FIXME: We also need to handle 2015 vs 2018 edition conflicts
       to_resolve = ModuleVisibility::create_public ();
       return true;
+    case HIR::Visibility::RESTRICTED:
+      to_resolve = ModuleVisibility::create_public ();
+      return resolve_module_path (visibility.get_path (),
+				  to_resolve.get_module_id ());
     default:
+      gcc_unreachable ();
       return false;
     }
 }
diff --git a/gcc/rust/privacy/rust-visibility-resolver.h b/gcc/rust/privacy/rust-visibility-resolver.h
index 16fb42f..89e6e2b 100644
--- a/gcc/rust/privacy/rust-visibility-resolver.h
+++ b/gcc/rust/privacy/rust-visibility-resolver.h
@@ -24,6 +24,7 @@
 #include "rust-hir-stmt.h"
 #include "rust-hir-item.h"
 #include "rust-hir-map.h"
+#include "rust-name-resolver.h"
 #include "rust-hir-visitor.h"
 
 namespace Rust {
@@ -32,7 +33,8 @@ namespace Privacy {
 class VisibilityResolver : public HIR::HIRVisItemVisitor
 {
 public:
-  VisibilityResolver (Analysis::Mappings &mappings);
+  VisibilityResolver (Analysis::Mappings &mappings,
+		      Rust::Resolver::Resolver &resolver);
 
   /**
    * Perform visibility resolving on an entire crate
@@ -40,6 +42,12 @@ public:
   void go (HIR::Crate &crate);
 
   /**
+   * Resolve a path to the module it refers
+   */
+  bool resolve_module_path (const HIR::SimplePath &restriction,
+			    DefId &to_resolve);
+
+  /**
    * Resolve the visibility of an item to its ModuleVisibility. This function
    * emits errors if necessary. The contents of the to_resolve parameter will be
    * overwritten on success.
@@ -84,11 +92,11 @@ public:
   virtual void visit (HIR::ExternBlock &block);
 
 private:
-  /* Mappings to insert visibilities into */
-  Analysis::Mappings &mappings;
-
   /* Stack of modules visited by this visitor */
   std::vector<DefId> module_stack;
+
+  Analysis::Mappings &mappings;
+  Rust::Resolver::Resolver &resolver;
 };
 
 } // namespace Privacy
diff --git a/gcc/rust/util/rust-hir-map.cc b/gcc/rust/util/rust-hir-map.cc
index 4062ce7..66d3e41 100644
--- a/gcc/rust/util/rust-hir-map.cc
+++ b/gcc/rust/util/rust-hir-map.cc
@@ -428,7 +428,7 @@ Mappings::lookup_hir_path_expr_seg (CrateNum crateNum, HirId id)
 
 void
 Mappings::insert_simple_path_segment (CrateNum crateNum, HirId id,
-				      AST::SimplePathSegment *path)
+				      const AST::SimplePathSegment *path)
 {
   rust_assert (lookup_simple_path_segment (crateNum, id) == nullptr);
 
@@ -437,7 +437,7 @@ Mappings::insert_simple_path_segment (CrateNum crateNum, HirId id,
   insert_location (crateNum, id, path->get_locus ());
 }
 
-AST::SimplePathSegment *
+const AST::SimplePathSegment *
 Mappings::lookup_simple_path_segment (CrateNum crateNum, HirId id)
 {
   auto it = astSimplePathSegmentMappings.find (crateNum);
@@ -453,7 +453,7 @@ Mappings::lookup_simple_path_segment (CrateNum crateNum, HirId id)
 
 void
 Mappings::insert_simple_path (CrateNum crateNum, HirId id,
-			      AST::SimplePath *path)
+			      const AST::SimplePath *path)
 {
   rust_assert (lookup_simple_path (crateNum, id) == nullptr);
 
@@ -462,7 +462,7 @@ Mappings::insert_simple_path (CrateNum crateNum, HirId id,
   insert_location (crateNum, id, path->get_locus ());
 }
 
-AST::SimplePath *
+const AST::SimplePath *
 Mappings::lookup_simple_path (CrateNum crateNum, HirId id)
 {
   auto it = astSimplePathMappings.find (crateNum);
diff --git a/gcc/rust/util/rust-hir-map.h b/gcc/rust/util/rust-hir-map.h
index 61b8c26..0bb870b 100644
--- a/gcc/rust/util/rust-hir-map.h
+++ b/gcc/rust/util/rust-hir-map.h
@@ -161,12 +161,13 @@ public:
   HIR::PathExprSegment *lookup_hir_path_expr_seg (CrateNum crateNum, HirId id);
 
   void insert_simple_path_segment (CrateNum crateNum, HirId id,
-				   AST::SimplePathSegment *path);
-  AST::SimplePathSegment *lookup_simple_path_segment (CrateNum crateNum,
-						      HirId id);
+				   const AST::SimplePathSegment *path);
+  const AST::SimplePathSegment *lookup_simple_path_segment (CrateNum crateNum,
+							    HirId id);
 
-  void insert_simple_path (CrateNum crateNum, HirId id, AST::SimplePath *path);
-  AST::SimplePath *lookup_simple_path (CrateNum crateNum, HirId id);
+  void insert_simple_path (CrateNum crateNum, HirId id,
+			   const AST::SimplePath *path);
+  const AST::SimplePath *lookup_simple_path (CrateNum crateNum, HirId id);
 
   void insert_hir_generic_param (CrateNum crateNum, HirId id,
 				 HIR::GenericParam *expr);
@@ -351,8 +352,9 @@ private:
   std::map<CrateNum, std::map<HirId, HIR::TraitItem *>> hirTraitItemMappings;
   std::map<CrateNum, std::map<HirId, HIR::ExternalItem *>>
     hirExternItemMappings;
-  std::map<CrateNum, std::map<HirId, AST::SimplePath *>> astSimplePathMappings;
-  std::map<CrateNum, std::map<HirId, AST::SimplePathSegment *>>
+  std::map<CrateNum, std::map<HirId, const AST::SimplePath *>>
+    astSimplePathMappings;
+  std::map<CrateNum, std::map<HirId, const AST::SimplePathSegment *>>
     astSimplePathSegmentMappings;
   std::map<CrateNum, std::map<HirId, HIR::PathExprSegment *>>
     hirPathSegMappings;
diff --git a/gcc/testsuite/rust/compile/pub_restricted_1.rs b/gcc/testsuite/rust/compile/pub_restricted_1.rs
index 01fb65e..9bda968 100644
--- a/gcc/testsuite/rust/compile/pub_restricted_1.rs
+++ b/gcc/testsuite/rust/compile/pub_restricted_1.rs
@@ -1,12 +1,11 @@
 pub mod foo {
     pub mod bar {
         pub fn baz() {}
+
+        pub(in foo::bar) struct A0;
     }
 }
 
-// this is invalid Rust: We just want to make sure the paths get resolved properly
-pub(in foo::bar::baz) struct A0;
-
 pub(in foo::fah::baz) struct A1; // { dg-error "cannot find simple path segment .fah." }
 pub(in fro::bulator::saindoux) struct A2; // { dg-error "cannot find simple path segment .fro." }
 pub(in foo::bar::saindoux) struct A3; // { dg-error "cannot find simple path segment .saindoux." }