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

Fix Debug implementations of some of the HashMap and BTreeMap iterator types #75377

Merged
merged 4 commits into from
Oct 3, 2020

Conversation

canova
Copy link
Contributor

@canova canova commented Aug 10, 2020

HashMap's ValuesMut, BTreeMaps ValuesMut, IntoValues and IntoKeys structs were printing both keys and values on their Debug implementations. But they are iterators over either keys or values. Irrelevant values should not be visible. With this PR, they only show relevant fields.
This fixes #75297.

Here's an example code. This prints this on nightly:

ValuesMut { inner: IterMut { range: [(1, "hello"), (2, "goodbye")], length: 2 } }
IntoKeys { inner: [(1, "hello"), (2, "goodbye")] }
IntoValues { inner: [(1, "hello"), (2, "goodbye")] }
[(2, "goodbye"), (1, "hello")]

After the patch this example prints these instead:

["hello", "goodbye"]
["hello", "goodbye"]
[1, 2]
["hello", "goodbye"]

I didn't add test cases for them, since I couldn't see any tests for Debug implementations anywhere. But please let me know if I should add it to a specific place.

r? @dtolnay

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2020
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 10, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Please also remove the unused bound from the Debug impls of hash_map::IntoKeys and hash_map::IntoValues.

@dtolnay
Copy link
Member

dtolnay commented Aug 10, 2020

@rfcbot fcp merge

This PR relaxes the bounds on the following stable Debug impls:

  impl<K, V> Debug for std::collections::btree_map::ValuesMut<'_, K, V>
  where
-     K: Debug,
      V: Debug;

  impl<K, V> Debug for std::collections::hash_map::ValuesMut<'_, K, V>
  where
-     K: Debug,
      V: Debug;

It also does the same for these, though all of these are unstable types:

  impl<K, V> Debug for std::collections::btree_map::IntoKeys<K, V>
  where
      K: Debug,
-     V: Debug;

  impl<K, V> Debug for std::collections::btree_map::IntoValues<K, V>
  where
-     K: Debug,
      V: Debug;

  impl<K, V> Debug for std::collections::hash_map::IntoKeys<K, V>
  where
      K: Debug,
-     V: Debug;

  impl<K, V> Debug for std::collections::hash_map::IntoValues<K, V>
  where
-     K: Debug,
      V: Debug;

This matches the existing signatures of the Debug impls on these stable types:

  • std::collections::btree_map::Keys
  • std::collections::btree_map::Values
  • std::collections::hash_map::Keys
  • std::collections::hash_map::Values

@rfcbot
Copy link

rfcbot commented Aug 10, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 10, 2020
@canova
Copy link
Contributor Author

canova commented Aug 11, 2020

Updated the bounds of IntoKeys and IntoValues of HashMap, thanks!

@canova canova requested a review from dtolnay August 11, 2020 20:04
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2020
@jyn514 jyn514 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 15, 2020
@Dylan-DPC-zz
Copy link

@KodrAus @SimonSapin @withoutboats this is waiting for your vote/concerns on the fcp

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 29, 2020
@rfcbot
Copy link

rfcbot commented Sep 29, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 29, 2020
@dtolnay
Copy link
Member

dtolnay commented Sep 29, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2020

📌 Commit 8faf550 has been approved by dtolnay

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 29, 2020
@bors
Copy link
Contributor

bors commented Sep 29, 2020

⌛ Testing commit 8faf550 with merge ef1001a2ff6c8deceed8a0478c665aee1426368b...

@bors
Copy link
Contributor

bors commented Sep 29, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2020
@canova
Copy link
Contributor Author

canova commented Oct 2, 2020

Hey @dtolnay, I don't see any failure related to the PR. Do you think I need to do something else before merging this?

@mati865
Copy link
Contributor

mati865 commented Oct 2, 2020

Spurious network error:

curl: (6) Could not resolve host: ci-mirrors.rust-lang.org

@dtolnay
Copy link
Member

dtolnay commented Oct 2, 2020

@bors retry

@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 Oct 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
…as-schievink

Rollup of 8 pull requests

Successful merges:

 - rust-lang#75377 (Fix Debug implementations of some of the HashMap and BTreeMap iterator types)
 - rust-lang#76107 (Write manifest for MAJOR.MINOR channel to enable rustup convenience)
 - rust-lang#76745 (Move Wrapping<T> ui tests into library)
 - rust-lang#77182 (Add missing examples for Fd traits)
 - rust-lang#77251 (Bypass const_item_mutation if const's type has Drop impl)
 - rust-lang#77264 (Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. )
 - rust-lang#77421 (Revert "resolve: Avoid "self-confirming" import resolutions in one more case")
 - rust-lang#77452 (Permit ty::Bool in const generics for v0 mangling)

Failed merges:

r? `@ghost`
@bors bors merged commit 1118ab9 into rust-lang:master Oct 3, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 3, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 9, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Debug implementations for HashMap and BTreeMap Keys/Values iterator structs