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 compare_and_swap #52201

Closed
wants to merge 1 commit into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jul 10, 2018

compare_and_swap can fail if the value in memory has changed since it was last read by the writer. In these situations, users will usually want to retry or abort. While developers are usually aware of this aspect of CAS, the compiler might as well be helpful and remind the user if they forget to check if their swap actually occurred.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2018
@Mark-Simulacrum
Copy link
Member

r? @SimonSapin

@jonhoo jonhoo force-pushed the cas_must_use branch 2 times, most recently from 0091234 to a93141b Compare July 10, 2018 02:50
@rust-highfive

This comment has been minimized.

`compare_and_swap` can fail if the value in memory has changed since it
was last read by the writer. In these situations, users will usually
want to retry or abort. While developers are usually aware of this aspect
of CAS, the compiler might as well be helpful and remind the user if
they forget to check if their swap actually occurred.
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.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.
    100% |████████████████████████████████| 1.3MB 930kB/s 
Collecting botocore==1.10.54 (from awscli)
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/2c/84/ca1e66a4c87afdac3ca0dc720e3907e94526947d5094faf8704c0eedaa67/botocore-1.10.54-py2.py3-none-any.whl (4.4MB)
    0% |                                | 10kB 44.0MB/s eta 0:00:01
    0% |▏                               | 20kB 44.0MB/s eta 0:00:01
    0% |▎                               | 30kB 49.6MB/s eta 0:00:01
    0% |▎                               | 40kB 36.3MB/s eta 0:00:01
---
[00:04:24]    Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[00:04:25]    Compiling alloc_system v0.0.0 (file:///checkout/src/liballoc_system)
[00:04:25]    Compiling panic_abort v0.0.0 (file:///checkout/src/libpanic_abort)
[00:04:29]    Compiling panic_unwind v0.0.0 (file:///checkout/src/libpanic_unwind)
[00:04:34] warning: unused return value of `core::sync::atomic::AtomicUsize::compare_and_swap` which must be used
[00:04:34]    --> libstd/sync/mpsc/oneshot.rs:187:21
[00:04:34]     |
[00:04:34] 187 |                     self.state.compare_and_swap(DATA, EMPTY, Ordering::SeqCst);
[00:04:34]     |
[00:04:34]     = note: #[warn(unused_must_use)] on by default
[00:04:34]     = note: #[warn(unused_must_use)] on by default
[00:04:34]     = note: if compare_and_swap does not return `current`, the stored value was not changed
[00:04:41]     Finished release [optimized] target(s) in 44.01s
[00:04:41] Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
[00:04:41] travis_fold:end:stage0-std

---
[00:19:49]    Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[00:19:50]    Compiling alloc_system v0.0.0 (file:///checkout/src/liballoc_system)
[00:19:50]    Compiling panic_abort v0.0.0 (file:///checkout/src/libpanic_abort)
[00:19:55]    Compiling panic_unwind v0.0.0 (file:///checkout/src/libpanic_unwind)
[00:20:02] error: unused return value of `core::sync::atomic::AtomicUsize::compare_and_swap` which must be used
[00:20:02]    --> libstd/sync/mpsc/oneshot.rs:187:21
[00:20:02]     |
[00:20:02] 187 |                     self.state.compare_and_swap(DATA, EMPTY, Ordering::SeqCst);
[00:20:02]     |
[00:20:02]     = note: `-D unused-must-use` implied by `-D warnings`
[00:20:02]     = note: `-D unused-must-use` implied by `-D warnings`
[00:20:02]     = note: if compare_and_swap does not return `current`, the stored value was not changed
[00:20:02] error: aborting due to previous error
[00:20:02] 
[00:20:02] error: Could not compile `std`.
[00:20:02] 
[00:20:02] 
[00:20:02] Caused by:
[00:20:02]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name std libstd/lib.rs --color always --error-format json --crate-type dylib --crate-type rlib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 --cfg feature="alloc_jemalloc" --cfg feature="backtrace" --cfg feature="jemalloc" --cfg feature="panic-unwind" --cfg feature="panic_unwind" -C metadata=5bc3f4baef690e48 -C extra-filename=-5bc3f4baef690e48 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps --extern alloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/liballoc-258b2707ebbce886.rlib --extern alloc_jemalloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/liballoc_jemalloc-d9a4aa4e4efab0a9.rlib --extern alloc_system=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/liballoc_system-123fd80138fcab30.rlib --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcompiler_builtins-86466043f402dfc1.rlib --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu//obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib
70300 ./obj/build/x86_64-unknown-linux-gnu/native/jemalloc
68792 ./src/llvm/lib
65420 ./src/llvm-emscripten/test/CodeGen
65160 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu

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)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 10, 2018

And it seems the compiler actually has a case where it's not checked 😉

@kennytm
Copy link
Member

kennytm commented Jul 10, 2018

Isn't all #[must_use] changes blocked by #48926

@oli-obk
Copy link
Contributor

oli-obk commented Jul 10, 2018

The function could indeed be const fn and thus automatically cause must_use warnings

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 10, 2018

@oli-obk I could make the functions const (and maybe that's separately a good change), but I think having the reason in the #[must_use] might actually be helpful. What do you think?

@kennytm
Copy link
Member

kennytm commented Jul 10, 2018

The block by #48926 has nothing to do with const fn. At least const fn isn't the reason why #50462 is blocked.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 11, 2018

Right, but in this specific case, the function can be made const fn, and if the "all const fns with unused results are warned about as must_use" RFC is merged, then it'll automatically lint about that.

@hanna-kruppe
Copy link
Contributor

Given that @est31 abandoned that RFC and more generally stated they wouldn't contribute to Rust any more, it seems a little silly to block on that. If someone else wants to pick up the matter, that would be cool of course.

Also note that various restrictions on that lint to reduce the number of false positives might prevent a const fn based lint from applying to this function. For example, the last draft of the RFC didn't apply the lint to any functions taking references to types with interior mutability, and @oli-obk themselves suggested only linting promotable const fns (which atomics probably wouldn't fall under).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 11, 2018

So yea... I guess it's blocked on #48926 for now.

closing until that discussion comes to a consensus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants