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.

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

Internal lint: suggest is_type_diagnostic_item over match_type where applicable #6013

Merged
merged 4 commits into from
Sep 15, 2020

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Sep 6, 2020

changelog: none

@rust-highfive
Copy link

r? @yaahc

(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 Sep 6, 2020
@bors
Copy link
Contributor

bors commented Sep 6, 2020

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

@ebroto ebroto force-pushed the diagnostic_item_restriction branch from d3f0a68 to 57933e5 Compare September 6, 2020 23:57
@ghost
Copy link

ghost commented Sep 7, 2020

This is a great idea.

Is there any reason to save the path to a diagnostic item at all? I think it might be easier to loop through the lines of utils/path.rs and apply the check there.

Maybe you could make a similar lint to check for lang items as well.

@ebroto
Copy link
Member Author

ebroto commented Sep 8, 2020

Is there any reason to save the path to a diagnostic item at all?

Maybe you could make a similar lint to check for lang items as well.

Yeah my initial idea was to get rid of diagnostic items in the paths module, but it's not as straightforward as it could seem. For example, in some cases paths to diagnostic items are put together in a slice with paths to other types, and fed to match_def_path.

But you make me think that maybe we could work on the clippy utils module to expose functions that take a type that could be either a diagnostic item, a lang item or another type, using an enum, and avoid using the relevant rustc API functions directly.
I would prefer to discuss this design in a Zulip thread though (it seems we're moving there) and get some feedback before trying to implement it.

If you agree I would go with merging this before, knowing it only addresses a part of the problem. I think it can be helpful to avoid missing it in the reviews.

@ghost
Copy link

ghost commented Sep 9, 2020

If you agree I would go with merging this before, knowing it only addresses a part of the problem. I think it can be helpful to avoid missing it in the reviews.

I think it's an good improvement as is. I'm happy with whatever you and the reviewer decide.

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.

I agree that we can merge this as is. Doing the same with LangItems would be great. Please address the comment by @giraffate and r=me after that.

@ebroto ebroto force-pushed the diagnostic_item_restriction branch from 57933e5 to ec26c8e Compare September 15, 2020 08:32
@ebroto ebroto force-pushed the diagnostic_item_restriction branch from ec26c8e to d0b5663 Compare September 15, 2020 08:46
@ebroto
Copy link
Member Author

ebroto commented Sep 15, 2020

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit d0b5663 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Sep 15, 2020

⌛ Testing commit d0b5663 with merge 12ce312...

@bors
Copy link
Contributor

bors commented Sep 15, 2020

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

@bors bors merged commit 12ce312 into rust-lang:master Sep 15, 2020
@ebroto ebroto deleted the diagnostic_item_restriction branch September 21, 2020 12:50
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.

6 participants