Disable unused_must_use for statically known bools by HKalbasi · Pull Request #8...

Copy link

Contributor

ecstatic-morse commented on Aug 14 •

The title is a bit misleading to me, since this is really about disabling unused_must_use when the right-hand side of a short-circuiting operator has side-effects (in this case diverges). Accordingly, the LogicPredicts stuff seems unnecessary: we only care about the right-hand side. There are mentoring instructions in this comment, although the one right after suggests removing unused_must_use entirely for short-circuiting logic, since divergence is just one possible side-effect. FWIW, I don't agree that disabling it entirely is the right course of action.

HKalbasi commented on Aug 14 •

There are slightly different mindsets here, but with the same goal. I don't think it is about side effect, for example:

is_missiles_ready() && fire_missiles();

In this example, fire_missiles() can have side effect but result of this operation is in interest, so I see unused_must_use here valid. What is special with diverging case is in this case, output can be predicted statically. That is:

let result = (is_missiles_ready() && fire_missiles()) || panic!("mission failed");

Here, result is always true regardless of what the code it is. So, there isn't any point in unused_must_use here.

With this mindset, there are examples that rhs leaf is divergent but result is non trivial, and in this cases, this PR still keeps warning:

let result = !is_missiles_ready() || (fire_missiles() && panic!("booom"));

Here, if !is_missiles_ready() is true then result is true, else if fire_missiles() is false then result is false. So result is interesting and unused_must_use isn't meaningless. This in this PR tests and warned.

With side effect mindset, we should disable unused_must_use for every bool expression, and this is a more radical and debate able move. I think this PR is a good moderate solution that covers common cases.

ecstatic-morse commented on Aug 14 •

Interesting. I think I understand your point-of-view a bit better, but I still think it's misguided. Specifically:

let result = !is_missiles_ready() || (fire_missiles() && panic!("booom"));

Here, if !is_missiles_ready() is true then result is true, else if fire_missiles() is false then result is false. So result is interesting and unused_must_use isn't meaningless. This in this PR tests and warned.

Compare this to checking the RHS for divergent expressions, which would (I believe) suppress warnings in a strictly more cases than this PR. Why are we adding logic to continue emitting a warning in such a niche case? The lesson from #69466 is that people want to use short-circuiting operators in lieu of control-flow, does that not apply here?

HKalbasi commented on Aug 14

I wanted to use this as a justification, because I felt it was controversial to suppress warning. If it is clear that side-effect-full expression should not be catch by unused_must_use (which is clear to me and I'm personally in favor of this), we can simply turn it off for && and || in all cases. is_missiles_ready() && fire_missiles(); is valid for control flow proposes.

Expressions with diverging rhs are strictly bigger set, but how you describe this set? Clearly side-effect-full expressions? (simple side-effect-full expressions doesn't cover fire_missiles case) In practice I think people use chains with single operator, or at most something like do_sth1() && do_sth2() || fallback() and in these cases both behave the same. Benefit of mine over rhs is it has a clear explanation and propose. If it isn't a concern (which seems it isn't) and/or rhs has benefits, I can replace it with rhs and I see no problem.

ecstatic-morse commented on Aug 14 •

Let's wait for @estebank to weigh in here.

To summarize, I want the rule to be:

must_use applies to the result of short-circuiting logic operators unless the expression on the right-hand side diverges.

While @HKalbasi wants to add an additional condition to the exception (let me know if this is wrong or if there is a different way to phrase this):

must_use applies to the result of short-circuiting logic operators unless the expression on the right-hand side diverges and the non-divergent path can only result in a single value (true or false).

OP's modification is relevant in cases like the following, where it will continue to emit a warning:

!is_missiles_ready() || (fire_missiles() && return);

Neither of these handles all possible side-effects, just unconditional divergence (e.g. panic, return, break). There's no way to do this in the general case. If we determine that false positives are unacceptable (neither me nor OP thinks so, but eddyb disagrees), we would disable the lint entirely.

estebank commented 29 days ago

I feel like @rust-lang/lang should chime in here, because it makes sense to me (and the code looks reasonable), but I feel we need to have them involved in accepting such a change. I can rationalize approving on my own by saying that this is "just a change of a warning" so we can move ahead with it, but would prefer to not be the only one making such a decision.

joshtriplett commented 16 days ago

We discussed this in today's @rust-lang/lang meeting.

Some people did agree that this seems like a kind of false positive, insofar as you are "using" the value through the combination of a short-circuiting operator and a diverging RHS.

However, all of the examples seem like code we're not inclined to encourage, and all of us felt like the compiler ought to produce some sort of warning here. In particular, all of this code seems more clearly written using if. For instance, instead of expr || panic!("..."), we'd rather see if !expr { panic!("..."); }.

We'd be happy to see a structured suggestion in that regard, steering people towards if and if !.

@rfcbot close

rfcbot commented 16 days ago •

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

HKalbasi commented 16 days ago

However, all of the examples seem like code we're not inclined to encourage, and all of us felt like the compiler ought to produce some sort of warning here.

@joshtriplett I think unused warning isn't point to what is wrong with this, and suggest let _ = expr || panic!("...") which is worse if being used for silencing the warning. There is a clippy lint for this, which suggest the correct if !expr { panic!("..."); }. I think the correct step in this direction is upgrading that lint to a compiler lint, and removing short circuit case from unused lint because it isn't really about using the result of an always true expression. It has better warning, better suggestion and clearly shows why compiler isn't happy with this anti-pattern.

scottmcm commented 15 days ago •

I definitely like the idea of separating it into two lints. I could easily imagine people wanting to allow x == 0 || continue; but still to not allow foo() || bar() where the RHS is bool. (And if that new lint for ! RHSs gets a nice structured suggestion, that's even better.)

I wonder if rustfmt is also partially to blame here. If the if ends up always being three lines (as is pretty common in a bunch of coding standards under which I've worked) then I can see extra motivation for just wanting to write things in the simpler operator form to get one line...

rfcbot commented 2 days ago

This is now entering its final comment period, as per the review above.