Merge #985

985: Parse macro!(); as MacroInvocation with semicolon r=CohenArthur a=CohenArthur

When parsing a macro invocation as a statement, the parser would parse
an expression and then try parsing a semicolon. Since no actual
lookahead was done (which is a good thing), we couldn't convert a
`MacroInvocation` to a `MacroInvocationSemi` after the fact.

Since, unlike function calls, macro invocations can act differently
based on whether or not they are followed by a semicolon, we actually
need to differentiate between the two up until expansion.

This commits adds a new virtual method for ExprWithoutBlock when
converting to ExprStmtWithoutBlock so that classes inheriting
ExprWithoutBlock can specify a new behavior. In the case of our
MacroInvocation class, it simply means toggling a boolean: If we're
converting a macro from an expression to a statement, it must mean that
it should contain a semicolon.

Closes #941 

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

Author: bors[bot]

gcc/rust/ast/rust-ast-full-decls.h

@@ -54,7 +54,6 @@ class MacroItem;
 class TraitItem;
 class InherentImplItem;
 class TraitImplItem;
-class MacroInvocationSemi;
 struct Crate;
 class PathExpr;
 

gcc/rust/ast/rust-ast-full-test.cc

@@ -1275,18 +1275,6 @@ TypeAlias::as_string () const
   return str;
 }
 
-std::string
-MacroInvocationSemi::as_string () const
-{
-  std::string str = "MacroInvocationSemi: ";
-
-  str += append_attributes (outer_attrs, OUTER);
-
-  str += "\n" + invoc_data.as_string ();
-
-  return str;
-}
-
 std::string
 ExternBlock::as_string () const
 {
@@ -1377,6 +1365,9 @@ MacroInvocation::as_string () const
 
   str += "\n " + invoc_data.as_string ();
 
+  str += "\n has semicolon: ";
+  str += has_semicolon () ? "true" : "false";
+
   return str;
 }
 
@@ -4921,12 +4912,6 @@ LifetimeParam::accept_vis (ASTVisitor &vis)
   vis.visit (*this);
 }
 
-void
-MacroInvocationSemi::accept_vis (ASTVisitor &vis)
-{
-  vis.visit (*this);
-}
-
 void
 PathInExpression::accept_vis (ASTVisitor &vis)
 {

gcc/rust/ast/rust-ast-visitor.h

@@ -52,7 +52,6 @@ class ASTVisitor
   // virtual void visit(TraitItem& trait_item) = 0;
   // virtual void visit(InherentImplItem& inherent_impl_item) = 0;
   // virtual void visit(TraitImplItem& trait_impl_item) = 0;
-  virtual void visit (MacroInvocationSemi &macro) = 0;
 
   // rust-path.h
   virtual void visit (PathInExpression &path) = 0;

gcc/rust/ast/rust-ast.h

@@ -861,7 +861,9 @@ class Item : public Stmt
   add_crate_name (std::vector<std::string> &names ATTRIBUTE_UNUSED) const
   {}
 
-  bool is_item () const override final { return true; }
+  // FIXME: ARTHUR: Is it okay to have removed that final? Is it *required*
+  // behavior that we have items that can also be expressions?
+  bool is_item () const override { return true; }
 
 protected:
   // Clone function implementation as pure virtual method
@@ -957,6 +959,8 @@ class ExprWithoutBlock : public Expr
   {
     return clone_expr_without_block_impl ();
   }
+
+  virtual ExprWithoutBlock *to_stmt () const { return clone_expr_impl (); }
 };
 
 /* HACK: IdentifierExpr, delete when figure out identifier vs expr problem in
@@ -1630,104 +1634,6 @@ class ASTFragment
   std::vector<SingleASTNode> &get_nodes () { return nodes; }
 };
 
-/* A macro invocation item (or statement) AST node (i.e. semi-coloned macro
- * invocation) */
-class MacroInvocationSemi : public MacroItem,
-			    public TraitItem,
-			    public InherentImplItem,
-			    public TraitImplItem,
-			    public ExternalItem
-{
-  std::vector<Attribute> outer_attrs;
-  MacroInvocData invoc_data;
-  Location locus;
-  NodeId node_id;
-
-  // this is the expanded macro
-  ASTFragment fragment;
-
-public:
-  std::string as_string () const override;
-
-  MacroInvocationSemi (MacroInvocData invoc_data,
-		       std::vector<Attribute> outer_attrs, Location locus)
-    : outer_attrs (std::move (outer_attrs)),
-      invoc_data (std::move (invoc_data)), locus (locus),
-      node_id (Analysis::Mappings::get ()->get_next_node_id ()),
-      fragment (ASTFragment::create_empty ())
-  {}
-
-  void accept_vis (ASTVisitor &vis) override;
-
-  // Clones this macro invocation semi.
-  std::unique_ptr<MacroInvocationSemi> clone_macro_invocation_semi () const
-  {
-    return std::unique_ptr<MacroInvocationSemi> (
-      clone_macro_invocation_semi_impl ());
-  }
-
-  void mark_for_strip () override { invoc_data.mark_for_strip (); }
-  bool is_marked_for_strip () const override
-  {
-    return invoc_data.is_marked_for_strip ();
-  }
-
-  // TODO: this mutable getter seems really dodgy. Think up better way.
-  const std::vector<Attribute> &get_outer_attrs () const { return outer_attrs; }
-  std::vector<Attribute> &get_outer_attrs () { return outer_attrs; }
-
-  Location get_locus () const override final { return locus; }
-
-  MacroInvocData &get_invoc_data () { return invoc_data; }
-
-  ASTFragment &get_fragment () { return fragment; }
-
-  void set_fragment (ASTFragment &&f) { fragment = std::move (f); }
-
-  NodeId get_macro_node_id () const { return node_id; }
-
-protected:
-  MacroInvocationSemi *clone_macro_invocation_semi_impl () const
-  {
-    return new MacroInvocationSemi (*this);
-  }
-
-  /* Use covariance to implement clone function as returning this object
-   * rather than base */
-  MacroInvocationSemi *clone_item_impl () const final override
-  {
-    return clone_macro_invocation_semi_impl ();
-  }
-
-  /* Use covariance to implement clone function as returning this object
-   * rather than base */
-  MacroInvocationSemi *clone_inherent_impl_item_impl () const final override
-  {
-    return clone_macro_invocation_semi_impl ();
-  }
-
-  /* Use covariance to implement clone function as returning this object
-   * rather than base */
-  MacroInvocationSemi *clone_trait_impl_item_impl () const final override
-  {
-    return clone_macro_invocation_semi_impl ();
-  }
-
-  /* Use covariance to implement clone function as returning this object
-   * rather than base */
-  MacroInvocationSemi *clone_trait_item_impl () const final override
-  {
-    return clone_macro_invocation_semi_impl ();
-  }
-
-  /* Use covariance to implement clone function as returning this object
-   * rather than base */
-  MacroInvocationSemi *clone_external_item_impl () const final override
-  {
-    return clone_macro_invocation_semi_impl ();
-  }
-};
-
 // A crate AST object - holds all the data for a single compilation unit
 struct Crate
 {

gcc/rust/ast/rust-item.h

@@ -28,7 +28,6 @@ namespace AST {
 // forward decls
 class BlockExpr;
 class TypePath;
-class MacroInvocationSemi;
 
 // TODO: inline?
 /*struct AbiName {
@@ -4306,7 +4305,6 @@ class ExternBlock : public VisItem
 
 // Replaced with forward decls - defined in "rust-macro.h"
 class MacroItem;
-class MacroInvocationSemi;
 class MacroRulesDefinition;
 } // namespace AST
 } // namespace Rust

gcc/rust/ast/rust-macro.h

@@ -27,7 +27,6 @@ namespace AST {
 
 // Decls as definitions moved to rust-ast.h
 class MacroItem;
-class MacroInvocationSemi;
 
 enum MacroFragSpec
 {
@@ -454,6 +453,10 @@ class MacroRulesDefinition : public MacroItem
  * compile time */
 class MacroInvocation : public TypeNoBounds,
 			public Pattern,
+			public MacroItem,
+			public TraitItem,
+			public TraitImplItem,
+			public InherentImplItem,
 			public ExprWithoutBlock
 {
   std::vector<Attribute> outer_attrs;
@@ -463,14 +466,22 @@ class MacroInvocation : public TypeNoBounds,
   // this is the expanded macro
   ASTFragment fragment;
 
+  // Important for when we actually expand the macro
+  bool is_semi_coloned;
+
+  NodeId node_id;
+
 public:
   std::string as_string () const override;
 
   MacroInvocation (MacroInvocData invoc_data,
-		   std::vector<Attribute> outer_attrs, Location locus)
+		   std::vector<Attribute> outer_attrs, Location locus,
+		   bool is_semi_coloned = false)
     : outer_attrs (std::move (outer_attrs)),
       invoc_data (std::move (invoc_data)), locus (locus),
-      fragment (ASTFragment::create_empty ())
+      fragment (ASTFragment::create_empty ()),
+      is_semi_coloned (is_semi_coloned),
+      node_id (Analysis::Mappings::get ()->get_next_node_id ())
   {}
 
   Location get_locus () const override final { return locus; }
@@ -497,12 +508,16 @@ class MacroInvocation : public TypeNoBounds,
     return ExprWithoutBlock::get_node_id ();
   }
 
+  NodeId get_macro_node_id () const { return node_id; }
+
   MacroInvocData &get_invoc_data () { return invoc_data; }
 
   ASTFragment &get_fragment () { return fragment; }
 
   void set_fragment (ASTFragment &&f) { fragment = std::move (f); }
 
+  bool has_semicolon () const { return is_semi_coloned; }
+
 protected:
   /* Use covariance to implement clone function as returning this object rather
    * than base */
@@ -529,6 +544,37 @@ class MacroInvocation : public TypeNoBounds,
   {
     return new MacroInvocation (*this);
   }
+
+  Item *clone_item_impl () const override
+  {
+    return clone_macro_invocation_impl ();
+  }
+
+  bool is_item () const override { return !has_semicolon (); }
+
+  TraitItem *clone_trait_item_impl () const override
+  {
+    return clone_macro_invocation_impl ();
+  };
+
+  TraitImplItem *clone_trait_impl_item_impl () const override
+  {
+    return clone_macro_invocation_impl ();
+  };
+
+  InherentImplItem *clone_inherent_impl_item_impl () const override
+  {
+    return clone_macro_invocation_impl ();
+  }
+
+  ExprWithoutBlock *to_stmt () const override
+
+  {
+    auto new_impl = clone_macro_invocation_impl ();
+    new_impl->is_semi_coloned = true;
+
+    return new_impl;
+  }
 };
 
 // more generic meta item path-only form

gcc/rust/ast/rust-stmt.h

@@ -211,8 +211,9 @@ class ExprStmtWithoutBlock : public ExprStmt
   std::string as_string () const override;
 
   ExprStmtWithoutBlock (std::unique_ptr<ExprWithoutBlock> expr, Location locus)
-    : ExprStmt (locus), expr (std::move (expr))
+    : ExprStmt (locus), expr (std::move (expr->to_stmt ()))
   {}
+
   /*ExprStmtWithoutBlock (std::unique_ptr<Expr> expr, Location locus)
     : ExprStmt (locus), expr (std::move (expr))
   {}*/
@@ -336,9 +337,6 @@ class ExprStmtWithBlock : public ExprStmt
   }
 };
 
-/* Replaced definition of MacroInvocationSemi with forward decl - defined in
- * rust-macro.h */
-class MacroInvocationSemi;
 } // namespace AST
 } // namespace Rust
 

gcc/rust/expand/rust-macro-expand.cc

@@ -310,7 +310,8 @@ class AttrVisitor : public AST::ASTVisitor
   {
     // supposedly does not require - cfg does nothing
   }
-  void visit (AST::MacroInvocationSemi &macro_invoc) override
+
+  void visit (AST::MacroInvocation &macro_invoc) override
   {
     // initial strip test based on outer attrs
     expander.expand_cfg_attrs (macro_invoc.get_outer_attrs ());
@@ -326,9 +327,13 @@ class AttrVisitor : public AST::ASTVisitor
 
     // TODO: maybe have cfg! macro stripping behaviour here?
 
-    expander.expand_invoc_semi (macro_invoc);
+    if (macro_invoc.has_semicolon ())
+      expander.expand_invoc_semi (macro_invoc);
+    else
+      expander.expand_invoc (macro_invoc);
 
-    // we need to visit the expanded fragments since it may need cfg expansion
+    // we need to visit the expanded fragments since it may need cfg
+    // expansion
     // and it may be recursive
     for (auto &node : macro_invoc.get_fragment ().get_nodes ())
       node.accept_vis (*this);
@@ -2534,28 +2539,6 @@ class AttrVisitor : public AST::ASTVisitor
     expander.mappings->insert_macro_def (&rules_def);
   }
 
-  void visit (AST::MacroInvocation &macro_invoc) override
-  {
-    // FIXME
-    // we probably need another recurision check here
-
-    // initial strip test based on outer attrs
-    expander.expand_cfg_attrs (macro_invoc.get_outer_attrs ());
-    if (expander.fails_cfg_with_expand (macro_invoc.get_outer_attrs ()))
-      {
-	macro_invoc.mark_for_strip ();
-	return;
-      }
-
-    // I don't think any macro token trees can be stripped in any way
-    expander.expand_invoc (macro_invoc);
-
-    // we need to visit the expanded fragments since it may need cfg expansion
-    // and it may be recursive
-    for (auto &node : macro_invoc.get_fragment ().get_nodes ())
-      node.accept_vis (*this);
-  }
-
   void visit (AST::MetaItemPath &) override {}
   void visit (AST::MetaItemSeq &) override {}
   void visit (AST::MetaWord &) override {}
@@ -3209,7 +3192,7 @@ MacroExpander::expand_invoc (AST::MacroInvocation &invoc)
 }
 
 void
-MacroExpander::expand_invoc_semi (AST::MacroInvocationSemi &invoc)
+MacroExpander::expand_invoc_semi (AST::MacroInvocation &invoc)
 {
   if (depth_exceeds_recursion_limit ())
     {

gcc/rust/expand/rust-macro-expand.h

@@ -146,11 +146,11 @@ struct MacroExpander
   // Expands all macros in the crate passed in.
   void expand_crate ();
 
-  /* Expands a macro invocation (not macro invocation semi) - possibly make both
+  /* Expands a macro invocation - possibly make both
    * have similar duck-typed interface and use templates?*/
   // should this be public or private?
   void expand_invoc (AST::MacroInvocation &invoc);
-  void expand_invoc_semi (AST::MacroInvocationSemi &invoc);
+  void expand_invoc_semi (AST::MacroInvocation &invoc);
 
   // Expands a single declarative macro.
   AST::ASTFragment expand_decl_macro (Location locus,