Skip to content

Conversation

DevinR528
Copy link
Contributor

@DevinR528 DevinR528 commented Jul 24, 2021

Closes #6845

changelog: Enable the banning of primitive types in [disallowed_type]

@rust-highfive
Copy link

r? @camsteffen

(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 Jul 24, 2021
@camsteffen
Copy link
Contributor

Closes #6845

Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Some minor requests. This is a nice enhancement!

match cx.qpath_res(path, ty.hir_id) {
Res::Def(_, did) => {
if let Some(name) = self.def_ids.get(&did) {
emit(cx, name, path.span());
Copy link
Contributor

@camsteffen camsteffen Jul 26, 2021

Choose a reason for hiding this comment

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

Can you change this to use cx.tcx.def_path_str? Also I think you should factor in check_qpath(self, qpath, span) for repeated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the last commit has what I think you mean by factor check_qpath but I'm unsure?

As for using cx.txc.def_path_str I'm fine with it I just wanted you to be aware of my reasoning for not using it. The user seeing the exact path that they banned in the Conf struct may be less surprising than banning a re-export and getting a different name in the warning. I'm fine either way just thought to mention it 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's okay if it doesn't match the config exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, it seems to actually follow the re-exports (notice syn::Ident) nice thanks for putting up with all my questions!

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem! cx.get_def_path gets the absolute path so it's confusing.

@DevinR528 DevinR528 force-pushed the bantype-fix branch 4 times, most recently from beaf60e to 6e7d679 Compare July 27, 2021 13:50
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Looks great! Please squash commits.

Fix docs of disallowed_type

Add ability to name primitive types without import path

Move primitive resolution to clippy_utils path_to_res fn

Refactor Res matching, fix naming and docs from review

Use tcx.def_path_str when emitting the lint
@DevinR528
Copy link
Contributor Author

As always thanks for all the help!

@camsteffen
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2021

📌 Commit a1bab3b has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Jul 27, 2021

⌛ Testing commit a1bab3b with merge 43905d9...

@bors
Copy link
Contributor

bors commented Jul 27, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 43905d9 to master...

@bors bors merged commit 43905d9 into rust-lang:master Jul 27, 2021
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.

Lint request: char_is_the_wrong_abstraction
5 participants