Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/liballoc/str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ impl str {
///
/// assert_eq!(new_year, new_year.to_lowercase());
/// ```
#[must_use]
#[stable(feature = "unicode_case_mapping", since = "1.2.0")]
pub fn to_lowercase(&self) -> String {
let mut s = String::with_capacity(self.len());
Expand Down Expand Up @@ -368,6 +369,7 @@ impl str {
///
/// assert_eq!(new_year, new_year.to_uppercase());
/// ```
#[must_use]
#[stable(feature = "unicode_case_mapping", since = "1.2.0")]
pub fn to_uppercase(&self) -> String {
let mut s = String::with_capacity(self.len());
Expand All @@ -378,6 +380,7 @@ impl str {
/// Escapes each char in `s` with [`char::escape_debug`].
///
/// [`char::escape_debug`]: primitive.char.html#method.escape_debug
#[must_use]
#[unstable(feature = "str_escape",
reason = "return type may change to be an iterator",
issue = "27791")]
Expand All @@ -388,6 +391,7 @@ impl str {
/// Escapes each char in `s` with [`char::escape_default`].
///
/// [`char::escape_default`]: primitive.char.html#method.escape_default
#[must_use]
#[unstable(feature = "str_escape",
reason = "return type may change to be an iterator",
issue = "27791")]
Expand All @@ -398,6 +402,7 @@ impl str {
/// Escapes each char in `s` with [`char::escape_unicode`].
///
/// [`char::escape_unicode`]: primitive.char.html#method.escape_unicode
#[must_use]
#[unstable(feature = "str_escape",
reason = "return type may change to be an iterator",
issue = "27791")]
Expand All @@ -420,6 +425,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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -438,6 +444,7 @@ impl str {
/// ```
/// assert_eq!("abc".repeat(4), String::from("abcabcabcabc"));
/// ```
#[must_use]
#[stable(feature = "repeat_str", since = "1.16.0")]
pub fn repeat(&self, n: usize) -> String {
unsafe { String::from_utf8_unchecked(self.as_bytes().repeat(n)) }
Expand All @@ -464,6 +471,7 @@ impl str {
///
/// [`make_ascii_uppercase`]: #method.make_ascii_uppercase
/// [`to_uppercase`]: #method.to_uppercase
#[must_use]
#[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")]
#[inline]
pub fn to_ascii_uppercase(&self) -> String {
Expand Down Expand Up @@ -494,6 +502,7 @@ impl str {
///
/// [`make_ascii_lowercase`]: #method.make_ascii_lowercase
/// [`to_lowercase`]: #method.to_lowercase
#[must_use]
#[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")]
#[inline]
pub fn to_ascii_lowercase(&self) -> String {
Expand Down
6 changes: 6 additions & 0 deletions src/liballoc/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ impl String {
/// // ...but this may make the vector reallocate
/// s.push('a');
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn with_capacity(capacity: usize) -> String {
Expand All @@ -432,6 +433,7 @@ impl String {
// required for this method definition, is not available. Since we don't
// require this method for testing purposes, I'll just stub it
// NB see the slice::hack module in slice.rs for more information
#[must_use]
#[inline]
#[cfg(test)]
pub fn from_str(_: &str) -> String {
Expand Down Expand Up @@ -643,6 +645,7 @@ impl String {
/// assert_eq!(String::from("𝄞mus\u{FFFD}ic\u{FFFD}"),
/// String::from_utf16_lossy(v));
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn from_utf16_lossy(v: &[u16]) -> String {
Expand Down Expand Up @@ -690,6 +693,7 @@ impl String {
/// assert_eq!(String::from("hello"), s);
/// }
/// ```
#[must_use]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@frol frol May 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn from_raw_parts(buf: *mut u8, length: usize, capacity: usize) -> String {
Expand Down Expand Up @@ -724,6 +728,7 @@ impl String {
///
/// assert_eq!("💖", sparkle_heart);
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn from_utf8_unchecked(bytes: Vec<u8>) -> String {
Expand Down Expand Up @@ -1420,6 +1425,7 @@ impl String {
/// assert_eq!(world, "World!");
/// # }
/// ```
#[must_use]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

#[inline]
#[stable(feature = "string_split_off", since = "1.16.0")]
pub fn split_off(&mut self, at: usize) -> String {
Expand Down
4 changes: 2 additions & 2 deletions src/liballoc/tests/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,14 +244,14 @@ fn test_split_off_empty() {
fn test_split_off_past_end() {
let orig = "Hello, world!";
let mut split = String::from(orig);
split.split_off(orig.len() + 1);
let _ = split.split_off(orig.len() + 1);
}

#[test]
#[should_panic]
fn test_split_off_mid_char() {
let mut orig = String::from("山");
orig.split_off(1);
let _ = orig.split_off(1);
}

#[test]
Expand Down