Warn by default when encountering a statement which only consists of an equality comparison

[crumblingstatue]

Rendered

[steveklabnik]

/cc @rust-lang/compiler @rust-lang/lang

[aturon]

Good idea, 👍 from me.

[petrochenkov]

This is a special case of "#[must_use] on functions" - #886.
I'd rather resurrect that RFC rather than add a special purpose lint for one case.

[crumblingstatue]

This is a special case of "#[must_use] on functions" - #886.
I'd rather resurrect that RFC rather than add a special purpose lint for one case.

So with that method, PartialEq::eq would be marked must_use? I would personally be fine with that solution. This should serve as additional motivation for that RFC.

This lint could be added in the meantime, and perhaps removed when the must_use solution is implemented. Although I admit that would be wasted effort.

[withoutboats]

@crumblingstatue unfortunately once added a lint cannot be removed, because that would break code that has (for example) #![deny(eq_statement)].

[crumblingstatue]

@crumblingstatue unfortunately once added a lint cannot be removed, because that would break code that has (for example) #![deny(eq_statement)].

Oh, I see. In that case, #886 should be carefully reconsidered before merging this RFC. I'll add it to the alternatives.

[oli-obk]

isn't the solution to simply move the lint from clippy to rustc? Together with the unnecessary_operation lint for maximal effect

[crumblingstatue]

The no_effect lint in clippy has a larger scope than this one, making it more prone to false positives, and less likely to be accepted into rustc. But I'll add merging this lint into rustc as a possible alternative.

Since unnecessary_operation is not required to solve the problem described in the motivation, it should probably belong in a separate RFC.

[oli-obk]

an intermediate solution is to name the rustc lint no_effect, only implement your suggested a == b; check, and thus ensure that the lint's name will never become obsolete, since it can be extended to more complex situations.

[sanxiyn]

What are false positives of no_effect lint in Clippy? As an initial author of that lint, I'd like to learn them and fix them. Unlike some other lints in Clippy, the lint is intended to be completely free of false positives.

(At the moment, there is no open issues for no_effect lint in Clippy repository.)

[crumblingstatue]

@sanxiyn
I'm not aware of any false positives either. What I'm stating is that since it's larger in scope than the lint proposed here, it is more likely to potentially have false positives, so it needs to be carefully investigated before merging into rustc.

[Manishearth]

IIRC no_effect is a conservative lint -- If anything, it has false negatives, but not false positives.

[joshtriplett]

I really like the #[must_use] approach as well. I feel like #886 got buried under a side issue of "which functions should have #[must_use] annotations", which prevented the underlying change of supporting #[must_use] on functions from going through.

Annotating PartialEq::{eq, ne} and Ord::{lt,le,gt,ge} with #[must_use] would handle this nicely, rather than adding a compiler special-case for the equality operator.

[aturon]

I'm going to propose we close this RFC in favor of reviving #886, as multiple people have proposed.

@rfcbot fcp close

[rfcbot]

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

• @Aatch
• @arielb1
• @aturon
• @bkoropoff
• @dotdash
• @eddyb
• @michaelwoerister
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, 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.

[crumblingstatue]

I would advise against closing this until an RFC proposing an alternative solution is actually merged.

This RFC has a clearly stated motivation, and also summarizes the alternatives, so it's a good fallback point if #886 isn't actually merged.

[withoutboats]

I'm in favor of 886 over this RFC. The previous RFC was closed for lack of strong motivation, and this RFC has found that motivation. A general mechanism seems better than a one-off lint.

@crumblingstatue If the conversation on 886 turns against we can re-open this RFC and consider it in light of that.

[sanxiyn]

@withoutboats Not everyone can reopen issues. If you promise to reopen when the situation arises, sure.

[withoutboats]

@crumblingstatue @sanxiyn Worth considering the significance of an open PR & who can open and close PRs.

From a purely technical standpoint, anyone can open a new PR with an identical diff; there are no real controls on what outstanding PRs exist against this repo.

But in terms of what becomes implemented in rustc, the reality is that decisions are not made on the basis of the existence of an open PR, but through the social process that happens on these threads. Open PRs & merged RFCs are just a mechanism for organizing and tracking that discussion.

The only reason this PR wouldn't be re-opened is if the outcome of the discussion on #886 is that in addition to not wanting #[must_use] on functions, we determine we don't want or cannot support a lint on PartialEq::partial_eq. Such a decision would be reached through discussion on that PR, with an FCP proposal & subsequent FCP period for people to voice disagreement with the decision.

So Aaron's proposing that we would be more likely to merge #886 than #1812, and since they are for the same purpose we should move discussion over there. Keeping this RFC open would just make it more likely for discussion to bifurcate into two threads, making it harder to reach a shared position (in that sense, my re-opening of #886 today may have been premature).

[aturon]

Thanks @withoutboats, that's basically how I see things as well. I don't think re-opening #886 is premature, though we will need someone to take over the RFC.

It's also not uncommon to have multiple RFCs open on the same topic; usually we try to go into FCP at the same time on such RFCs. I think we should try to move quickly on #886.

[aturon]

Given that the entirety of the lang team has signed off on moving to FCP for closure (and lints are generally lang team purview), I'm going to manually move to FCP.

This RFC is entering its final comment period, with disposition to close, in favor of pursuing #886.

[withoutboats]

Closing this.