Skip to content

Make String a must_use type #75934

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion library/alloc/src/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//! Some examples of the [`format!`] extension are:
//!
//! ```
//! # #![allow(unused_must_use)]
//! format!("Hello"); // => "Hello"
//! format!("Hello, {}!", "world"); // => "Hello, world!"
//! format!("The number is {}", 1); // => "The number is 1"
Expand Down Expand Up @@ -44,7 +45,7 @@
//! the iterator advances. This leads to behavior like this:
//!
//! ```
//! format!("{1} {} {0} {}", 1, 2); // => "2 1 1 2"
//! println!("{1} {} {0} {}", 1, 2); // => "2 1 1 2"
//! ```
//!
//! The internal iterator over the argument has not been advanced by the time
Expand All @@ -71,6 +72,7 @@
//! For example, the following [`format!`] expressions all use named argument:
//!
//! ```
//! # #![allow(unused_must_use)]
//! format!("{argument}", argument = "test"); // => "test"
//! format!("{name} {}", 1, name = 2); // => "2 1"
//! format!("{a} {c} {b}", a="a", b='b', c=3); // => "a 3 b"
Expand Down
1 change: 1 addition & 0 deletions library/alloc/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ macro_rules! vec {
/// # Examples
///
/// ```
/// # #![allow(unused_must_use)]
/// format!("test");
/// format!("hello {}", "world!");
/// format!("x = {}, y = {y}", 10, y = 30);
Expand Down
1 change: 1 addition & 0 deletions library/alloc/src/str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,7 @@ impl str {
/// A panic upon overflow:
///
/// ```should_panic
/// # #![allow(unused_must_use)]
/// // this will panic at runtime
/// "0123456789abcdef".repeat(usize::MAX);
/// ```
Expand Down
3 changes: 3 additions & 0 deletions library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ use crate::vec::Vec;
#[derive(PartialOrd, Eq, Ord)]
#[cfg_attr(not(test), rustc_diagnostic_item = "string_type")]
#[stable(feature = "rust1", since = "1.0.0")]
#[must_use = "unused allocation might not be optimized out by compiler"]
pub struct String {
vec: Vec<u8>,
}
Expand Down Expand Up @@ -949,6 +950,7 @@ impl String {
/// # Examples
///
/// ```
/// # #![allow(unused_must_use)]
/// #![feature(try_reserve)]
/// use std::collections::TryReserveError;
///
Expand Down Expand Up @@ -987,6 +989,7 @@ impl String {
/// # Examples
///
/// ```
/// # #![allow(unused_must_use)]
/// #![feature(try_reserve)]
/// use std::collections::TryReserveError;
///
Expand Down
6 changes: 3 additions & 3 deletions library/core/tests/fmt/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ mod debug_map {
}
}

format!("{:?}", Foo);
println!("{:?}", Foo);
}

#[test]
Expand All @@ -460,7 +460,7 @@ mod debug_map {
}
}

format!("{:?}", Foo);
println!("{:?}", Foo);
}

#[test]
Expand All @@ -474,7 +474,7 @@ mod debug_map {
}
}

format!("{:?}", Foo);
println!("{:?}", Foo);
}
}

Expand Down
2 changes: 1 addition & 1 deletion library/core/tests/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ fn test_unwrap_panic1() {
#[should_panic]
fn test_unwrap_panic2() {
let x: Option<String> = None;
x.unwrap();
drop(x.unwrap());
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ impl DocFolder for Cache {
});

if pushed {
self.stack.pop().expect("stack already empty");
let _ = self.stack.pop().expect("stack already empty");
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 lines like this are a good example of why String and Vec<T> aren't must_use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These cases are rare in Rustc codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @scottmcm here. If it's on String, then it should be on Vec too, and if it's on Vec, then it should also be on every container. At which point you'd have to somewhat argue why it's not on basically everything that's not Copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With all respects, I just want some actions on resolving #75742 . If the team doesn't like this approach,
what about marking String::from, str::into, fmt::format with must_use attr?

}
if parent_pushed {
self.parent_stack.pop().expect("parent stack already empty");
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/block-result/consider-removing-last-semi.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pub fn f() -> String { //~ ERROR mismatched types
"bla".to_string()
}

#[allow(unused_must_use)]
pub fn g() -> String { //~ ERROR mismatched types
"this won't work".to_string();
"removeme".to_string()
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/block-result/consider-removing-last-semi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pub fn f() -> String { //~ ERROR mismatched types
"bla".to_string();
}

#[allow(unused_must_use)]
pub fn g() -> String { //~ ERROR mismatched types
"this won't work".to_string();
"removeme".to_string();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ LL | "bla".to_string();
| - help: consider removing this semicolon

error[E0308]: mismatched types
--> $DIR/consider-removing-last-semi.rs:8:15
--> $DIR/consider-removing-last-semi.rs:9:15
|
LL | pub fn g() -> String {
| - ^^^^^^ expected struct `std::string::String`, found `()`
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/deriving/deriving-in-fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ pub fn main() {
}

let f = Foo { foo: 10 };
format!("{:?}", f);
println!("{:?}", f);
}
4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-2063.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ impl ToStr2 for T {
}

#[allow(dead_code)]
fn new_t(x: T) {
x.my_to_string();
fn new_t(x: T) -> String {
x.my_to_string()
}

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-20676.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ use std::fmt;

fn main() {
let a: &dyn fmt::Debug = &1;
format!("{:?}", a);
println!("{:?}", a);
}
1 change: 1 addition & 0 deletions src/test/ui/istr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ fn test_stack_assign() {
assert!((s != u));
}

#[allow(unused_must_use)]
fn test_heap_lit() { "a big string".to_string(); }

fn test_heap_assign() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/nll/issue-61424.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
fn main() {
let x; //~ ERROR: variable does not need to be mutable
x = String::new();
dbg!(x);
let _ = dbg!(x);
}
2 changes: 1 addition & 1 deletion src/test/ui/nll/issue-61424.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
fn main() {
let mut x; //~ ERROR: variable does not need to be mutable
x = String::new();
dbg!(x);
let _ = dbg!(x);
}
2 changes: 1 addition & 1 deletion src/test/ui/overloaded/overloaded-deref-count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn main() {

// Immutable deref used for calling a method taking &self. (The
// typechecker is smarter now about doing this.)
(*n).to_string();
let _ = (*n).to_string();
assert_eq!(n.counts(), (3, 3));

// Mutable deref used for calling a method taking &mut self.
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ pub fn main() {
// for example `clippy-driver --rustc --version` will print the rustc version that clippy-driver
// uses
if let Some(pos) = orig_args.iter().position(|arg| arg == "--rustc") {
orig_args.remove(pos);
drop(orig_args.remove(pos));
orig_args[0] = "rustc".to_string();

// if we call "rustc", we need to pass --sysroot here as well
Expand All @@ -372,7 +372,7 @@ pub fn main() {

if wrapper_mode {
// we still want to be able to invoke it normally though
orig_args.remove(1);
drop(orig_args.remove(1));
}

if !wrapper_mode && (orig_args.iter().any(|a| a == "--help" || a == "-h") || orig_args.len() == 1) {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/format.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix

#![allow(clippy::print_literal, clippy::redundant_clone)]
#![allow(unused_must_use, clippy::print_literal, clippy::redundant_clone)]
#![warn(clippy::useless_format)]

struct Foo(pub String);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/format.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix

#![allow(clippy::print_literal, clippy::redundant_clone)]
#![allow(unused_must_use, clippy::print_literal, clippy::redundant_clone)]
#![warn(clippy::useless_format)]

struct Foo(pub String);
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/question_mark.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ impl MoveStruct {
}

fn func() -> Option<i32> {
fn f() -> Option<String> {
Some(String::new())
fn f() -> Option<u32> {
Some(42)
}

f()?;
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ impl MoveStruct {
}

fn func() -> Option<i32> {
fn f() -> Option<String> {
Some(String::new())
fn f() -> Option<u32> {
Some(42)
}

if f().is_none() {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/redundant_clone.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn main() {

with_branch(Alpha, true);
cannot_double_move(Alpha);
cannot_move_from_type_with_drop();
drop(cannot_move_from_type_with_drop());
borrower_propagation();
not_consumed();
issue_5405();
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/redundant_clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn main() {

with_branch(Alpha, true);
cannot_double_move(Alpha);
cannot_move_from_type_with_drop();
drop(cannot_move_from_type_with_drop());
borrower_propagation();
not_consumed();
issue_5405();
Expand Down