[PATCH] D85716: [AST][RecoveryExpr] Fix a bogus unused diagnostic when the type is preserved.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:353
       return;
+    if (isa<RecoveryExpr>(E))
+      return;
----------------
This seems like a very specific patch to a special case of a potentially more general problem...


================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:88
+void func() {
+  // verify that no -Wunused-variable diagnostic on the inner Derived expression.
+  (Derived(Derived())); // expected-error {{call to implicitly-deleted default constructor}}
----------------
(nit: I think this is -Wunused but not -Wunused-variable)


================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:88
+void func() {
+  // verify that no -Wunused-variable diagnostic on the inner Derived expression.
+  (Derived(Derived())); // expected-error {{call to implicitly-deleted default constructor}}
----------------
sammccall wrote:
> (nit: I think this is -Wunused but not -Wunused-variable)
what makes you think this is firing on the *inner* expression? The diagnostic location is that of the outer expression. I think the existing logic is pretty close to correct (and the diagnostic is actually correct here, albeit through luck), and if we want to change it, the right fix belongs somewhere else.

The warning doesn't fire for just `Derived();` - isUnusedResultAWarning returns false for RecoveryExpr. Nor does it fire for `(Derived());` - isUnusedResultAWarning looks inside the paren expr and gets the same result for the contents.

For the case in this test, we hit the CXXFunctionalCastExpr/CStyleExprCast case of isUnusedResultAWarning. It says:

> If this is a cast to a constructor conversion, check the operand.
> Otherwise, the result of the cast is unused.

But this is a bit misleading: given `Foo(2+2)` the "operand" isn't 2+2, but rather a CXXConstructExpr.  And isUnusedResultAWarning returns true for this case (usually).

Back to the case at hand: we diverge because the cast type is Dependent instead of ConstructorConversion.
This seems like it could be a bug - in this configuration the operand is not type-dependent so why can't it resolve the overload?
But in any case, I don't think isUnusedResultAWarning should be assuming a dependent cast is safe to warn on - rather either return false or delegate to the inner expr (which in this case is a RecoveryExpr).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85716