Merge pull request #1315 from hzeller/20220405-provide-detailed-locat…

…ion-of-redefined-symbol

SymbolTable: provide more detailed information about re-defined symbols.

common/text/text_structure.cc

@@ -132,10 +132,23 @@ LineColumnRange TextStructureView::GetRangeForToken(
     const LineColumn eofPos = GetLineColAtOffset(Contents().length());
     return {eofPos, eofPos};
   }
+  // TODO(hzeller): This should simply be GetRangeForText(token.text()),
+  // but the more thorough error checking in GetRangeForText()
+  // exposes a token overrun in verilog_analyzer_test.cc
+  // Defer to fix in separate change.
   return {GetLineColAtOffset(token.left(Contents())),
           GetLineColAtOffset(token.right(Contents()))};
 }
 
+LineColumnRange TextStructureView::GetRangeForText(
+    absl::string_view text) const {
+  const int from = std::distance(Contents().begin(), text.begin());
+  const int to = std::distance(Contents().begin(), text.end());
+  CHECK_GE(from, 0) << '"' << text << '"';
+  CHECK_LE(to, Contents().length()) << '"' << text << '"';
+  return {GetLineColAtOffset(from), GetLineColAtOffset(to)};
+}
+
 TokenRange TextStructureView::TokenRangeOnLine(size_t lineno) const {
   if (lineno + 1 < line_token_map_.size()) {
     return make_range(line_token_map_[lineno], line_token_map_[lineno + 1]);

common/text/text_structure.h

@@ -107,6 +107,10 @@ class TextStructureView {
   // Convenience function: Given the token, return the range it covers.
   LineColumnRange GetRangeForToken(const TokenInfo& token) const;
 
+  // Convenience function: Given a text snippet, that needs to be a substring
+  // of Contents(), return the range it covers.
+  LineColumnRange GetRangeForText(absl::string_view text) const;
+
   const std::vector<TokenSequence::const_iterator>& GetLineTokenMap() const {
     return line_token_map_;
   }

common/text/text_structure_test.cc

@@ -216,6 +216,36 @@ TEST_F(TokenRangeTest, GetRangeOfTokenVerifyAllRangesExclusive) {
   }
 }
 
+TEST_F(TokenRangeTest, GetRangeOfTokenEofTokenAcceptedUniversally) {
+  // For the EOF token, the returned range should automatically be relative
+  // to the TextView no matter where it comes from.
+  EXPECT_EQ(data_.GetRangeForToken(data_.EOFToken()),
+            data_.GetRangeForToken(TokenInfo::EOFToken()));
+}
+
+TEST_F(TokenRangeTest, GetRangeForTokenOrText) {
+  const TokenInfo& token = data_.FindTokenAt({0, 7});
+  EXPECT_EQ(token.text(), "world");
+  {  // Extract from token
+    const LineColumnRange range = data_.GetRangeForToken(token);
+    EXPECT_EQ(range.start.line, 0);
+    EXPECT_EQ(range.start.column, 7);
+  }
+  {  // Extract from token text
+    const LineColumnRange range = data_.GetRangeForText(token.text());
+    EXPECT_EQ(range.start.line, 0);
+    EXPECT_EQ(range.start.column, 7);
+  }
+
+  {  // Entire text range
+    const LineColumnRange range = data_.GetRangeForText(data_.Contents());
+    EXPECT_EQ(range.start.line, 0);
+    EXPECT_EQ(range.start.column, 0);
+    EXPECT_EQ(range.end.line, data_.Lines().size() - 1);
+    EXPECT_EQ(range.end.column, data_.Lines().back().length());
+  }
+}
+
 TEST_F(TokenRangeTest, FindTokenAtPosition) {
   EXPECT_EQ(data_.FindTokenAt({0, 0}).text(), "hello");
   EXPECT_EQ(data_.FindTokenAt({0, 4}).text(), "hello");
@@ -231,13 +261,6 @@ TEST_F(TokenRangeTest, FindTokenAtPosition) {
   EXPECT_TRUE(data_.FindTokenAt({42, 7}).isEOF());
 }
 
-TEST_F(TokenRangeTest, GetRangeOfTokenEofTokenAcceptedUniversally) {
-  // For the EOF token, the returned range should automatically be relative
-  // to the TextView no matter where it comes from.
-  EXPECT_EQ(data_.GetRangeForToken(data_.EOFToken()),
-            data_.GetRangeForToken(TokenInfo::EOFToken()));
-}
-
 // Checks that when lower == upper, returned range is empty.
 TEST_F(TokenRangeTest, TokenRangeSpanningOffsetsEmpty) {
   const size_t test_offsets[] = {0, 1, 4, 12, 18, 22, 26};

verilog/analysis/symbol_table.cc

@@ -915,7 +915,7 @@ class SymbolTable::Builder : public TreeContextVisitor {
                                              .syntax_origin = &element,
                                          });
     if (!p.second) {
-      DiagnoseSymbolAlreadyExists(name);
+      DiagnoseSymbolAlreadyExists(name, p.first->first);
     }
     return &p.first->second;  // scope of the new (or pre-existing symbol)
   }
@@ -939,7 +939,7 @@ class SymbolTable::Builder : public TreeContextVisitor {
             .declared_type = *ABSL_DIE_IF_NULL(declaration_type_info_),  // copy
         });
     if (!p.second) {
-      DiagnoseSymbolAlreadyExists(name);
+      DiagnoseSymbolAlreadyExists(name, p.first->first);
     }
     VLOG(1) << "end of " << __FUNCTION__ << ": " << name;
     return p.first->second;  // scope of the new (or pre-existing symbol)
@@ -1209,10 +1209,19 @@ class SymbolTable::Builder : public TreeContextVisitor {
     Descend(variable);
   }
 
-  void DiagnoseSymbolAlreadyExists(absl::string_view name) {
-    diagnostics_.push_back(absl::AlreadyExistsError(
-        absl::StrCat("Symbol \"", name, "\" is already defined in the ",
-                     CurrentScopeFullPath(), " scope.")));
+  void DiagnoseSymbolAlreadyExists(absl::string_view name,
+                                   absl::string_view previous) {
+    std::ostringstream here_print;
+    here_print << source_->GetTextStructure()->GetRangeForText(name);
+
+    std::ostringstream previous_print;
+    previous_print << source_->GetTextStructure()->GetRangeForText(previous);
+
+    // TODO(hzeller): output in some structured form easy to use downstream.
+    diagnostics_.push_back(absl::AlreadyExistsError(absl::StrCat(
+        source_->ReferencedPath(), ":", here_print.str(), " Symbol \"", name,
+        "\" is already defined in the ", CurrentScopeFullPath(), " scope at ",
+        previous_print.str())));
   }
 
   absl::StatusOr<SymbolTableNode*> LookupOrInjectOutOfLineDefinition(

verilog/analysis/symbol_table_test.cc

@@ -2575,9 +2575,9 @@ TEST(BuildSymbolTableTest, ModuleExplicitAndImplicitDeclarations) {
 
 TEST(BuildSymbolTableTest, ModuleImplicitRedeclared) {
   TestVerilogSourceFile src("foo.sv",
-                            "module m;"
-                            "assign a = 1'b0;"
-                            "wire a;"
+                            "module m;\n"
+                            "assign a = 1'b0;\n"
+                            "wire a;\n"
                             "endmodule\n");
   const auto status = src.Parse();
   ASSERT_TRUE(status.ok()) << status.message();
@@ -2587,7 +2587,8 @@ TEST(BuildSymbolTableTest, ModuleImplicitRedeclared) {
   EXPECT_EQ(build_diagnostics.size(), 1);
   EXPECT_FALSE(build_diagnostics.front().ok());
   EXPECT_EQ(build_diagnostics.front().message(),
-            "Symbol \"a\" is already defined in the $root::m scope.");
+            "foo.sv:3:6: Symbol \"a\" is already defined in the $root::m scope "
+            "at 2:8:");
 }
 
 TEST(BuildSymbolTableTest, ClassDeclarationSingle) {