[clangd] Skip function parameter decls when evaluating variables on hover.

[VitaNuo]

Simplify.

[hokein]

thanks.

clang-tools-extra/clangd/unittests/HoverTests.cpp

3727

I believe this case should trigger a crash, but I don't see the crash with a trunk-built clangd, do we miss something here?

[VitaNuo]

thanks!

clang-tools-extra/clangd/unittests/HoverTests.cpp

3727

It crashes on invalid code. I've inspected two user workspaces:

class Foo{};
void foo(Foo param = nullptr);

void foo(ClassName param = functionReturningObjectOfSimilarSoundingButUnrelatedClass());

I guess clangd is not even expected to act soundly in the face of non-compiling code.
But since these crashes are easy to get rid of, and evaluating parameters in function declarations is pointless anyways, we can just avoid these crashes at no extra cost. I cannot use non-compiling code in a hover test, though.

[hokein]

clang-tools-extra/clangd/Hover.cpp

663

We're ignoring all ParmVarDecl cases,

The crash here is for broken code. For the crash case, the AST node looks like

`-ParmVarDecl 0x563b7cc853c0 <col:10, col:20> col:14 invalid param 'Foo':'Foo' cinit
    `-OpaqueValueExpr 0x563b7cc854a0 <col:20> 'Foo':'Foo'

One alternative is to exclude invalid VarDecl, then the Hover feature still keep working on valid ParmVarDecl.

if (const auto* Var = dyn_cast<VarDecl>(D); Var && !Var->isInvalidDecl()) {
   ...
}

clang-tools-extra/clangd/unittests/HoverTests.cpp

3727

thanks for the clarification. That makes sense.

We can still use these broken code (the first one should be good enough) in the HoverTest, we need to add a magic comment /* error-ok */.

[VitaNuo]

Address review comments.

[VitaNuo]

Simplify.

[VitaNuo]

Thanks for the review!

clang-tools-extra/clangd/Hover.cpp

663

Why would you do that?

663

Sorry this was an unintended comment.

[hokein]

Thanks, looks good, just one nit to simplify the unittest.

clang-tools-extra/clangd/unittests/HoverTests.cpp

3726

nit: instead of creating a completely-new TEST, it seems simpler to just add a testcase in the existing TEST(Hover, All).

{R"cpp(// Should not crash on an invalid param decl.
       class Foo {};
       // error-ok
       void foo(Foo [[fo^o]] = nullptr);
       )cpp",
    [](HoverInfo &HI) {
      HI.Name = "foo";
      HI.Type = "Foo";
      HI.Kind = index::SymbolKind::Parameter;
      HI.NamespaceScope = "";
      HI.LocalScope = "foo::";
      HI.Definition = "Foo foo = <null expr>";
    }},

[VitaNuo]

Thanks!

clang-tools-extra/clangd/unittests/HoverTests.cpp

3726

I'm not sure I agree. The Hover, All test takes already about 5 seconds to scroll through on the screen :)

On top of that, other test cases that are a result of crash fixes seem to be separate too.
Therefore, I'd prefer to keep it as a separate test case for now.

clang-tools-extra/clangd/Hover.cpp

HoverInfo getHoverContents(const NamedDecl *D, const PrintingPolicy &PP,
   else if (const auto *VT = dyn_cast<VarTemplateDecl>(D))
     HI.Type = printType(VT->getTemplatedDecl()->getType(), Ctx, PP);
   else if (const auto *TN = dyn_cast<TypedefNameDecl>(D))
     HI.Type = printType(TN->getUnderlyingType().getDesugaredType(Ctx), Ctx, PP);
   else if (const auto *TAT = dyn_cast<TypeAliasTemplateDecl>(D))
     HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast<VarDecl>(D)) {
+  if (const auto *Var = dyn_cast<VarDecl>(D); Var && !Var->isInvalidDecl()) {
     if (const Expr *Init = Var->getInit())
       HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(D)) {
     // Dependent enums (e.g. nested in template classes) don't have values yet.
     if (!ECD->getType()->isDependentType())
       HI.Value = toString(ECD->getInitVal(), 10);
   }
 
   HI.Definition = printDefinition(D, PP, TB);

clang-tools-extra/clangd/unittests/HoverTests.cpp

 #include "clang/AST/Attr.h"
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include <functional>
+#include <optional>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using PassMode = HoverInfo::PassType::PassMode;
TEST(Hover, ForwardStructNoCrash) {
 
   TestTU TU = TestTU::withCode(T.code());
   auto AST = TU.build();
   auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
   ASSERT_TRUE(HI);
   EXPECT_EQ(*HI->Value, "&bar");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+    const char *const Code;
+    const std::optional<std::string> HoverValue;
+  } Cases[] = {
+      {R"cpp(
+        // error-ok testing behavior on invalid decl
+        class Foo {};
+        void foo(Foo p^aram = nullptr);
+        )cpp",
+       std::nullopt},
+      {R"cpp(
+        class Foo {};
+        void foo(Foo *p^aram = nullptr);
+        )cpp",
+       "nullptr"},
+  };
+
+  for (const auto &C : Cases) {
+    Annotations T(C.Code);
+    TestTU TU = TestTU::withCode(T.code());
+    auto AST = TU.build();
+    auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+    ASSERT_TRUE(HI);
+    ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
     using m_int = int;
     m_int ^[[a]];
   )cpp");
 
   Config Cfg;
   Cfg.Hover.ShowAKA = false;