Add special case lookup handling for enum values

bindings/python/UtilBindings.cpp

@@ -136,6 +136,8 @@ void registerUtil(py::module_& m) {
         .def("isMacroArgLoc", &SourceManager::isMacroArgLoc, "location"_a)
         .def("isIncludedFileLoc", &SourceManager::isIncludedFileLoc, "location"_a)
         .def("isPreprocessedLoc", &SourceManager::isPreprocessedLoc, "location"_a)
+        .def("isBeforeInCompilationUnit", &SourceManager::isBeforeInCompilationUnit, "left"_a,
+             "right"_a)
         .def("getExpansionLoc", &SourceManager::getExpansionLoc, "location"_a)
         .def("getExpansionRange", &SourceManager::getExpansionRange, "location"_a)
         .def("getOriginalLoc", &SourceManager::getOriginalLoc, "location"_a)

include/slang/ast/Lookup.h

@@ -332,7 +332,8 @@ class SLANG_EXPORT Lookup {
     static void unqualifiedImpl(const Scope& scope, std::string_view name, LookupLocation location,
                                 std::optional<SourceRange> sourceRange, bitmask<LookupFlags> flags,
                                 SymbolIndex outOfBlockIndex, LookupResult& result,
-                                const Scope& originalScope);
+                                const Scope& originalScope,
+                                const syntax::SyntaxNode* originalSyntax);
 
     static void qualified(const syntax::ScopedNameSyntax& syntax, const ASTContext& context,
                           bitmask<LookupFlags> flags, LookupResult& result);

include/slang/text/SourceManager.h

@@ -107,6 +107,12 @@ class SLANG_EXPORT SourceManager {
     /// Determines whether the given location is from a macro expansion or an include file.
     bool isPreprocessedLoc(SourceLocation location) const;
 
+    /// Determines whether the @a left location comes before the @a right location
+    /// within the "compilation unit space", which is a hypothetical source space where
+    /// all macros and include files have been expanded out into a flat file.
+    /// Returns std::nullopt if the locations are in unrelated compilation units.
+    std::optional<bool> isBeforeInCompilationUnit(SourceLocation left, SourceLocation right) const;
+
     /// Gets the expansion location of a given macro location.
     SourceLocation getExpansionLoc(SourceLocation location) const;
 

source/ast/Lookup.cpp

@@ -27,6 +27,7 @@
 #include "slang/diagnostics/ParserDiags.h"
 #include "slang/parsing/LexerFacts.h"
 #include "slang/syntax/AllSyntax.h"
+#include "slang/text/SourceManager.h"
 #include "slang/util/String.h"
 
 namespace slang::ast {
@@ -1088,7 +1089,8 @@ void Lookup::name(const NameSyntax& syntax, const ASTContext& context, bitmask<L
         return;
 
     // Perform the lookup.
-    unqualifiedImpl(scope, name.text, context.getLocation(), name.range, flags, {}, result, scope);
+    unqualifiedImpl(scope, name.text, context.getLocation(), name.range, flags, {}, result, scope,
+                    &syntax);
 
     if (!result.found) {
         if (flags.has(LookupFlags::AlwaysAllowUpward)) {
@@ -1135,7 +1137,9 @@ const Symbol* Lookup::unqualified(const Scope& scope, std::string_view name,
         return nullptr;
 
     LookupResult result;
-    unqualifiedImpl(scope, name, LookupLocation::max, std::nullopt, flags, {}, result, scope);
+    unqualifiedImpl(scope, name, LookupLocation::max, std::nullopt, flags, {}, result, scope,
+                    nullptr);
+
     SLANG_ASSERT(result.selectors.empty());
     unwrapResult(scope, std::nullopt, result, /* unwrapGenericClasses */ false);
 
@@ -1149,7 +1153,8 @@ const Symbol* Lookup::unqualifiedAt(const Scope& scope, std::string_view name,
         return nullptr;
 
     LookupResult result;
-    unqualifiedImpl(scope, name, location, sourceRange, flags, {}, result, scope);
+    unqualifiedImpl(scope, name, location, sourceRange, flags, {}, result, scope, nullptr);
+
     SLANG_ASSERT(result.selectors.empty());
     unwrapResult(scope, sourceRange, result, /* unwrapGenericClasses */ false);
 
@@ -1673,7 +1678,7 @@ bool Lookup::findAssertionLocalVar(const ASTContext& context, const NameSyntax&
 void Lookup::unqualifiedImpl(const Scope& scope, std::string_view name, LookupLocation location,
                              std::optional<SourceRange> sourceRange, bitmask<LookupFlags> flags,
                              SymbolIndex outOfBlockIndex, LookupResult& result,
-                             const Scope& originalScope) {
+                             const Scope& originalScope, const SyntaxNode* originalSyntax) {
     auto reportRecursiveError = [&](const Symbol& symbol) {
         if (sourceRange) {
             auto& diag = result.addDiag(scope, diag::RecursiveDefinition, *sourceRange);
@@ -1760,6 +1765,20 @@ void Lookup::unqualifiedImpl(const Scope& scope, std::string_view name, LookupLo
                 result.flags |= LookupResultFlags::FromForwardTypedef;
             }
         }
+        else if (symbol->kind == SymbolKind::TransparentMember && originalSyntax) {
+            // Enum values are special in that we can't always get a precise ordering
+            // of where they are declared in a scope, since they can be declared inside
+            // a nested subexpression. Check for correct location by looking at the
+            // actual source locations involved.
+            auto& wrapped = symbol->as<TransparentMemberSymbol>().wrapped;
+            if (wrapped.kind == SymbolKind::EnumValue) {
+                if (auto sm = scope.getCompilation().getSourceManager()) {
+                    auto loc = originalSyntax->getFirstToken().location();
+                    locationGood =
+                        sm->isBeforeInCompilationUnit(wrapped.location, loc).value_or(true);
+                }
+            }
+        }
 
         if (locationGood || flags.has(LookupFlags::AllowDeclaredAfter)) {
             // Unwrap the symbol if it's hidden behind an import or hoisted enum member.
@@ -1954,7 +1973,7 @@ void Lookup::unqualifiedImpl(const Scope& scope, std::string_view name, LookupLo
     }
 
     return unqualifiedImpl(*location.getScope(), name, location, sourceRange, flags,
-                           outOfBlockIndex, result, originalScope);
+                           outOfBlockIndex, result, originalScope, originalSyntax);
 }
 
 void Lookup::qualified(const ScopedNameSyntax& syntax, const ASTContext& context,
@@ -2003,7 +2022,7 @@ void Lookup::qualified(const ScopedNameSyntax& syntax, const ASTContext& context
         case SyntaxKind::ClassName:
             // Start by trying to find the first name segment using normal unqualified lookup
             unqualifiedImpl(scope, name, context.getLocation(), first.range, flags, {}, result,
-                            scope);
+                            scope, nullptr);
             break;
         case SyntaxKind::UnitScope: {
             // Walk upward to find the compilation unit scope.
@@ -2016,8 +2035,8 @@ void Lookup::qualified(const ScopedNameSyntax& syntax, const ASTContext& context
             do {
                 auto& symbol = current->asSymbol();
                 if (symbol.kind == SymbolKind::CompilationUnit) {
-                    unqualifiedImpl(*current, name, location, first.range, flags, {}, result,
-                                    scope);
+                    unqualifiedImpl(*current, name, location, first.range, flags, {}, result, scope,
+                                    nullptr);
                     break;
                 }
 

source/text/SourceManager.cpp

@@ -177,6 +177,47 @@ bool SourceManager::isPreprocessedLoc(SourceLocation location) const {
     return isMacroLoc(location) || isIncludedFileLoc(location);
 }
 
+std::optional<bool> SourceManager::isBeforeInCompilationUnit(SourceLocation left,
+                                                             SourceLocation right) const {
+    // Simple check: if they're in the same buffer, just do an easy compare
+    if (left.buffer() == right.buffer())
+        return left.offset() < right.offset();
+
+    auto moveUp = [this](SourceLocation& sl) {
+        if (sl && !isFileLoc(sl))
+            sl = getExpansionLoc(sl);
+        else {
+            SourceLocation included = getIncludedFrom(sl.buffer());
+            if (!included)
+                return true;
+            sl = included;
+        }
+        return false;
+    };
+
+    // Otherwise we have to build the full include / expansion chain and compare.
+    SmallMap<BufferID, size_t, 16> leftChain;
+    do {
+        leftChain.emplace(left.buffer(), left.offset());
+    } while (left.buffer() != right.buffer() && !moveUp(left));
+
+    SmallMap<BufferID, size_t, 16>::iterator it;
+    while ((it = leftChain.find(right.buffer())) == leftChain.end()) {
+        if (moveUp(right))
+            break;
+    }
+
+    if (it != leftChain.end())
+        left = SourceLocation(it->first, it->second);
+
+    // At this point, we either have a nearest common ancestor, or the two
+    // locations are simply in totally different compilation units.
+    if (left.buffer() == right.buffer())
+        return left.offset() < right.offset();
+
+    return std::nullopt;
+}
+
 SourceLocation SourceManager::getExpansionLoc(SourceLocation location) const {
     std::shared_lock lock(mutex);
     return getExpansionRangeImpl(location, lock).start();

tests/unittests/ast/TypeTests.cpp

@@ -2264,13 +2264,11 @@ TEST_CASE("Hierarchy-dependent type equivalence") {
     compilation.addSyntaxTree(tree);
     NO_COMPILATION_ERRORS;
 
-    const InstanceBodySymbol& top = compilation.getRoot().lookupName<InstanceSymbol>("top").body;
-    const Type* type1 =
-        &top.lookupName<InstanceSymbol>("if1").body.lookupName<VariableSymbol>("ready").getType();
-    const Type* type2 =
-        &top.lookupName<InstanceSymbol>("if2").body.lookupName<VariableSymbol>("ready").getType();
+    auto& root = compilation.getRoot();
+    auto& type1 = root.lookupName<VariableSymbol>("top.if1.ready").getType();
+    auto& type2 = root.lookupName<VariableSymbol>("top.if2.ready").getType();
 
-    CHECK(!type1->isEquivalent(*type2));
+    CHECK(!type1.isEquivalent(type2));
 }
 
 TEST_CASE("More enum member lookup cases") {
@@ -2334,3 +2332,27 @@ endinterface
     CHECK(diags[0].code == diag::MethodArgTypeMismatch);
     CHECK(diags[1].code == diag::MethodReturnMismatch);
 }
+
+TEST_CASE("Enum lookup location corner cases") {
+    auto tree = SyntaxTree::fromText(R"(
+localparam int A = 1;
+module m(input int a = C, enum {C,D} c);
+    localparam int q = A + $bits(enum{A=4,B});
+    localparam int r = A;
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 1);
+    CHECK(diags[0].code == diag::UsedBeforeDeclared);
+
+    auto& root = compilation.getRoot();
+    auto& q = root.lookupName<ParameterSymbol>("m.q");
+    CHECK(q.getValue().integer() == 33);
+
+    auto& r = root.lookupName<ParameterSymbol>("m.r");
+    CHECK(r.getValue().integer() == 4);
+}