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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:663
+  const auto *Var = dyn_cast<VarDecl>(D);
+  if (Var && !llvm::isa<ParmVarDecl>(Var)) {
     if (const Expr *Init = Var->getInit())
----------------
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()) {
   ...
}
```


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());
----------------
VitaNuo wrote:
> hokein wrote:
> > 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?
> It crashes on invalid code. I've inspected two user workspaces:
> 1.
> 
> ```
> class Foo{};
> void foo(Foo param = nullptr);
> ```
> 
> 2.
> ```
> 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.
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 */`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015