Page MenuHomePhabricator

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

Authored by aaron.ballman on Dec 20 2018, 1:54 PM.

Details

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.

Diff Detail

Event Timeline

aaron.ballman created this revision.Dec 20 2018, 1:54 PM
rsmith added inline comments.Dec 20 2018, 2:01 PM
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 added inline comments.Dec 20 2018, 2:03 PM
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 marked 3 inline comments as done.Dec 21 2018, 7:05 AM
aaron.ballman added inline comments.
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 added inline comments.Dec 21 2018, 2:53 PM
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 marked 3 inline comments as done.

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 added inline comments.Dec 24 2018, 9:57 AM
include/clang/Sema/Sema.h
5337

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

lib/Parse/ParseStmt.cpp
23

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

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

lib/Sema/TreeTransform.h
3297

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

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

aaron.ballman marked 3 inline comments as done.Dec 24 2018, 11:05 AM
aaron.ballman added inline comments.
include/clang/Sema/Sema.h
5337

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

Can do.

lib/Sema/TreeTransform.h
6529

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

Quuxplusone added inline comments.Dec 24 2018, 12:33 PM
include/clang/Sema/Sema.h
5337

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 added inline comments.Dec 26 2018, 12:36 PM
include/clang/Sema/Sema.h
5337

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 marked an inline comment as done.

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

aaron.ballman marked 2 inline comments as done.Jan 2 2019, 12:24 PM
aaron.ballman added inline comments.
include/clang/Sema/Sema.h
5337

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 accepted this revision.Jan 3 2019, 2:48 PM

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

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

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

This revision is now accepted and ready to land.Jan 3 2019, 2:48 PM
aaron.ballman marked 3 inline comments as done.Jan 4 2019, 8:07 AM
aaron.ballman added inline comments.
test/Parser/switch-recovery.cpp
108

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

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 closed this revision.Jan 4 2019, 9:02 AM

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