[clang][Diagnostics] Provide a source range for 'use of undeclared identifier' diagnostics

[tbaeder]

Summary

Just a small improvement.

Before:

array.cpp:1279:9: error: use of undeclared identifier 'foo'
 1279 | int a = foo();
      |         ^
array.cpp:1280:9: error: use of undeclared identifier 'foo'
 1280 | int b = foo;
      |         ^
2 errors generated.

After:

array.cpp:1279:9: error: use of undeclared identifier 'foo'
 1279 | int a = foo();
      |         ^~~
array.cpp:1280:9: error: use of undeclared identifier 'foo'
 1280 | int b = foo;
      |         ^~~
2 errors generated.

[aaron.ballman]

The changes are reasonable but should have test coverage. We test source ranges with -fdiagnostics-print-source-range-info as in https://github.com/llvm/llvm-project/blob/929a8c9f72dc405779a8aaf82304efdb7f1ab5e4/clang/test/Misc/diag-greatergreater.cpp#L4

[tbaeder]

Right, I expect (hope?) precommit CI to fail, but I'm stuck at an airport right now so I'll leave tests for when I'm back home.

[tbaeder]

clang/lib/Sema/SemaExpr.cpp

2187

I'm not passing the TypoRange here now, which regresses the test case I posted. Apparently the handling of -fmacro-backtrace-limit depends on the range passed here? That seems weird.

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

2187

Is it failing within checkRangesForMacroArgExpansion() in DiagnosticRenderer.cpp? It looks like this change effectively undoes the work from ecd36ee80b7a6ac73c84da19f8a75c4c025a7625

clang/test/Misc/reduced-diags-macros-backtrace.cpp

30–38

These notes make it harder to read the diagnostic instead of easier, I think they should remain suppressed.

42–56

Same here.

[tbaeder]

clang/lib/Sema/SemaExpr.cpp

2187

Well yes, the test case below simply didn't have any source ranges before (Ranges was empty), but now it does. The code behaves as expected I assume, just that... it doesn't do what I'd expect it to do. And it doesn't do what the test case below expects either.

I agree that the output before this patch is better (for the below test case), but the output shouldn't be affected by the source range we pass :/

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

2187

Yeah, but I think you'll have to solve this issue before we can land these changes.

[tbaeder]

Abandoning this. The code in question was dead before as far as I can tell, since the Ranges list was always empty. I can't add this without regressing any of the existing tests.

clang/include/clang/Sema/Lookup.h

public:
   }
 
   /// Gets the location of the identifier.  This isn't always defined:
   /// sometimes we're doing lookups on synthesized names.
   SourceLocation getNameLoc() const {
     return NameInfo.getLoc();
   }
 
+  SourceRange getNameRange() const {
+    return SourceRange(NameInfo.getBeginLoc(), NameInfo.getEndLoc());
+  }
+
   /// Get the Sema object that this lookup result is searching
   /// with.
   Sema &getSema() const { return *SemaPtr; }
 
   /// A class for iterating through a result set and possibly
   /// filtering out results.  The results returned are possibly
   /// sugared.
   class Filter {

clang/lib/Sema/SemaExpr.cpp

Sema::DecomposeUnqualifiedId(const UnqualifiedId &Id,
   } else {
     NameInfo = GetNameFromUnqualifiedId(Id);
     TemplateArgs = nullptr;
   }
 }
 
 static void emitEmptyLookupTypoDiagnostic(
     const TypoCorrection &TC, Sema &SemaRef, const CXXScopeSpec &SS,
-    DeclarationName Typo, SourceLocation TypoLoc, ArrayRef<Expr *> Args,
+    DeclarationName Typo, SourceRange TypoRange, 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(TypoRange.getBegin(), diag::err_no_member)
+          << Typo << Ctx << TypoRange;
     else
-      SemaRef.Diag(TypoLoc, DiagnosticID) << Typo;
+      SemaRef.Diag(TypoRange.getEnd(), DiagnosticID) << Typo << TypoRange;
     return;
   }
 
   std::string CorrectedStr = TC.getAsString(SemaRef.getLangOpts());
   bool DroppedSpecifier =
       TC.WillReplaceSpecifier() && Typo.getAsString() == CorrectedStr;
   unsigned NoteID = TC.getCorrectionDeclAs<ImplicitParamDecl>()
                         ? diag::note_implicit_param_decl
                         : diag::note_previous_decl;
   if (!Ctx)
     SemaRef.diagnoseTypo(TC, SemaRef.PDiag(DiagnosticSuggestID) << Typo,
                          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 << Ctx << DroppedSpecifier << TypoRange,
                          SemaRef.PDiag(NoteID));
 }
 
 /// Diagnose a lookup that found results in an enclosing class during error
 /// recovery. This usually indicates that the results were found in a dependent
 /// base class that could not be searched as part of a template definition.
 /// Always issues a diagnostic (though this may be only a warning in MS
 /// compatibility mode).
if (isa<CXXRecordDecl>(DC)) {
       }
 
       R.clear();
     }
 
     DC = DC->getLookupParent();
   }
 
+  SourceRange TypoRange = R.getNameRange();
   // 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, Name, TypoRange, Args,
                                         diagnostic, diagnostic_suggest);
         },
         nullptr, CTK_ErrorRecovery);
     if (*Out)
       return true;
   } else if (S &&
              (Corrected = CorrectTypo(R.getLookupNameInfo(), R.getLookupKind(),
                                       S, &SS, CCC, CTK_ErrorRecovery))) {
bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
   if (!SS.isEmpty()) {
     Diag(R.getNameLoc(), diag::err_no_member)
       << Name << computeDeclContext(SS, false)
       << SS.getRange();
     return true;
   }
 
   // Give up, we can't recover.
-  Diag(R.getNameLoc(), diagnostic) << Name;
+  Diag(R.getNameLoc(), diagnostic) << Name << TypoRange;
   return true;
 }
 
 /// In Microsoft mode, if we are inside a template class whose parent class has
 /// dependent base classes, and we can't resolve an unqualified identifier, then
 /// assume the identifier is a member of a dependent base class.  We can only
 /// recover successfully in static methods, instance methods, and other contexts
 /// where 'this' is available.  This doesn't precisely match MSVC's

clang/test/Misc/reduced-diags-macros-backtrace.cpp

 // RUN: not %clang_cc1 -fsyntax-only -fmacro-backtrace-limit 0 %s 2>&1 | FileCheck %s --check-prefix=ALL
 // RUN: not %clang_cc1 -fsyntax-only -fmacro-backtrace-limit 2 %s 2>&1 | FileCheck %s --check-prefix=SKIP
 
 #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);
 
+
 // ALL: {{.*}}:12:9: error: use of undeclared identifier 'p'
 // ALL-NEXT: int a = LEVEL1(b);
-// ALL-NEXT:         ^
+// ALL-NEXT:         ^~~~~~~~~
 // ALL-NEXT: {{.*}}:10:19: note: expanded from macro 'LEVEL1'
 // ALL-NEXT: #define LEVEL1(x) LEVEL2(x)
-// ALL-NEXT:                   ^
+// ALL-NEXT:                   ^~~~~~~~~
 // ALL-NEXT: {{.*}}:9:19: note: expanded from macro 'LEVEL2'
 // ALL-NEXT: #define LEVEL2(x) LEVEL3(x)
-// ALL-NEXT:                   ^
+// ALL-NEXT:                   ^~~~~~~~~
 // ALL-NEXT: {{.*}}:8:19: note: expanded from macro 'LEVEL3'
 // ALL-NEXT: #define LEVEL3(x) LEVEL4(x)
-// ALL-NEXT:                   ^
+// ALL-NEXT:                   ^~~~~~~~~
 // ALL-NEXT: {{.*}}:7:23: note: expanded from macro 'LEVEL4'
 // ALL-NEXT: #define LEVEL4(x) ADD(p,x)
 // ALL-NEXT:                       ^
+// ALL-NEXT: {{.*}}:6:20: note: expanded from macro 'ADD'
+// ALL-NEXT: #define ADD(x,y) G(x) + y
+// ALL-NEXT:                    ^
+// ALL-NEXT: {{.*}}:5:16: note: expanded from macro 'G'
+// ALL-NEXT: #define G(x) F(x) + 2
+// ALL-NEXT:                ^
+// ALL-NEXT: {{.*}}:4:14: note: expanded from macro 'F'
+// ALL-NEXT: #define F(x) x + 1
+// ALL-NEXT:              ^
 // ALL-NEXT: {{.*}}:12:16: error: use of undeclared identifier 'b'
 // ALL-NEXT: int a = LEVEL1(b);
 // ALL-NEXT:                ^
+// ALL-NEXT: {{.*}}:10:26: note: expanded from macro 'LEVEL1'
+// ALL-NEXT: #define LEVEL1(x) LEVEL2(x)
+// ALL-NEXT:                          ^
+// ALL-NEXT: {{.*}}:9:26: note: expanded from macro 'LEVEL2'
+// ALL-NEXT: #define LEVEL2(x) LEVEL3(x)
+// ALL-NEXT:                          ^
+// ALL-NEXT: {{.*}}:8:26: note: expanded from macro 'LEVEL3'
+// ALL-NEXT: #define LEVEL3(x) LEVEL4(x)
+// ALL-NEXT:                          ^
+// ALL-NEXT: {{.*}}:7:25: note: expanded from macro 'LEVEL4'
+// ALL-NEXT: #define LEVEL4(x) ADD(p,x)
+// ALL-NEXT:                         ^
+// ALL-NEXT: {{.*}}:6:25: note: expanded from macro 'ADD'
+// ALL-NEXT: #define ADD(x,y) G(x) + y
+// ALL-NEXT:                         ^
   // ALL-NEXT: 2 errors generated.
 
 // SKIP: {{.*}}:12:9: error: use of undeclared identifier 'p'
 // SKIP-NEXT: int a = LEVEL1(b);
-// SKIP-NEXT:         ^
+// SKIP-NEXT:         ^~~~~~~~~
 // SKIP-NEXT: {{.*}}:10:19: note: expanded from macro 'LEVEL1'
 // SKIP-NEXT: #define LEVEL1(x) LEVEL2(x)
-// SKIP-NEXT:                   ^
-// SKIP-NEXT: note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
-// SKIP-NEXT: {{.*}}:7:23: note: expanded from macro 'LEVEL4'
-// SKIP-NEXT: #define LEVEL4(x) ADD(p,x)
-// SKIP-NEXT:                       ^
+// SKIP-NEXT:                   ^~~~~~~~~
+// SKIP-NEXT: note: (skipping 5 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
+// SKIP-NEXT: {{.*}}:4:14: note: expanded from macro 'F'
+// SKIP-NEXT: #define F(x) x + 1
+// SKIP-NEXT:              ^
 // SKIP-NEXT: {{.*}}:12:16: error: use of undeclared identifier 'b'
 // SKIP-NEXT: int a = LEVEL1(b);
 // SKIP-NEXT:                ^
+// SKIP-NEXT: {{.*}}:10:26: note: expanded from macro 'LEVEL1'
+// SKIP-NEXT: #define LEVEL1(x) LEVEL2(x)
+// SKIP-NEXT:                          ^
+// SKIP-NEXT: note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
+// SKIP-NEXT: {{.*}}:6:25: note: expanded from macro 'ADD'
+// SKIP-NEXT: #define ADD(x,y) G(x) + y
+// SKIP-NEXT:                         ^
 // SKIP-NEXT: 2 errors generated.