Suggest a similarly-spelled identifier on undefined-identifier errors (#9124)

Author: expipiplus1

source/core/slang-string-util.cpp

@@ -2,6 +2,7 @@
 
 #include "slang-blob.h"
 #include "slang-char-util.h"
+#include "slang-math.h"
 #include "slang-text-io.h"
 
 namespace Slang
@@ -975,4 +976,61 @@ int StringUtil::parseIntAndAdvancePos(UnownedStringSlice text, Index& pos)
     return result;
 }
 
+// Shared Levenshtein core. When `caseInsensitive` is set, characters are folded
+// with `CharUtil::toLower` during the comparison, avoiding the need to allocate
+// lowercased copies of the inputs.
+static Index _calcLevenshteinDistance(
+    const UnownedStringSlice& a,
+    const UnownedStringSlice& b,
+    bool caseInsensitive)
+{
+    const Index lenA = a.getLength();
+    const Index lenB = b.getLength();
+    if (lenA == 0)
+        return lenB;
+    if (lenB == 0)
+        return lenA;
+
+    auto fold = [&](char c) -> char { return caseInsensitive ? CharUtil::toLower(c) : c; };
+
+    // Classic dynamic-programming Levenshtein using two rolling rows so the
+    // working set is O(lenB) rather than O(lenA * lenB).
+    List<Index> prevRow;
+    List<Index> currRow;
+    prevRow.setCount(lenB + 1);
+    currRow.setCount(lenB + 1);
+
+    for (Index j = 0; j <= lenB; ++j)
+        prevRow[j] = j;
+
+    for (Index i = 1; i <= lenA; ++i)
+    {
+        currRow[0] = i;
+        const char ca = fold(a[i - 1]);
+        for (Index j = 1; j <= lenB; ++j)
+        {
+            const Index cost = (ca == fold(b[j - 1])) ? 0 : 1;
+            const Index deletion = prevRow[j] + 1;
+            const Index insertion = currRow[j - 1] + 1;
+            const Index substitution = prevRow[j - 1] + cost;
+            currRow[j] = Math::Min(deletion, insertion, substitution);
+        }
+        prevRow.swapWith(currRow);
+    }
+
+    return prevRow[lenB];
+}
+
+Index StringUtil::calcLevenshteinDistance(const UnownedStringSlice& a, const UnownedStringSlice& b)
+{
+    return _calcLevenshteinDistance(a, b, /*caseInsensitive:*/ false);
+}
+
+Index StringUtil::calcLevenshteinDistanceCaseInsensitive(
+    const UnownedStringSlice& a,
+    const UnownedStringSlice& b)
+{
+    return _calcLevenshteinDistance(a, b, /*caseInsensitive:*/ true);
+}
+
 } // namespace Slang

source/core/slang-string-util.h

@@ -198,6 +198,20 @@ struct StringUtil
     /// Modifies pos to the position where parsing ends.
     /// Returns parsed integer.
     static int parseIntAndAdvancePos(UnownedStringSlice text, Index& pos);
+
+    /// Compute the Levenshtein edit distance between `a` and `b`: the minimum number of
+    /// single-character insertions, deletions, or substitutions needed to turn one string
+    /// into the other. Used, for example, to suggest the closest known identifier for a
+    /// misspelled name ("did you mean ...?"). The comparison is case-sensitive; use
+    /// `calcLevenshteinDistanceCaseInsensitive` (or lower-case the inputs beforehand) for a
+    /// case-insensitive distance.
+    static Index calcLevenshteinDistance(const UnownedStringSlice& a, const UnownedStringSlice& b);
+
+    /// Like `calcLevenshteinDistance`, but folds characters with `CharUtil::toLower` during
+    /// the comparison, so it does not allocate lower-cased copies of the inputs.
+    static Index calcLevenshteinDistanceCaseInsensitive(
+        const UnownedStringSlice& a,
+        const UnownedStringSlice& b);
 };
 
 /* A helper class that allows parsing of lines from text with iteration. Uses

source/slang/slang-check-decl.cpp

@@ -1775,6 +1775,11 @@ QualType getTypeForDeclRef(
         // TODO: This code could break if we ever go down this path with
         // an identifier that doesn't have a name.
         //
+        // Note: unlike the `VarExpr` lookup path, we do not attach a "did you
+        // mean ...?" suggestion here. This site is reached for a name that
+        // already resolved to a contextual keyword (not an ordinary in-scope
+        // lookup), so a Levenshtein match against the lexical scope would be
+        // misleading.
         sink->diagnose(
             Diagnostics::UndefinedIdentifier{.name = declRef.getName(), .location = loc});
     }

source/slang/slang-check-expr.cpp

@@ -12,6 +12,7 @@
 // * `slang-check-conversion.cpp` is responsible for the logic of handling type conversion/coercion
 
 #include "../core/slang-math.h"
+#include "../core/slang-string-util.h"
 #include "core/slang-char-util.h"
 #include "slang-ast-decl.h"
 #include "slang-ast-natural-layout.h"
@@ -5053,6 +5054,127 @@ Expr* SemanticsExprVisitor::visitInvokeExpr(InvokeExpr* expr)
     return checkedExpr;
 }
 
+// Find the in-scope identifier whose spelling is closest to `name`, to power a
+// "did you mean ...?" suggestion when `name` failed to resolve. Walks the scope
+// chain (and each scope's sibling chain) collecting the names of direct members,
+// computes the case-insensitive Levenshtein distance to each, and returns the
+// single closest candidate within a small distance threshold. Returns nullptr if
+// nothing is close enough (so the caller can simply omit the suggestion), or if
+// two candidates tie at the closest distance (so the output never depends on
+// import/scope-walk order).
+//
+// Only direct members of the lexical scope chain are considered; inherited and
+// extension members reached via the `this`-parameter breadcrumb in real lookup
+// are deliberately not searched, to keep this off the hot path of successful
+// lookups. `semantics` is used to skip candidates the user could not access.
+static Name* findClosestInScopeName(
+    SemanticsVisitor* semantics,
+    Name* name,
+    Scope* scope,
+    Decl* declToExclude)
+{
+    if (!name)
+        return nullptr;
+
+    const UnownedStringSlice target = getUnownedStringSliceText(name);
+
+    // Scale the allowed edit distance with the identifier length and cap it, so
+    // that we only offer genuinely-close names (e.g. `lenght` -> `length`, or
+    // `f_a` -> `f_b`) and never a wildly different one (e.g. the keyword `case`
+    // -> the module `core`, a distance-2 edit on a 4-char name). The heuristic is
+    // roughly one edit per three characters with a floor of one. Names shorter
+    // than 3 chars are too short to suggest against; and we also refuse
+    // pathologically long names, since suggestions are advisory and not worth an
+    // O(N*M) Levenshtein DP per candidate on an adversarial multi-kilobyte
+    // identifier.
+    if (target.getLength() < 3 || target.getLength() > 256)
+        return nullptr;
+    const Index maxDistance = Math::Min<Index>(3, Math::Max<Index>(1, target.getLength() / 3));
+
+    Index bestDistance = maxDistance + 1;
+    // The *distinct* candidate names sharing the current best distance. Names are
+    // deduped (the "nub" of the candidate list) so that, e.g., two overloads
+    // both called `length` count once: they would print the identical
+    // suggestion, so they are not a genuine ambiguity. A suggestion is offered
+    // only when this set ends up with exactly one name.
+    HashSet<Name*> bestNames;
+
+    for (Scope* s = scope; s; s = s->parent)
+    {
+        for (Scope* sib = s; sib; sib = sib->nextSibling)
+        {
+            auto containerDecl = sib->containerDecl;
+            if (!containerDecl)
+                continue;
+
+            // Don't suggest names from the core module. Its global scope holds
+            // thousands of builtins, so almost any identifier finds a spurious
+            // close match there (e.g. `instance` -> `distance`); restricting to
+            // user-written declarations keeps suggestions quiet and relevant.
+            // Skipping the whole container here (rather than per-member) also
+            // avoids iterating — and, for any on-demand-deserialized core
+            // module, materializing — its members via `getDirectMemberDecls()`.
+            if (isFromCoreModule(containerDecl))
+                continue;
+
+            for (auto candidateDecl : containerDecl->getDirectMemberDecls())
+            {
+                Name* candidateName = candidateDecl->getName();
+                if (!candidateName || candidateName == name)
+                    continue;
+
+                // Skip the declaration currently being checked, mirroring the
+                // `getDeclToExcludeFromLookup()` exclusion that real lookup
+                // applies: suggesting it would name something the same lookup
+                // path still cannot resolve.
+                if (candidateDecl == declToExclude)
+                    continue;
+
+                const UnownedStringSlice candidateText = getUnownedStringSliceText(candidateName);
+                if (candidateText.getLength() == 0)
+                    continue;
+
+                // Cheap length pre-filter: |lenA - lenB| is a lower bound on the
+                // edit distance, so skip candidates that cannot possibly be
+                // within `maxDistance` before paying for the O(lenA*lenB) DP (and
+                // before forcing any lazy member materialization downstream).
+                if (Math::Abs(candidateText.getLength() - target.getLength()) > maxDistance)
+                    continue;
+
+                // Don't suggest a declaration the user could not have referenced:
+                // real lookup already filtered inaccessible (`private`/`internal`)
+                // candidates, so offering one as a "did you mean" would name a
+                // forbidden symbol (and leak imported module contents via typos).
+                if (!semantics->isDeclVisibleFromScope(makeDeclRef(candidateDecl), scope))
+                    continue;
+
+                const Index distance =
+                    StringUtil::calcLevenshteinDistanceCaseInsensitive(target, candidateText);
+                if (distance < bestDistance)
+                {
+                    bestDistance = distance;
+                    bestNames.clear();
+                    bestNames.add(candidateName);
+                }
+                else if (distance == bestDistance)
+                {
+                    bestNames.add(candidateName);
+                }
+            }
+        }
+    }
+
+    // Offer a suggestion only when there is a single, unambiguous closest name.
+    // Multiple distinct names at the best distance would make the chosen one
+    // depend on import/scope-walk order, so suppress the suggestion instead.
+    if (bestDistance > maxDistance || bestNames.getCount() != 1)
+        return nullptr;
+    Name* best = nullptr;
+    for (auto n : bestNames)
+        best = n;
+    return best;
+}
+
 Expr* SemanticsExprVisitor::visitVarExpr(VarExpr* expr)
 {
     // If we've already resolved this expression, don't try again.
@@ -5097,8 +5219,19 @@ Expr* SemanticsExprVisitor::visitVarExpr(VarExpr* expr)
     }
 
     if (!diagnosed)
-        getSink()->diagnose(
-            Diagnostics::UndefinedIdentifier{.name = expr->name, .location = expr->loc});
+    {
+        // If a similarly-spelled identifier is in scope, attach a "did you mean
+        // 'length'?" note to the error; this turns a bare "undefined identifier
+        // 'lenght'" into an actionable hint. The note only renders when
+        // `suggestionLocation` is valid, so a null suggestion is harmless.
+        auto suggestion =
+            findClosestInScopeName(this, expr->name, expr->scope, getDeclToExcludeFromLookup());
+        getSink()->diagnose(Diagnostics::UndefinedIdentifier{
+            .name = expr->name,
+            .suggestion = suggestion,
+            .location = expr->loc,
+            .suggestionLocation = suggestion ? expr->loc : SourceLoc{}});
+    }
 
     return resultExpr;
 }

source/slang/slang-check-modifier.cpp

@@ -1968,6 +1968,11 @@ Modifier* SemanticsVisitor::checkModifier(
                     genDecl->ownedScope);
                 if (!scrutineeResults.isValid())
                 {
+                    // No "did you mean ...?" suggestion here (unlike the
+                    // `VarExpr` path): a `__target_intrinsic` scrutinee names a
+                    // capability/target identifier, not an ordinary in-scope
+                    // declaration, so a Levenshtein match against the lexical
+                    // scope would be misleading.
                     getSink()->diagnose(Diagnostics::UndefinedIdentifier{
                         .name = targetIntrinsic->scrutinee.name,
                         .location = targetIntrinsic->scrutinee.loc});

source/slang/slang-diagnostics.lua

@@ -1130,7 +1130,10 @@ err(
     "undefined-identifier",
     30015,
     "undefined identifier",
-    span { loc = "location", message = "undefined identifier '~name:Name'." }
+    span { loc = "location", message = "undefined identifier '~name:Name'." },
+    -- Optional "did you mean ...?" note, rendered only when `suggestionLocation`
+    -- is set to a valid location by the caller (i.e. a close in-scope name exists).
+    note { message = "did you mean '~suggestion:Name'?", span { loc = "suggestionLocation" } }
 )
 
 err(

tests/diagnostics/autodiff-custom-diff-unresolved.slang

@@ -12,6 +12,7 @@ void __d_f(float x, float.Differential dout)
 /*CHECK:
                     ^^^^^ undefined identifier
                     ^^^^^ undefined identifier '__d_g'.
+                    ^^^^^ did you mean '__d_f'?
  ^^^^^^^^^^^^^^^^^^ cannot resolve derivative function
  ^^^^^^^^^^^^^^^^^^ cannot resolve the custom derivative function
 */