unused_results is stricter on Nightly and Beta than on Stable

[briansmith]

This compiles fine on stable, but fails on nightly and beta:

https://play.rust-lang.org/?gist=9504fbb631c253be8e43bcb872524e0e&version=nightly

#![deny(unused_results)]

fn main() {
   let mut p = std::path::PathBuf::from("asdf");
   p.set_extension("foo");
}

Nightly results:

  Compiling playground v0.0.1 (file:///playground)
error: unused result
 --> src/main.rs:5:4
  |
5 |    p.set_extension("foo");
  |    ^^^^^^^^^^^^^^^^^^^^^^^
  |
note: lint level defined here
 --> src/main.rs:1:9
  |
1 | #![deny(unused_results)]
  |         ^^^^^^^^^^^^^^

error: aborting due to previous error

Regardless of whether the change in behavior is an intentional backward compatibility break, it should be documented in the release notes.

See also #43806.

[zackmdavis]

This is specific to booleans. Not intentional. (I could see a case for the new behavior, but that doesn't matter; if we're going to change this, it should be on purpose.) Fix forthcoming (if it doesn't make the 1.21 beta branch cut, it would need to be backported). Very sorry for regression. 😞 😰 ☠️

[eddyb]

FWIW lint changes are not considered backwards-compatibility breaking.
cc @rust-lang/lang

[whitequark]

@eddyb Then we should mention that Rust's (or perhaps rustc's?) backwards compatibility guarantee only holds with --cap-lints warn. It's the default in Cargo for non-root crates, but root crates could still result in breakage on rustc updates if e.g. compiled by scripts.

[eddyb]

@whitequark Yeah, --cap-lints is a relatively new trick and the stability document (I don't even know where to look for it - I think it was an RFC) should be updated if it hasn't been already.

[briansmith]

@alexcrichton This isn't just a regression from stable to nightly, it is a regression from stable to beta.

FWIW lint changes are not considered backwards-compatibility breaking.

OTOH, this is probably the most common lint for which #[deny(...)] and #[forbid(...)] are used.

[eddyb]

@briansmith As @whitequark mentioned above, lints can't break dependencies from compiling.

[arielb1]

1.21 is now beta - this needs to be backported.

[est31]

I think this issue is treated wrongly by reverting behaviour (#44122). Rustc needs the ability to add and remove lints in any way wanted, otherwise there won't be any improvement of lints in the future.

E.g. I've seen the "unused_mut" lint become more powerful, it detected some non required mut annotations. Why shouldn't it be possible to improve it? Stability? If you say #![deny(lint)] you give up that stability guarantee, except if its some very obvious thing like unsafe.

Of course though, @zackmdavis is right, a lint change should be done knowingly and on purpose.

[nikomatsakis]

Wait, I am a bit confused. The unused_results lint is something that warns whenever a function calls result is not used? And it doesn't warn about booleans? That surprises me a bit (in fact, I was not aware of this lint in general). But handling bool does feel more like a bug fix to the lint -- how did it come about?

[zackmdavis]

how did it come about?

The booleans-are-not-unused-results behavior dates back to the introduction of the (default allow) lint itself in the 0.10 timeframe. No explicit justification seems to be given, although one might imagine that APIs that do something and return a bool that the caller is likely to not care about (e.g., HashSet::insert) were considered common enough to merit special treatment? The regression (because it was unintended, I don't think it's prejudicial to call it a regression) was in #43728.

[withoutboats]

Can I get confirmation that I understand the situation? My impression is that #43728 was an incorrect implementation of rust-lang/rfcs#1940.

The unused_results lint is only triggered on types which have been tagged #[must_use]. The boolean primitive has never been considered a #[must_use] type; this isn't because boolean was missed by the lint or anything, but because we intentionally opt into the types we use with this lint, and bool has never been one of them.

What we agreed was to allow you to tag specific functions with #[must_use], and to tag PartialEq::eq in that way. Until we decide otherwise, only PartialEq::eq should raise this lint, not any other function that returns bool.

Unless I've misunderstood the situation, it seems to me that we need to backport the revert because we have not decided to consider bool a must_use type.

[briansmith]

@withoutboats That's my understanding; i.e. the fix for this should be in 1.20 that is being released this week.

[withoutboats]

@briansmith Agreed. I just want to make the distinction that we can add new lints (in particular, we agreed to add this lint to ==), we just haven't decided to add the lint that's been added by mistake.

[zackmdavis]

@withoutboats

Can I get confirmation that I understand the situation?

Almost (skip or skim details below if uninterested)—

My impression is that #43728 was an incorrect implementation of rust-lang/rfcs#1940.

Yes.

The unused_results lint is only triggered on types which have been tagged #[must_use].

Not quite. There are two relevant lints here: unused_must_use (which defaults to warn), and unused_results (which defaults to allow, which is why no one has heard of it), which both get set during the same lint pass. The first, unused_must_use, triggers for for unused types—and now, with the fn_must_use feature flag, return values of functions annotated with #[must_use]. Whereas the historical behavior of unused_results has been to trigger for all unused values that didn't already trigger unused_must_use, except for values of the types !, (), or bool. (I'm not aware of this detailed behavior of unused_results being documented anywhere.) #43728 unintentionally stopped exempting !, (), and bool from unused_results, but the old behavior has been or is being restored in #43813 (landed, for ! and ()) and #44122 (r+'d, and the beta-backport at issue, for bool) because no one had intended to modify the relatively obscure unused_results lint in the first place.

and to tag PartialEq::eq in that way. Until we decide otherwise, only PartialEq::eq should raise this lint

The libs team has also approved must_use for the other comparison methods (implemented in f5ac228). This is less consequential than you might think, because unfortunately—and contrary to the assumptions of many of the RFC discussants—making the comparison methods must_use doesn't automatically trigger lints for the much more common comparison operators (unless anyone has any alternative implementation ideas that would do so??). #44103 is currently open to also lint the operators (under the same fn_must_use feature flag).

[withoutboats]

@zackmdavis Oh wow! I was unaware of this default-allow lint, that explains how no one found this.

Its unfortunate this was found so close to the release; I'd like to consider whether its actually worth backporting a revert but we only have a day or two to decide what to do.

[briansmith]

@zackmdavis Oh wow! I was unaware of this default-allow lint, that explains how no one found this.

Its unfortunate this was found so close to the release; I'd like to consider whether its actually worth backporting a revert but we only have a day or two to decide what to do.

I was confusing unused_results with unused_must_use above, especially when I said that it is one of the lints that #![deny]/#![forbid] are most frequently used for. For unused_results, I actually do think it is reasonable to warn on unused bool results and I wouldn't mind if it were decided that the new behavior is correct. I already updated my code to build on 1.20 as it exists today anyway.

[eddyb]

Wait now I'm curious, what is unused_results, if not unused_must_use? Is it allow by default by any chance?

[sfackler]

unused_results warns on every non unit return type like gcc's -Wunused-result.

[withoutboats]

@eddyb It appears that unused_results is a lint that treats every type except (), !, and bool as must_use. I don't have any idea why bool has been excepted from this.

[withoutboats]

@rfcbot fcp close

I want to propose we treat this as a bugfix in the unused_result lint (hence closing this issue). We kind of have to decide within the next 24 hours, but I'll try to bring it to everyone's attention tomorrow.

[zackmdavis]

I don't have any idea why bool has be excepted from [unused_results].

@alexcrichton might remember?

[rfcbot]

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

• @aturon
• @eddyb
• @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.

[eddyb]

We kind of have to decide within the next 24 hours

I thought @alexcrichton was talking about beta being already branched off somewhere else?

[nikomatsakis]

@zackmdavis

First, thanks for the detailed summary.

With respect to the question at hand, I personally consider exempting bool from the lint to be a bug -- I feel like the whole point of the lint is to be pedantic, and bool is one of the exact types where I would most want this lint, precisely because it's the kind of type that people sometimes return as an error indicator but which also has many other uses (similar to Option but unlike, say, Result). That said, I personally never use the lint in practice, and I certainly see the value in preserving existing behavior absent an affirmative decision to change it.

Anyway, on a different topic:

doesn't automatically trigger lints for the much more common comparison operators (unless anyone has any alternative implementation ideas that would do so??

I can probably help with this. =) Where should I comment? (Tracking issue?)

[alexcrichton]

I don't have any idea why bool has be excepted from [unused_results].

@alexcrichton might remember?

Nothing like trying to remember a commit from three years ago! Nah unfortunately though I don't remember why bool was excepted. Your explanation above though, sounds like the most plausible reason!

---

We kind of have to decide within the next 24 hours

I thought @alexcrichton was talking about beta being already branched off somewhere else?

Yes beta branched a few days ago so this, but with the current disposition to close I don't think we're going to backport/stop anything at this point.

[zackmdavis]

@nikomatsakis

doesn't automatically trigger lints for the much more common comparison operators (unless anyone has any alternative implementation ideas that would do so??

I can probably help with this. =) Where should I comment? (Tracking issue?)

Tracking issue is #43302, although I would argue that the currently-unreviewed #44103 (having the lint look at the operators) adequately solves the problem even if it's not the "automatic" implementation folks originally imagined.

[rfcbot]

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

[rfcbot]

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

[rfcbot]

The final comment period is now complete.

[Mark-Simulacrum]

Closing, seems we've sufficiently figured this out and no objections were raised.