Hi, I frequently call Array::map, and occasionally find that I will map an array, and forget to use the output when the underlying type is Copy. I was thinking it makes sense to annotate Array::map with #[must_use], which would at least throw a warning if the map is not consumed.
I don't know if this is the right forum for asking this question, but if willing I can submit a PR quickly for this.
This is the right place to ask.
I think this is reasonable, although there are differing opinions on how much stuff to mark #[must_use].
In this case map couldn't technically be implemented as a &mut self method (it could not change the type if it was), but this may not be immediately obvious to everyone, and there is no meaningful way to use it without using the result. ↩︎
Ah great.
I think it's very similar to the case of iterators, where I am doing something like
let a = [....;3];
a.map(|v| v / 0.5); // forgot to reassign to a
Should I wait for some kind of consensus or when is appropriate to submit a PR?
The one difference to Iterator::map() is that array::map() is not lazy.
E.g.
let a = [1, 2, 3, 4];
a.map(|i| println!("{i}"));
works and prints four lines, while
let a = [1, 2, 3, 4];
a.into_iter().map(|i| println!("{i}"));
does nothing.
It's not very idiomatic to use array::map() that way, but it's possible and that might be one reason why it isn't marked #[must_use] yet.
I think that would be a good reason to add array::for_each, then let map be must_use. However, the map docs also say "[T; N]::map is only necessary if you really need a new array of the same size as the result," which would not be the same situation with for_each.
I'd rather see:
let _ = a.map(|i| println!("{i}"));
or
drop(a.map(|i| println!("{i}")));
Basically the same argument holds for almost all functions that return something: yes, sometimes you may want to ignore the result, but you can easily do so explicitly and it's more readable than having it dropped silently. So I think more-less, perhaps with a few exceptions, all functions that return something should be marked #[must_use].
In fact I'd prefer it if every expression statement was required to be of type (), and otherwise it would just be a compile error, which would make #[must_use] obsolete.
I feel pretty strongly against adding Array::for_each, at that point it the array should be directly used in a for loop or converted to an iterator, unless there's something I'm missing.
Certainly there are some situations where every output should be consumed, but I think pragmatically that would be a huge pain. Why I think that Array::map should have #[must_use] is because pragmatically it is not meant to produce side effects (every situation I'm using it in is a transformation of the input, generally vectors in 3D/2D). With both iterators and arrays, it is likely a bug if the output is not consumed, although the actual iterated values won't actually be accessed in the case of iterators.
Thanks! I will probably turn this on in my projects.
But I feel that every time I change such a setting from the default I'm creating a new variant of the language, creating a split. When I switch to reading another project I will have to remember that implicit dropping is possible again. So there is value in making certain things enforced as hard errors rather than configurable lints.
This lint is "allow" by default because it can be noisy, and may not be an actual problem. For example, calling the
removemethod of aVecorHashMapreturns the previous value, which you may not care about. Using this lint would require explicitly ignoring or discarding such values.
Maybe methods that are meant to sometimes be ignored, like Vec::remove, should be marked with something like #[may_ignore]. Then the unused_results lint could be promoted to a warning, and in a later edition maybe ignoring a #[must_use] thing could even be an error by default (doing this in another edition would at least avoid breaking crates that aren't maintained anymore).
The only trouble is that many crates would also like to mark some types and/or functions with #[may_ignore] (but probably only after this attribute stabilized), so enabling warnings for unused_results right away could add too much noise.
You might be interested in this recent thread:
As a former Go programmer: I don't think it's worth it.
being overzealous with errors causes programmers to be overzealous with workarounds, possibly disabling the error even when it identifies an actual error.
i much prefer rust's lint system.
But unused_results is an allow-by-default lint, so you don't even get a warning. At the very least I think it should be a warn-by-default. I don't see how it's overzealous: I'd much rather know about ignored results. The workaround is to explicitly ignore the result you want to ignore, but that seems like the right thing to do in most such cases rather than overzealous.
@scottmcm says:
Maybe it's just because it's allow by default, so people either don't know about it or don't want to change the rules from the default?
unused_results fires even on things like
let mut echo = Command::new("sh");
echo.arg("-c");
if foo() {
echo.arg("echo hello");
} else {
echo.arg("echo bye");
}
let out = echo.output().expect("failed to execute process");
where changing this to
let mut echo = Command::new("sh");
let _ = echo.arg("-c");
if foo() {
let _ = echo.arg("echo hello");
} else {
let _ = echo.arg("echo bye");
}
let out = echo.output().expect("failed to execute process");
to suppress the warning doesn't make things better. If anything, it makes things uglier.
This is why I think this builder pattern where you mutate something and return &mut self is an antipattern. This is essentially returning the same object in two different ways at the same time. A method should either mutate in place or return self, not both, and if you want to return self it should be passed by value not by reference.
Given that this confusing API exists, I still prefer the second way of calling it to be explicit about what is happening.
Having a #[not_must_use] (subject to name bikeshedding, which doesn't interest me) would solve this. Allowing specific methods to opt out of the behaviour. This is different than #[allow(...)] or let _ = ... since that would be per call site. I think both are needed for this to be usable in practice.
Command::arg should be #[MayIgnore] and simultaneously its argument &mut self should be #[NotCountedAsUsed] (or whatever is the best way to spell it), so that if you forgot to call Command::output the variable echo will be flagged as unused.
I like the idea of NotCountedAsUsed. It could possibly also help with this issue I filed a long time ago: Rust should warn for unused "pure" objects · Issue #127685 · rust-lang/rust · GitHub
Unfortunately there is no other way to allow chaining without requiring it. Chaining is very convenient in most cases, but there are some cases where not chaining is a much better option.
There is:
struct Command;
impl Command {
fn new(s: &str) -> Self {
Self
}
fn arg(self, s: &str) -> Self {
self
}
fn output(self) -> i32 {
7
}
}
fn main() {
let mut echo = Command::new("sh").arg("-c").arg("-d").arg("-e");
if true {
echo = echo.arg("echo hello");
} else {
echo = echo.arg("echo bye");
}
let out = echo.output();
println!("{out}");
}