[PATCH] D101743: [clangd] Fix hover crash on broken code

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf800ac830941: [clangd] Fix hover crash on broken code 
(authored by kadircet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101743/new/

https://reviews.llvm.org/D101743

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(undefined); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(undefined); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101743: [clangd] Fix hover crash on broken code

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2447
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(x); } };)cpp");
+

hokein wrote:
> IIUC, x refers to an undefined variable right? might be renamed it to 
> `undefine`.
> 
> Instead of creating a new test, maybe add this no-crash test to `TEST(Hover, 
> NoHover) {`, looks like a good fit there.
> IIUC, x refers to an undefined variable right? might be renamed it to 
> undefine.

Done.

> Instead of creating a new test, maybe add this no-crash test to TEST(Hover, 
> NoHover) {, looks like a good fit there.

This produces a hover though. I think we should collect all the cases about 
crash-ness here, independent of whether they produce a hover or not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101743/new/

https://reviews.llvm.org/D101743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101743: [clangd] Fix hover crash on broken code

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 342671.
kadircet added a comment.

- s/x/undefined


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101743/new/

https://reviews.llvm.org/D101743

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(undefined); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(undefined); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101743: [clangd] Fix hover crash on broken code

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2447
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(x); } };)cpp");
+

IIUC, x refers to an undefined variable right? might be renamed it to 
`undefine`.

Instead of creating a new test, maybe add this no-crash test to `TEST(Hover, 
NoHover) {`, looks like a good fit there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101743/new/

https://reviews.llvm.org/D101743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101743: [clangd] Fix hover crash on broken code

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: usaxena95, arphaman.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101743

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(x); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2438,6 +2438,20 @@
   }
 }
 
+TEST(Hover, NoCrash) {
+  Annotations T(R"cpp(
+/* error-ok */
+template T foo(T);
+
+// Setter variable heuristic might fail if the callexpr is broken.
+struct X { int Y; void [[^setY]](float) { Y = foo(x); } };)cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto  : T.points())
+getHover(AST, P, format::getLLVMStyle(), nullptr);
+}
+
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -505,7 +505,7 @@
   if (auto *CE = llvm::dyn_cast(RHS->IgnoreCasts())) {
 if (CE->getNumArgs() != 1)
   return llvm::None;
-auto *ND = llvm::dyn_cast(CE->getCalleeDecl());
+auto *ND = llvm::dyn_cast_or_null(CE->getCalleeDecl());
 if (!ND || !ND->getIdentifier() || ND->getName() != "move" ||
 !ND->isInStdNamespace())
   return llvm::None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits