Use #[must_use] determination from the compiler#16633
Conversation
This comment has been minimized.
This comment has been minimized.
35d95f3 to
7392cc1
Compare
|
Lintcheck changes for d4b1ee5
This comment will be updated if you push new changes |
7392cc1 to
e083a22
Compare
|
Hey! Since rust-lang/rust#153018 merged a while ago, can this be revived? |
This comment has been minimized.
This comment has been minimized.
62ad561 to
b0ba9bd
Compare
|
r? @dswij rustbot has assigned @dswij. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@WaffleLapkin Could you have a look at the second commit in this PR, and see if it would be possible to get out the |
b0ba9bd to
d284340
Compare
This comment has been minimized.
This comment has been minimized.
d284340 to
0914507
Compare
This comment has been minimized.
This comment has been minimized.
| --> tests/ui/double_must_use.rs:40:37 | ||
| | | ||
| LL | async fn async_must_use_result() -> Result<(), ()> { | ||
| | ^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
maybe special casing this to say "async function returns a future which is always considered as #[must_use]" or something like that could make sense.
| LL | pub fn must_use_result_with_uninhabited_2() -> Result<T, !> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| note: the return type is `T` in a `Result` with an uninhabited error |
There was a problem hiding this comment.
I'm not sure if this is worded nicely... The return type is not T... it's Result<...>. But then must-useness depends on T... mh.
There was a problem hiding this comment.
You have to parse this as "the return type is (T in a Result with an uninhabited error)". Note that the part in "()" is generated by the code lifted up from the compiler to describe the Result<T, !> type.
There was a problem hiding this comment.
I also think that the wording could be improved, but that's not a blocker for this PR.
|
@samueltardieu I think you can refactor compiler's |
We use a particular version of the compiler through the |
|
Would you be interested in doing that refactor yourself / should I do it? Either way I think it might be easier to merge this as-is and refactor later. |
I'll do it.
Agreed, this way I will be able to do the refactor + switch to the refactored compiler version once this one is merged and synced back into the compiler repository. |
This comment has been minimized.
This comment has been minimized.
0914507 to
9c65d0f
Compare
This comment has been minimized.
This comment has been minimized.
|
Friendly ping @dswij |
This comment has been minimized.
This comment has been minimized.
This is inspired by the compiler. Ideally, this function should be provided by the compiler itself.
9c65d0f to
d4b1ee5
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
r? clippy |
| /// payload is an uninhabited type, the `Ok`/`Continue` payload type will be used instead. | ||
| /// | ||
| /// The [`MustUsePath`] can be used to describe the type through [`describe_must_use_type`]. | ||
| pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<MustUsePath> { |
There was a problem hiding this comment.
Please rename the function to better match the intent. Perhaps opt_must_use_path?
| LL | pub fn must_use_result_with_uninhabited_2() -> Result<T, !> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| note: the return type is `T` in a `Result` with an uninhabited error |
There was a problem hiding this comment.
I also think that the wording could be improved, but that's not a blocker for this PR.
View all comments
changelog: [
double_must_use,let_underscore_must_use,must_use_candidates]: better determination of#[must_use]needs by directly using the compiler algorithmFor example,
Box<T>is considered#[must_use]whenTis.