std::process::Command::spawn could benefit from [must_use]

[nickdavies]

I was writing some code that used Command::new for 3 commands that I wanted to run in a row and I accidentally copy/pasted code that used spawn instead of say output so my code silently kicked off all 3 commands at the same time and didn't wait for them to exit.

I feel like most cases that you call spawn you probably should use the return value for that so having must_use on either std::process::command::spawn or on std::process::Child would be an easy way to make it harder to mess up here.

Given the following code: (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=03f37b71d1c7c82b7f137b83d4800db5)

    Command::new("bash")
        .args(&["-c", "sleep 5; mkdir test"])
        .spawn()
        .expect("failed to run mkdir test");
        
    let result = Command::new("ls")
        .args(&["test"])
        .output()
        .expect("failed to run ls test");
        
    println!("{:?}", result);

The current output is:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 7.04s
     Running `target/debug/playground`

Output { status: ExitStatus(ExitStatus(512)), stdout: "", stderr: "ls: cannot access 'test': No such file or directory\n" }

Ideally the output should look like:

warning: unused return value of `child` that must be used
  --> src/main.rs:11:5
   |
11 |     ...... <real code here> ...
   |     ^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default

I would be happy to make the PR to make this change myself however I am not sure if:

1. This change seems reasonable to others
2. If the attribute should be on the Child struct or just for this 1 function

I also wasn't 100% sure what the right category for this was so feel free to move it around.

[inquisitivecrystal]

I feel like this may be a bad idea, though I'm not entirely sure. In general, #[must_use] is reserved for that is almost certainly incorrect if the return value isn't used. Aren't there use cases for not using the return value here?

[BGR360]

Aren't there use cases for not using the return value here

Probably, like when you know you just want to spawn the subprocess in the background without waiting for it.

Maybe someone can make an argument that that's bad practice, but even still, if you add #[must_use], then that's a breaking change isn't it?

@rustbot label -T-compiler +T-libs

[inquisitivecrystal]

Aren't there use cases for not using the return value here

Probably, like when you know you just want to spawn the subprocess in the background without waiting for it.

Maybe someone can make an argument that that's bad practice, but even still, if you add #[must_use], then that's a breaking change isn't it?

It isn't strictly speaking a breaking change. unused_must_use is a warn-by-default lint, and adding one of those isn't generally considered breaking.

On the other hand, that's not the only factor to be considered. There's the user experience element. We don't want to even warn by default unless we think they're likely to be making a mistake, since overwhelming people with warnings makes the language less pleasant to use. Furthermore, if we start generating too many warnings, people may opt out, either of the specific warning (mildly unfortunate) or of all warnings in general (very unfortunate). Of course, the risks for any specific warning are low.

Now, it's worth mentioning that this particular lint has a relatively clean and idiomatic way of ignoring it in a specific case: let _ = command.spawn(). The standard here is probably a bit lower than it would be in many cases.

I'd guess the best thing to do is to nominate this for discussion by T-libs, so they can decide on whether the warning is helpful here.

[nickdavies]

Sorry for the slow reply here holidays got in the way!

Aren't there use cases for not using the return value here?

There are cases where you'd want to ignore this entirely however I would say they are reasonably rare and I think that the value of being prompted (by a warning) to consider the outcome of your process is probably a reasonable choice.

Probably, like when you know you just want to spawn the subprocess in the background without waiting for it.

Even in these situations I think you usually want to at least know the PID (to log or record it somewhere) but I guess that's not an warning condition.

We don't want to even warn by default unless we think they're likely to be making a mistake, since overwhelming people with warnings makes the language less pleasant to use. Furthermore, if we start generating too many warnings, people may opt out, either of the specific warning (mildly unfortunate) or of all warnings in general (very unfortunate). Of course, the risks for any specific warning are low.

I strongly agree with this. I wouldn't want to spam people if this is the normal, so I would love others thoughts on more use-cases for fire and forget style spawning situations. I am sure they exist but my guess is they are the exception rather than the norm. From my usages in the past I almost always want either:

• to wait for exit code
• stdin, stdout and/or stderr
• record the PID somewhere

Other examples from the existing libs

One thing I did after submitting this is compare to:

• thread::spawn
• tokio::spawn

which both feel like reasonable examples to compare against. Neither currently have must_use so perhaps there is precedent here for not adding this although I still feel like for things within the same process it's much more common to have some shared memory or channel to keep thing in sync with which you don't have in this case.

[m-ou-se]

Hi, thanks for reporting this issue.

This issue was very briefly discussed in the libs-api meeting today.

This spawn function doesn't fall within the guidelines for #[must_use] that we've been using. See also rust-lang/libs-team#35.

It's very reasonable to want to spawn a process without waiting for it to complete. We shouldn't force let _ = or something similar for that use case. We only apply #[must_use] in cases where we can be sure not using the value is a bug.

(Also note that spawn returns a Result, which is already #[must_use], so applying it to the function wouldn't help. It'd have to be on the Child type.)

Based on this, I'm closing this issue.