Properly diagnose [[nodiscard]] on the body of a range-based for loop

[aaron.ballman]

Summary

If the range-based for loop function body is not a compound statement, we would fail to diagnose discarded results from the statement. This only impacted range-based for loops because other kinds of for loops already manually check the body for unused expression results.

This addresses PR39837.

[rsmith]

lib/Sema/SemaStmt.cpp

2846

While this looks correct per the current approach to this function, we really shouldn't be duplicating calls to this everywhere. Can we move all the calls to a single call in ActOnFinishFullStmt?

[rsmith]

lib/Sema/SemaStmt.cpp

2846

Looks like that's not actually called from almost anywhere, but checking from ActOnFinishFullExpr in the case where DiscardedValue is true seems appropriate.

[aaron.ballman]

lib/Sema/SemaStmt.cpp

2846

That seems sensible, but how will that work with GNU statement expressions? If we check for unused expression results, then we will trigger on code like this:

int a = ({blah(); yada(); 0;});
// or
int b = ({blah(); yada(); some_no_discard_call();});

I don't know of a way to handle that case -- we call ActOnFinishFullExpr() while parsing the compound statement for the StmtExpr, so there's no way to know whether there's another statement coming (without lookahead) to determine whether to suppress the diagnostic for the last expression statement. Do you have ideas on how to handle that?

[rsmith]

lib/Sema/SemaStmt.cpp

2846

If we're calling ActOnFinishFullExpr there with DiscardedValue == true, that would be a bug that we should be fixing regardless. I don't think the lookahead is so bad itself -- it should just be a one-token lookahead for a } after the ; -- but it might be awkward to wire it into our compound-statement / expression-statement parsing. Still, it seems necessary for correctness.

[aaron.ballman]

Updated based on review feedback.

The lookahead wasn't too annoying to thread through, but I did run into surprises in TreeTransform where I can't use the lookahead to determine whether the expression statement should be warned on or not. The solution I have is serviceable, but perhaps there's a better approach to be taken.

The changes introduce warnings where they were previously missed, such as in init-statements. The only unfortunate behavioral side effect comes from range-based for loops that have an expression as the first statement -- in addition to the error, we now trigger an unused expression result warning, but restructuring the code to avoid it would be challenging for such an edge case (and the warning behavior seems reasonable).

[rsmith]

include/clang/Sema/Sema.h

5337 ↗

(On Diff #179475)

Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a discarded-value expression?

lib/Parse/ParseStmt.cpp

23 ↗

(On Diff #179475)

The parser shouldn't be looking inside Sema's data structures. Can you add a method to the Sema interface for the query you make below instead?

lib/Sema/SemaExprCXX.cpp

7785 ↗

(On Diff #179475)

It doesn't make sense for a WarnOn flag to control how we build the AST like this.

lib/Sema/TreeTransform.h

3297 ↗

(On Diff #179475)

What happens if the last statement in an expression statement contains a lambda? Will we consider all expression statements in it as not being discarded-value expressions? Alternate suggestion below.

6529 ↗

(On Diff #179475)

Instead of this counter mechanism (which doesn't seem reliable), can you just pass a flag to TransformStmt?

[aaron.ballman]

include/clang/Sema/Sema.h

5337 ↗

(On Diff #179475)

It probably can; but then it feels like the logic is backwards from the suggested changes as I understood them. If it's a discarded value expression, then the value being unused should *not* be diagnosed because the expression only exists for its side effects (not its value computations), correct?

lib/Parse/ParseStmt.cpp

23 ↗

(On Diff #179475)

Can do.

lib/Sema/TreeTransform.h

6529 ↗

(On Diff #179475)

I'll give it a shot, it seems plausible.

[• Quuxplusone]

include/clang/Sema/Sema.h

5337 ↗

(On Diff #179475)

Peanut gallery says: There are at least three things that need to be computed somewhere: (1) Is this expression's value discarded? (2) Is this expression the result of a [[nodiscard]] function? (3) Is the diagnostic enabled? It is unclear to me who's responsible for computing which of these things. I.e., it is unclear to me whether WarnOnDiscardedValue=true means "Hey ActOnFinishFullExpr, please give a warning because this value is being discarded" (conjunction of 1,2, and maybe 3) or "Hey ActOnFinishFullExpr, please give a warning if this value is being discarded" (conjunction of 2 and maybe 3).

I also think it is needlessly confusing that ActOnFinishFullExpr gives WarnOnDiscardedValue a defaulted value of false but ActOnExprStmt gives WarnOnDiscardedValue a defaulted value of true. Defaulted values (especially of boolean type) are horrible, but context-dependent defaulted values are even worse.

[rsmith]

include/clang/Sema/Sema.h

5337 ↗

(On Diff #179475)

I don't think it makes sense for ActOnFinishFullExpr to have a default argument for DiscardedValue, because there's really no reason to assume one way or the other -- the values of some full-expressions are used, and the values of others are not. A default of false certainly seems wrong.

For ActOnExprStmt, the default argument makes sense to me: the expression in an expression-statement is by definition a discarded-value expression (http://eel.is/c++draft/stmt.stmt#stmt.expr-1.sentence-2) -- it's only the weird special case for a final expression-statement in an statement-expression that bucks the trend here.

If it's a discarded value expression, then the value being unused should *not* be diagnosed because the expression only exists for its side effects (not its value computations), correct?

No. If it's a discarded-value expression, that means the value of the full-expression is not being used, so it should be diagnosed. If it's not a discarded-value expression, then the value of the full-expression is used for something (eg, it's a condition or an array bound or a template argument) and so we should not warn. Indeed, the wording for [[nodiscard]] suggests to warn (only) on potentially-evaluated discarded-value expressions.

Discarded-value expressions are things like expression-statements, the left-hand-side of a comma operator, and the operands of casts to void. (Note in the cast-to-void case is explicitly called out by the [[nodiscard]] wording as a discarded-value expression that should not warn: http://eel.is/c++draft/dcl.attr.nodiscard#2.sentence-2)

[aaron.ballman]

Updated based on review feedback. The vast majority of the changes are from the removal of a default argument to ActOnFullExpr().

[aaron.ballman]

include/clang/Sema/Sema.h

5337 ↗

(On Diff #179475)

Thank you for the explanation @rsmith, my mental model was exactly backwards. :-D I was thrown off by http://eel.is/c++draft/expr.prop#expr.context-2.sentence-1 because I read it as saying the discarded nature of the results are *desired* rather than problematic. e.g., some statements only appear for their side effects, do not warn about such statements because the context makes it obvious that this is expected. I should have looked it it in the context of the nodiscard attribute wording rather than in isolation.

[rsmith]

Thanks, this looks great. A couple of the changes to the tests look like the diagnostic output is slightly worse in some error recovery conditions, but generally this is a nice improvement.

test/Parser/switch-recovery.cpp

108 ↗

(On Diff #179920)

Hmm, why do we only get one warning here? I'd expect one warning for the 8; and one for the x; (after applying the fixes from the errors).

test/SemaCXX/for-range-examples.cpp

181 ↗

(On Diff #179920)

The new warnings here aren't ideal; do you know why they show up?

[aaron.ballman]

test/Parser/switch-recovery.cpp

108 ↗

(On Diff #179920)

We get the one for the 8; but the other one is a TypoExpr and it claims it's type dependent, and we don't warn on type dependent so we bail out pretty early in Expr::isUnusedResultAWarning().

test/SemaCXX/for-range-examples.cpp

181 ↗

(On Diff #179920)

Because the first part in a for loop can be an expression, but only if it's not a range-based for loop. However, I was able to do some lookahead to retain the old behavior here.

[aaron.ballman]

Committed with minor modifications for the range-based for loop changes in r350404.

lib/Sema/SemaStmt.cpp

   if (isa<ObjCForCollectionStmt>(S))
     return FinishObjCForCollectionStmt(S, B);
 
   CXXForRangeStmt *ForStmt = cast<CXXForRangeStmt>(S);
   ForStmt->setBody(B);
 
   DiagnoseEmptyStmtBody(ForStmt->getRParenLoc(), B,
                         diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 
   DiagnoseForRangeVariableCopies(*this, ForStmt);
 
   return S;
 }
 
 StmtResult Sema::ActOnGotoStmt(SourceLocation GotoLoc,
                                SourceLocation LabelLoc,

test/SemaCXX/warn-unused-result.cpp

   // Silencing the warning by cast to void still works.
   (void)s.DoThing();
   (void)s++;
   (void)p++;
   (void)++s;
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+    f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837