[clang][Sema] Improve error recovery for id-expressions referencing invalid decls

[HighCommander4]

Passing AcceptInvalidDecl=true to BuildDeclarationNameExpr() allows the RecoveryExpr that's constructed to retain a DeclRefExpr pointing to the invalid decl as a child, preserving information about the reference for use by tools such as clangd.

Fixes clangd/clangd#1820

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Passing AcceptInvalidDecl=true to BuildDeclarationNameExpr() allows the RecoveryExpr that's constructed to retain a DeclRefExpr pointing to the invalid decl as a child, preserving information about the reference for use by tools such as clangd.

Fixes clangd/clangd#1820

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaExpr.cpp (+1-1)
• (modified) clang/test/AST/ast-dump-recovery.cpp (+6)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 4049ab3bf6cafb..3030d388147b24 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -2927,7 +2927,7 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
     return BuildTemplateIdExpr(SS, TemplateKWLoc, R, ADL, TemplateArgs);
   }
 
-  return BuildDeclarationNameExpr(SS, R, ADL);
+  return BuildDeclarationNameExpr(SS, R, ADL, /*AcceptInvalidDecl=*/true);
 }
 
 /// BuildQualifiedDeclarationNameExpr - Build a C++ qualified
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
index cfb013585ad744..f628fa913da1e6 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -402,6 +402,7 @@ void returnInitListFromVoid() {
   // CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 8
 }
 
+void FuncTakingUnknown(Unknown);
 void RecoveryExprForInvalidDecls(Unknown InvalidDecl) {
   InvalidDecl + 1;
   // CHECK:      BinaryOperator {{.*}}
@@ -411,6 +412,11 @@ void RecoveryExprForInvalidDecls(Unknown InvalidDecl) {
   InvalidDecl();
   // CHECK:      CallExpr {{.*}}
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
+  FuncTakingUnknown(InvalidDecl);
+  // CHECK:      CallExpr {{.*}} '<dependent type>'
+  // CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} '<overloaded function type>'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'InvalidDecl' 'int'
 }
 
 void RecoverToAnInvalidDecl() {

[HighCommander4]

Changing to Draft status as CI is showing some test failures that I will need to investigate:

Failed Tests (4):
  Clang :: OpenMP/nvptx_unsupported_type_messages.cpp
  Clang :: SemaCXX/constant-expression-cxx11.cpp
  Clang :: SemaCXX/cxx2b-consteval-if.cpp
  Clang :: SemaCXX/deduced-return-type-cxx14.cpp

[HighCommander4]

Changing to Draft status as CI is showing some test failures that I will need to investigate:

Failed Tests (4):
  Clang :: OpenMP/nvptx_unsupported_type_messages.cpp
  Clang :: SemaCXX/constant-expression-cxx11.cpp
  Clang :: SemaCXX/cxx2b-consteval-if.cpp
  Clang :: SemaCXX/deduced-return-type-cxx14.cpp

The failures are related to new diagnostics that are issued as a side effect of running additional checks in BuildDeclarationNameExpr().

Accepting these additional diagnostics would regress some previous changes such as 1cf4541 and https://reviews.llvm.org/D95928. I think I'll need to find a way to avoid them instead.

[HighCommander4]

The failures are related to new diagnostics that are issued as a side effect of running additional checks in BuildDeclarationNameExpr().

A closer investigation has revealed that this diagnosis wasn't quite accurate.

The three SemaCXX failures were not in fact caused by new diagnostics issued as part of doing extra semantic analysis in BuildDeclarationNameExpr(). Rather, the original version of the patch had the unintended effect of having this overload of BuildDeclarationNameExpr() construct an UnresolvedLookupExpr even when the only lookup result was an invalid decl, where previously it would fail in that scenario.

In my updated patch, I made an adjustment to this function to behave similarly to the other overload, i.e. wrap the resulting expression in a RecoveryExpr in the invalid-decl case. This fixed all three SemaCXX failures.

The OpenMP failure remains, though there too it's not the case that the extra diagnostic is directly produced by the extra checks that now run in BuildDeclarationNameExpr(). Rather, I suspect that the extra semantic analysis we're doing indirectly leads to extra diagnostics. I haven't yet tracked down the chain of causation there.

[mizvekov]

Minor nit, otherwise LGTM once you fix outstanding issues.

[HighCommander4]

(OpenMP failure still remains to be fixed.)

[HighCommander4]

Here is a reduced testcase for the OpenMP test failure:

#pragma omp declare target

static long double ld_return1e() { return 0; }

void external() {
  void *p1 = reinterpret_cast<void*>(&ld_return1e);
}

#pragma omp end declare target

When built with the following command:

clang -cc1 -fopenmp -triple nvptx64-unknown-unknown -aux-triple x86_64-unknown-linux test.cpp -fopenmp-is-target-device -fsyntax-only

Here are the diagnostics before this patch:

test.cpp:6:39: error: 'ld_return1e' requires 128 bit size 'long double' type support, but target 'nvptx64-unknown-unknown' does not support it
    6 |   void *p1 = reinterpret_cast<void*>(&ld_return1e);
      |                                       ^
test.cpp:3:20: note: 'ld_return1e' defined here
    3 | static long double ld_return1e() { return 0; }
      |                    ^
1 error generated

and here are the diagnostics after this patch:

test.cpp:6:39: error: 'ld_return1e' requires 128 bit size 'long double' type support, but target 'nvptx64-unknown-unknown' does not support it
    6 |   void *p1 = reinterpret_cast<void*>(&ld_return1e);
      |                                       ^
test.cpp:3:20: note: 'ld_return1e' defined here
    3 | static long double ld_return1e() { return 0; }
      |                    ^
test.cpp:3:20: error: 'ld_return1e' requires 128 bit size 'long double' type support, but target 'nvptx64-unknown-unknown' does not support it
    3 | static long double ld_return1e() { return 0; }
      |                    ^
test.cpp:6:39: note: called by 'external'
    6 |   void *p1 = reinterpret_cast<void*>(&ld_return1e);
      |                                       ^
test.cpp:3:20: note: 'ld_return1e' defined here
    3 | static long double ld_return1e() { return 0; }
      |                    ^
2 errors generated.

Basically, the patch somehow causes the same diagnostic to be issued a second time at a different location.