[clang-tools-extra] [clangd] Check for valid location in ExtractionContext::exprIsValidOutside (PR #71162)

[Björn Pettersson via cfe-commits]

https://github.com/bjope updated https://github.com/llvm/llvm-project/pull/71162

>From 1fb659fa15568ec8256824339860de724853c6e3 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Thu, 2 Nov 2023 22:00:52 +0100
Subject: [PATCH 1/2] [clangd] Check for valid source location in
 ExtractionContext::exprIsValidOutside

If the code has a call to an implicitly declared function, an
expression could end up referencing declarations without valid
source locations. So when doing the exprIsValidOutside check we
could end up calling SourceManager::isPointWithin using invalid
source locations, and then a debug build would crash with an
assertion failure in SourceManager::isBeforeInTranslationUnit.

This patch make sure that we deal with the invalid locations
(by considering a ReferencedDecl with invalid location as not
being inside the Scope).
---
 .../clangd/refactor/tweaks/ExtractVariable.cpp        |  3 ++-
 clang-tools-extra/clangd/test/implicit-function.test  | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 clang-tools-extra/clangd/test/implicit-function.test

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 1e4edc6d6b4bb39..79bf962544242bb 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -176,7 +176,8 @@ bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
   SourceLocation ScopeBegin = Scope->getBeginLoc();
   SourceLocation ScopeEnd = Scope->getEndLoc();
   for (const Decl *ReferencedDecl : ReferencedDecls) {
-    if (SM.isPointWithin(ReferencedDecl->getBeginLoc(), ScopeBegin, ScopeEnd) &&
+    if (ReferencedDecl->getBeginLoc().isValid() &&
+        SM.isPointWithin(ReferencedDecl->getBeginLoc(), ScopeBegin, ScopeEnd) &&
         SM.isPointWithin(ReferencedDecl->getEndLoc(), ScopeBegin, ScopeEnd))
       return false;
   }
diff --git a/clang-tools-extra/clangd/test/implicit-function.test b/clang-tools-extra/clangd/test/implicit-function.test
new file mode 100644
index 000000000000000..85731f358b4ae8e
--- /dev/null
+++ b/clang-tools-extra/clangd/test/implicit-function.test
@@ -0,0 +1,11 @@
+// RUN: cp %s %t.c
+// RUN: not clangd -enable-config=0 -log=verbose -check=%t.c 2>&1 | FileCheck -strict-whitespace %s
+
+// Regression test for a situation when we used to hit an assertion failure
+// due to missing source location (for the implicit function declaration).
+
+// CHECK-DAG: [-Wimplicit-function-declaration] Line {{.*}}: call to undeclared function 'foo'
+// CHECK-DAG: [init_element_not_constant] Line {{.*}}: initializer element is not a compile-time constant
+// CHECK: All checks completed, 2 errors
+
+int x = foo();

>From 58084d9b753c08be76fe0b171297352e84d61f87 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Fri, 10 Nov 2023 11:54:25 +0100
Subject: [PATCH 2/2] TO BE SQUASHED

Changed the regression test into using unittest.
---
 clang-tools-extra/clangd/test/implicit-function.test  | 11 -----------
 .../clangd/unittests/tweaks/ExtractVariableTests.cpp  |  8 ++++++++
 2 files changed, 8 insertions(+), 11 deletions(-)
 delete mode 100644 clang-tools-extra/clangd/test/implicit-function.test

diff --git a/clang-tools-extra/clangd/test/implicit-function.test b/clang-tools-extra/clangd/test/implicit-function.test
deleted file mode 100644
index 85731f358b4ae8e..000000000000000
--- a/clang-tools-extra/clangd/test/implicit-function.test
+++ /dev/null
@@ -1,11 +0,0 @@
-// RUN: cp %s %t.c
-// RUN: not clangd -enable-config=0 -log=verbose -check=%t.c 2>&1 | FileCheck -strict-whitespace %s
-
-// Regression test for a situation when we used to hit an assertion failure
-// due to missing source location (for the implicit function declaration).
-
-// CHECK-DAG: [-Wimplicit-function-declaration] Line {{.*}}: call to undeclared function 'foo'
-// CHECK-DAG: [init_element_not_constant] Line {{.*}}: initializer element is not a compile-time constant
-// CHECK: All checks completed, 2 errors
-
-int x = foo();
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index 3d74a941071f849..eb5b06cfee43166 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -64,6 +64,14 @@ TEST_F(ExtractVariableTest, Test) {
       int x = [[1]];
     })cpp";
   EXPECT_AVAILABLE(AvailableC);
+
+  ExtraArgs = {"-xc"};
+  const char *NoCrashCasesC = R"cpp(
+    // error-ok: broken code, but shouldn't crash
+    int x = [[foo()]];
+    )cpp";
+  EXPECT_UNAVAILABLE(NoCrashCasesC);
+
   ExtraArgs = {"-xobjective-c"};
   const char *AvailableObjC = R"cpp(
     __attribute__((objc_root_class))