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

#[must_use = false] #3737

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link
Contributor

@lolbinarycat lolbinarycat commented Dec 7, 2024

Allow annotating functions with #[must_use = false] to suppress any warning generated due to its return type being marked as #[must_use].

Pre-RFC on IRLO

Rendered

@VitWW
Copy link

VitWW commented Dec 7, 2024

Maybe it is better to use positive logic. not negative and have #[may_unused] instead of #[must_use = false].
Or ... #[allow(dead_code)]

Your RFC number is lucky :)


Yet another "opt-in/opt-out" dance, similar to `#[non_exhaustive]`.

The usecase is fairly niche.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The usecase is fairly niche.
The use case is fairly niche.

@clarfonthey
Copy link
Contributor

I don't really like this approach because, well, error handling exists for a reason. If the error doesn't matter in a large number of cases, then you should just unwrap it or panic instead of returning it, not outright ignore it.

This also means that people who do want to be pedantic about errors no longer get notified if they forget to use them.

Basically, I think the solution should be to offer methods which don't return an error and then say what they do instead in their name (unchecked, panicking, etc.) rather than returning an error which can be ignored.

@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 7, 2024
@clarfonthey
Copy link
Contributor

Maybe it is better to use positive logic. not negative and have #[may_unused] instead of #[must_use = false]. Or ... #[allow(dead_code)]

Your RFC number is lucky :)

This isn't quite natural English, and honestly, the false flag makes the most sense; any other way of saying "this was marked as must use, but doesn't have to be used" is going to be longer to say and this feels very discoverable.

@joshtriplett
Copy link
Member

I don't really like this approach because, well, error handling exists for a reason. If the error doesn't matter in a large number of cases, then you should just unwrap it or panic instead of returning it, not outright ignore it.

I think it's probably a bad idea for things returning Result, but there may be other cases of a function returning type marked must_use and not wanting the function to be must_use.

It'd help to have an example other than Result.

@kornelski
Copy link
Contributor

This would be especially interesting if Rust supported making every function must_use by default, since in practice small minority of functions have a discardable result (as demonstrated by Swift's default and @discardableResult).

@clarfonthey
Copy link
Contributor

This would be especially interesting if Rust supported making every function must_use by default, since in practice small minority of functions have a discardable result (as demonstrated by Swift's default and @discardableResult).

This would mean that we must use (), a very common function return value, and I disagree. ;)

I do see where you're coming from, but I do think that it's very important to clarify what you mean by "a small minority of functions" because that's extremely difficult to quantify. I would hesitate to make any sweeping claims about this without actually searching through some kind of dataset.

lolbinarycat and others added 2 commits December 7, 2024 20:29
Co-authored-by: Josh Triplett <[email protected]>
@lolbinarycat
Copy link
Contributor Author

This would be especially interesting if Rust supported making every function must_use by default, since in practice small minority of functions have a discardable result (as demonstrated by Swift's default and @discardableResult).

This would mean that we must use (), a very common function return value, and I disagree. ;)

I do see where you're coming from, but I do think that it's very important to clarify what you mean by "a small minority of functions" because that's extremely difficult to quantify. I would hesitate to make any sweeping claims about this without actually searching through some kind of dataset.

() would almost certainly be the exception to this rule.

also, this was marked as a future possibility, either via editions or putting must_use on a module.

@lolbinarycat
Copy link
Contributor Author

I don't really like this approach because, well, error handling exists for a reason. If the error doesn't matter in a large number of cases, then you should just unwrap it or panic instead of returning it, not outright ignore it.

I think it's probably a bad idea for things returning Result, but there may be other cases of a function returning type marked must_use and not wanting the function to be must_use.

It'd help to have an example other than Result.

Generally this is useful for any must_use type defined in a different crate that is being returned from a function with side effects. Another example would be iterators.

@clarfonthey
Copy link
Contributor

Generally this is useful for any must_use type defined in a different crate that is being returned from a function with side effects. Another example would be iterators.

That's fair, although generally, iterator types aren't marked must_use; only functions returning iterators which do nothing unless used. For example, Vec::drain returns an iterator but doesn't mark it must_use, since it's a perfectly reasonable use case to simply drop the iterator instead of reading it.

I think that a good example would be one where a type is marked must_use for a good reason, and similarly, a return value would be worth marking as must_use = false. So far, I haven't found any compelling examples, and I just comment on the Result case, which feels wrong.

To be clear, I think that the feature is perfectly reasonable and well-defined; I just am not sure that it would actually solve a problem as much as it'd encourage people to ignore return values they really shouldn't.

@lolbinarycat
Copy link
Contributor Author

That's fair, although generally, iterator types aren't marked must_use; only functions returning iterators which do nothing unless used. For example, Vec::drain returns an iterator but doesn't mark it must_use, since it's a perfectly reasonable use case to simply drop the iterator instead of reading it.

That's an exception, if you look at any other iterator, such as std::ascii::EscapeDefualt, you will see it is the type that has the annotation, not the function.

I think that a good example would be one where a type is marked must_use for a good reason, and similarly, a return value would be worth marking as must_use = false. So far, I haven't found any compelling examples, and I just comment on the Result case, which feels wrong.

I feel like Result is the perfect example of this, though? It has very good reason to be marked as must_use, but there are some Result returning functions where the best option is almost always to log or ignore it. indeed, there are some times where an early return would usually be bad, there is no reason to bring down the whole program because of a non-fatal error.

"it feels wrong" is not very useful feedback.

To be clear, I think that the feature is perfectly reasonable and well-defined; I just am not sure that it would actually solve a problem as much as it'd encourage people to ignore return values they really shouldn't.

this is a tool for library authors, not really for applications. If an application author wants to ignore the unused_must_use lint, they can already do that.

@clarfonthey
Copy link
Contributor

That's fair, although generally, iterator types aren't marked must_use; only functions returning iterators which do nothing unless used. For example, Vec::drain returns an iterator but doesn't mark it must_use, since it's a perfectly reasonable use case to simply drop the iterator instead of reading it.

That's an exception, if you look at any other iterator, such as std::ascii::EscapeDefualt, you will see it is the type that has the annotation, not the function.

Right, I guess that it's weird that the iterator as marked this way instead of the method, but I wouldn't necessarily say that this is an "exception rather than the rule." If an adapter is only returned by one method, then it kind of doesn't matter whether you mark the type or the method, although it does effect downstream uses of that adapter. Perhaps that's a good reason to not mark the adapters as must_use, but the methods instead.

Like, yes, I acknowledge that this would solve this problem now without a change to the standard library, but considering how we're now proposing a language change, it feels worth considering whether a standard library change and an ecosystem change would be more fitting.

I think that a good example would be one where a type is marked must_use for a good reason, and similarly, a return value would be worth marking as must_use = false. So far, I haven't found any compelling examples, and I just comment on the Result case, which feels wrong.

I feel like Result is the perfect example of this, though? It has very good reason to be marked as must_use, but there are some Result returning functions where the best option is almost always to log or ignore it. indeed, there are some times where an early return would usually be bad, there is no reason to bring down the whole program because of a non-fatal error.

"it feels wrong" is not very useful feedback.

It's not just that it feels wrong: the point of returning a Result is to explicitly clarify that an error must be handled. If you think that a majority of your users won't want to handle the error, why return it at all? Plus, this case would prevent particularly pedantic users from ensuring that they do handle the error, and require an entirely separate lint for must_use_even_if_you_think_i_mustnt (name pending) to ensure that they do handle it.

To be clear, I think that the feature is perfectly reasonable and well-defined; I just am not sure that it would actually solve a problem as much as it'd encourage people to ignore return values they really shouldn't.

this is a tool for library authors, not really for applications. If an application author wants to ignore the unused_must_use lint, they can already do that.

I mean, this kind of feels like an either/or situation, since within each binary application is a library application too. I can see a lot of users choosing to add this to helper methods for their applications alongside the actual application (generally, you have more than just main) and so I think it's worth mentioning that case too.

The reason why I mention that case is because it feels much more likely of a use case: if only one application is using a function, then they can tailor the benefits to that one application. Whereas a library has to assume what all of its users want, which feels like a more difficult call to make.


Again, my point is: if a return value is not necessarily useful, why return it at all? If you're expecting users to ignore the entire return value, then it's likely that you have a Result<(), E> case, and you could completely sidestep the issue by using Option<E> instead. This also further conveys to the user that they might not care about the error if it's present, while still offering it.

The point here is that Result conveys not only the literal value of an error, but the semantic implication that the error should be handled as well, which is why it's must_use. If you don't want to have those attached semantics, the solution is to not use Result, not to add in a language feature overriding these semantics.

@lolbinarycat
Copy link
Contributor Author

Right, I guess that it's weird that the iterator as marked this way instead of the method, but I wouldn't necessarily say that this is an "exception rather than the rule." If an adapter is only returned by one method, then it kind of doesn't matter whether you mark the type or the method, although it does effect downstream uses of that adapter. Perhaps that's a good reason to not mark the adapters as must_use, but the methods instead.

Like, yes, I acknowledge that this would solve this problem now without a change to the standard library, but considering how we're now proposing a language change, it feels worth considering whether a standard library change and an ecosystem change would be more fitting.

my point was that you said most iterators had the annotation on the constructor, while the reality is contrary to this fact.

also, it's worth noting that lints are a compiler feature, not a language feature.

I feel like Result is the perfect example of this, though? It has very good reason to be marked as must_use, but there are some Result returning functions where the best option is almost always to log or ignore it. indeed, there are some times where an early return would usually be bad, there is no reason to bring down the whole program because of a non-fatal error.
"it feels wrong" is not very useful feedback.

It's not just that it feels wrong: the point of returning a Result is to explicitly clarify that an error must be handled. If you think that a majority of your users won't want to handle the error, why return it at all? Plus, this case would prevent particularly pedantic users from ensuring that they do handle the error, and require an entirely separate lint for must_use_even_if_you_think_i_mustnt (name pending) to ensure that they do handle it.

it has never been possible to force a Result to be used, let _ = is always a way of suppressing it. there is absolutely no need for an "opt back in" attribute, and suggesting so is silly.

The point of returning a Result in these situations is to allow error handling (or more likely, to allow logging of the error), but not to require it.

To be clear, I think that the feature is perfectly reasonable and well-defined; I just am not sure that it would actually solve a problem as much as it'd encourage people to ignore return values they really shouldn't.

this is a tool for library authors, not really for applications. If an application author wants to ignore the unused_must_use lint, they can already do that.

I mean, this kind of feels like an either/or situation, since within each binary application is a library application too. I can see a lot of users choosing to add this to helper methods for their applications alongside the actual application (generally, you have more than just main) and so I think it's worth mentioning that case too.

what sort of helper method are you imagining where ignoring the return value is

The reason why I mention that case is because it feels much more likely of a use case: if only one application is using a function, then they can tailor the benefits to that one application. Whereas a library has to assume what all of its users want, which feels like a more difficult call to make.

Again, my point is: if a return value is not necessarily useful, why return it at all? If you're expecting users to ignore the entire return value, then it's likely that you have a Result<(), E> case, and you could completely sidestep the issue by using Option<E> instead. This also further conveys to the user that they might not care about the error if it's present, while still offering it.

and if the Ok value of the Result is not ()?

The point here is that Result conveys not only the literal value of an error, but the semantic implication that the error should be handled as well, which is why it's must_use. If you don't want to have those attached semantics, the solution is to not use Result, not to add in a language feature overriding these semantics.

This is your interpretation of the type, but I have seen no evidence that this is the intent. If anything, this semantic seems to be a side effect of the must_use annotation on Result.

@kennytm

This comment was marked as resolved.

@marcusdesai
Copy link

marcusdesai commented Dec 12, 2024

I find the proposal unconvincing because I find the use-case unconvincing, and I just want to pick up on this statement to explain that:

This is your interpretation of the type, but I have seen no evidence that this is the intent. If anything, this semantic seems to be a side effect of the must_use annotation on Result.

My interpretation of a function returning Result is simply that the function might fail. The implication which follows from that is that the function could return an error that requires handling. Now, as a downstream user of APIs with functions returning Result, I'm already empowered to ignore this if I want, but also, I am humanly liable to forget or miss which functions might fail and which do not. So, must_use serves the important purpose of making that implication of possible failure explicit, which helps me write my code because I don't need to always remember to check such functions.

So, with that in mind, when I read the RFC's motivation:

The primary motivation is that there are some situations where a function is technically fallible, but it is almost always correct to not check its result.

I don't agree that this motivates must_use = false. Specifically: "a function is technically fallible, but it is almost always correct to not check" reads, to me, like a function which must be checked, precisely because it might fail, which is exactly what Result encodes. If such a function is annotated in a way that means the lack of a check is not flagged, then that could easily cause an issue. Personally, I don't really want library authors to be in the position of choosing whether I should be informed or reminded (by a lint) about a Result which might need using.

That said, if there are example use-cases where it is clear that a must_use value does not require use in any circumstance, then I'd find this proposal more convincing. At the same time, perhaps it would be better to just remove must_use from functions like that, if possible?

@felinira
Copy link

I did recently have a situation where this could have been useful. impl Future return values that are explicitly designed to be droppable.

Example

pub fn main() {
    must_not_use();
}

fn must_not_use() -> impl Future<Output = ()> {
    async {
        println!("this future does not have to be used");
    }
}

Some futures do not need to be used, such as various JoinHandle-like types. Implementing those kind of types yourself always requires an explicit wrapper type to be able to get rid of the must_use annotation, even when technically impl Future would have been enough.

@lolbinarycat
Copy link
Contributor Author

My interpretation of a function returning Result is simply that the function might fail. The implication which follows from that is that the function could return an error that requires handling.

and some classes of a error, "do nothing" is a perfectly fine error recovery routine

I don't agree that this motivates must_use = false. Specifically: "a function is technically fallible, but it is almost always correct to not check" reads, to me, like a function which must be checked, precisely because it might fail, which is exactly what Result encodes.

you misunderstand me, i don't mean "will be correct for almost all calls" i mean "will be most correct in almost all hypothetical call sites"

That said, if there are example use-cases where it is clear that a must_use value does not require use in any circumstance, then I'd find this proposal more convincing. At the same time, perhaps it would be better to just remove must_use from functions like that, if possible?

do you mean "remove it from the types"? this entire proposal is "let functions remove must_use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants