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

Assert covariance of BTree{Map,Set} and associated iterators #88058

Closed
wants to merge 1 commit into from

Conversation

nbdd0121
Copy link
Contributor

Difference etc are not yet covariant, see #30642.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Aug 15, 2021
@steffahn
Copy link
Member

There’s also the variance of the lifetime parameter that could be asserted for types like Iter<'a, K, V>. You also don’t really need separate tests for the key and value, e.g. a single

fn iter<'a>(x: Iter<'static, &'static (), &'static ()>) -> Iter<'a, &'a (), &'a ()> {
    x
}

for btree_map::Iter should be enough.

@nbdd0121
Copy link
Contributor Author

I followed HashMap, which has separate key and value assertions.

@steffahn
Copy link
Member

steffahn commented Aug 15, 2021

Alright, still the covariance of the lifetime argument of Iter, Keys, Values, Range, and even IterMut, RangeMut and ValuesMut could be included in the tests. Whether they’re tested separately or not.

@steffahn
Copy link
Member

If HashMap doesn’t check those either, then that may be a reasonable addition there as well, I guess.

@nbdd0121
Copy link
Contributor Author

In that case I'll just merge the checks into one... otherwise it'll too verbose.

As for HashMap and LinkedList I think it'll be better to file it under a separate PR. I'd like to keep this PR limited to just adding new checks for BTree{Map,Set}.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Aug 15, 2021

It turns out there are already tests for variances, just that they are named test_variance instead of assert_covariance so my grep doesn't detect them. Sigh 🤦

@nbdd0121 nbdd0121 closed this Aug 15, 2021
@nbdd0121
Copy link
Contributor Author

Of course these tests don't cover lifetime parameters, so it might worth doing that.

@nbdd0121 nbdd0121 deleted the covariance branch September 4, 2021 02:54
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.

4 participants