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

[wip] library: mark more functions that return String as #[must_use] #50462

Closed
wants to merge 1 commit into from

Conversation

@matthiaskrgr
Copy link
Contributor

matthiaskrgr commented May 5, 2018

   library: mark more functions that return String as #[must_use]
    If the return value (String) of the function is not used, the function call is technically in vain
    (unless there are side effects) and the entire function call could be ommitted.
    Warn about unused return values of -> String functions.
    Example code:
       let mut x = Sting::from("hello");
       x.push_str(" world!");
       x.to_uppercase();
       println!("{}", x);
    will print "hello world!" instead of "HELLO WORLD!") because the result of .to_uppercase() is not caught
    in a variable.
    std::str::to_lowercase()
    std::str::to_uppercase()
    std::str::escape_debug()
    std::str::escape_default()
    std::str::escape_unicode()
    std::str::into_string()
    std::str::repeat()
    std::str::to_ascii_uppercase()
    std::str::to_ascii_lowercase()
    std::String::with_capacity()
    std::String::from_str()
    std::String::from_utf16_lossy()
    std::String::from_raw_parts()
    std::String::from_utf8_unchecked()
    std::String::split_off()

TODO:

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 5, 2018

r? @shepmaster

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

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 5, 2018

The job x86_64-gnu-llvm-3.9 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.
Testing alloc stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:10:15]    Compiling libc v0.2.40
[01:10:16]    Compiling rand v0.4.2
[01:10:19]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[01:10:30] error: unused return value of `std::string::String::split_off` which must be used
[01:10:30]     |
[01:10:30]     |
[01:10:30] 247 |     split.split_off(orig.len() + 1);
[01:10:30]     |
[01:10:30]     = note: `-D unused-must-use` implied by `-D warnings`
[01:10:30] 
[01:10:30] 
[01:10:30] error: unused return value of `std::string::String::split_off` which must be used
[01:10:30]     |
[01:10:30]     |
[01:10:30] 254 |     orig.split_off(1);
[01:10:30] 
[01:10:30] error: aborting due to 2 previous errors
[01:10:30] 
[01:10:30] error: Could not compile `alloc`.
[01:10:30] error: Could not compile `alloc`.
[01:10:30] 
[01:10:30] To learn more, run the command again with --verbose.
[01:10:30] 
[01:10:30] 
[01:10:30] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--" "--quiet"
[01:10:30] 
[01:10:30] 
[01:10:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:10:30] Build completed unsuccessfully in 0:26:40
[01:10:30] Build completed unsuccessfully in 0:26:40
[01:10:30] Makefile:58: recipe for target 'check' failed
[01:10:30] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2a7ba538
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
60840 ./src/llvm-emscripten/lib
56092 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/bin
55380 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release
53660 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/incremental/syntax-33ta18b3panbi
53656 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/incremental/syntax-33ta18b3panbi/s-f0r1w561vd-1iks737-9sj2cmggjcjg
48604 ./obj/build/x86_64-unknown-linux-gnu/stage0/bin
47892 ./obj/build/x86_64-unknown-linux-gnu/stage0-std
47052 ./src/test
46720 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/release

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)

@matthiaskrgr matthiaskrgr changed the title [wip] library: mark more functions that return String as #[must_use] library: mark more functions that return String as #[must_use] May 5, 2018

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented May 5, 2018

Could you provide some rationale for this change? I'm too lazy to look for it now, but I seem to recall there being discussions around why we shouldn't add #[must_use] to every method in the standard library.

@matthiaskrgr

This comment has been minimized.

Copy link
Contributor Author

matthiaskrgr commented May 5, 2018

Ah, I didn't not know about this discussion.
Anyway, this is similar to #50177

The idea is, if a user uses code like

    let x = String::from("Hello world");
    x.to_uppercase();
    println!("{}", x);

,the x.to_uppercase(); is essentially dead code (unless there are side effects and since the String is not modified inplace) however there is no warning about it.

These functions that return a new String should have their return value captured, otherwise the operation has no effect and might lead to confusion.

@@ -2142,6 +2150,7 @@ pub trait ToString {
/// since `fmt::Write for String` never returns an error itself.
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: fmt::Display + ?Sized> ToString for T {
#[must_use]

This comment has been minimized.

Copy link
@ollie27

ollie27 May 5, 2018

Contributor

The #[must_use] attribute needs to be in the trait definition. It doesn't work when added to impls: #48486.

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented May 6, 2018

(Discussion issue for #[must_use] functions in the standard library is #48926.)

@frol

This comment has been minimized.

Copy link

frol commented May 6, 2018

Could you provide some rationale for this change?

@shepmaster I have just recently seen an answer on Reddit which made this kind of mistake using .to_uppercase() (the comment is already removed), so it seems worthwhile to me to be warned by a compiler given it is already possible to do that.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented May 6, 2018

I'd also prefer that the rationale go into the commit message. Commit messages that duplicate the diff are mostly useless because we can always look at the diff again; we can't look back at the author's rationale.

I'd also recommend squashing the commits, once the review feedback is finished.

@matthiaskrgr

This comment has been minimized.

Copy link
Contributor Author

matthiaskrgr commented May 6, 2018

sure, will do

@matthiaskrgr matthiaskrgr force-pushed the matthiaskrgr:must_use_strings branch from 03b98d5 to efe47ba May 6, 2018

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 6, 2018

The job x86_64-gnu-llvm-3.9 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.
[00:03:13]    Compiling rustc_tsan v0.0.0 (file:///checkout/src/librustc_tsan)
[00:03:29]    Compiling libc v0.0.0 (file:///checkout/src/rustc/libc_shim)
[00:03:29]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[00:03:29]    Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[00:03:29] error[E0518]: attribute should be applied to function
[00:03:29]      |
[00:03:29] 2184 |   #[inline]
[00:03:29]      |   ^^^^^^^^^
[00:03:29]      |   ^^^^^^^^^
[00:03:29] 2185 |   #[stable(feature = "string_to_string_specialization", since = "1.17.0")]
[00:03:29] 2186 | / impl ToString for String {
[00:03:29] 2187 | |     #[must_use]
[00:03:29] 2188 | |     fn to_string(&self) -> String {
[00:03:29] 2190 | |     }
[00:03:29] 2191 | | }
[00:03:29] 2191 | | }
[00:03:29]      | |_- not a function
[00:03:30]    Compiling alloc_system v0.0.0 (file:///checkout/src/liballoc_system)
[00:03:30]    Compiling panic_abort v0.0.0 (file:///checkout/src/libpanic_abort)
[00:03:32] error: aborting due to previous error
[00:03:32] 
[00:03:32] 
[00:03:32] For more information about this error, try `rustc --explain E0518`.
[00:03:32] error: Could not compile `alloc`.
[00:03:32] 
[00:03:32] Caused by:
[00:03:32]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name alloc liballoc/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=3 -C metadata=5c6ca57f52fc716b -C extra-filename=-5c6ca57f52fc716b --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcore-fb1e36473ec4786e.rlib --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcompiler_builtins-bad063b3019d016c.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/build/compiler_builtins-af41331a61619951/out` (exit code: 101)
[00:03:34] error: build failed
[00:03:34] error: build failed
[00:03:34] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:34] expected success, got: exit code: 101
[00:03:34] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:34] travis_fold:end:stage0-std

[00:03:34] travis_time:end:stage0-std:start=1525618708642499454,finish=1525618743514838858,duration=34872339404


[00:03:34] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:34] Build completed unsuccessfully in 0:00:36
[00:03:34] make: *** [tidy] Error 1
[00:03:34] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:21378f00
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@@ -11,7 +11,7 @@
//! A UTF-8 encoded, growable string.
//!
//! This module contains the [`String`] type, a trait for converting
//! [`ToString`]s, and several error types that may result from working with
//! [`ToString`]s, and several error types that may result from working withF

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

Looks like a typo snuck in.

@@ -689,6 +693,7 @@ impl String {
/// assert_eq!(String::from("hello"), s);
/// }
/// ```
#[must_use]

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

I think I'd disagree with this one — this is used for FFI when passing back a decomposed String to deallocate it.

This comment has been minimized.

Copy link
@frol

frol May 6, 2018

Wouldn't the code be more readable if one is forced to use drop() explicitly if the only intention is to deallocate?

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

Not in my opinion, no. Idiomatic Rust doesn't tend to use explicit calls to drop frequently.

This comment has been minimized.

Copy link
@frol

frol May 6, 2018

I am just a stranger here, but I have some experience with Python, which mantra is "explicit is better than implicit".

Idiomatic Rust doesn't tend to use unsafe blocks frequently, either. I would even say that explicitness is even more valuable in unsafe code.

#[stable(feature = "string_to_string_specialization", since = "1.17.0")]
impl ToString for String {
#[inline]

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

This should not be changed.

@@ -1419,6 +1425,7 @@ impl String {
/// assert_eq!(world, "World!");
/// # }
/// ```
#[must_use]

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

This function does have side effects (&mut self) — why should the user need to care about the return value here? What should they use instead of ignoring the result?

This comment has been minimized.

Copy link
@frol

frol May 6, 2018

If I ignore the result of split_off, it seems that I should better use truncate, shouldn't I?

This comment has been minimized.

Copy link
@shepmaster

shepmaster May 6, 2018

Member

Is there a way to customize the error message provided by #[must_use] on a case-by-case basis to suggest that truncate is more appropriate? Otherwise, should that go into the documentation?

This comment has been minimized.

Copy link
@zackmdavis

zackmdavis May 6, 2018

Member

The attribute can take a string literal with an additional message: usage, output. (The output format may change soon.)

This comment has been minimized.

Copy link
@scottmcm

scottmcm May 7, 2018

Member

This one I like provided it has the message. It sets a nice "if it allows redirecting to a cheaper method" precedent (which, in retrospect, collect is also following).

(Should probably be on Vec::split_off too, though.)

@@ -418,6 +423,7 @@ impl str {
///
/// assert_eq!(boxed_str.into_string(), string);
/// ```
#[must_use]
#[stable(feature = "box_str", since = "1.4.0")]
#[inline]
pub fn into_string(self: Box<str>) -> String {

This comment has been minimized.

Copy link
@scottmcm

scottmcm May 7, 2018

Member

This one feels insufficiently justified. It's not allocating. It consumes the input so if you didn't need it you'll probably know. It does almost nothing, so will probably disappear on its own if unused.

I agree that it could be must_use, but as I understand it the goal isn't to put it everywhere possible.

This comment has been minimized.

Copy link
@frol

frol May 7, 2018

I don't understand why we should apply so hardly-defined rules to where must_use should be used and where it shouldn't. I suggest the following reasoning: if the function call doesn't make sense without consuming the result, it should be marked with must_use.

It does almost nothing, so will probably disappear on its own if unused.

Why does Rust warn about unused uses then? They do nothing and just "disappear" completely if not used.

@@ -378,6 +378,7 @@ impl String {
/// ```
/// let s = String::new();
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new() -> String {

This comment has been minimized.

Copy link
@scottmcm

scottmcm May 7, 2018

Member

This one's another case of a trivial method where, sure, not using it isn't ideal stylistically, but it's just setting 3 pointers to constants, so overall I feel "meh".

Edit: Also, it's inconsistent for String::new to have it but not Vec::new.

This comment has been minimized.

Copy link
@matthiaskrgr

matthiaskrgr May 10, 2018

Author Contributor

Ok, for now I removed #[must_use] from std::String::new()

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 7, 2018

#[must_use] takes a string argument (#[must_use = "reasons reasons"]). Please add some explanatory text to each one of these to help users know why the result should be used.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 7, 2018

I did this for a bunch of existing ones in #50511

@matthiaskrgr matthiaskrgr force-pushed the matthiaskrgr:must_use_strings branch from efe47ba to 42c4faf May 10, 2018

library: mark more functions that return String as #[must_use]
If the return value (String) of the function is not used, the function call is technically in vain
(unless there are side effects) and the entire function call could be ommitted.

Warn about unused return values of -> String functions.

Example code:

   let mut x = Sting::from("hello");
   x.push_str(" world!");
   x.to_uppercase();
   println!("{}", x);

will print "hello world!" instead of "HELLO WORLD!") because the result of .to_uppercase() is not caught
in a variable.

std::str::to_lowercase()
std::str::to_uppercase()
std::str::escape_debug()
std::str::escape_default()
std::str::escape_unicode()
std::str::into_string()
std::str::repeat()
std::str::to_ascii_uppercase()
std::str::to_ascii_lowercase()

std::String::with_capacity()
std::String::from_str()
std::String::from_utf16_lossy()
std::String::from_raw_parts()
std::String::from_utf8_unchecked()
std::String::split_off()

@matthiaskrgr matthiaskrgr force-pushed the matthiaskrgr:must_use_strings branch from 42c4faf to aa127d8 May 10, 2018

@matthiaskrgr matthiaskrgr changed the title library: mark more functions that return String as #[must_use] [wip] library: mark more functions that return String as #[must_use] May 10, 2018

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 14, 2018

Ping from triage @shepmaster! The author pushed some new commits.

@matthiaskrgr

This comment has been minimized.

Copy link
Contributor Author

matthiaskrgr commented May 14, 2018

This still WIP, I haven't had s lot of time in the recent days unfortunately.

@matthiaskrgr

This comment has been minimized.

Copy link
Contributor Author

matthiaskrgr commented May 15, 2018

I made a separate pull request for Vec::new() and String::new() here: #50766

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 28, 2018

Marking as blocked on #48926.

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jul 10, 2018

Ping from triage! Thanks for your PR, @matthiaskrgr. It looks like blocking issue / RFC will need some time before it is resolved, so we're closing this PR for now. Feel free to reopen it in the future.

@TimNN TimNN closed this Jul 10, 2018

@TimNN TimNN added S-blocked-closed and removed S-blocked labels Jul 10, 2018

@matthiaskrgr matthiaskrgr deleted the matthiaskrgr:must_use_strings branch Aug 28, 2018

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.