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

Warn on unused results for operation methods on nums #59839

Merged
merged 2 commits into from Apr 23, 2019

Conversation

Projects
None yet
8 participants
@KodrAus
Copy link
Contributor

commented Apr 10, 2019

From a suggestion by @llogiq

Adds a #[must_use] attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating self, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}

Will produce a warning like:

warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original

If this is something we're on board with, we could do something similar for f32 and f64 too. There are probably other methods on integers that make sense.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (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.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:046ca9e6:start=1554895634719746745,finish=1554895636999312526,duration=2279565781
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[01:06:33] .................................................................................................... 500/2961
[01:06:45] .................................................................................................... 600/2961
[01:07:01] .................................................................................................... 700/2961
[01:07:12] .................................................................................................... 800/2961
[01:07:21] ................F................................................................................... 900/2961
[01:07:49] .................................................................................................... 1100/2961
[01:07:58] .................................................................................................... 1200/2961
[01:08:08] .................................................................................................... 1300/2961
[01:08:20] .................................................................................................... 1400/2961
---
[01:12:14] failures:
[01:12:14] 
[01:12:14] ---- [run-pass] run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs stdout ----
[01:12:14] normalized stderr:
[01:12:14] warning: unused return value of `core::num::<impl u32>::wrapping_add` that must be used
[01:12:14]    |
[01:12:14]    |
[01:12:14] LL |         y.wrapping_add(1);
[01:12:14]    |
[01:12:14]    = note: #[warn(unused_must_use)] on by default
[01:12:14]    = note: this returns the result of the operation, without modifying the original
[01:12:14] 
[01:12:14] 
[01:12:14] 
[01:12:14] 
[01:12:14] 
[01:12:14] The actual stderr differed from the expected stderr.
[01:12:14] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers/expect-infer-supply-two-infers.stderr
[01:12:14] To update references, rerun the tests and pass the `--bless` flag
[01:12:14] To only update this specific test, also pass `--test-args functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs`
[01:12:14] error: 1 errors occurred comparing output.
[01:12:14] status: exit code: 0
[01:12:14] status: exit code: 0
[01:12:14] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers/auxiliary"
[01:12:14] ------------------------------------------
[01:12:14] 
[01:12:14] ------------------------------------------
[01:12:14] stderr:
[01:12:14] stderr:
[01:12:14] ------------------------------------------
[01:12:14] {"message":"unused return value of `core::num::<impl u32>::wrapping_add` that must be used","code":{"code":"unused_must_use","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs","byte_start":397,"byte_end":415,"line_start":15,"line_end":15,"column_start":9,"column_end":27,"is_primary":true,"text":[{"text":"        y.wrapping_add(1);","highlight_start":9,"highlight_end":27}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unused_must_use)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"this returns the result of the operation, without modifying the original","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning: unused return value of `core::num::<impl u32>::wrapping_add` that must be used\n  --> /checkout/src/test/run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs:15:9\n   |\nLL |         y.wrapping_add(1);\n   |         ^^^^^^^^^^^^^^^^^^\n   |\n   = note: #[warn(unused_must_use)] on by default\n   = note: this returns the result of the operation, without modifying the original\n\n"}
[01:12:14] ------------------------------------------
[01:12:14] 
[01:12:14] thread '[run-pass] run-pass/functions-closures/closure-expected-type/expect-infer-supply-two-infers.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
[01:12:14] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
---
[01:12:14] 
[01:12:14] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:516:22
[01:12:14] 
[01:12:14] 
[01:12:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:12:14] 
[01:12:14] 
[01:12:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:12:14] Build completed unsuccessfully in 0:10:54
[01:12:14] Build completed unsuccessfully in 0:10:54
[01:12:14] make: *** [check] Error 1
[01:12:14] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0840f0f0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Apr 10 12:39:42 UTC 2019

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)

@llogiq

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

You can prepend a let _ = to the y.wrapping_add(1); in src/test/ui/rfc-2361-dbg-macro/dbg-macro-expected-behavior.rs to get rid of the warning and allow the test to pass.

@KodrAus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I do also have a compile-fail test for this that I didn't bother checking in.

It might be worth looking at how we want to decide what numeric operations should have a #[must_use] attribute and whether they should have an additional note on them (right now the note I've added in this PR is inconsistent with the one on the Add trait). I picked numeric methods that have two operands and Self somewhere in the return type, but we could limit that to only methods that are variants of an ops trait, or expand it cover to all numeric methods that take self by value and return a new Self.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

ping from triage @KodrAus any updates on this? can you get someone from the team to review it?

@KodrAus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@rust-highfive rust-highfive assigned SimonSapin and unassigned Kimundi Apr 23, 2019

@KodrAus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Sorry @SimonSapin I don't mean to be assigning you all the PRs today :)

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned SimonSapin Apr 23, 2019

@sfackler

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

📌 Commit 23154db has been approved by sfackler

kennytm added a commit to kennytm/rust that referenced this pull request Apr 23, 2019

Rollup merge of rust-lang#59839 - KodrAus:must-use-num, r=sfackler
Warn on unused results for operation methods on nums

From a suggestion by @llogiq

Adds a `#[must_use]` attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating `self`, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

```rust
struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}
```

Will produce a warning like:

```
warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original
```

If this is something we're on board with, we could do something similar for `f32` and `f64` too. There are probably other methods on integers that make sense.

bors added a commit that referenced this pull request Apr 23, 2019

Auto merge of #60189 - kennytm:rollup-8xgg35r, r=kennytm
Rollup of 6 pull requests

Successful merges:

 - #59839 (Warn on unused results for operation methods on nums)
 - #60146 (Update fonts used by rustdoc)
 - #60169 (Warn when ignore-tidy-linelength is present, but no lines are too long)
 - #60172 (Disallow double trailing newlines in tidy)
 - #60177 (Promote rust comments to rustdoc)
 - #60180 (Update cargo, books)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019

Rollup merge of rust-lang#59839 - KodrAus:must-use-num, r=sfackler
Warn on unused results for operation methods on nums

From a suggestion by @llogiq

Adds a `#[must_use]` attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating `self`, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

```rust
struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}
```

Will produce a warning like:

```
warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original
```

If this is something we're on board with, we could do something similar for `f32` and `f64` too. There are probably other methods on integers that make sense.

Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019

Rollup merge of rust-lang#59839 - KodrAus:must-use-num, r=sfackler
Warn on unused results for operation methods on nums

From a suggestion by @llogiq

Adds a `#[must_use]` attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating `self`, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

```rust
struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}
```

Will produce a warning like:

```
warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original
```

If this is something we're on board with, we could do something similar for `f32` and `f64` too. There are probably other methods on integers that make sense.

bors added a commit that referenced this pull request Apr 23, 2019

Auto merge of #60208 - Centril:rollup-vg8gl67, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #59739 (Stabilize futures_api)
 - #59839 (Warn on unused results for operation methods on nums)
 - #60146 (Update fonts used by rustdoc)
 - #60169 (Warn when ignore-tidy-linelength is present, but no lines are too long)
 - #60177 (Promote rust comments to rustdoc)
 - #60191 (Add f16c target_feature)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019

Rollup merge of rust-lang#59839 - KodrAus:must-use-num, r=sfackler
Warn on unused results for operation methods on nums

From a suggestion by @llogiq

Adds a `#[must_use]` attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating `self`, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

```rust
struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}
```

Will produce a warning like:

```
warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original
```

If this is something we're on board with, we could do something similar for `f32` and `f64` too. There are probably other methods on integers that make sense.

Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019

Rollup merge of rust-lang#59839 - KodrAus:must-use-num, r=sfackler
Warn on unused results for operation methods on nums

From a suggestion by @llogiq

Adds a `#[must_use]` attribute to operation methods on integers that take self by value as the first operand and another value as the second. It makes it clear that these methods return the result of the operation instead of mutating `self`, which is the source of a rather embarrassing bug I had in a codebase of mine recently...

As an example:

```rust
struct Int {
   value: i64,
}

impl Int {
    fn add(&mut self, other: i64) {
        self.value.wrapping_add(other);
    }
}
```

Will produce a warning like:

```
warning: unused return value of `core::num::<impl i64>::wrapping_add` that must be used
 --> src/main.rs:7:7
  |
7 |       self.value.wrapping_add(other);
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_must_use)] on by default
  = note: this returns the result of the operation, without modifying the original
```

If this is something we're on board with, we could do something similar for `f32` and `f64` too. There are probably other methods on integers that make sense.

bors added a commit that referenced this pull request Apr 23, 2019

Auto merge of #60211 - Centril:rollup-akw4r85, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #59823 ([wg-async-await] Drop `async fn` arguments in async block )
 - #59839 (Warn on unused results for operation methods on nums)
 - #60146 (Update fonts used by rustdoc)
 - #60169 (Warn when ignore-tidy-linelength is present, but no lines are too long)
 - #60177 (Promote rust comments to rustdoc)
 - #60191 (Add f16c target_feature)

Failed merges:

r? @ghost

@bors bors merged commit 23154db into rust-lang:master Apr 23, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.