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

[hokein]

Summary

Because of typo-correction, the AST can be transformed, and the transformed
AST is marginally useful for diagnostics purpose, the following
diagnostics usually do harm than good (easily cause confusions).

Given the following code:

void abcc();
void test() {
  if (abc());
  // diagnostic 1 (for the typo-correction): the typo is correct to `abcc()`, so the code is treate as `if (abcc())` in AST perspective;
  // diagnostic 2 (for mismatch type): we perform an type-analysis on `if`, discover the type is not match
}

The secondary diagnostic "convertable to bool" is likely bogus to users.

The idea is to use RecoveryExpr (clang's dependent mechanism) to preserve the
recovery behavior but suppress all follow-up diagnostics.

[hokein]

clang/test/Modules/submodules-merge-defs.cpp

21 ↗

(On Diff #299916)

this (and the case below) are slight regressions, but I think the following diagnostics are not a big deal.

[sammccall]

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?

clang/lib/Sema/SemaOverload.cpp

12737–12738

while here, drop the stale "returns true" bit? The return type covers the contract well enough IMO

12794

nit: now member call -> member access as we never really go on to build a member call

12808–12809

comment is now out of date.

12808–12809

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)

12809

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"

12814

I think giving the variable a new name here would be clearer

clang/test/Modules/submodules-merge-defs.cpp

21 ↗

(On Diff #299916)

I agree, the secondary diagnostics aren't wrong but I don't think they're necessary either.

[hokein]

address comments and add AST tests.

clang/lib/Sema/SemaOverload.cpp

12808–12809

(Which I guess means emitting the first one here)

yes, exactly.

reasons:

• BuildCallExpr could emit diagnostics if we pass a non-dependent callee to it, this is something we want to avoid;
• we'd save some cost -- BuildCallExpr has dedicated code path to handle dependent expr;

[sammccall]

clang/lib/Sema/SemaOverload.cpp

12808–12809

Sorry, I meant emitting the second one here. (Or changing the callexpr recovery to use the first form, but that's a larger patch).

I understand we save some code, but not very much (the second form is easy to produce), and we produce quite different ASTs for essentially the same pattern (the only difference is whether we attempted recovery or not). This doesn't seem like a good trade...

[hokein]

address comment.

clang/lib/Sema/SemaOverload.cpp

12808–12809

Changed to the second form for consistency. And it doesn't break any existing tests (neither improvements nor regressions).

[sammccall]

Thanks! LG with comment nit

clang/lib/Sema/SemaOverload.cpp

12818

is it a deliberate decision to drop the return type of the recovery function here too? If so, mention it in the comment (currentyl you only talk about not preserving the real call node)

clang/test/AST/ast-dump-recovery.cpp

277

(so this could be void. It will still trigger some follow-on diagnostics though)

clang/test/SemaCXX/typo-correction-delayed.cpp

216

nit: convertible

[hokein]

address comments.

clang/lib/Sema/SemaOverload.cpp

12818

yes, we deliberately use a dependent-type for recovery-expr to suppress diagnostics (rely on clang's dependent mechanism).

clang/test/AST/ast-dump-recovery.cpp

277

As described in the previous comment, the type should always be dependent.

[sammccall]

Still LG thank you!

clang/lib/Sema/SemaOverload.cpp

public:
 
   ~BuildRecoveryCallExprRAII() {
     SemaRef.IsBuildingRecoveryCallExpr = false;
   }
 };
 
 }
 
 /// Attempts to recover from a call where no functions were found.
-///
-/// Returns true if new candidates were found.
 static ExprResult
 BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
                       UnresolvedLookupExpr *ULE,
                       SourceLocation LParenLoc,
                       MutableArrayRef<Expr *> Args,
                       SourceLocation RParenLoc,
                       bool EmptyLookup, bool AllowTypoCorrection) {
   // Do not try to recover if it is already building a recovery call.
   // This stops infinite loops for template instantiations like
BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   assert(!R.empty() && "lookup results empty despite recovery");
 
   // If recovery created an ambiguity, just bail out.
   if (R.isAmbiguous()) {
     R.suppressDiagnostics();
     return ExprError();
   }
 
-  // Build an implicit member call if appropriate.  Just drop the
+  // Build an implicit member access expression if appropriate. Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
     NewFn = SemaRef.BuildPossibleImplicitMemberExpr(SS, TemplateKWLoc, R,
                                                     ExplicitTemplateArgs, S);
   else if (ExplicitTemplateArgs || TemplateKWLoc.isValid())
     NewFn = SemaRef.BuildTemplateIdExpr(SS, TemplateKWLoc, R, false,
                                         ExplicitTemplateArgs);
   else
     NewFn = SemaRef.BuildDeclarationNameExpr(SS, R, false);
 
   if (NewFn.isInvalid())
     return ExprError();
 
-  // This shouldn't cause an infinite loop because we're giving it
-  // an expression with viable lookup results, which should never
-  // end up here.
-  return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
-                               MultiExprArg(Args.data(), Args.size()),
-                               RParenLoc);
+  auto CallE =
+      SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
+                            MultiExprArg(Args.data(), Args.size()), RParenLoc);
+  if (CallE.isInvalid())
+    return ExprError();
+  // 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 behavior but suppress all following diagnostics by
+  // using RecoveryExpr. We deliberately drop the return type of the recovery
+  // function, and rely on clang's dependent mechanism to suppress following
+  // diagnostics.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+                                    CallE.get()->getEndLoc(), {CallE.get()});
 }
 
 /// Constructs and populates an OverloadedCandidateSet from
 /// the given function.
 /// \returns true when an the ExprResult output parameter has been set.
 bool Sema::buildOverloadedCallSet(Scope *S, Expr *Fn,
                                   UnresolvedLookupExpr *ULE,
                                   MultiExprArg Args,

clang/test/AST/ast-dump-recovery.cpp

void InvalidCondition() {
   // FIXME: figure out why the type of ConditionalOperator is not int.
   // CHECK:      ConditionalOperator {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: | `-UnresolvedLookupExpr {{.*}}
   // CHECK-NEXT: |-IntegerLiteral {{.*}} 'int' 1
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // RecoveryExpr is always dependent-type in this case in order to suppress
+  // following diagnostics.
+  // CHECK:      RecoveryExpr {{.*}} '<dependent type>'
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT:     `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}

clang/test/SemaCXX/typo-correction-delayed.cpp

 // expected-error@-1 {{use of undeclared identifier 'c'}}
 }
 
 namespace PR23350 {
 int z = 1 ? N : ;  // expected-error {{expected expression}}
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$}}}}
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$}}}}
 // expected-error@+1 {{expected ';' after top level declarator}}
 unsigned int a = 0(uintmax_t