[clang] Add source range to 'use of undeclared identifier' diagnostics #117671

tbaederr · 2024-11-26T05:36:15Z

No description provided.

llvmbot · 2024-11-26T05:36:54Z

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/117671.diff

1 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (+34-26)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6c7472ce92703b..cf3cc900f365b2 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -2351,20 +2351,23 @@ Sema::DecomposeUnqualifiedId(const UnqualifiedId &Id,
   }
 }
 
-static void emitEmptyLookupTypoDiagnostic(
-    const TypoCorrection &TC, Sema &SemaRef, const CXXScopeSpec &SS,
-    DeclarationName Typo, SourceLocation TypoLoc, ArrayRef<Expr *> Args,
-    unsigned DiagnosticID, unsigned DiagnosticSuggestID) {
+static void emitEmptyLookupTypoDiagnostic(const TypoCorrection &TC,
+                                          Sema &SemaRef, const CXXScopeSpec &SS,
+                                          const DeclarationNameInfo &Typo,
+                                          ArrayRef<Expr *> Args,
+                                          unsigned DiagnosticID,
+                                          unsigned DiagnosticSuggestID) {
   DeclContext *Ctx =
       SS.isEmpty() ? nullptr : SemaRef.computeDeclContext(SS, false);
   if (!TC) {
     // Emit a special diagnostic for failed member lookups.
     // FIXME: computing the declaration context might fail here (?)
     if (Ctx)
-      SemaRef.Diag(TypoLoc, diag::err_no_member) << Typo << Ctx
-                                                 << SS.getRange();
+      SemaRef.Diag(Typo.getLoc(), diag::err_no_member)
+          << Typo.getName() << Ctx << Typo.getSourceRange();
     else
-      SemaRef.Diag(TypoLoc, DiagnosticID) << Typo;
+      SemaRef.Diag(Typo.getLoc(), DiagnosticID)
+          << Typo.getName() << Typo.getSourceRange();
     return;
   }
 
@@ -2375,12 +2378,14 @@ static void emitEmptyLookupTypoDiagnostic(
                         ? diag::note_implicit_param_decl
                         : diag::note_previous_decl;
   if (!Ctx)
-    SemaRef.diagnoseTypo(TC, SemaRef.PDiag(DiagnosticSuggestID) << Typo,
+    SemaRef.diagnoseTypo(TC,
+                         SemaRef.PDiag(DiagnosticSuggestID) << Typo.getName(),
                          SemaRef.PDiag(NoteID));
   else
-    SemaRef.diagnoseTypo(TC, SemaRef.PDiag(diag::err_no_member_suggest)
-                                 << Typo << Ctx << DroppedSpecifier
-                                 << SS.getRange(),
+    SemaRef.diagnoseTypo(TC,
+                         SemaRef.PDiag(diag::err_no_member_suggest)
+                             << Typo.getName() << Ctx << DroppedSpecifier
+                             << SS.getRange(),
                          SemaRef.PDiag(NoteID));
 }
 
@@ -2448,13 +2453,14 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
                                TemplateArgumentListInfo *ExplicitTemplateArgs,
                                ArrayRef<Expr *> Args, DeclContext *LookupCtx,
                                TypoExpr **Out) {
-  DeclarationName Name = R.getLookupName();
+  const DeclarationNameInfo &NameInfo = R.getLookupNameInfo();
 
   unsigned diagnostic = diag::err_undeclared_var_use;
   unsigned diagnostic_suggest = diag::err_undeclared_var_use_suggest;
-  if (Name.getNameKind() == DeclarationName::CXXOperatorName ||
-      Name.getNameKind() == DeclarationName::CXXLiteralOperatorName ||
-      Name.getNameKind() == DeclarationName::CXXConversionFunctionName) {
+  if (DeclarationName::NameKind Kind = NameInfo.getName().getNameKind();
+      Kind == DeclarationName::CXXOperatorName ||
+      Kind == DeclarationName::CXXLiteralOperatorName ||
+      Kind == DeclarationName::CXXConversionFunctionName) {
     diagnostic = diag::err_undeclared_use;
     diagnostic_suggest = diag::err_undeclared_use_suggest;
   }
@@ -2506,13 +2512,12 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
   // We didn't find anything, so try to correct for a typo.
   TypoCorrection Corrected;
   if (S && Out) {
-    SourceLocation TypoLoc = R.getNameLoc();
     assert(!ExplicitTemplateArgs &&
            "Diagnosing an empty lookup with explicit template args!");
     *Out = CorrectTypoDelayed(
         R.getLookupNameInfo(), R.getLookupKind(), S, &SS, CCC,
         [=](const TypoCorrection &TC) {
-          emitEmptyLookupTypoDiagnostic(TC, *this, SS, Name, TypoLoc, Args,
+          emitEmptyLookupTypoDiagnostic(TC, *this, SS, NameInfo, Args,
                                         diagnostic, diagnostic_suggest);
         },
         nullptr, CTK_ErrorRecovery, LookupCtx);
@@ -2522,8 +2527,8 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
                        CorrectTypo(R.getLookupNameInfo(), R.getLookupKind(), S,
                                    &SS, CCC, CTK_ErrorRecovery, LookupCtx))) {
     std::string CorrectedStr(Corrected.getAsString(getLangOpts()));
-    bool DroppedSpecifier =
-        Corrected.WillReplaceSpecifier() && Name.getAsString() == CorrectedStr;
+    bool DroppedSpecifier = Corrected.WillReplaceSpecifier() &&
+                            NameInfo.getAsString() == CorrectedStr;
     R.setLookupName(Corrected.getCorrection());
 
     bool AcceptableWithRecovery = false;
@@ -2591,12 +2596,15 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
                             ? diag::note_implicit_param_decl
                             : diag::note_previous_decl;
       if (SS.isEmpty())
-        diagnoseTypo(Corrected, PDiag(diagnostic_suggest) << Name,
+        diagnoseTypo(Corrected,
+                     PDiag(diagnostic_suggest)
+                         << NameInfo.getName() << NameInfo.getSourceRange(),
                      PDiag(NoteID), AcceptableWithRecovery);
       else
-        diagnoseTypo(Corrected, PDiag(diag::err_no_member_suggest)
-                                  << Name << computeDeclContext(SS, false)
-                                  << DroppedSpecifier << SS.getRange(),
+        diagnoseTypo(Corrected,
+                     PDiag(diag::err_no_member_suggest)
+                         << NameInfo.getName() << computeDeclContext(SS, false)
+                         << DroppedSpecifier << SS.getRange(),
                      PDiag(NoteID), AcceptableWithRecovery);
 
       // Tell the callee whether to try to recover.
@@ -2609,13 +2617,13 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
   // FIXME: computing the declaration context might fail here (?)
   if (!SS.isEmpty()) {
     Diag(R.getNameLoc(), diag::err_no_member)
-      << Name << computeDeclContext(SS, false)
-      << SS.getRange();
+        << NameInfo.getName() << computeDeclContext(SS, false) << SS.getRange();
     return true;
   }
 
   // Give up, we can't recover.
-  Diag(R.getNameLoc(), diagnostic) << Name;
+  Diag(R.getNameLoc(), diagnostic)
+      << NameInfo.getName() << NameInfo.getSourceRange();
   return true;
 }

tbaederr · 2024-11-26T08:21:25Z

I knew there was something oddly familiar about this: https://reviews.llvm.org/D150191?id=521215 🤦‍♂️

clang/lib/Frontend/DiagnosticRenderer.cpp

cor3ntin · 2024-11-26T09:21:09Z

Can you add a description? Maybe a changelog

github-actions · 2024-11-26T09:28:05Z

✅ With the latest revision this PR passed the C/C++ code formatter.

tbaederr · 2024-11-26T09:47:49Z

It's the same problem again.

For this code:

#define F(x) x + 1
#define G(x) F(x) + 2
#define ADD(x,y) G(x) + y
#define LEVEL4(x) ADD(p,x)
#define LEVEL3(x) LEVEL4(x)
#define LEVEL2(x) LEVEL3(x)
#define LEVEL1(x) LEVEL2(x)

int a = LEVEL1(b);

The previous output was:

./array.cpp:34:9: error: use of undeclared identifier 'p'
   34 | int a = LEVEL1(b);
      |         ^
./array.cpp:32:19: note: expanded from macro 'LEVEL1'
   32 | #define LEVEL1(x) LEVEL2(x)
      |                   ^
./array.cpp:31:19: note: expanded from macro 'LEVEL2'
   31 | #define LEVEL2(x) LEVEL3(x)
      |                   ^
./array.cpp:30:19: note: expanded from macro 'LEVEL3'
   30 | #define LEVEL3(x) LEVEL4(x)
      |                   ^
./array.cpp:29:23: note: expanded from macro 'LEVEL4'
   29 | #define LEVEL4(x) ADD(p,x)
      |                       ^
./array.cpp:34:16: error: use of undeclared identifier 'b'
   34 | int a = LEVEL1(b);
      |                ^
2 errors generated.

and we now print:

./array.cpp:34:9: error: use of undeclared identifier 'p'
   34 | int a = LEVEL1(b);
      |         ^~~~~~~~~
./array.cpp:32:19: note: expanded from macro 'LEVEL1'
   32 | #define LEVEL1(x) LEVEL2(x)
      |                   ^~~~~~~~~
./array.cpp:31:19: note: expanded from macro 'LEVEL2'
   31 | #define LEVEL2(x) LEVEL3(x)
      |                   ^~~~~~~~~
./array.cpp:30:19: note: expanded from macro 'LEVEL3'
   30 | #define LEVEL3(x) LEVEL4(x)
      |                   ^~~~~~~~~
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
./array.cpp:28:20: note: expanded from macro 'ADD'
   28 | #define ADD(x,y) G(x) + y
      |                    ^
./array.cpp:27:16: note: expanded from macro 'G'
   27 | #define G(x) F(x) + 2
      |                ^
./array.cpp:26:14: note: expanded from macro 'F'
   26 | #define F(x) x + 1
      |              ^
./array.cpp:34:16: error: use of undeclared identifier 'b'
   34 | int a = LEVEL1(b);
      |                ^
./array.cpp:32:26: note: expanded from macro 'LEVEL1'
   32 | #define LEVEL1(x) LEVEL2(x)
      |                          ^
./array.cpp:31:26: note: expanded from macro 'LEVEL2'
   31 | #define LEVEL2(x) LEVEL3(x)
      |                          ^
./array.cpp:30:26: note: expanded from macro 'LEVEL3'
   30 | #define LEVEL3(x) LEVEL4(x)
      |                          ^
./array.cpp:29:25: note: expanded from macro 'LEVEL4'
   29 | #define LEVEL4(x) ADD(p,x)
      |                         ^
./array.cpp:28:25: note: expanded from macro 'ADD'
   28 | #define ADD(x,y) G(x) + y
      |                         ^
2 errors generated.

but this is not a bug introduced by the changes, rather, the newly supplied (valid!) source ranges now get taken into account in checkRangesForMacroArgExpansion() in Frontend/DiagnosticRenderer.cpp.

AaronBallman · 2024-11-27T17:50:12Z

but this is not a bug introduced by the changes, rather, the newly supplied (valid!) source ranges now get taken into account in checkRangesForMacroArgExpansion() in Frontend/DiagnosticRenderer.cpp.

Yeah, I think we need to solve that before moving forward with the changes, which is why this keeps not being improved. (It always seems to come back to bite us when we decide that invalid source locations are an in-band indicator of anything.)

tbaederr · 2024-11-29T15:49:31Z

This is the best version I could come up with. Other than that, the code as it is seems to do what is advertised, i.e. the new output is the desired output.

AaronBallman

Generally speaking, this looks like it's heading in the right direction.

clang/lib/Basic/SourceManager.cpp

clang/lib/Frontend/DiagnosticRenderer.cpp

In rangeInsideSameMacroArgExpansion, check the given Ranges instead of the SpellingRanges.

tbaederr · 2025-04-17T15:56:48Z

Ping

AaronBallman

LGTM!

llvm-ci · 2025-04-18T06:32:53Z

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building clang at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/20492