Skip to content

Clippy Book Chapter Updates Reborn: Trait Checking #10626

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

Merged
merged 5 commits into from
Sep 2, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Apr 10, 2023

This PR adds a new chapter to the book: "Trait Checking". No major changes from the source (just some typos, re-phrasing, the usual).

Notes

changelog: Add a new "Trait Checking" chapter to the book

@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2023

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 10, 2023
@blyxyas
Copy link
Member Author

blyxyas commented Apr 10, 2023

r? @flip1995


impl LateLintPass<'_> for CheckTokioAsyncReadExtTrait {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
if match_trait_method(cx, expr, &paths::TOKIO_IO_ASYNCREADEXT) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: please use a path that is not to an external crate (I don't want to give the idea that adding paths to external crates is accepted. We try to avoid that as much as possible)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, just a question. Why is this specific path accepted?

Copy link
Member

Choose a reason for hiding this comment

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

We used to be less strict on external crates/didn't have a policy. Deprecating useful lints for those crates now isn't worth the hassle. So we have some legacy code from that time.

@bors
Copy link
Contributor

bors commented Apr 16, 2023

☔ The latest upstream changes (presumably #10652) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas blyxyas force-pushed the book-trait_checking branch from ab06ae7 to 1c06375 Compare April 17, 2023 08:27
@bors
Copy link
Contributor

bors commented Aug 16, 2023

☔ The latest upstream changes (presumably #10644) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas blyxyas force-pushed the book-trait_checking branch from 5e982c2 to 2a3f75b Compare August 17, 2023 20:56
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This LGTM.

All of the remaining 3 PRs are now ready to be merged. However, some links need to be figured out I think. @blyxyas Can I ask you to do that for those 3 PRs and then r=me on them in whatever order makes the most sense.

After all those PRs are merged, I think the only thing left to do is address the FIXMEs in those chapters and cross link those chapters.

[`clippy_utils::paths`][paths] with the `match_trait_method` to determine trait
implementation.

> **Note**: This approach should be avoided if possible, the best thing to do would be to make a PR to [`rust-lang/rust`][rust].
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
> **Note**: This approach should be avoided if possible, the best thing to do would be to make a PR to [`rust-lang/rust`][rust].
> **Note**: This approach should be avoided if possible, the best thing to do would be to make a PR to [`rust-lang/rust`][rust] adding a diagnostic item.

@blyxyas
Copy link
Member Author

blyxyas commented Sep 1, 2023

I'll fix them. Btw, are you talking about the link order in SUMMARY.md or the missing links in the FIXMEs

@flip1995
Copy link
Member

flip1995 commented Sep 1, 2023

Mainly missing links in FIXME comments (can and maybe should be done in a follow up PR). If you think the order in the ToC needs to be adapted go ahead. I didn't pay much attention to that TBH.

@blyxyas
Copy link
Member Author

blyxyas commented Sep 2, 2023

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Sep 2, 2023

📌 Commit e1a3f63 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 2, 2023

⌛ Testing commit e1a3f63 with merge b65e544...

@bors
Copy link
Contributor

bors commented Sep 2, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing b65e544 to master...

@bors bors merged commit b65e544 into rust-lang:master Sep 2, 2023
@blyxyas blyxyas deleted the book-trait_checking branch October 5, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants