Error recovery when Sema finds only one (non-viable) candidate?

Clang Frontend
#1

Consider the following broken code:

int foo(int);
void bar() { return foo(); }

Currently it produces two diagnostics:

  • no matching call for foo
  • note: foo(int) not viable, wrong number of args
    And it produces no AST for the call (bar’s CompoundStmt is empty).

I’d really like it recover and produce a CallExpr to the one candidate, similar to what happens for typo-correction.
My own motivation is to get go-to-definition in libclang/clangd to work better with broken code, though I think there are other benefits.

The new behavior might be something like:

  • only call for foo is not viable
  • note: foo(int) not viable, wrong number of args
  • bar() shouldn’t return a value
    And a CallExpr emitted with no args (and maybe an ‘invalid’ flag).

Q1: does this seem desirable in principle?

#2

Consider the following broken code:

int foo(int);
void bar() { return foo(); }

Currently it produces two diagnostics:
- no matching call for foo
- note: foo(int) not viable, wrong number of args
And it produces no AST for the call (bar's CompoundStmt is empty).

I'd really like it recover and produce a CallExpr to the one candidate,
similar to what happens for typo-correction.
My own motivation is to get go-to-definition in libclang/clangd to work
better with broken code, though I think there are other benefits.

Another case is when you are typing a function call and need to lookup
the function documentation - hitting F1/Help is difficult to implement
without any hint/candidate of the not-yet-completed function call.

The new behavior might be something like:
- only call for foo is not viable
- note: foo(int) not viable, wrong number of args
- bar() shouldn't return a value
And a CallExpr emitted with no args (and maybe an 'invalid' flag).

Q1: does this seem desirable in principle?

From an IDE point of view, it definitely is!

There are some practical issues:
- A naive implementation (https://reviews.llvm.org/D61057) sometimes
emits pre-recovery "not viable: X" diagnostics and post-recovery "error:
X" for the same reason X. e.g. for cuda device->host calls.
- Post recovery diagnostics can be noisy in more subtle ways. Test
failures for naive implementation:
https://gist.github.com/sam-mccall/bb73940e27a79ee41523e52048872f26

Maybe it's best to start with only simple cases, e.g. recovering only
from too many/too few arguments doesn't break any tests, and covers a
lot of broken-code cases.

Sounds good.

Q2: does anyone have strong opinions about how this should work in
practice, see hidden pitfalls, and/or want to review code?

I'm wondering whether the introduction of the invalid flag could lead to
inconsistencies somewhere (maybe analyzers/checkers?). For example, with
the new behavior a CallExpr would exist that points to a particular
candidate, having e.g. N function parameters. But the actual function
has M != N arguments (let's pretend that default arguments do not exist
for the sake of the example). It might make sense to have this new
behavior opt-in.

Besides having an (invalid) CallExpr, it would be also very useful to
have the function arguments handled. For example, if a function call is
invalid due to whatever reason, but the arguments "hello" and "there"
still could be resolved/looked-up in the current scope, the invalid
CallExpr should have appropriate DeclRefExprs children for these arguments.

This would enable go-to-definition, help/f1 and highlighting for the
arguments of not yet fully finished function calls.

I've contributed only some simple patches so far, but I could help with
testing and reviewing where I can.

#3

We were discussing this idea further today.
There are some limitations:

  • we’d weakening current invariants, which could break existing code in rarely-tested cases. An invalid flag helps fix these problems once you find them, but not before then.
  • callexpr is only one place we have this type of problem, with whole subtrees of the AST being discarded. Breaking invariants on many types of nodes would add lots of complexity and make the AST harder to reason about.

An alternative would be to add some new type of node, that would go into the AST instead of CallExpr.
e.g. RecoveryExpr, with:

  • a source range
  • StmtClass of the node we were trying to create (CallExpr)
  • children (in this case, Exprs for the callee and parameters)
    This addresses the above concerns somewhat:
  • As a new node type, only code aware of the concept will interact with it directly. We only have to worry about code that handles Expr generically, and generic things like RecursiveASTVisitor (which can skip such nodes by default).
  • The node can likely be generic enough to handle cases other than CallExpr.(

I don’t have experience adding new node types, but will try to prototype this.