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

Iterate over maybe_unused_trait_imports when checking dead trait imports #97609

Merged
merged 1 commit into from
Jun 5, 2022
Merged

Iterate over maybe_unused_trait_imports when checking dead trait imports #97609

merged 1 commit into from
Jun 5, 2022

Conversation

Elliot-Roberts
Copy link
Contributor

Closes #96873
r? @cjgillot

Some questions, if you have time:

  • Is there a way to shorten the rustc_data_structures::fx::FxIndexSet path in the query declaration? I wasn't sure where to put a use.
  • Was returning by reference from the query the right choice here?
  • How would I go about evaluating the importance of the is_dummy() call in check_crate? I don't see failing tests when I comment it out. Should I just try to determine whether dummy spans can ever be put into maybe_unused_trait_imports?
  • Am I doing anything silly with the various ID types?
  • Is that let-else with unreachable!() bad? (i.e is there a better idiom? Would panic!("<explanation>") be better?)
  • If I want to evaluate the perf of using a Vec as mentioned in Iterate over maybe_unused_trait_imports when checking dead trait imports #96873, is the best way to use the CI or is it feasible locally?

Thanks :)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 1, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 1, 2022
cjgillot
cjgillot reviewed Jun 1, 2022
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks @Elliot-Roberts! This looks very good. I left a few comments.

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check_unused.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check_unused.rs Outdated
let item = tcx.hir().expect_item(id);
// if item.span.is_dummy() {
// continue;
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that the point of this check is to avoid reporting for use items that are injected by macros or the compiler. You can leave it as-is, it's very fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could I add a test with a macro that injects an unused trait use? I'm not sure how to check the compiler-injected ones though.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd need to write a proc macro for this. The proc macro would have to create a use Stuff; item, where the use and ; have a dummy span, and Stuff has a real one from the macro's input code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Elliot-Roberts crafting the test can wait a follow-up PR. If you don't mind uncommenting this code, the PR is good to merge.

compiler/rustc_typeck/src/check_unused.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check_unused.rs Outdated Show resolved Hide resolved
@Elliot-Roberts Elliot-Roberts marked this pull request as ready for review June 4, 2022 19:41
@cjgillot
Copy link
Contributor

cjgillot commented Jun 4, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 76c6845 has been approved by cjgillot

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 4, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2022
@bors
Auto merge of rust-lang#97742 - matthiaskrgr:rollup-fr3j0t8, r=matthi…
43874a2 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#97609 (Iterate over `maybe_unused_trait_imports` when checking dead trait imports)
 - rust-lang#97688 (test const_copy to make sure bytewise pointer copies are working)
 - rust-lang#97707 (Improve soundness of rustc_data_structures)
 - rust-lang#97731 (Add regresion test for rust-lang#87142)
 - rust-lang#97735 (Don't generate "Impls on Foreign Types" for std)
 - rust-lang#97737 (Fix pretty printing named bound regions under -Zverbose)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 326315b into rust-lang:master Jun 5, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 5, 2022
@Elliot-Roberts Elliot-Roberts deleted the unused-trait-refactor branch June 6, 2022 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Iterate over maybe_unused_trait_imports when checking dead trait imports
5 participants
@Elliot-Roberts @rust-highfive @cjgillot @bors @rustbot