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-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,110 @@ 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)
+{
+    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). Roughly one edit
+    // per three characters with a floor of one, matching the heuristic used by
+    // other compilers. Names shorter than 3 chars are too short to suggest against.
+    if (target.getLength() < 3)
+        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;
+
+            for (auto candidateDecl : containerDecl->getDirectMemberDecls())
+            {
+                Name* candidateName = candidateDecl->getName();
+                if (!candidateName || candidateName == name)
+                    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.
+                if (isFromCoreModule(candidateDecl))
+                    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 +5202,18 @@ 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);
+        getSink()->diagnose(Diagnostics::UndefinedIdentifier{
+            .name = expr->name,
+            .suggestion = suggestion,
+            .location = expr->loc,
+            .suggestionLocation = suggestion ? expr->loc : SourceLoc{}});
+    }
 
     return resultExpr;
 }

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
 */

tests/diagnostics/did-you-mean-identifier.slang

@@ -0,0 +1,107 @@
+// Test for issue #9124: an undefined identifier that is a close misspelling of
+// an in-scope name produces a "did you mean ...?" suggestion note.
+//
+// This test runs in exhaustive mode: every emitted diagnostic must be
+// annotated. That is what enforces the *negative* cases below — if a spurious
+// "did you mean" note were ever produced (e.g. if a future change dropped the
+// min-length guard, raised the distance cap, or removed the core-module
+// filter), it would be an unannotated diagnostic and fail the test.
+
+//DIAGNOSTIC_TEST:SIMPLE(diag=CHECK):-target spirv
+
+float closeMatch(float myVariable)
+{
+    return myVariabl;
+//CHECK:   ^^^^^^^^^ undefined identifier
+//CHECK:   ^^^^^^^^^ undefined identifier 'myVariabl'.
+//CHECK:   ^^^^^^^^^ did you mean 'myVariable'?
+}
+
+// A name with no nearby candidate must NOT produce a suggestion note.
+float noMatch()
+{
+    return zzzzqqq;
+//CHECK:   ^^^^^^^ undefined identifier
+//CHECK:   ^^^^^^^ undefined identifier 'zzzzqqq'.
+}
+
+// Min-length threshold: identifiers shorter than 3 chars never get a
+// suggestion, even when an in-scope candidate is one edit away (`ab` vs `ax`).
+float tooShort(float ax)
+{
+    return ab;
+//CHECK:   ^^ undefined identifier
+//CHECK:   ^^ undefined identifier 'ab'.
+}
+
+// Distance-cap boundary. For a 6-char target the cap is
+// min(3, max(1, 6/3)) = 2. A candidate at distance 2 is suggested.
+float atCap(float abcdef)
+{
+    // `abXYef` -> `abcdef` is distance 2 (c,d substituted) == maxDistance.
+    return abXYef;
+//CHECK:   ^^^^^^ undefined identifier
+//CHECK:   ^^^^^^ undefined identifier 'abXYef'.
+//CHECK:   ^^^^^^ did you mean 'abcdef'?
+}
+
+// One past the cap: distance 3 on the same 6-char target must NOT suggest.
+float overCap(float abcdef)
+{
+    // `abXYZf` -> `abcdef` is distance 3 (c,d,e substituted) > maxDistance.
+    return abXYZf;
+//CHECK:   ^^^^^^ undefined identifier
+//CHECK:   ^^^^^^ undefined identifier 'abXYZf'.
+}
+
+// Core-module exclusion: a typo of a builtin type name must not be suggested,
+// since core-module declarations are deliberately excluded.
+float coreExcluded()
+{
+    return flota;
+//CHECK:   ^^^^^ undefined identifier
+//CHECK:   ^^^^^ undefined identifier 'flota'.
+}
+
+// Tie-break suppression: when two distinctly-named in-scope candidates are
+// equally close to the typo, the winner would depend on scope/import order, so
+// no suggestion is offered. Here both `abcd` and `abce` are at distance 1 from
+// `abcf`.
+float ambiguous(float abcd, float abce)
+{
+    return abcf;
+//CHECK:   ^^^^ undefined identifier
+//CHECK:   ^^^^ undefined identifier 'abcf'.
+}
+
+// Same-name overloads are NOT ambiguous: both `overloaded` decls would print the
+// identical suggestion, so the deduped candidate set has one name and the
+// suggestion is still offered.
+int overloaded(int x) { return x; }
+int overloaded(float x) { return 0; }
+int sameNameOverloads()
+{
+    return overloade;
+//CHECK:   ^^^^^^^^^ undefined identifier
+//CHECK:   ^^^^^^^^^ undefined identifier 'overloade'.
+//CHECK:   ^^^^^^^^^ did you mean 'overloaded'?
+}
+
+// Inherited-member limitation: only the lexical scope chain is searched, not
+// inherited members reached via the implicit `this`. A typo of an inherited
+// field gets no suggestion (documented limitation, pinned so any future
+// improvement is a deliberate change).
+struct Base
+{
+    float lengthField;
+}
+struct Derived : Base
+//CHECK:         ^^^^ support for inheritance is unstable
+{
+    float compute()
+    {
+        return lengthFiel;
+//CHECK:       ^^^^^^^^^^ undefined identifier
+//CHECK:       ^^^^^^^^^^ undefined identifier 'lengthFiel'.
+    }
+}

tests/diagnostics/transitive-namespace-import.slang/test.slang

@@ -22,6 +22,7 @@ namespace ns
         f_a(); // Error.
 //CHECK:^^^ undefined identifier
 //CHECK:^^^ undefined identifier 'f_a'.
+//CHECK:^^^ did you mean 'f_b'?
     }
 }
 
@@ -35,5 +36,6 @@ namespace ns2
         f_a(); // Error.
 //CHECK:^^^ undefined identifier
 //CHECK:^^^ undefined identifier 'f_a'.
+//CHECK:^^^ did you mean 'f_b'?
     }
 }