Add PartialOrd, Ord derivations to TypeId

[sdleffler]

I want to be able to sort a Vec of types which contain TypeIds, so an Ord derivation would be very useful to me. Hash and PartialEq/Eq already exist, so the missing PartialOrd and Ord derivations feel like an oversight to me.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

[clarfonthey]

I think that the main reason why Ord and PartialOrd implementations are omitted is because there's no natural way to sort TypeIds; should it be by type name, or by ID?

Why would you like to sort data in your Vec?

[sdleffler]

@clarcharr since the entire purpose of Rust's #[derive(PartialOrd, Ord)] is to produce arbitrary orderings such that a given type can be used in any data structure which requires an ordering, I don't feel like the significance of the ordering matters much here; I just want to be able to use TypeIds in structs which are used as keys for BTreeMap or BTreeSet or even sorted in a Vec. As for my particular use case, I want to quickly deduplicate a very small number of structs which contain TypeIds, as efficiently as possible. The inputs to this operation are iterators and the output should also be an iterator. I figured that repeated iteration through HashSets might be more expensive than Vec iterators, created through IntoIterator, so I wanted to be able to use .sort(), .dedup(), and then finally .into_iter().

For sorting the names of TypeIds, at least, I feel like you could just use .to_string() and then sort by the produced string, if you're really concerned about sorting by name.

[clarfonthey]

@sdleffler it seems to make more sense in that case to use a HashSet, which would have O(n) deduplication instead of O(nlog n) with the default library sort & dedup.

Technically you could use radix sort and a vector to do your O(n) deduplication but I suspect that it'll still be slower than a HashSet.

I suspect that if you're in a case where you need to use TypeId, the extra memory allocation is trivial compared to the overhead of dynamic type checking. The point of not implementing ordering is because ordering doesn't make sense, and also because I'm moderately certain that the intrinsic can return different values (and hence different orders) between runs or builds. There are ways of solving that problem.

[sdleffler]

@clarcharr the "dynamic type checking" overhead isn't actually present in my use case; in fact, I could probably just write a custom Ord implementation for my type which ignores the TypeId (wish I'd thought of that earlier, anyways.) This is because the TypeId is present due to a phantom type, which is brought back out long after all the deduping/combining of these values is done. Also, the value of the u64 internal to TypeId didn't seem to change in between runs when I tested by transmuting the TypeId. I also tested by using a hasher to observe the value. I suspect it would probably change between builds, though. Anyways, since you can't depend on the hash value between builds either, I don't feel that providing an order that can't be depended on between builds is much of a problem.

Also, the dynamic type checking overhead can't be high in any case, can it? I feel like in most cases it'll be so simple to be almost negligible. Equality between TypeIds, as well as hashing, is all done via a #[derive].

[sfackler]

@rfcbot fcp merge

This seems reasonable to me!

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

Concerns:

• forward-compatibility (Add PartialOrd, Ord derivations to TypeId #38981 (comment))

Once these reviewers reach consensus, 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.

[alexcrichton]

@rfcbot concern forward-compatibility

Hm I'm a little concerned here about the motivation. To me there is no inherent ordering amongst type ids, and often people might expect an ordering (e.g. i8 < i32) but we would provide no guarantees about this. This ordering would also likely change from release-to-release as the guts of the compiler implementation changes.

@sdleffler could you elaborate on why you want to order type ids?

[sdleffler]

@alexcrichton my original use case is no longer relevant (since it wasn't actually necessary to distinguish between the values I had by the values of the TypeIds they contained.) However, I feel that an arbitrary ordering for TypeId would be well in line with its Hash instance, allowing it to be used as a key for, say, BTreeMap and BTreeSet; because we already have a Hash instance the value of which will change between releases (at least), I feel that an ordering which works similarly would be fairly reasonable. Also, it would allow structures which contain it to have PartialOrd/Ord instances #[derive]d to be used as keys in those structures. I feel it's worth noting that if someone is determined to work around this, they can extract the value of the inner u64 either through mem::transmute or via a useless Hasher which only stores the unchanged u64 value, so it's completely possible to write a wrapper which implements PartialOrd/Ord.

[alexcrichton]

Yeah that makes sense, I've often wished for a way to easily throw something into BTreeMap or such where I just wanted a deterministic ordering, not necessarily one true ordering.

The risk for reliance here and actually causing breakage I will agree is indeed quite low. If others try to build this out of tree I agree it's better that we do it ourselves in tree, perhaps just with a clause that the exact order can't be relied upon.

[sfackler]

@BurntSushi @aturon @brson ping

@alexcrichton I feel pretty okay telling people who expect u8's type ID to be smaller than u32's that they are wrong :P

[alexcrichton]

@sfackler yes that also seems plausible to me. @sdleffler can you update the docs here to explicitly say that although there's an ordering between two type ids it's subject to change from release-to-release and cannot be relied upon to stay the same?

[sdleffler]

@alexcrichton I've added a warning - how's the wording look to you?

[clarfonthey]

@sdleffer it's already assumed that Hashes will vary between builds, so I'd just drop that and only mention Ord/PartialOrd.

[alexcrichton]

@sdleffler looks good to me, thanks!

[sdleffler]

@clarcharr I didn't see that when I glanced over the docs, so I figured I'd add it.

[brson]

The motivation that sometimes you just want a ordering for unordered types I find somewhat unconvincing because this is a special case of a general problem. Consider would we add Ord to float? No. What about other types that one might want to establish some ordering for? Is this the only case of a type in std without an ordering that can be applied an artifical ordering? By accepting this it seems like we are setting precedent - are we going to end up with yet another stream of PRs to fill in the gaps of a new policy for other types?

[aturon]

@brson I think this is an OK precedent to set, personally. For example, allowing file descriptors to be used as an ordered key seems useful for exactly the same reasons, even though the ordering between them isn't semantically significant.

[alexcrichton]

@bors: r+

[bors]

📌 Commit b08ab1e has been approved by alexcrichton

[bors]

⌛ Testing commit b08ab1e with merge 55013cd...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 55013cd to master...