[FIRRTL] Fix: Enforce flat namespace for modules (#8345)

Added a check for duplicates in the symbolTable.  Added and removed tests for
the same.

- fixes #7773 
- fixes #1537

Author: Jatin-exe

lib/Dialect/FIRRTL/Import/FIRParser.cpp

@@ -1430,6 +1430,9 @@ struct FIRModuleContext : public FIRParser {
                           insertNameIntoGlobalScope);
   }
 
+  // Removes a symbol from symbolTable (Workaround since symbolTable is private)
+  void removeSymbolEntry(StringRef name);
+
   /// Resolved a symbol table entry to a value.  Emission of error is optional.
   ParseResult resolveSymbolEntry(Value &result, SymbolValueEntry &entry,
                                  SMLoc loc, bool fatal = true);
@@ -1511,6 +1514,11 @@ struct FIRModuleContext : public FIRParser {
 
 } // end anonymous namespace
 
+// Removes a symbol from symbolTable (Workaround since symbolTable is private)
+void FIRModuleContext::removeSymbolEntry(StringRef name) {
+  symbolTable.erase(name);
+}
+
 /// Add a symbol entry with the specified name, returning failure if the name
 /// is already defined.
 ///
@@ -1522,12 +1530,22 @@ ParseResult FIRModuleContext::addSymbolEntry(StringRef name,
                                              bool insertNameIntoGlobalScope) {
   // Do a lookup by trying to do an insertion.  Do so in a way that we can tell
   // if we hit a missing element (SMLoc is null).
-  auto entryIt =
-      symbolTable.try_emplace(name, SMLoc(), SymbolValueEntry()).first;
-  if (entryIt->second.first.isValid()) {
-    emitError(loc, "redefinition of name '" + name + "'")
-            .attachNote(translateLocation(entryIt->second.first))
-        << "previous definition here";
+  auto [entryIt, inserted] =
+      symbolTable.try_emplace(name, SMLoc(), SymbolValueEntry());
+
+  // If insertion failed, the name already exists
+  if (!inserted) {
+    if (entryIt->second.first.isValid()) {
+      // Valid activeSMLoc: active symbol in current scope redeclared
+      emitError(loc, "redefinition of name '" + name + "' ")
+              .attachNote(translateLocation(entryIt->second.first))
+          << "previous definition here.";
+    } else {
+      // Invalid activeSMLoc: symbol from a completed scope redeclared
+      emitError(loc, "redefinition of name '" + name + "' ")
+          << "- FIRRTL has flat namespace and requires all "
+          << "declarations in a module to have unique names.";
+    }
     return failure();
   }
 
@@ -1536,7 +1554,6 @@ ParseResult FIRModuleContext::addSymbolEntry(StringRef name,
   entryIt->second = {loc, entry};
   if (currentScope && !insertNameIntoGlobalScope)
     currentScope->scopedDecls.push_back(&*entryIt);
-
   return success();
 }
 
@@ -4878,10 +4895,12 @@ ParseResult FIRStmtParser::parseContract(unsigned blockIndent) {
 
   // Declare the results.
   for (auto [id, loc, value, result] :
-       llvm::zip(ids, locs, values, contract.getResults()))
+       llvm::zip(ids, locs, values, contract.getResults())) {
+    // Remove previous symbol to avoid duplicates
+    moduleContext.removeSymbolEntry(id);
     if (failed(moduleContext.addSymbolEntry(id, result, loc)))
       return failure();
-
+  }
   return success();
 }
 

test/Dialect/FIRRTL/parse-basic.fir

@@ -211,20 +211,6 @@ circuit MyModule :     ; CHECK: firrtl.circuit "MyModule" {
     ; CHECK: }
     when reset : connect _t, _t_2 else : connect _t, _t_2
 
-    ; CHECK: firrtl.when %reset : !firrtl.uint<1> {
-    ; CHECK:   [[N4A:%.+]] = firrtl.node interesting_name %_t_2
-    ; CHECK:   firrtl.matchingconnect %_t, [[N4A]]
-    ; CHECK: } else {
-    ; CHECK:   [[N4B:%.+]] = firrtl.node interesting_name %_t_2
-    ; CHECK:   firrtl.matchingconnect %_t, [[N4B]]
-    ; CHECK: }
-    when reset :
-      node n4 = _t_2
-      connect _t, n4
-    else :
-      node n4 = _t_2   ; 'n4' name is in unique scopes.
-      connect _t, n4
-
     ; CHECK: [[TMP:%.+]] = firrtl.constant 4
     ; CHECK: [[COND:%.+]] = firrtl.lt %reset, [[TMP]]
     ; CHECK: firrtl.when [[COND]] : !firrtl.uint<1> {

test/Dialect/FIRRTL/parse-errors.fir

@@ -1667,3 +1667,49 @@ circuit Top:
   public module Top:
     ; expected-error @below {{contracts are a FIRRTL 5.1.0+ feature}}
     contract:
+
+// -----
+FIRRTL version 4.0.0
+circuit DuplicateWhenElse:
+  public module DuplicateWhenElse:
+    input reset: UInt<1>
+    output _t: UInt<1>
+    wire _t_2: UInt<1>
+
+    when reset :
+      node n4 = _t_2
+      connect _t, n4
+    else :
+      ; expected-error @+1 {{redefinition of name 'n4' - FIRRTL has flat namespace and requires all declarations in a module to have unique names}}
+      node n4 = _t_2   ; 'n4' is duplicated across scopes
+      connect _t, n4
+
+// -----
+FIRRTL version 4.0.0
+circuit SameScopeRedef:
+  public module SameScopeRedef:
+    input reset: UInt<1>
+    output _t: UInt<1>
+    wire _t_2: UInt<1>
+
+    ; expected-note @+1 {{previous definition here}}
+    node x = _t_2
+    ; expected-error @+1 {{redefinition of name 'x'}}
+    node x = _t_2   ; 'x' is duplicated in same scope
+    connect _t, x
+
+// -----
+FIRRTL version 4.0.0
+circuit NestedWhenRedef:
+  public module NestedWhenRedef:
+    input reset: UInt<1>
+    output _t: UInt<1>
+    wire _t_2: UInt<1>
+
+    when reset :
+      ; expected-note @+1 {{previous definition here}}
+      node val = _t_2
+      when reset :
+        ; expected-error @+1 {{redefinition of name 'val'}}
+        node val = _t_2   ; 'val' is duplicated in nested when
+      connect _t, val