Merge #999

999: Refactor ABI options as part of HIR function qualifiers r=philberty a=philberty

This is a refactor to cleanup HIR::ExternBlock and HIR::FunctionQualifiers
to have an enum of ABI options to improve the error handling.

Co-authored-by: Philip Herron <[email protected]>

Author: bors[bot]

gcc/rust/ast/rust-item.h

@@ -484,25 +484,20 @@ struct SelfParam
 // Qualifiers for function, i.e. const, unsafe, extern etc.
 struct FunctionQualifiers
 {
-public:
-  /* Whether the function is neither const nor async, const only, or async
-   * only. */
-
 private:
   AsyncConstStatus const_status;
   bool has_unsafe;
   bool has_extern;
-  std::string extern_abi; // e.g. extern "C" fn() -> i32 {}
-  // TODO: maybe ensure that extern_abi only exists if extern exists?
-
-  // should this store location info?
+  std::string extern_abi;
+  Location locus;
 
 public:
-  FunctionQualifiers (AsyncConstStatus const_status, bool has_unsafe,
-		      bool has_extern = false,
+  FunctionQualifiers (Location locus, AsyncConstStatus const_status,
+		      bool has_unsafe, bool has_extern = false,
 		      std::string extern_abi = std::string ())
     : const_status (const_status), has_unsafe (has_unsafe),
-      has_extern (has_extern), extern_abi (std::move (extern_abi))
+      has_extern (has_extern), extern_abi (std::move (extern_abi)),
+      locus (locus)
   {
     if (!this->extern_abi.empty ())
       {
@@ -517,6 +512,9 @@ struct FunctionQualifiers
   bool is_unsafe () const { return has_unsafe; }
   bool is_extern () const { return has_extern; }
   std::string get_extern_abi () const { return extern_abi; }
+  bool has_abi () const { return !extern_abi.empty (); }
+
+  Location get_locus () const { return locus; }
 };
 
 // A function parameter
@@ -723,7 +721,7 @@ class Method : public InherentImplItem, public TraitImplItem
   // Creates an error state method.
   static Method create_error ()
   {
-    return Method ("", FunctionQualifiers (NONE, true),
+    return Method ("", FunctionQualifiers (Location (), NONE, true),
 		   std::vector<std::unique_ptr<GenericParam>> (),
 		   SelfParam::create_error (), std::vector<FunctionParam> (),
 		   nullptr, WhereClause::create_empty (), nullptr,

gcc/rust/hir/rust-ast-lower-item.h

@@ -800,15 +800,23 @@ class ASTLoweringItem : public ASTLoweringBase
 	extern_items.push_back (std::unique_ptr<HIR::ExternalItem> (lowered));
       }
 
+    ABI abi = ABI::RUST;
+    if (extern_block.has_abi ())
+      {
+	const std::string &extern_abi = extern_block.get_abi ();
+	abi = get_abi_from_string (extern_abi);
+	if (abi == ABI::UNKNOWN)
+	  rust_error_at (extern_block.get_locus (), "unknown ABI option");
+      }
+
     auto crate_num = mappings->get_current_crate ();
     Analysis::NodeMapping mapping (crate_num, extern_block.get_node_id (),
 				   mappings->get_next_hir_id (crate_num),
 				   mappings->get_next_localdef_id (crate_num));
 
     HIR::ExternBlock *hir_extern_block
-      = new HIR::ExternBlock (mapping, extern_block.get_abi (),
-			      std::move (extern_items), std::move (vis),
-			      extern_block.get_inner_attrs (),
+      = new HIR::ExternBlock (mapping, abi, std::move (extern_items),
+			      std::move (vis), extern_block.get_inner_attrs (),
 			      extern_block.get_outer_attrs (),
 			      extern_block.get_locus ());
 

gcc/rust/hir/rust-ast-lower.cc

@@ -602,11 +602,17 @@ ASTLoweringBase::lower_qualifiers (const AST::FunctionQualifiers &qualifiers)
     = qualifiers.is_unsafe () ? Unsafety::Unsafe : Unsafety::Normal;
   bool has_extern = qualifiers.is_extern ();
 
-  // FIXME turn this into the Rust::ABI enum
-  std::string extern_abi = qualifiers.get_extern_abi ();
+  ABI abi = ABI::RUST;
+  if (qualifiers.has_abi ())
+    {
+      const std::string &extern_abi = qualifiers.get_extern_abi ();
+      abi = get_abi_from_string (extern_abi);
+      if (has_extern && abi == ABI::UNKNOWN)
+	rust_error_at (qualifiers.get_locus (), "unknown ABI option");
+    }
 
   return HIR::FunctionQualifiers (qualifiers.get_const_status (), unsafety,
-				  has_extern, extern_abi);
+				  has_extern, abi);
 }
 
 void

gcc/rust/hir/tree/rust-hir-full-test.cc

@@ -1030,10 +1030,7 @@ ExternBlock::as_string () const
   std::string str = VisItem::as_string ();
 
   str += "extern ";
-  if (has_abi ())
-    {
-      str += "\"" + abi + "\" ";
-    }
+  str += "\"" + get_string_from_abi (abi) + "\" ";
 
   // inner attributes
   str += "\n inner attributes: ";
@@ -2058,10 +2055,7 @@ FunctionQualifiers::as_string () const
   if (has_extern)
     {
       str += "extern";
-      if (extern_abi != "")
-	{
-	  str += " \"" + extern_abi + "\"";
-	}
+      str += " \"" + get_string_from_abi (abi) + "\"";
     }
 
   return str;

gcc/rust/hir/tree/rust-hir-item.h

@@ -19,6 +19,7 @@
 #ifndef RUST_HIR_ITEM_H
 #define RUST_HIR_ITEM_H
 
+#include "rust-abi.h"
 #include "rust-ast-full-decls.h"
 #include "rust-common.h"
 #include "rust-hir.h"
@@ -481,28 +482,22 @@ struct FunctionQualifiers
   AsyncConstStatus const_status;
   Unsafety unsafety;
   bool has_extern;
-  std::string extern_abi; // e.g. extern "C" fn() -> i32 {}
-  // TODO: maybe ensure that extern_abi only exists if extern exists?
+  ABI abi;
 
 public:
   FunctionQualifiers (AsyncConstStatus const_status, Unsafety unsafety,
-		      bool has_extern = false,
-		      std::string extern_abi = std::string ())
+		      bool has_extern, ABI abi)
     : const_status (const_status), unsafety (unsafety), has_extern (has_extern),
-      extern_abi (std::move (extern_abi))
-  {
-    if (!this->extern_abi.empty ())
-      {
-	// having extern is required; not having it is an implementation error
-	gcc_assert (has_extern);
-      }
-  }
+      abi (abi)
+  {}
 
   std::string as_string () const;
 
   AsyncConstStatus get_status () const { return const_status; }
 
   bool is_const () const { return const_status == AsyncConstStatus::CONST_FN; }
+
+  ABI get_abi () const { return abi; }
 };
 
 // A function parameter
@@ -3082,15 +3077,9 @@ class ExternalFunctionItem : public ExternalItem
 // An extern block HIR node
 class ExternBlock : public VisItem
 {
-  // bool has_abi;
-  std::string abi;
-
-  // bool has_inner_attrs;
+  ABI abi;
   AST::AttrVec inner_attrs;
-
-  // bool has_extern_items;
   std::vector<std::unique_ptr<ExternalItem>> extern_items;
-
   Location locus;
 
 public:
@@ -3102,17 +3091,14 @@ class ExternBlock : public VisItem
   // Returns whether extern block has extern items.
   bool has_extern_items () const { return !extern_items.empty (); }
 
-  // Returns whether extern block has ABI name.
-  bool has_abi () const { return !abi.empty (); }
-
-  std::string get_abi () const { return abi; }
+  ABI get_abi () const { return abi; }
 
-  ExternBlock (Analysis::NodeMapping mappings, std::string abi,
+  ExternBlock (Analysis::NodeMapping mappings, ABI abi,
 	       std::vector<std::unique_ptr<ExternalItem>> extern_items,
 	       Visibility vis, AST::AttrVec inner_attrs,
 	       AST::AttrVec outer_attrs, Location locus)
     : VisItem (std::move (mappings), std::move (vis), std::move (outer_attrs)),
-      abi (std::move (abi)), inner_attrs (std::move (inner_attrs)),
+      abi (abi), inner_attrs (std::move (inner_attrs)),
       extern_items (std::move (extern_items)), locus (locus)
   {}
 

gcc/rust/parse/rust-parse-impl.h

@@ -2620,13 +2620,13 @@ AST::FunctionQualifiers
 Parser<ManagedTokenSource>::parse_function_qualifiers ()
 {
   AsyncConstStatus const_status = NONE;
-  // bool has_const = false;
   bool has_unsafe = false;
   bool has_extern = false;
   std::string abi;
 
   // Check in order of const, unsafe, then extern
   const_TokenPtr t = lexer.peek_token ();
+  Location locus = t->get_locus ();
   switch (t->get_id ())
     {
     case CONST:
@@ -2662,7 +2662,7 @@ Parser<ManagedTokenSource>::parse_function_qualifiers ()
 	}
     }
 
-  return AST::FunctionQualifiers (const_status, has_unsafe, has_extern,
+  return AST::FunctionQualifiers (locus, const_status, has_unsafe, has_extern,
 				  std::move (abi));
 }
 

gcc/rust/typecheck/rust-hir-type-check-implitem.h

@@ -127,17 +127,11 @@ class TypeCheckTopLevelExternItem : public TypeCheckBase
 			      function.get_item_name ()),
       function.get_locus ()};
 
-    auto abi = get_abi_from_string (parent.get_abi ());
-    if (abi == ABI::UNKNOWN)
-      {
-	rust_error_at (parent.get_locus (), "unknown abi");
-      }
-
     auto fnType = new TyTy::FnType (function.get_mappings ().get_hirid (),
 				    function.get_mappings ().get_defid (),
 				    function.get_item_name (), ident, flags,
-				    abi, std::move (params), ret_type,
-				    std::move (substitutions));
+				    parent.get_abi (), std::move (params),
+				    ret_type, std::move (substitutions));
 
     context->insert_type (function.get_mappings (), fnType);
   }

gcc/testsuite/rust/compile/abi-options1.rs

@@ -0,0 +1,7 @@
+extern "foobar" {
+    // { dg-error "unknown ABI option" "" { target *-*-* } .-1 }
+    fn printf(s: *const i8, ...);
+}
+
+pub extern "baz" fn test() {}
+// { dg-error "unknown ABI option" "" { target *-*-* } .-1 }