Revert "[flang] Detect circularly defined interfaces of procedures"

banach-space · banach-space · commit 00e6513374eb · 2021-03-01T10:34:49.000Z
This reverts commit 07de084. The original patch has caused 6 out 8 of Flang's public buildbots to fail. As I'm not sure what the fix should be, I'm reverting this for now. Please see https://reviews.llvm.org/D97201 for more context and discussion.
diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
@@ -17,7 +17,7 @@
 #include <array>
 #include <list>
 #include <optional>
-#include <unordered_set>
+#include <set>
 #include <vector>
 
 namespace llvm {
@@ -38,12 +38,6 @@ using SymbolRef = common::Reference<const Symbol>;
 using SymbolVector = std::vector<SymbolRef>;
 using MutableSymbolRef = common::Reference<Symbol>;
 using MutableSymbolVector = std::vector<MutableSymbolRef>;
-struct SymbolHash {
-  std::size_t operator()(SymbolRef symRef) const {
-    return (std::size_t)(&symRef.get());
-  }
-};
-using SymbolSet = std::unordered_set<SymbolRef, SymbolHash>;
 
 // A module or submodule.
 class ModuleDetails {
@@ -600,10 +594,9 @@ class Symbol {
 
   bool operator==(const Symbol &that) const { return this == &that; }
   bool operator!=(const Symbol &that) const { return !(*this == that); }
-  // For maps using symbols as keys and sorting symbols.  Collate them by their
-  // position in the cooked character stream
   bool operator<(const Symbol &that) const {
-    return sortName_ < that.sortName_;
+    // For sets of symbols: collate them by source location
+    return name_.begin() < that.name_.begin();
   }
 
   int Rank() const {
@@ -660,7 +653,6 @@ class Symbol {
 private:
   const Scope *owner_;
   SourceName name_;
-  const char *sortName_; // used in the "<" operator for sorting symbols
   Attrs attrs_;
   Flags flags_;
   Scope *scope_{nullptr};
@@ -695,7 +687,6 @@ template <std::size_t BLOCK_SIZE> class Symbols {
     Symbol &symbol = Get();
     symbol.owner_ = &owner;
     symbol.name_ = name;
-    symbol.sortName_ = name.begin();
     symbol.attrs_ = attrs;
     symbol.details_ = std::move(details);
     return symbol;
@@ -774,6 +765,7 @@ inline bool operator<(SymbolRef x, SymbolRef y) { return *x < *y; }
 inline bool operator<(MutableSymbolRef x, MutableSymbolRef y) {
   return *x < *y;
 }
+using SymbolSet = std::set<SymbolRef>;
 
 } // namespace Fortran::semantics
 
diff --git a/flang/lib/Evaluate/characteristics.cpp b/flang/lib/Evaluate/characteristics.cpp
@@ -369,7 +369,7 @@ static std::optional<Procedure> CharacterizeProcedure(
     std::string procsList{GetSeenProcs(seenProcs)};
     context.messages().Say(symbol.name(),
         "Procedure '%s' is recursively defined.  Procedures in the cycle:"
-        " %s"_err_en_US,
+        " '%s'"_err_en_US,
         symbol.name(), procsList);
     return std::nullopt;
   }
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
@@ -1003,7 +1003,6 @@ class DeclarationVisitor : public ArraySpecVisitor,
     context().SetError(symbol);
     return symbol;
   }
-  bool HasCycle(const Symbol &, const ProcInterface &);
 };
 
 // Resolve construct entities and statement entities.
@@ -2133,7 +2132,7 @@ static bool NeedsType(const Symbol &symbol) {
 
 void ScopeHandler::ApplyImplicitRules(
     Symbol &symbol, bool allowForwardReference) {
-  if (context().HasError(symbol) || !NeedsType(symbol)) {
+  if (!NeedsType(symbol)) {
     return;
   }
   if (const DeclTypeSpec * type{GetImplicitType(symbol)}) {
@@ -2157,8 +2156,10 @@ void ScopeHandler::ApplyImplicitRules(
   if (allowForwardReference && ImplicitlyTypeForwardRef(symbol)) {
     return;
   }
-  Say(symbol.name(), "No explicit type declared for '%s'"_err_en_US);
-  context().SetError(symbol);
+  if (!context().HasError(symbol)) {
+    Say(symbol.name(), "No explicit type declared for '%s'"_err_en_US);
+    context().SetError(symbol);
+  }
 }
 
 // Extension: Allow forward references to scalar integer dummy arguments
@@ -3640,35 +3641,6 @@ Symbol &DeclarationVisitor::DeclareUnknownEntity(
   }
 }
 
-bool DeclarationVisitor::HasCycle(
-    const Symbol &procSymbol, const ProcInterface &interface) {
-  SymbolSet procsInCycle;
-  procsInCycle.insert(procSymbol);
-  const ProcInterface *thisInterface{&interface};
-  bool haveInterface{true};
-  while (haveInterface) {
-    haveInterface = false;
-    if (const Symbol * interfaceSymbol{thisInterface->symbol()}) {
-      if (procsInCycle.count(*interfaceSymbol) > 0) {
-        for (const auto procInCycle : procsInCycle) {
-          Say(procInCycle->name(),
-              "The interface for procedure '%s' is recursively "
-              "defined"_err_en_US,
-              procInCycle->name());
-          context().SetError(*procInCycle);
-        }
-        return true;
-      } else if (const auto *procDetails{
-                     interfaceSymbol->detailsIf<ProcEntityDetails>()}) {
-        haveInterface = true;
-        thisInterface = &procDetails->interface();
-        procsInCycle.insert(*interfaceSymbol);
-      }
-    }
-  }
-  return false;
-}
-
 Symbol &DeclarationVisitor::DeclareProcEntity(
     const parser::Name &name, Attrs attrs, const ProcInterface &interface) {
   Symbol &symbol{DeclareEntity<ProcEntityDetails>(name, attrs)};
@@ -3678,20 +3650,20 @@ Symbol &DeclarationVisitor::DeclareProcEntity(
           "The interface for procedure '%s' has already been "
           "declared"_err_en_US);
       context().SetError(symbol);
-    } else if (HasCycle(symbol, interface)) {
-      return symbol;
-    } else if (interface.type()) {
-      symbol.set(Symbol::Flag::Function);
-    } else if (interface.symbol()) {
-      if (interface.symbol()->test(Symbol::Flag::Function)) {
+    } else {
+      if (interface.type()) {
         symbol.set(Symbol::Flag::Function);
-      } else if (interface.symbol()->test(Symbol::Flag::Subroutine)) {
-        symbol.set(Symbol::Flag::Subroutine);
+      } else if (interface.symbol()) {
+        if (interface.symbol()->test(Symbol::Flag::Function)) {
+          symbol.set(Symbol::Flag::Function);
+        } else if (interface.symbol()->test(Symbol::Flag::Subroutine)) {
+          symbol.set(Symbol::Flag::Subroutine);
+        }
       }
+      details->set_interface(interface);
+      SetBindNameOn(symbol);
+      SetPassNameOn(symbol);
     }
-    details->set_interface(interface);
-    SetBindNameOn(symbol);
-    SetPassNameOn(symbol);
   }
   return symbol;
 }
@@ -5033,7 +5005,7 @@ Symbol *DeclarationVisitor::NoteInterfaceName(const parser::Name &name) {
 
 void DeclarationVisitor::CheckExplicitInterface(const parser::Name &name) {
   if (const Symbol * symbol{name.symbol}) {
-    if (!context().HasError(*symbol) && !symbol->HasExplicitInterface()) {
+    if (!symbol->HasExplicitInterface()) {
       Say(name,
           "'%s' must be an abstract interface or a procedure with "
           "an explicit interface"_err_en_US,
diff --git a/flang/test/Semantics/resolve102.f90 b/flang/test/Semantics/resolve102.f90
@@ -1,15 +1,15 @@
 ! RUN: %S/test_errors.sh %s %t %f18
 
 ! Tests for circularly defined procedures
-!ERROR: Procedure 'sub' is recursively defined.  Procedures in the cycle: 'p2', 'sub'
+!ERROR: Procedure 'sub' is recursively defined.  Procedures in the cycle: ''sub', 'p2''
 subroutine sub(p2)
   PROCEDURE(sub) :: p2
 
   call sub()
 end subroutine
 
 subroutine circular
-  !ERROR: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'p', 'sub'
+  !ERROR: Procedure 'p' is recursively defined.  Procedures in the cycle: ''p', 'sub', 'p2''
   procedure(sub) :: p
 
   call p(sub)
@@ -21,7 +21,7 @@ subroutine sub(p2)
 end subroutine circular
 
 program iface
-  !ERROR: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'p', 'sub'
+  !ERROR: Procedure 'p' is recursively defined.  Procedures in the cycle: ''p', 'sub', 'p2''
   procedure(sub) :: p
   interface
     subroutine sub(p2)
@@ -38,7 +38,7 @@ Program mutual
   Call p(sub)
 
   contains
-    !ERROR: Procedure 'sub1' is recursively defined.  Procedures in the cycle: 'arg', 'p', 'sub1'
+    !ERROR: Procedure 'sub1' is recursively defined.  Procedures in the cycle: ''p', 'sub1', 'arg''
     Subroutine sub1(arg)
       procedure(sub1) :: arg
     End Subroutine
@@ -54,7 +54,7 @@ Program mutual1
   Call p(sub)
 
   contains
-    !ERROR: Procedure 'sub1' is recursively defined.  Procedures in the cycle: 'p2', 'sub', 'arg', 'p', 'sub1'
+    !ERROR: Procedure 'sub1' is recursively defined.  Procedures in the cycle: ''p', 'sub1', 'arg', 'sub', 'p2''
     Subroutine sub1(arg)
       procedure(sub) :: arg
     End Subroutine
@@ -63,24 +63,3 @@ Subroutine sub(p2)
       Procedure(sub1) :: p2
     End Subroutine
 End Program
-
-program twoCycle
-  !ERROR: The interface for procedure 'p1' is recursively defined
-  !ERROR: The interface for procedure 'p2' is recursively defined
-  procedure(p1) p2
-  procedure(p2) p1
-  call p1
-  call p2
-end program
-
-program threeCycle
-  !ERROR: The interface for procedure 'p1' is recursively defined
-  !ERROR: The interface for procedure 'p2' is recursively defined
-  procedure(p1) p2
-  !ERROR: The interface for procedure 'p3' is recursively defined
-  procedure(p2) p3
-  procedure(p3) p1
-  call p1
-  call p2
-  call p3
-end program

Original file line number	Diff line number	Diff line change
`@@ -369,7 +369,7 @@ static std::optional<Procedure> CharacterizeProcedure(`
`369`	`369`	`std::string procsList{GetSeenProcs(seenProcs)};`
`370`	`370`	`context.messages().Say(symbol.name(),`
`371`	`371`	`"Procedure '%s' is recursively defined. Procedures in the cycle:"`
`372`		`- " %s"_err_en_US,`
	`372`	`+ " '%s'"_err_en_US,`
`373`	`373`	`symbol.name(), procsList);`
`374`	`374`	`return std::nullopt;`
`375`	`375`	`}`