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.

Sign up for GitHub

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

Jump to bottom

Don't emit RETURN_SELF_NOT_MUST_USE lint if Self already is marked as #[must_use] #8146

Merged
merged 1 commit into from Dec 19, 2021
Merged

Don't emit RETURN_SELF_NOT_MUST_USE lint if Self already is marked as #[must_use] #8146

merged 1 commit into from Dec 19, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 19, 2021 •
edited by xFrednet

New bug discovered with this lint. Hopefully, this is the last one.

changelog: none

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 19, 2021
@GuillaumeGomez
Copy link
Member Author

r? @xFrednet

xFrednet
xFrednet reviewed Dec 19, 2021
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

One small question, it otherwise looks good to me 🙃

clippy_lints/src/return_self_not_must_use.rs
@@ -50,6 +51,7 @@ fn check_method(cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'tcx>, fn_def: LocalD
if decl.implicit_self.has_implicit_self();
// We only show this warning for public exported methods.
if cx.access_levels.is_exported(fn_def);
if !cx.tcx.hir().attrs(hir_id).iter().any(|attr| attr.has_name(sym::must_use));
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this case covered by the must_use_attr check in line 57?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed but apparently it's not working as it should. I'll clean up though. Good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

(Which was fixed in #8143 but I realized it would be better checked here instead.)

@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

And removed the warning as well.

@xFrednet
Copy link
Member

LGTM, thank you for following up on these bugs 👍. Let's hope it was the last one 🙃

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 19, 2021

📌 Commit 07a00ef has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Dec 19, 2021

⌛ Testing commit 07a00ef with merge 1962ce0...

@GuillaumeGomez
Copy link
Member Author

I hope as well! Thanks for the quick reviews!

@bors
Copy link
Collaborator

bors commented Dec 19, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 1962ce0 to master...

@bors bors merged commit 1962ce0 into rust-lang:master Dec 19, 2021
@GuillaumeGomez GuillaumeGomez deleted the must-use-self branch December 19, 2021 15:07
@lukexor lukexor mentioned this pull request Dec 20, 2021
Closed
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 22, 2021 •
edited

@xFrednet Just to confirm: I still have this bug showing up when working on glib on the latest rust nightly:

$ rustc --version
rustc 1.59.0-nightly (e100ec5bc 2021-12-21)
$ cargo clippy --version
clippy 0.1.59 (e100ec5 2021-12-21)

Has this patch been "merged" into the nightly or not yet?

@xFrednet
Copy link
Member

Hey @GuillaumeGomez, the fix has not been merged into nightly. The sync is usually done Thursdays, meaning today it should be synced. However, it might be delayed due to holidays and such. I can ping you on the sync PR or here if you like 🙃

@GuillaumeGomez
Copy link
Member Author

Yes please! Thank you!

@flip1995
Copy link
Member

The regular sync will be next week Thursday. I can do an out-of-cycle sync today, if you need this fix tomorrow.

@GuillaumeGomez
Copy link
Member Author

No hurry, don't worry. Before beta/stable release would be nice though. ;)

@flip1995
Copy link
Member

Yes, this will get in before beta branch 👍 (famous last words)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@GuillaumeGomez @rust-highfive @xFrednet @bors @flip1995 @Manishearth