#[must_use] for functions #43728

zackmdavis · 2017-08-08T04:57:00Z

This implements RFC 1940.

The RFC and discussion thereof seem to suggest that tagging PartialEq::eq and friends as #[must_use] would automatically lint for unused comparisons, but it doesn't work out that way (at least the way I've implemented it): unused .eq method calls get linted, but not == expressions. (The lint operates on the HIR, which sees binary operations as their own thing, even if they ultimately just call .eq &c..)

What do you think??

Resolves #43302.

rust-highfive · 2017-08-08T04:57:12Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

zackmdavis · 2017-08-08T05:28:36Z

Hm, maybe the feature gate is too conservative (the RFC and discussion don't seem to lean that way)?

zackmdavis · 2017-08-08T05:44:49Z

(force-pushed version without feature gate)

arielb1 · 2017-08-08T11:05:36Z

r? @eddyb

eddyb · 2017-08-08T11:11:10Z

@bors r+

bors · 2017-08-08T11:11:11Z

📌 Commit 88b686e has been approved by eddyb

The return value of a function annotated with `must_use`, must be used. This is in the matter of rust-lang#43302.

Note that this doesn't actually give us warnings on `a == b;` and the like, as some observers may have hoped. This is in the matter of rust-lang#43302.

zackmdavis · 2017-08-08T18:36:34Z

Wait, sorry, I needed to fix the commit message of what is now 3645b06 (it still mentioned the feature gate). (The eternal shame of a factually untrue statement blotting the Git history forever is not something I want on my conscience.)

@eddyb reapprove?

eddyb · 2017-08-08T18:39:56Z

@bors r+

bors · 2017-08-08T18:39:57Z

📌 Commit f5ac228 has been approved by eddyb

scottmcm · 2017-08-08T21:53:50Z

src/libcore/cmp.rs

@@ -625,6 +627,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
    /// let result = std::f64::NAN.partial_cmp(&1.0);
    /// assert_eq!(result, None);
    /// ```
+    #[must_use]


This method is the only newly-annotated one that's not an operator. Intentional?

(I'm by no means against it, and see no situation where it being unused would be good, but I recall the RFC intentionally leaving it off most places for now.)

Intentional; it felt like "part of the set", even if it's not really an operator.

bors · 2017-08-09T04:03:55Z

⌛ Testing commit f5ac228 with merge 3f977ba...

#[must_use] for functions This implements [RFC 1940](rust-lang/rfcs#1940). The RFC and discussion thereof seem to suggest that tagging `PartialEq::eq` and friends as `#[must_use]` would automatically lint for unused comparisons, but it doesn't work out that way (at least the way I've implemented it): unused `.eq` method calls get linted, but not `==` expressions. (The lint operates on the HIR, which sees binary operations as their own thing, even if they ultimately just call `.eq` _&c._.) What do _you_ think?? Resolves #43302.

bors · 2017-08-09T06:32:21Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 3f977ba to master...

crumblingstatue · 2017-08-09T06:51:07Z

unused .eq method calls get linted, but not == expressions

I just want to point out that the main motivation for reopening this RFC was exactly to lint against unused ==.

alexcrichton · 2017-08-11T19:51:39Z

Thanks for the PR here @zackmdavis!

I think, though, that the libs team may not have gotten a chance to sign off on the standard-library related changes here. I'll bring this up in the next libs triage to make sure we're on board with it, but if we decide to be more conservative for now, would you be up for backing these out temporarily?

@eddyb

…, r=alexcrichton feature-gate #[must_use] for functions as `fn_must_use` @eddyb I [was](rust-lang#43728 (comment)) [dithering](rust-lang#43728 (comment)) on this, but [your comment](rust-lang#43302 (comment)) makes it sound like we do want a feature gate for this? Please advise. r? @eddyb

This, as rust-lang#43813, is due to the author of rust-lang#43728 (specifically, 3645b06) being a damnably contemptible fool. Before this entire fiasco, we would return early from the unusedness late lints pass if the type of the expression within the `hir::StmtSemi` was `!`, `()`, or a boolean: these types would never get to the point of being marked as unused results. That is, until the dunce who somehow (!?) came to be trusted with the plum responsibility of implementing RFC 1940 (`#[must_use]` for functions) went and fouled everything up, removing the early returns based on the (stupid) thought that there would be no harm in it, since we would need to continue to check these types being returned from must_use functions (which was true for the booleans, at least). But there was harm—harm that any quarter-way-competent programmer would have surely forseen! For after the new functional-must-use checks, there was nothing to stop the previously-returned-early types from falling through to be marked by the unused-results lint!—a monumentally idiotic error that has cost the project tens of precious developer- and reviewer-minutes dealing with the fallout here and in rust-lang#43813. If 3645b06 is representative of the standard of craftsmanship the rising generation of software engineers holds themselves to, I weep for the future of our technological civilization. Resolves rust-lang#44119.

Although RFC 1940 is about annotating functions with `#[must_use]`, a key part of the motivation was linting unused equality operators. (See https://github.com/rust-lang/rfcs/pull/1812#issuecomment-265695898—it seems to have not been clear to discussants at the time that marking the comparison methods as `must_use` would not give us the lints on comparison operators, at least in (what the present author understood as) the most straightforward implementation, as landed in rust-lang#43728 (3645b06).) To rectify the situation, we here lint unused comparison operators as part of the unused-must-use lint (feature gated by the `fn_must_use` feature flag, which now arguably becomes a slight (tolerable in the opinion of the present author) misnomer). This is in the matter of rust-lang#43302.

@crumblingstatue

add comparison operators to must-use lint (under `fn_must_use` feature) Although RFC 1940 is about annotating functions with `#[must_use]`, a key part of the motivation was linting unused equality operators. (See https://github.com/rust-lang/rfcs/pull/1812#issuecomment-265695898—it seems to have not been clear to discussants at the time that marking the comparison methods as `must_use` would not give us the lints on comparison operators, at least in (what the present author understood as) the most straightforward implementation, as landed in rust-lang#43728 (3645b06).) To rectify the situation, we here lint unused comparison operators as part of the unused-must-use lint (feature gated by the `fn_must_use` feature flag, which now arguably becomes a slight (tolerable in the opinion of the present author) misnomer). This is in the matter of rust-lang#43302. cc @crumblingstatue

Changelog: Version 1.21.0 (2017-10-12) ========================== Language -------- - [You can now use static references for literals.][43838] Example: ```rust fn main() { let x: &'static u32 = &0; } ``` - [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540] Example: ```rust my_macro!(Vec<i32>::new); // Always worked my_macro!(Vec::<i32>::new); // Now works ``` Compiler -------- - [Upgraded jemalloc to 4.5.0][43911] - [Enabled unwinding panics on Redox][43917] - [Now runs LLVM in parallel during translation phase.][43506] This should reduce peak memory usage. Libraries --------- - [Generate builtin impls for `Clone` for all arrays and tuples that are `T: Clone`][43690] - [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459] - [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`, `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565] Stabilized APIs --------------- [`std::mem::discriminant`] Cargo ----- - [You can now call `cargo install` with multiple package names][cargo/4216] - [Cargo commands inside a virtual workspace will now implicitly pass `--all`][cargo/4335] - [Added a `[patch]` section to `Cargo.toml` to handle prepublication dependencies][cargo/4123] [RFC 1969] - [`include` & `exclude` fields in `Cargo.toml` now accept gitignore like patterns][cargo/4270] - [Added the `--all-targets` option][cargo/4400] - [Using required dependencies as a feature is now deprecated and emits a warning][cargo/4364] Misc ---- - [Cargo docs are moving][43916] to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo) - [The rustdoc book is now available][43863] at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc) - [Added a preview of RLS has been made available through rustup][44204] Install with `rustup component add rls-preview` - [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348] Previously only showed `std::os::unix`. Compatibility Notes ------------------- - [Changes in method matching against higher-ranked types][43880] This may cause breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880] - [rustc's JSON error output's byte position start at top of file.][42973] Was previously relative to the rustc's internal `CodeMap` struct which required the unstable library `libsyntax` to correctly use. - [`unused_results` lint no longer ignores booleans][43728] [42565]: rust-lang/rust#42565 [42973]: rust-lang/rust#42973 [43348]: rust-lang/rust#43348 [43459]: rust-lang/rust#43459 [43506]: rust-lang/rust#43506 [43540]: rust-lang/rust#43540 [43690]: rust-lang/rust#43690 [43728]: rust-lang/rust#43728 [43838]: rust-lang/rust#43838 [43863]: rust-lang/rust#43863 [43880]: rust-lang/rust#43880 [43911]: rust-lang/rust#43911 [43916]: rust-lang/rust#43916 [43917]: rust-lang/rust#43917 [44204]: rust-lang/rust#44204 [cargo/4123]: rust-lang/cargo#4123 [cargo/4216]: rust-lang/cargo#4216 [cargo/4270]: rust-lang/cargo#4270 [cargo/4335]: rust-lang/cargo#4335 [cargo/4364]: rust-lang/cargo#4364 [cargo/4400]: rust-lang/cargo#4400 [RFC 1969]: rust-lang/rfcs#1969 [info/43880]: rust-lang/rust#44224 (comment) [`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html

rust-highfive assigned nikomatsakis Aug 8, 2017

zackmdavis force-pushed the fnused branch from 1877325 to 88b686e Compare August 8, 2017 05:44

arielb1 assigned eddyb and unassigned nikomatsakis Aug 8, 2017

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2017

zackmdavis added 2 commits August 8, 2017 11:31

#[must_use] for functions (RFC 1940)

3645b06

The return value of a function annotated with `must_use`, must be used. This is in the matter of rust-lang#43302.

mark comparison trait methods as #[must_use]

f5ac228

Note that this doesn't actually give us warnings on `a == b;` and the like, as some observers may have hoped. This is in the matter of rust-lang#43302.

zackmdavis force-pushed the fnused branch from 88b686e to f5ac228 Compare August 8, 2017 18:32

scottmcm reviewed Aug 8, 2017

View reviewed changes

bors merged commit f5ac228 into rust-lang:master Aug 9, 2017

crumblingstatue mentioned this pull request Aug 9, 2017

Tracking issue for RFC 1940: allow #[must_use] on functions #43302

Closed

3 tasks

zackmdavis mentioned this pull request Aug 9, 2017

feature-gate #[must_use] for functions as fn_must_use #43776

Merged

Mark-Simulacrum mentioned this pull request Aug 11, 2017

Unused results lint fails on trivial program #43806

Closed

alexcrichton mentioned this pull request Aug 11, 2017

Decide whether or not to have #[must_use] on PartialEq #43810

Closed

GuillaumeGomez mentioned this pull request Aug 16, 2017

#[must_use] for gtk::init? gtk-rs/gtk#554

Closed

zackmdavis mentioned this pull request Aug 26, 2017

add comparison operators to must-use lint (under fn_must_use feature) #44103

Merged

zackmdavis mentioned this pull request Aug 28, 2017

unused_results is stricter on Nightly and Beta than on Stable #44119

Closed

sfackler mentioned this pull request Aug 30, 2017

must_use_fn rust-lang/rfcs#2134

Closed

kennytm mentioned this pull request Aug 31, 2017

#[must_use] now suddenly warns in nightly #44213

Closed

zackmdavis mentioned this pull request Sep 11, 2017

Updated RELEASES.md for 1.21.0 #44481

Merged

zackmdavis deleted the fnused branch January 13, 2018 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#[must_use] for functions #43728

#[must_use] for functions #43728

zackmdavis commented Aug 8, 2017 •

edited

Loading

rust-highfive commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

arielb1 commented Aug 8, 2017

eddyb commented Aug 8, 2017

bors commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

eddyb commented Aug 8, 2017

bors commented Aug 8, 2017

scottmcm Aug 8, 2017

zackmdavis Aug 8, 2017

bors commented Aug 9, 2017

bors commented Aug 9, 2017

crumblingstatue commented Aug 9, 2017 •

edited

Loading

alexcrichton commented Aug 11, 2017

#[must_use] for functions #43728

#[must_use] for functions #43728

Conversation

zackmdavis commented Aug 8, 2017 • edited Loading

rust-highfive commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

arielb1 commented Aug 8, 2017

eddyb commented Aug 8, 2017

bors commented Aug 8, 2017

zackmdavis commented Aug 8, 2017

eddyb commented Aug 8, 2017

bors commented Aug 8, 2017

scottmcm Aug 8, 2017

Choose a reason for hiding this comment

zackmdavis Aug 8, 2017

Choose a reason for hiding this comment

bors commented Aug 9, 2017

bors commented Aug 9, 2017

crumblingstatue commented Aug 9, 2017 • edited Loading

alexcrichton commented Aug 11, 2017

zackmdavis commented Aug 8, 2017 •

edited

Loading

crumblingstatue commented Aug 9, 2017 •

edited

Loading