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

[sammccall]

clang/lib/Sema/SemaStmt.cpp

353 ↗

(On Diff #284621)

This seems like a very specific patch to a special case of a potentially more general problem...

clang/test/SemaCXX/recovery-expr-type.cpp

88

(nit: I think this is -Wunused but not -Wunused-variable)

88

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).

[hokein]

address review comment:
a dependent-kind cast expr should not be warned about the unused result.

clang/test/SemaCXX/recovery-expr-type.cpp

88

oh, my bad. I misunderstood this part of code -- I missed the critical function isUnusedResultAWarning, I saw it when reading this part of code, but I skipped it because the function name made me think "this is a trivial function that just detects whether the -Wunused flag is on".

I think you analysis is right here. Thanks!

what makes you think this is firing on the *inner* expression? The diagnostic location is that of the outer expression.

hmm, I was confused by the ~annotation of the diagnostic.

(Derived(Derived()));
  ^      ~~~~~~~

[sammccall]

clang/test/SemaCXX/recovery-expr-type.cpp

80

nit: can the testcase be simplified?
Isn't struct T { T() = delete; }; enough (with no inheritance)

[hokein]

simplify the testcase.

clang/lib/AST/Expr.cpp

if (CE->getCastKind() == CK_ToVoid) {
       }
       return false;
     }
 
     // If this is a cast to a constructor conversion, check the operand.
     // Otherwise, the result of the cast is unused.
     if (CE->getCastKind() == CK_ConstructorConversion)
       return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    if (CE->getCastKind() == CK_Dependent)
+      return false;
 
     WarnE = this;
     if (const CXXFunctionalCastExpr *CXXCE =
             dyn_cast<CXXFunctionalCastExpr>(this)) {
       Loc = CXXCE->getBeginLoc();
       R1 = CXXCE->getSubExpr()->getSourceRange();
     } else {
       const CStyleCastExpr *CStyleCE = cast<CStyleCastExpr>(this);

clang/test/SemaCXX/recovery-expr-type.cpp

 }
 
 // verify that "type 'double' cannot bind to a value of unrelated type 'int'" diagnostic is suppressed.
 namespace test5 {
   template<typename T> using U = T; // expected-note {{template parameter is declared here}}
   template<typename...Ts> U<Ts...>& f(); // expected-error {{pack expansion used as argument for non-pack parameter of alias template}}
   double &s1 = f(); // expected-error {{no matching function}}
 }
+
+namespace test6 {
+struct T {
+  T() = delete; // expected-note {{has been explicitly marked deleted here}}
+};
+
+void func() {
+  // verify that no -Wunused-value diagnostic.
+  (T(T())); // expected-error {{call to deleted constructor}}
+}
+}