From 763fd5dcc5367d751259f0b7d58ac0f24e0e0a99 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 19 Feb 2015 11:58:02 +1100 Subject: [PATCH 01/19] Allow `#[must_use]` on functions, rather than just types. Mark `Result::{ok,err}` `#[must_use]`. --- text/0000-must-use-functions.md | 139 ++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 text/0000-must-use-functions.md diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md new file mode 100644 index 00000000000..6999389c509 --- /dev/null +++ b/text/0000-must-use-functions.md @@ -0,0 +1,139 @@ +- Feature Name: (fill me in with a unique ident, my_awesome_feature) +- Start Date: (fill me in with today's date, YYYY-MM-DD) +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary + +Support the `#[must_use]` attribute on arbitrary functions, to make +the compiler lint when a call to such a function is ignored. Mark +`Result::{ok, err}` `#[must_use]`. + +# Motivation + +The `#[must_use]` lint is extremely useful for ensuring that values +that are likely to be important are handled, even if by just +explicitly ignoring them with, e.g., `let _ = ...;`. This expresses +the programmers intention clearly, so that there is less confusion +about whether, for example, ignoring the possible error from a `write` +call is intentional or just an accidental oversight. + +Rust has got a lot of mileage out connecting the `#[must_use]` lint to +specific types: types like `Result`, `MutexGuard` (any guard, ina +general) and the lazy iterator adapters have narrow enough use cases +that the programmer usually wants to do something with them. These +types are marked `#[must_use]` and the compiler will print an error if +a semicolon ever throws away a value of that type: + +```rust +fn returns_result() -> Result<(), ()> { + Ok(()) +} + +fn ignore_it() { + returns_result(); +} +``` + +``` +test.rs:6:5: 6:11 warning: unused result which must be used, #[warn(unused_must_use)] on by default +test.rs:6 returns_result(); + ^~~~~~~~~~~~~~~~~ +``` + +However, not every "important" (or, "usually want to use") result can +be a type that can be marked `#[must_use]`, for example, sometimes +functions return unopinionated type like `Option<...>` or `u8` that +may lead to confusion if they are ignored. For example, the `Result` type provides + +```rust +pub fn ok(self) -> Option { + match self { + Ok(x) => Some(x), + Err(_) => None, + } +} +``` + +to view any data in the `Ok` variant as an `Option`. Notably, this +does no meaningful computation, in particular, it does not *enforce* +that the `Result` is `ok()`. Someone reading a line of code +`returns_result().ok();` where the returned value is unused +cannot easily tell if that behaviour was correct, or if something else +was intended, possibilities include: + +- `let _ = returns_result();` to ignore the result (as + `returns_result().ok();` does), +- `returns_result().unwrap();` to panic if there was an error, +- `returns_result().ok().something_else();` to do more computation. + +This is somewhat problematic in the context of `Result` in particular, +because `.ok()` does not really (in the authors opinion) represent a +meaningful use of the `Result`, but it still silences the +`#[must_use]` error. + +These cases can be addressed by allowing specific functions to +explicitly opt-in to also having important results, e.g. `#[must_use] +fn ok(self) -> Option`. This is a natural generalisation of +`#[must_use]` to allow fine-grained control of context sensitive info. + +# Detailed design + +If a semicolon discards the result of a function or method tagged with +`#[must_use]`, the compiler will emit a lint message (under same lint +as `#[must_use]` on types). An optional message `#[must_use = "..."]` +will be printed, to provide the user with more guidance. + +```rust +#[must_use] +fn foo() -> u8 { 0 } + + +struct Bar; + +impl Bar { + #[must_use = "maybe you meant something else"] + fn baz(&self) -> Option { None } +} + +fn qux() { + foo(); // warning: unused result that must be used + Bar.baz(); // warning: unused result that must be used: maybe you meant something else +} +``` + + +# Drawbacks + +This adds a little more complexity to the `#[must_use]` system. + +The rule stated doesn't cover every instance were a `#[must_use]` +function is ignored (it does cover all instances of a `#[must_use]` +type), e.g. `(foo());` and `{ ...; foo() };` will not be picked up +(that is, passing the result through another piece of syntax). This +could be tweaked. + +`Result::ok` is occasionally used for silencing the `#[must_use]` +error of `Result`, i.e. the ignoring of `foo().ok();` is +intentional. However, the most common way do ignore such things is +with `let _ =`, and `ok()` is rarely used in comparison, in most +code-bases: 2 instances in the rust-lang/rust codebase (vs. nearly 400 +text matches for `let _ =`) and 4 in the servo/servo (vs. 55 `let _ +=`). Yet another way to write this is `drop(foo())`, although neither +this nor `let _ =` have the method chaining style. + +# Alternatives + +- Adjust the rule to propagate `#[must_used]`ness through parentheses + and blocks, so that `(foo());`, `{ foo() };` and even `if cond { + foo() } else { 0 };` are linted. + +- Provide an additional method on `Result`, e.g. `fn ignore(self) {}`, so + that users who wish to ignore `Result`s can do so in the method + chaining style: `foo().ignore();`. + +# Unresolved questions + +- Are there many other functions in the standard library/compiler + would benefit from `#[must_use]`? From cfc59a6043f7457101a724ab0afc29503c49f448 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 19 Feb 2015 12:03:54 +1100 Subject: [PATCH 02/19] Add metadata. --- text/0000-must-use-functions.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 6999389c509..b57410087c9 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -1,6 +1,6 @@ -- Feature Name: (fill me in with a unique ident, my_awesome_feature) -- Start Date: (fill me in with today's date, YYYY-MM-DD) -- RFC PR: (leave this empty) +- Feature Name: none? +- Start Date: 2015-02-18 +- RFC PR: [rust-lang/rfcs#886](https://github.com/rust-lang/rfcs/pull/886) - Rust Issue: (leave this empty) # Summary @@ -137,3 +137,4 @@ this nor `let _ =` have the method chaining style. - Are there many other functions in the standard library/compiler would benefit from `#[must_use]`? +- Should this be feature gated? From 3b0d1e67fcc61e74f73653c6990e81de679b65cd Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 19 Feb 2015 12:53:26 +1100 Subject: [PATCH 03/19] Tweak a paragraph. --- text/0000-must-use-functions.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index b57410087c9..7ba8cb367db 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -108,11 +108,13 @@ fn qux() { This adds a little more complexity to the `#[must_use]` system. -The rule stated doesn't cover every instance were a `#[must_use]` -function is ignored (it does cover all instances of a `#[must_use]` -type), e.g. `(foo());` and `{ ...; foo() };` will not be picked up -(that is, passing the result through another piece of syntax). This -could be tweaked. +The rule stated doesn't cover every instance where a `#[must_use]` +function is ignored, e.g. `(foo());` and `{ ...; foo() };` will not be +picked up, even though it is passing the result through a piece of +no-op syntax. This could be tweaked. Notably, the type-based rule doesn't +have this problem, since that sort of "passing-through" causes the +outer piece of syntax to be of the `#[must_use]` type, and so is +considered for the lint itself. `Result::ok` is occasionally used for silencing the `#[must_use]` error of `Result`, i.e. the ignoring of `foo().ok();` is From e68a4d55b44bc30dd46201002b99947967efe262 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 19 Feb 2015 12:57:10 +1100 Subject: [PATCH 04/19] More drawbacks. --- text/0000-must-use-functions.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 7ba8cb367db..5106b9ce627 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -106,7 +106,9 @@ fn qux() { # Drawbacks -This adds a little more complexity to the `#[must_use]` system. +This adds a little more complexity to the `#[must_use]` system, and +may be misused by library authors (but then, many features may be +misused). The rule stated doesn't cover every instance where a `#[must_use]` function is ignored, e.g. `(foo());` and `{ ...; foo() };` will not be @@ -125,6 +127,11 @@ text matches for `let _ =`) and 4 in the servo/servo (vs. 55 `let _ =`). Yet another way to write this is `drop(foo())`, although neither this nor `let _ =` have the method chaining style. +Marking functions `#[must_use]` is a breaking change in certain cases, +e.g. if someone is ignoring their result and has the relevant lint (or +warnings in general) set to be an error. This is a general problem of +improving/expanding lints. + # Alternatives - Adjust the rule to propagate `#[must_used]`ness through parentheses From 86245c5d050014450b70f1fca88aec285f5210ba Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Fri, 20 Feb 2015 01:25:49 +1100 Subject: [PATCH 05/19] Add detailed analysis of `ok` vs `let _ =`. --- text/0000-must-use-functions.md | 116 ++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 5106b9ce627..893c95c076c 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -147,3 +147,119 @@ improving/expanding lints. - Are there many other functions in the standard library/compiler would benefit from `#[must_use]`? - Should this be feature gated? + +# Appendix: is this going to affect "most code-bases"? + +(tl;dr: unlikely.) + +@mahkoh stated: + +> -1. I, and most code-bases, use ok() to ignore Result. + +Let's investigate. + +I sampled 50 random projects on [Rust CI](http://rust-ci.org), and +grepped for `\.ok` and `let _ =`. + +## Methodology + +Initially just I scrolled around and clicked things, may 10-15, the +rest were running this JS `var list = $("a"); +window.open(list[(Math.random() * list.length) | 0].href, '_blank')` +to open literally random links in a new window. The grepping was +performed by running `runit url`, where `runit` is: ```bash function +runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' +| wc -l; git grep 'let _ =' | wc -l; } ``` If there were any `ok`s, I +manually read the grep to see if they were used on not. + +## Data + +| repo | used `\.ok` | unused `\.ok` | `let _ =` | +|------|-------------|---------------|-----------| +| https://github.com/csherratt/obj | 9 | 0 | 1 | +| https://github.com/csherratt/snowmew | 16 | 0 | 0 | +| https://github.com/bluss/petulant-avenger-graphlibrary | 0 | 0 | 12 | +| https://github.com/uutils/coreutils | 15 | 0 | 1 | +| https://github.com/apoelstra/rust-bitcoin/ | 5 | 0 | 3 | +| https://github.com/emk/abort_on_panic-rs | 0 | 0 | 1 | +| https://github.com/japaric/parallel.rs | 2 | 0 | 0 | +| https://github.com/phildawes/racer | 15 | 0 | 0 | +| https://github.com/zargony/rust-fuse | 7 | 7 | 0 | +| https://github.com/jakub-/rust-instrumentation | 0 | 0 | 2 | +| https://github.com/andelf/rust-iconv | 14 | 0 | 0 | +| https://github.com/pshc/brainrust | 25 | 0 | 0 | +| https://github.com/andelf/rust-2048 | 3 | 0 | 0 | +| https://github.com/PistonDevelopers/vecmath | 0 | 0 | 2 | +| https://github.com/japaric/serial.rs | 1 | 0 | 0 | +| https://github.com/servo/html5ever | 14 | 0 | 1 | +| https://github.com/sfackler/r2d2 | 8 | 0 | 0 | +| https://github.com/jamesrhurst/rust-metaflac | 2 | 0 | 0 | +| https://github.com/arjantop/rust-bencode | 3 | 0 | 1 | +| https://github.com/Azdle/dolos | 0 | 2 | 0 | +| https://github.com/ogham/exa | 2 | 0 | 0 | +| https://github.com/aatxe/irc-services | 0 | 0 | 5 | +| https://github.com/nwin/chatIRC | 0 | 0 | 8 | +| https://github.com/reima/rustboy | 1 | 0 | 2 | + +These had no matches at all for `.ok` or `let _ =`: + +- https://github.com/hjr3/hal-rs, +- https://github.com/KokaKiwi/lua-rs, +- https://github.com/dwrensha/capnpc-rust, +- https://github.com/samdoshi/portmidi-rs, +- https://github.com/PistonDevelopers/graphics, +- https://github.com/vberger/ircc-rs, +- https://github.com/stainless-steel/temperature, +- https://github.com/chris-morgan/phantom-enum, +- https://github.com/jeremyletang/rust-portaudio, +- https://github.com/tikue/rust-ml, +- https://github.com/FranklinChen/rust-tau, +- https://github.com/GuillaumeGomez/rust-GSL, +- https://github.com/andelf/rust-httpc, +- https://github.com/huonw/stable_vec, +- https://github.com/TyOverby/rust-termbox, +- https://github.com/japaric/stats.rs, +- https://github.com/omasanori/mesquite, +- https://github.com/andelf/rust-iconv, +- https://github.com/aatxe/dnd, +- https://github.com/pshc/brainrust, +- https://github.com/vsv/rustulator, +- https://github.com/erickt/rust-mongrel2, +- https://github.com/Geal/rust-csv, +- https://github.com/vhbit/base32-rs, +- https://github.com/PistonDevelopers/event, +- https://github.com/untitaker/rust-atomicwrites. + +Disclosure, `snowmew` and `coreutils` were explicitly selected after +recognising their names (i.e. non-randomly), but this before the +`runit` script was used, and before any grepping was performed in any +of these projects. + +The data in R form if you wish to play with it yourself: +```r +structure(list(used.ok = c(9, 16, 0, 15, 5, 0, 2, 15, 7, 0, 14, +25, 3, 0, 1, 14, 8, 2, 3, 0, 2, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, +0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), unused.ok = c(0, +0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, +0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, +0, 0, 0, 0, 0, 0, 0), let = c(1, 0, 12, 1, 3, 1, 0, 0, 0, 2, +0, 0, 0, 2, 0, 1, 0, 0, 1, 0, 0, 5, 8, 2, 0, 0, 0, 0, 0, 0, 0, +0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)), .Names = c("used.ok", +"unused.ok", "let"), row.names = c(NA, -50L), class = "data.frame") +``` + +## Analysis + +I will assume that a crate author uses *either* `let _ =` or `\.ok()` +for ignoring `Result`s, but not both. The crates with neither `let _ +=`s nor unused `.ok()`s are not interesting, as they haven't indicated +a preference either way. Removing those leaves 14 crates, 2 of which +use `\.ok()` and 12 of which use `let _ =`. + +The null hypothesis is that `\.ok()` is used at least as much as `let +_ =`. A one-sided binomial test (e.g. `binom.test(c(2, 12), +alternative = "less")` in R) has p-value 0.007, leading me to reject +the null hypothesis and accept the alternative, that `let _ =` is used +more than `\.ok`. + +(Sorry for the frequentist analysis.) From f95ee6c14b5af9d2ed6cb5ada2ef6c79adc61e1c Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Fri, 20 Feb 2015 01:28:30 +1100 Subject: [PATCH 06/19] Typo. --- text/0000-must-use-functions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 893c95c076c..8cbadd3ce0b 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -19,7 +19,7 @@ about whether, for example, ignoring the possible error from a `write` call is intentional or just an accidental oversight. Rust has got a lot of mileage out connecting the `#[must_use]` lint to -specific types: types like `Result`, `MutexGuard` (any guard, ina +specific types: types like `Result`, `MutexGuard` (any guard, in general) and the lazy iterator adapters have narrow enough use cases that the programmer usually wants to do something with them. These types are marked `#[must_use]` and the compiler will print an error if From 86e3e777d48ab3b555a382aa3c6e27f06cc77bed Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Fri, 20 Feb 2015 01:30:02 +1100 Subject: [PATCH 07/19] Point to the appendix. --- text/0000-must-use-functions.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 8cbadd3ce0b..efb1bbdf4cd 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -124,8 +124,9 @@ intentional. However, the most common way do ignore such things is with `let _ =`, and `ok()` is rarely used in comparison, in most code-bases: 2 instances in the rust-lang/rust codebase (vs. nearly 400 text matches for `let _ =`) and 4 in the servo/servo (vs. 55 `let _ -=`). Yet another way to write this is `drop(foo())`, although neither -this nor `let _ =` have the method chaining style. +=`). See the appendix for a more formal treatment of this +question. Yet another way to write this is `drop(foo())`, although +neither this nor `let _ =` have the method chaining style. Marking functions `#[must_use]` is a breaking change in certain cases, e.g. if someone is ignoring their result and has the relevant lint (or From 96c62fad39679c9500230d0a8c8fef28b8691f63 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Fri, 20 Feb 2015 01:38:49 +1100 Subject: [PATCH 08/19] Formatting, mention ignored URLs. --- text/0000-must-use-functions.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index efb1bbdf4cd..9ca81a8a715 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -167,11 +167,18 @@ grepped for `\.ok` and `let _ =`. Initially just I scrolled around and clicked things, may 10-15, the rest were running this JS `var list = $("a"); window.open(list[(Math.random() * list.length) | 0].href, '_blank')` -to open literally random links in a new window. The grepping was -performed by running `runit url`, where `runit` is: ```bash function -runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' -| wc -l; git grep 'let _ =' | wc -l; } ``` If there were any `ok`s, I -manually read the grep to see if they were used on not. +to open literally random links in a new window. Links that were not +projects (including 404s from deleted projects) and duplicates were +ignored. The grepping was performed by running `runit url`, where +`runit` is the shell function: + +```bash +function runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' +| wc -l; git grep 'let _ =' | wc -l; } +``` + +If there were any `ok`s, I manually read the grep to see if they were +used on not. ## Data From ca877fb5c4d92afd3be3d1ee85e7474a276b93fe Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Fri, 20 Feb 2015 01:40:39 +1100 Subject: [PATCH 09/19] Whoops, an invalid newline snuck in. --- text/0000-must-use-functions.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 9ca81a8a715..bef70b716a6 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -173,8 +173,7 @@ ignored. The grepping was performed by running `runit url`, where `runit` is the shell function: ```bash -function runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' -| wc -l; git grep 'let _ =' | wc -l; } +function runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' | wc -l; git grep 'let _ =' | wc -l; } ``` If there were any `ok`s, I manually read the grep to see if they were From fd0e5b69d2c9baf2d59660a9b31abd0a72e116da Mon Sep 17 00:00:00 2001 From: Igor Polyakov Date: Thu, 2 Mar 2017 04:57:19 -0800 Subject: [PATCH 10/19] Updated the RFC to include lessons learned from #1812 --- text/0000-must-use-functions.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index bef70b716a6..14417ff68bc 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -78,6 +78,13 @@ explicitly opt-in to also having important results, e.g. `#[must_use] fn ok(self) -> Option`. This is a natural generalisation of `#[must_use]` to allow fine-grained control of context sensitive info. +One of the most important use-cases for this would be annotating `PartialEq::{eq, ne}` with `#[must_use]`. + +There's a bug in Android where instead of `modem_reset_flag = 0;` the file affected has `modem_reset_flag == 0;`. +Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. + +See further discussion in [#1812.](https://github.com/rust-lang/rfcs/pull/1812) + # Detailed design If a semicolon discards the result of a function or method tagged with From f0a894f514cfd9f13e52d5e68422bf671484d8b5 Mon Sep 17 00:00:00 2001 From: iopq Date: Sun, 18 Jun 2017 03:37:58 -0700 Subject: [PATCH 11/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 175 -------------------------------- 1 file changed, 175 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 14417ff68bc..2dcd039eccd 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -41,43 +41,6 @@ test.rs:6 returns_result(); ^~~~~~~~~~~~~~~~~ ``` -However, not every "important" (or, "usually want to use") result can -be a type that can be marked `#[must_use]`, for example, sometimes -functions return unopinionated type like `Option<...>` or `u8` that -may lead to confusion if they are ignored. For example, the `Result` type provides - -```rust -pub fn ok(self) -> Option { - match self { - Ok(x) => Some(x), - Err(_) => None, - } -} -``` - -to view any data in the `Ok` variant as an `Option`. Notably, this -does no meaningful computation, in particular, it does not *enforce* -that the `Result` is `ok()`. Someone reading a line of code -`returns_result().ok();` where the returned value is unused -cannot easily tell if that behaviour was correct, or if something else -was intended, possibilities include: - -- `let _ = returns_result();` to ignore the result (as - `returns_result().ok();` does), -- `returns_result().unwrap();` to panic if there was an error, -- `returns_result().ok().something_else();` to do more computation. - -This is somewhat problematic in the context of `Result` in particular, -because `.ok()` does not really (in the authors opinion) represent a -meaningful use of the `Result`, but it still silences the -`#[must_use]` error. - -These cases can be addressed by allowing specific functions to -explicitly opt-in to also having important results, e.g. `#[must_use] -fn ok(self) -> Option`. This is a natural generalisation of -`#[must_use]` to allow fine-grained control of context sensitive info. - One of the most important use-cases for this would be annotating `PartialEq::{eq, ne}` with `#[must_use]`. There's a bug in Android where instead of `modem_reset_flag = 0;` the file affected has `modem_reset_flag == 0;`. @@ -125,16 +88,6 @@ have this problem, since that sort of "passing-through" causes the outer piece of syntax to be of the `#[must_use]` type, and so is considered for the lint itself. -`Result::ok` is occasionally used for silencing the `#[must_use]` -error of `Result`, i.e. the ignoring of `foo().ok();` is -intentional. However, the most common way do ignore such things is -with `let _ =`, and `ok()` is rarely used in comparison, in most -code-bases: 2 instances in the rust-lang/rust codebase (vs. nearly 400 -text matches for `let _ =`) and 4 in the servo/servo (vs. 55 `let _ -=`). See the appendix for a more formal treatment of this -question. Yet another way to write this is `drop(foo())`, although -neither this nor `let _ =` have the method chaining style. - Marking functions `#[must_use]` is a breaking change in certain cases, e.g. if someone is ignoring their result and has the relevant lint (or warnings in general) set to be an error. This is a general problem of @@ -146,134 +99,6 @@ improving/expanding lints. and blocks, so that `(foo());`, `{ foo() };` and even `if cond { foo() } else { 0 };` are linted. -- Provide an additional method on `Result`, e.g. `fn ignore(self) {}`, so - that users who wish to ignore `Result`s can do so in the method - chaining style: `foo().ignore();`. - # Unresolved questions -- Are there many other functions in the standard library/compiler - would benefit from `#[must_use]`? - Should this be feature gated? - -# Appendix: is this going to affect "most code-bases"? - -(tl;dr: unlikely.) - -@mahkoh stated: - -> -1. I, and most code-bases, use ok() to ignore Result. - -Let's investigate. - -I sampled 50 random projects on [Rust CI](http://rust-ci.org), and -grepped for `\.ok` and `let _ =`. - -## Methodology - -Initially just I scrolled around and clicked things, may 10-15, the -rest were running this JS `var list = $("a"); -window.open(list[(Math.random() * list.length) | 0].href, '_blank')` -to open literally random links in a new window. Links that were not -projects (including 404s from deleted projects) and duplicates were -ignored. The grepping was performed by running `runit url`, where -`runit` is the shell function: - -```bash -function runit () { cd ~/tmp; git clone $1; cd $(basename $1); git grep '\.ok' | wc -l; git grep 'let _ =' | wc -l; } -``` - -If there were any `ok`s, I manually read the grep to see if they were -used on not. - -## Data - -| repo | used `\.ok` | unused `\.ok` | `let _ =` | -|------|-------------|---------------|-----------| -| https://github.com/csherratt/obj | 9 | 0 | 1 | -| https://github.com/csherratt/snowmew | 16 | 0 | 0 | -| https://github.com/bluss/petulant-avenger-graphlibrary | 0 | 0 | 12 | -| https://github.com/uutils/coreutils | 15 | 0 | 1 | -| https://github.com/apoelstra/rust-bitcoin/ | 5 | 0 | 3 | -| https://github.com/emk/abort_on_panic-rs | 0 | 0 | 1 | -| https://github.com/japaric/parallel.rs | 2 | 0 | 0 | -| https://github.com/phildawes/racer | 15 | 0 | 0 | -| https://github.com/zargony/rust-fuse | 7 | 7 | 0 | -| https://github.com/jakub-/rust-instrumentation | 0 | 0 | 2 | -| https://github.com/andelf/rust-iconv | 14 | 0 | 0 | -| https://github.com/pshc/brainrust | 25 | 0 | 0 | -| https://github.com/andelf/rust-2048 | 3 | 0 | 0 | -| https://github.com/PistonDevelopers/vecmath | 0 | 0 | 2 | -| https://github.com/japaric/serial.rs | 1 | 0 | 0 | -| https://github.com/servo/html5ever | 14 | 0 | 1 | -| https://github.com/sfackler/r2d2 | 8 | 0 | 0 | -| https://github.com/jamesrhurst/rust-metaflac | 2 | 0 | 0 | -| https://github.com/arjantop/rust-bencode | 3 | 0 | 1 | -| https://github.com/Azdle/dolos | 0 | 2 | 0 | -| https://github.com/ogham/exa | 2 | 0 | 0 | -| https://github.com/aatxe/irc-services | 0 | 0 | 5 | -| https://github.com/nwin/chatIRC | 0 | 0 | 8 | -| https://github.com/reima/rustboy | 1 | 0 | 2 | - -These had no matches at all for `.ok` or `let _ =`: - -- https://github.com/hjr3/hal-rs, -- https://github.com/KokaKiwi/lua-rs, -- https://github.com/dwrensha/capnpc-rust, -- https://github.com/samdoshi/portmidi-rs, -- https://github.com/PistonDevelopers/graphics, -- https://github.com/vberger/ircc-rs, -- https://github.com/stainless-steel/temperature, -- https://github.com/chris-morgan/phantom-enum, -- https://github.com/jeremyletang/rust-portaudio, -- https://github.com/tikue/rust-ml, -- https://github.com/FranklinChen/rust-tau, -- https://github.com/GuillaumeGomez/rust-GSL, -- https://github.com/andelf/rust-httpc, -- https://github.com/huonw/stable_vec, -- https://github.com/TyOverby/rust-termbox, -- https://github.com/japaric/stats.rs, -- https://github.com/omasanori/mesquite, -- https://github.com/andelf/rust-iconv, -- https://github.com/aatxe/dnd, -- https://github.com/pshc/brainrust, -- https://github.com/vsv/rustulator, -- https://github.com/erickt/rust-mongrel2, -- https://github.com/Geal/rust-csv, -- https://github.com/vhbit/base32-rs, -- https://github.com/PistonDevelopers/event, -- https://github.com/untitaker/rust-atomicwrites. - -Disclosure, `snowmew` and `coreutils` were explicitly selected after -recognising their names (i.e. non-randomly), but this before the -`runit` script was used, and before any grepping was performed in any -of these projects. - -The data in R form if you wish to play with it yourself: -```r -structure(list(used.ok = c(9, 16, 0, 15, 5, 0, 2, 15, 7, 0, 14, -25, 3, 0, 1, 14, 8, 2, 3, 0, 2, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, -0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), unused.ok = c(0, -0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, -0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, -0, 0, 0, 0, 0, 0, 0), let = c(1, 0, 12, 1, 3, 1, 0, 0, 0, 2, -0, 0, 0, 2, 0, 1, 0, 0, 1, 0, 0, 5, 8, 2, 0, 0, 0, 0, 0, 0, 0, -0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)), .Names = c("used.ok", -"unused.ok", "let"), row.names = c(NA, -50L), class = "data.frame") -``` - -## Analysis - -I will assume that a crate author uses *either* `let _ =` or `\.ok()` -for ignoring `Result`s, but not both. The crates with neither `let _ -=`s nor unused `.ok()`s are not interesting, as they haven't indicated -a preference either way. Removing those leaves 14 crates, 2 of which -use `\.ok()` and 12 of which use `let _ =`. - -The null hypothesis is that `\.ok()` is used at least as much as `let -_ =`. A one-sided binomial test (e.g. `binom.test(c(2, 12), -alternative = "less")` in R) has p-value 0.007, leading me to reject -the null hypothesis and accept the alternative, that `let _ =` is used -more than `\.ok`. - -(Sorry for the frequentist analysis.) From 5b332c25863501b8f61d84525005f85794635e61 Mon Sep 17 00:00:00 2001 From: iopq Date: Sun, 18 Jun 2017 03:47:20 -0700 Subject: [PATCH 12/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 2dcd039eccd..2f96ee8b9c4 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -44,7 +44,12 @@ test.rs:6 returns_result(); One of the most important use-cases for this would be annotating `PartialEq::{eq, ne}` with `#[must_use]`. There's a bug in Android where instead of `modem_reset_flag = 0;` the file affected has `modem_reset_flag == 0;`. -Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. +Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. By marking this function `#[must_use]` the compiler would complain about things like: + +``` + modem_reset_flag == false; //warning + modem_reset_flag = false; //ok +``` See further discussion in [#1812.](https://github.com/rust-lang/rfcs/pull/1812) From a2c640b45e7faa5cc97d3ac7dffbb4cbf3674e5e Mon Sep 17 00:00:00 2001 From: iopq Date: Sun, 18 Jun 2017 04:23:50 -0700 Subject: [PATCH 13/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index 2f96ee8b9c4..d7ee2d7be66 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -78,6 +78,14 @@ fn qux() { } ``` +The primary motivation is to mark `PartialEq` functions as `#[must_use]`: + +``` +#[must_use = "the result of testing for equality should not be discarded"] +fn eq(&self, other: &Rhs) -> bool; +``` + +The same thing for `ne`, and also `lt`, `gt`, `ge`, `gt` in `PartialOrd`. There is no reason to discard the results of those operations. # Drawbacks From ab711951bacff2852f9732dc44630f862a7f96c5 Mon Sep 17 00:00:00 2001 From: iopq Date: Sun, 18 Jun 2017 04:28:46 -0700 Subject: [PATCH 14/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index d7ee2d7be66..a9fde67764f 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -85,7 +85,7 @@ The primary motivation is to mark `PartialEq` functions as `#[must_use]`: fn eq(&self, other: &Rhs) -> bool; ``` -The same thing for `ne`, and also `lt`, `gt`, `ge`, `gt` in `PartialOrd`. There is no reason to discard the results of those operations. +The same thing for `ne`, and also `lt`, `gt`, `ge`, `gt` in `PartialOrd`. There is no reason to discard the results of those operations. This means the `impl`s of these functions are not changed, it still issues a warning even for a custom `impl`. # Drawbacks @@ -111,6 +111,8 @@ improving/expanding lints. - Adjust the rule to propagate `#[must_used]`ness through parentheses and blocks, so that `(foo());`, `{ foo() };` and even `if cond { foo() } else { 0 };` are linted. + +- Should we let particular `impl`s of a function have this attribute? Current design allows you to attach it inside the declaration of the trait. # Unresolved questions From 8fb6b5daf2c0150c57af0c99dab258bca2283537 Mon Sep 17 00:00:00 2001 From: iopq Date: Mon, 19 Jun 2017 14:06:03 -0700 Subject: [PATCH 15/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index a9fde67764f..cca88c24df6 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -47,8 +47,8 @@ There's a bug in Android where instead of `modem_reset_flag = 0;` the file affec Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. By marking this function `#[must_use]` the compiler would complain about things like: ``` - modem_reset_flag == false; //warning - modem_reset_flag = false; //ok + modem_reset_flag == returns_bool(); //warning + modem_reset_flag = returns_bool(); //ok ``` See further discussion in [#1812.](https://github.com/rust-lang/rfcs/pull/1812) From 2711ff86bd000ac626bbb4ecda6bffd7c7682180 Mon Sep 17 00:00:00 2001 From: iopq Date: Mon, 19 Jun 2017 14:06:58 -0700 Subject: [PATCH 16/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index cca88c24df6..a9fde67764f 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -47,8 +47,8 @@ There's a bug in Android where instead of `modem_reset_flag = 0;` the file affec Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. By marking this function `#[must_use]` the compiler would complain about things like: ``` - modem_reset_flag == returns_bool(); //warning - modem_reset_flag = returns_bool(); //ok + modem_reset_flag == false; //warning + modem_reset_flag = false; //ok ``` See further discussion in [#1812.](https://github.com/rust-lang/rfcs/pull/1812) From 30f8ab03c83f63e485ca081ae00e7cf99e63a116 Mon Sep 17 00:00:00 2001 From: iopq Date: Mon, 19 Jun 2017 14:07:48 -0700 Subject: [PATCH 17/19] Update 0000-must-use-functions.md --- text/0000-must-use-functions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index a9fde67764f..eed42ba57cf 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -44,7 +44,7 @@ test.rs:6 returns_result(); One of the most important use-cases for this would be annotating `PartialEq::{eq, ne}` with `#[must_use]`. There's a bug in Android where instead of `modem_reset_flag = 0;` the file affected has `modem_reset_flag == 0;`. -Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. By marking this function `#[must_use]` the compiler would complain about things like: +Rust does not do better in this case. If you wrote `modem_reset_flag == false;` the compiler would be perfectly happy and wouldn't warn you. By marking `PartialEq` `#[must_use]` the compiler would complain about things like: ``` modem_reset_flag == false; //warning From 6f7aa4237ebb652bc7b393f71131ab1b97127b23 Mon Sep 17 00:00:00 2001 From: iopq Date: Mon, 10 Jul 2017 20:58:51 -0700 Subject: [PATCH 18/19] updated summary --- text/0000-must-use-functions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-must-use-functions.md b/text/0000-must-use-functions.md index eed42ba57cf..4f4aa8e6344 100644 --- a/text/0000-must-use-functions.md +++ b/text/0000-must-use-functions.md @@ -7,7 +7,7 @@ Support the `#[must_use]` attribute on arbitrary functions, to make the compiler lint when a call to such a function is ignored. Mark -`Result::{ok, err}` `#[must_use]`. +`PartialEq::{eq, ne}` `#[must_use]` as well as `PartialOrd::{lt, gt, le, ge}`. # Motivation @@ -85,7 +85,7 @@ The primary motivation is to mark `PartialEq` functions as `#[must_use]`: fn eq(&self, other: &Rhs) -> bool; ``` -The same thing for `ne`, and also `lt`, `gt`, `ge`, `gt` in `PartialOrd`. There is no reason to discard the results of those operations. This means the `impl`s of these functions are not changed, it still issues a warning even for a custom `impl`. +The same thing for `ne`, and also `lt`, `gt`, `ge`, `le` in `PartialOrd`. There is no reason to discard the results of those operations. This means the `impl`s of these functions are not changed, it still issues a warning even for a custom `impl`. # Drawbacks From f4b68532206f0a3e0664877841b407ab1302c79a Mon Sep 17 00:00:00 2001 From: Aaron Turon Date: Mon, 17 Jul 2017 15:41:04 -0700 Subject: [PATCH 19/19] RFC 1940 is Allow `#[must_use]` on functions --- ...{0000-must-use-functions.md => 1940-must-use-functions.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-must-use-functions.md => 1940-must-use-functions.md} (97%) diff --git a/text/0000-must-use-functions.md b/text/1940-must-use-functions.md similarity index 97% rename from text/0000-must-use-functions.md rename to text/1940-must-use-functions.md index 4f4aa8e6344..f631e911876 100644 --- a/text/0000-must-use-functions.md +++ b/text/1940-must-use-functions.md @@ -1,7 +1,7 @@ - Feature Name: none? - Start Date: 2015-02-18 -- RFC PR: [rust-lang/rfcs#886](https://github.com/rust-lang/rfcs/pull/886) -- Rust Issue: (leave this empty) +- RFC PR: https://github.com/rust-lang/rfcs/pull/1940 +- Rust Issue: https://github.com/rust-lang/rust/issues/43302 # Summary