[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Cool, this makes sense to me. I hadn't realized this doesn't go through typoexpr, that's a bit unfortunate we can't address this just in one place.

This is a change of behavior with recoveryAST off, but this change basically only affects C++ so that's not a widely-used configuration.

I think the resulting AST is better as the errors there are visible.
Can we have a test specifically of the shape of the AST produced?



================
Comment at: clang/lib/Sema/SemaOverload.cpp:12729
 ///
 /// Returns true if new candidates were found.
 static ExprResult
----------------
while here, drop the stale "returns true" bit? The return type covers the contract well enough IMO


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12786
 
   // Build an implicit member call if appropriate.  Just drop the
   // casts and such from the call, we don't really care.
----------------
nit: now member call -> member access as we never really go on to build a member call


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12801
 
+  // Offering "follow-up" diagnostics on these recovery call expressions seems
+  // like a bad risk/reward tradeoff, e.g. following diagnostics on a
----------------
This seems a bit abrupt (because it's not obvious what the previous code is doing I guess).

Maybe

"We now have recovered a callee. However, building a real call may lead to incorrect
secondary diagnostics if our recovery wasn't correct.
We keep the recovery diagnostics and AST but suppress following diagnostics by
using RecoveryExpr"


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12806
+  // using RecoveryExpr.
+  NewFn = SemaRef.CreateRecoveryExpr(NewFn.get()->getBeginLoc(),
+                                     NewFn.get()->getEndLoc(), {NewFn.get()});
----------------
I think giving the variable a new name here would be clearer


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12811
+
   // This shouldn't cause an infinite loop because we're giving it
   // an expression with viable lookup results, which should never
----------------
comment is now out of date.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12814
   // end up here.
   return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
                                MultiExprArg(Args.data(), Args.size()),
----------------
This results in (IIUC):

```
CallExpr
>RecoveryExpr (indirection we inserted)
>>DeclRefExpr (callee)
>Arg1
```

Whereas when overload resolution fails, we create:
```
RecoveryExpr (call)
> OverloadExpr (callee)
> Arg1
```

I can see advantages for either, but is there a reason not to be consistent?
(Which I guess means emitting the first one here)


================
Comment at: clang/test/Modules/submodules-merge-defs.cpp:21
 extern class A pre_a2;
-int pre_use_a = use_a(pre_a2); // expected-error 2{{'A' must be defined}} expected-error {{'use_a' must be declared}}
+int pre_use_a = use_a(pre_a2); // expected-error {{'use_a' must be declared}}
 // expected-note at defs.h:2 +{{here}}
----------------
hokein wrote:
> this (and the case below) are slight regressions, but I think the following diagnostics are not a big deal.
I agree, the secondary diagnostics aren't wrong but I don't think they're necessary either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946