Add lint for maps with zero-sized value types

[korrat]

Hi, this is my first time contributing to clippy or rust in general, so I'm not sure about the details of contributing. Please excuse me and let me now if I did anything wrong. I have a couple of questions:

1. I'm not sure what category this lint should be. I've put it in "nursery" for now.
2. Should I squash commits this is reviewed/merged?

changelog: Add lint for maps with zero-sized value types

Fixes #1641

[rust-highfive]

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Manishearth]

I'm in the middle of a move and may cut back on reviewing for a few weeks. I'm going to reassign my PRs for now, though i'll still try to review smaller things.

r? @ebroto

[ebroto]

Thanks for your contribution and sorry for the delay in reviewing, I'm really short on time lately.

---

I think we can simplify a lot this lint if we move the body of your current check_ty method to the implementation of LateLintPass' check_ty, transforming the hir::Ty into a ty::Ty with hir_ty_to_ty as you do currently elsewhere.

We should keep the check you do to avoid linting in trait impls, that is a good idea 👍

[ebroto]

Should I squash commits this is reviewed/merged?

In this case, I think it makes sense as there are some intermediary commits. But let's wait until the PR is approved, it's more helpful if the remarks are addressed as follow-up commits.

[korrat]

This PR currently causes some tests to fail with ICEs. I believe that this is due to the use of hir_ty_to_ty which returns TyKind::Error in some cases. Commenting out this part of the code makes the tests pass again. So far I have been unable to find a way around the use of hir_ty_to_ty without severely limiting the scope of the analysis.

Is there any way to turn a HIR Ty into a Ty with explicit error handling? Otherwise, I see the following options:

1. Implement the lint only in local scopes for now.
2. Only consider the obvious case of HashMap<K, ()> when checking non-local items

What is your opinion on this?

[korrat]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ebroto]

Indeed, hir_ty_to_ty seems to choke with inference variables and unnamed lifetimes. To solve this, the proposed solution first would check if there are typecheck results available and use those, otherwise fail back to hir_ty_to_ty.

Typecheck results are available in bodies, where inferred types are valid, so if there are no results we should not find type inference.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[giraffate]

ping from triage @korrat. There seems to be fixes left to be done. Do you have any questions on how to proceed here?

[korrat]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ebroto]

Just the nit (sorry about that 😄) and your suggestion about the tests, and this should be ready to go!

Thanks for making this move forward.

[korrat]

Thanks for being so quick in reviewing this again and again and sorry for the delay previously. Somehow the notification about your comment ended up in my spam mail folder where I didn't see it until yesterday.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ebroto]

This LGTM now! Thanks for all the effort you've put into this, and don't hesitate to take another issue if you had fun with this one :)

Could you squash your commits before merging? I should have said that in my last review, my apologies.

[korrat]

Sure, I squashed the commits. I didn't want to make the review harder for you earlier.

[ebroto]

@bors r+

Thanks!

[bors]

📌 Commit f77f1db has been approved by ebroto

[bors]

⌛ Testing commit f77f1db with merge 6c70133...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 6c70133 to master...