From 763fd5dcc5367d751259f0b7d58ac0f24e0e0a99 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 19 Feb 2015 11:58:02 +1100 Subject: [PATCH 1/9] 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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 6/9] 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 7/9] 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 8/9] 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 9/9] 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