Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add #[must_use] to all functions 'fn(float) -> float' #63871

Open
wants to merge 5 commits into
base: master
from

Conversation

@BatmanAoD
Copy link

commented Aug 24, 2019

These are pure functions.

Part of #48926

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@BatmanAoD BatmanAoD marked this pull request as ready for review Aug 24, 2019

@rkruppe

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

My impression (confirmed by re-reading the linked issue and others cross-referenced there) was that we don't currently have a precedent for applying must_use sweepingly to entire classses of functions, and instead only add it to individual functions when there's concrete motivation for that specific functions being must_use (i.e., real world bugs involving that function's result being ignored). This PR deviates from that, which... at least requires broader decision-making than just merging it.

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 25, 2019

@rkruppe I personally misused .round(), but I don't think there's anything special about round that makes it more likely to be misused. So while I agree that the last comment in that thread recommends a conservative metric for annotatinmg on a per-function basis only, I think that annotating only round would be a code smell at best.

@killercup

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

I don't have any opinion on whether this should land, but if it does, I'd like there to be a message that makes the error a bit more expressive (something along the lines of #[must_use = "method returns a new number and does not mutate the caller"]).

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 25, 2019

@killercup That sounds good! How about "...mutate the original value" rather than "caller"?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

I feel like this makes sense, and fits the spirit of must_use. I also feel like many of these functions, because they operate on a number, might look like they could mutate the number in-place, so I think they fit the use case of must_use.

I don't know of any general policy for whether we should add must_use annotations to any function that fits such a pattern. I personally feel we should, but I also do want to get some opinions and broader consensus first. As such:

@rfcbot merge

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@joshtriplett I think it would be good to continue the discussion in the linked tracking issue and eventually determine a heuristic that Clippy can apply to treat certain types of functions as must_use even if they're not explicitly annotated. By introducing such a heuristic in Clippy first, we can have an immediate benefit but also determine how widespread the effect of a lint in the compiler itself would be.

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

I've added the message discussed above. I've also added must_use to functions that take additional arguments beyond self but still return a single new float.

One such function, sub_abs, is deprecated, but I'm assuming it's okay to add new annotations to deprecated functions.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-28T15:42:35.1416231Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-28T15:42:35.1643996Z ##[command]git config gc.auto 0
2019-08-28T15:42:35.1818089Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-28T15:42:35.1896048Z ##[command]git config --get-all http.proxy
2019-08-28T15:42:35.2065713Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63871/merge:refs/remotes/pull/63871/merge
---
2019-08-28T16:46:00.4040306Z .................................................................................................... 1500/8969
2019-08-28T16:46:06.3208847Z .................................................................................................... 1600/8969
2019-08-28T16:46:19.5727777Z .............................................i...............i...................................... 1700/8969
2019-08-28T16:46:28.1883740Z .................................................................................................... 1800/8969
2019-08-28T16:46:43.0283762Z ....................................iiiii........................................................... 1900/8969
2019-08-28T16:46:54.4033275Z .................................................................................................... 2100/8969
2019-08-28T16:46:57.0661977Z .................................................................................................... 2200/8969
2019-08-28T16:47:01.5411426Z .................................................................................................... 2300/8969
2019-08-28T16:47:09.1004222Z .................................................................................................... 2400/8969
---
2019-08-28T16:50:15.0688482Z .......................i...............i............................................................ 4700/8969
2019-08-28T16:50:27.1544472Z .................................................................................................... 4800/8969
2019-08-28T16:50:33.6882152Z .................................................................................................... 4900/8969
2019-08-28T16:50:44.9591664Z .................................................................................................... 5000/8969
2019-08-28T16:50:50.4001723Z ....ii.ii........................................................................................... 5100/8969
2019-08-28T16:51:05.0707634Z .................................................................................................... 5300/8969
2019-08-28T16:51:12.6433592Z ...................................................................i................................ 5400/8969
2019-08-28T16:51:20.3248160Z .................................................................................................... 5500/8969
2019-08-28T16:51:27.5880059Z .................................................................................................... 5600/8969
2019-08-28T16:51:27.5880059Z .................................................................................................... 5600/8969
2019-08-28T16:51:38.2835392Z .............................................................ii...i..ii...........i................. 5700/8969
2019-08-28T16:52:04.9100807Z .................................................................................................... 5900/8969
2019-08-28T16:52:14.9167931Z .................................................................................................... 6000/8969
2019-08-28T16:52:14.9167931Z .................................................................................................... 6000/8969
2019-08-28T16:52:22.0602498Z ..............................................................i..ii................................. 6100/8969
2019-08-28T16:52:50.8454658Z .................................................................................................... 6300/8969
2019-08-28T16:52:53.1025249Z .................i.................................................................................. 6400/8969
2019-08-28T16:52:55.3296970Z .........................................................................................i.......... 6500/8969
2019-08-28T16:52:58.1492226Z .................................................................................................... 6600/8969
---
2019-08-28T16:57:48.2766523Z  finished in 21.888
2019-08-28T16:57:48.2977318Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:57:48.8260180Z 
2019-08-28T16:57:48.8261035Z running 149 tests
2019-08-28T16:57:51.9096410Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/149
2019-08-28T16:57:53.9185632Z ..iiii..............i.........iii.i......ii......
2019-08-28T16:57:53.9186127Z 
2019-08-28T16:57:53.9190093Z  finished in 5.621
2019-08-28T16:57:53.9382574Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:57:54.1012892Z 
---
2019-08-28T16:57:56.2285015Z  finished in 2.290
2019-08-28T16:57:56.2476844Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:57:56.4090429Z 
2019-08-28T16:57:56.4091164Z running 9 tests
2019-08-28T16:57:56.4092802Z iiiiiiiii
2019-08-28T16:57:56.4093573Z 
2019-08-28T16:57:56.4098390Z  finished in 0.162
2019-08-28T16:57:56.4289001Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:57:56.5917661Z 
---
2019-08-28T16:58:14.8812607Z  finished in 18.452
2019-08-28T16:58:14.9006080Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:58:15.0668052Z 
2019-08-28T16:58:15.0668290Z running 122 tests
2019-08-28T16:58:40.1871805Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....i..........iiii..........i...ii...i.......ii.i 100/122
2019-08-28T16:58:44.9389986Z .i.i......iii.i.....ii
2019-08-28T16:58:44.9390461Z 
2019-08-28T16:58:44.9395059Z  finished in 30.039
2019-08-28T16:58:44.9404925Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-28T16:58:44.9407951Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-08-28T17:13:02.6142835Z 
2019-08-28T17:13:02.6143545Z    Doc-tests core
2019-08-28T17:13:07.9449659Z 
2019-08-28T17:13:07.9455248Z running 2379 tests
2019-08-28T17:13:20.8317847Z ......iiiii......................................................................................... 100/2379
2019-08-28T17:13:33.6255985Z .........................................................................ii......................... 200/2379
2019-08-28T17:14:04.7392363Z .................................................................................................... 400/2379
2019-08-28T17:14:04.7392363Z .................................................................................................... 400/2379
2019-08-28T17:14:16.4858111Z ..............................i..i.................iiii............................................. 500/2379
2019-08-28T17:14:42.1247437Z .................................................................................................... 700/2379
2019-08-28T17:14:55.2519132Z .................................................................................................... 800/2379
2019-08-28T17:15:07.9026516Z .................................................................................................... 900/2379
2019-08-28T17:15:20.7127913Z .................................................................................................... 1000/2379
---
2019-08-28T17:18:40.8366711Z    Compiling rand_pcg v0.1.1
2019-08-28T17:18:40.8367188Z    Compiling rand_chacha v0.1.0
2019-08-28T17:18:41.2274185Z    Compiling rand v0.6.1
2019-08-28T17:18:45.6157653Z    Compiling std v0.0.0 (/checkout/src/libstd)
2019-08-28T17:19:11.5044247Z error: unused return value of `realstd::f32::<impl f32>::clamp` that must be used
2019-08-28T17:19:11.5045683Z     --> src/libstd/f32.rs:1635:9
2019-08-28T17:19:11.5046242Z      |
2019-08-28T17:19:11.5046805Z 1635 |         1.0f32.clamp(3.0, 1.0);
2019-08-28T17:19:11.5047799Z      |
2019-08-28T17:19:11.5048328Z      = note: `-D unused-must-use` implied by `-D warnings`
2019-08-28T17:19:11.5048328Z      = note: `-D unused-must-use` implied by `-D warnings`
2019-08-28T17:19:11.5048903Z      = note: method returns a new number and does not mutate the original value
2019-08-28T17:19:11.5049110Z 
2019-08-28T17:19:11.5049632Z error: unused return value of `realstd::f32::<impl f32>::clamp` that must be used
2019-08-28T17:19:11.5050146Z     --> src/libstd/f32.rs:1641:9
2019-08-28T17:19:11.5050610Z      |
2019-08-28T17:19:11.5051127Z 1641 |         1.0f32.clamp(NAN, 1.0);
2019-08-28T17:19:11.5052618Z      |
2019-08-28T17:19:11.5052618Z      |
2019-08-28T17:19:11.5053162Z      = note: method returns a new number and does not mutate the original value
2019-08-28T17:19:11.5053365Z 
2019-08-28T17:19:11.5053852Z error: unused return value of `realstd::f32::<impl f32>::clamp` that must be used
2019-08-28T17:19:11.5054337Z     --> src/libstd/f32.rs:1647:9
2019-08-28T17:19:11.5054808Z      |
2019-08-28T17:19:11.5055349Z 1647 |         1.0f32.clamp(3.0, NAN);
2019-08-28T17:19:11.5056608Z      |
2019-08-28T17:19:11.5056608Z      |
2019-08-28T17:19:11.5057150Z      = note: method returns a new number and does not mutate the original value
2019-08-28T17:19:11.8629187Z error: aborting due to 3 previous errors
2019-08-28T17:19:11.8630009Z 
2019-08-28T17:19:12.1287704Z error: Could not compile `std`.
2019-08-28T17:19:12.1287885Z 
2019-08-28T17:19:12.1287885Z 
2019-08-28T17:19:12.1299251Z To learn more, run the command again with --verbose.
2019-08-28T17:19:12.1315777Z 
2019-08-28T17:19:12.1316011Z 
2019-08-28T17:19:12.1317169Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "std" "--" "--quiet"
2019-08-28T17:19:12.1317382Z 
2019-08-28T17:19:12.1317418Z 
2019-08-28T17:19:12.1325659Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-28T17:19:12.1325952Z Build completed unsuccessfully in 1:29:20
2019-08-28T17:19:12.1325952Z Build completed unsuccessfully in 1:29:20
2019-08-28T17:19:12.1383692Z == clock drift check ==
2019-08-28T17:19:12.1413497Z   local time: Wed Aug 28 17:19:12 UTC 2019
2019-08-28T17:19:12.2288502Z   network time: Wed, 28 Aug 2019 17:19:12 GMT
2019-08-28T17:19:12.2293658Z == end clock drift check ==
2019-08-28T17:19:13.0935030Z ##[error]Bash exited with code '1'.
2019-08-28T17:19:13.0971514Z ##[section]Starting: Checkout
2019-08-28T17:19:13.0973778Z ==============================================================================
2019-08-28T17:19:13.0973835Z Task         : Get sources
2019-08-28T17:19:13.0973901Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

Ah, okay, those are should_panic tests. I'll un-annotate clamp for now. I wonder if it would be appropriate to allow-unused by default in should_panic tests?

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

We could fix the tests (let _ =), but that could lead to some ecosystem breakage if anyone else is intentionally triggering panic with clamp (or any other floating-point lib function). How do we make the decision?

@@ -1015,7 +1015,8 @@ impl f32 {
/// assert!((2.0f32).clamp(-2.0, 1.0) == 1.0);
/// assert!((std::f32::NAN).clamp(-2.0, 1.0).is_nan());
/// ```
#[must_use = "method returns a new number and does not mutate the original value"]
// The tests below invoke `clamp` without a return value in order to cause a `panic`.

This comment has been minimized.

Copy link
@fbstj

fbstj Aug 29, 2019

Contributor

are these worthy of FIXME or some kind of (new?) issue number?

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Aug 29, 2019

@BatmanAoD

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

I've gone ahead and restored the annotation for clamp and fixed the tests.

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 14, 2019

Ping from triage
@joshtriplett @fbstj can you please re-review this PR?

CC @BatmanAoD

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.