Handle Result<T, !> and ControlFlow<!, T> as T wrt #[must_use]#16353
Handle Result<T, !> and ControlFlow<!, T> as T wrt #[must_use]#16353Jarcho merged 1 commit intorust-lang:masterfrom
Result<T, !> and ControlFlow<!, T> as T wrt #[must_use]#16353Conversation
rustbot
added
the
S-waiting-on-review
|
Lintcheck changes for 5137bb3
This comment will be updated if you push new changes |
samueltardieu
force-pushed
the
issues/rustc-148214
branch
from
e4e245f to
3ed43d8
Compare
|
The second push was made to prevent suggesting using |
|
r? clippy |
clippy_utils/src/ty/mod.rs
Outdated
| // Returns whether the `ty` has `#[must_use]` attribute. If `simplify_uninhabited` is set and | ||
| // `ty` is a `Result`/`ControlFlow` whose `Err`/`Break` payload is an uninhabited type, | ||
| // the `Ok`/`Continue` payload type will be used instead. See <https://github.com/rust-lang/rust/pull/148214>. | ||
| pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, simplify_uninhabited: bool) -> bool { |
There was a problem hiding this comment.
What cases are handled by simplify_uninhabited = false?
There was a problem hiding this comment.
See answer for both questions below.
| // The `simplify_uninhabited` argument can be later changed to `true` when | ||
| // rustc's `must_use` lint handles `Result<T, !>` and `ControlFlow<!, T>` as `T`. | ||
| || is_must_use_ty(cx, return_ty(cx, item_id), false) |
There was a problem hiding this comment.
Since rustc will be changing how it handles must_use shouldn't we just change the recommendation now?
There was a problem hiding this comment.
Right now it doesn't, and thus has not documented this. This would lead us to suggest adding #[must_use] to with_result() in:
pub struct S;
pub fn with_result() -> Result<S, std::convert::Infallible> {
Ok(S)
}while for the time being Result<_, _> is always must_use.
At first, I was worried about us triggering double_must_use if we suggest adding #[must_use] here and the user does it, but we won't because this PR modifies double_must_use. So we could suggest adding #[must_use], but that would still be confusing for the user since Result<_, _> is always must_use for the time being.
When it is implemented and documented that Result<T, E> is must_use only when T is in the case where E is uninhabited, we can suggest adding the attribute to the function.
There was a problem hiding this comment.
It isn't needed now, but it will be needed in the future. Since it's going to change in the near future we should be telling people to change things now so an update to the new compiler still warns. I would add a note to the diagnostic about this, but we should still lint.
|
r? Jarcho |
There is a proposal to change the behaviour of rustc's `must_use` lint to consider `Result<T, U>` and `ControlFlow<U, T>` as `T` when `U` is uninhabited. See <rust-lang/rust#148214>. This might make the user adding extra `#[must_use]` attributes to functions returning `Result<T, !>` or `ControlFlow<!, T>`, which would trigger the `double_must_use` lint in Clippy without the current change.
samueltardieu
force-pushed
the
issues/rustc-148214
branch
from
3ed43d8 to
5137bb3
Compare
rustbot
removed
the
S-waiting-on-review
|
@flip1995 I don't know where you stand regarding the current rustup, but if you get a chance to include this PR in it, it would allow this change to be present in Rust 1.95, and unblock rust-lang/rust#148214 for Rust 1.96. Otherwise we can also do a beta backport once the beta branch is cut, even though it would be great to do this near the start of the beta release cycle. |
|
Same as for the other PR :) I hope I can get back to the sync this evening/night. |
5 participants
There is a proposal to change the behaviour of rustc's
must_uselint to considerResult<T, U>andControlFlow<U, T>asTwhenUis uninhabited. See rust-lang/rust#148214.This might make the user adding extra
#[must_use]attributes to functions returningResult<T, !>orControlFlow<!, T>, which would trigger thedouble_must_uselint in Clippy without the current change.changelog: [
double_muse_use,drop_non_drop,let_underscore_must_use]: considerResult<T, U>andControlFlow<U, T>asTwrt the#[must_use]attribute ifUis uninhabited.