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

Pick over the standard libraries for types that don't (but can) implement common traits #15294

Closed
huonw opened this issue Jul 1, 2014 · 44 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@huonw
Copy link
Member

huonw commented Jul 1, 2014

... and implement/derive them.

Traits like Eq, Hash, Ord, Clone are sometimes forgotten (or have been missed, in the various library changes that have happened), and this is annoying for downstream users. To paper over the deeper issue (how to make adding these impls more natural/harder to forget), impls for all the various traits can be added to types.

List of types missing things just from a basic scan:

  • many types in collections (only implement the Partial comparison traits, and pretty much none of them implement Hash)
  • semver::Version
  • all types in num are missing Hash
  • url::Path, url::UserInfo
  • uuid::UuidVariant, uuid::UuidVersion
@aturon
Copy link
Member

aturon commented Jul 1, 2014

cc me

@wycats
Copy link
Contributor

wycats commented Jul 1, 2014

Also things like Path, Command, etc. It's common to want to be able to create sets of even somewhat exotic objects.

@chris-morgan
Copy link
Member

I’m imagining a tool which produces a massive table where each column is a trait and each row is a struct or enum, with each cell shaded appropriately for whether there is an implementation (under any constraints) of the trait for the type.

bors added a commit that referenced this issue Jul 10, 2014
Allows use cases like this one:

``` rust
use std::io::Command;

fn main() {
    let mut cmd = Command::new("ls");
    cmd.arg("-l");

    for &dir in ["a", "b", "c"].iter() {
        println!("{}", cmd.clone().arg(dir));
    }
}
```

Output:
```
ls '-l' 'a'
ls '-l' 'b'
ls '-l' 'c'
```
Without the `clone()`, you'll end up with:
```
ls '-l' 'a'
ls '-l' 'a' 'b'
ls '-l' 'a' 'b' 'c'
```

cc #15294
bors added a commit that referenced this issue Jul 27, 2014
Implements PartialEq/Eq/Clone/Hash/FromIterator/Extendable for SmallIntMap and Clone/Show for TrieMap/TrieSet. cc #15294
@nham
Copy link
Contributor

nham commented Aug 1, 2014

For items in collections, and among the traits Clone, Default, FromIterator, Extendable, Hash, PartialEq, Eq, PartialOrd, and Show, here's what's still missing:

  • EnumSet is missing a bunch. I've held off on implementing these because I wasn't sure it was sticking around since we have bitflags! now
  • PriorityQueue is missing Hash/PartialEq/Eq/Show/PartialOrd. I'm not sure if it's currently possible to implement these
  • BTree is missing Default/FromIterator/Extendable/Hash

bors added a commit that referenced this issue Aug 2, 2014
…hton

This implements:

 - Eq and Ord for DList, RingBuf, TreeMap and TreeSet
 - FromIterator and Extendable for BitvSet

cc #15294
@alexcrichton
Copy link
Member

Show is currently used in a way that I believe all primitive types must implement it. The burden of not implementing it is quite high. That being said, the way Show is being used today is perhaps not the way we would like to use Show, so I personally like taking a look at @aturon's three examples.

I'm not entirely sold on the usefulness of a semi-polished, but slightly manual, and still non-lossy middle ground between full-on serialization and debugging. Do we know of what examples we have today would fall in this category?

I believe there are also some competing concerns with to_string. I don't really want to have to define the way to serialize my type three times (once for debugging, once for to_string, once for Encodable), even if it's just through deriving-based traits. Additionally our "debugging" view would be quite close to to_string

foo.to_string();
// vs
format!("{}", foo);

I personally would tend to err on the side of saying "to_string is lossy" and "Show should be implementing on all primitives", but I also can't shake the feeling that there's likely a better middle ground.

@SimonSapin
Copy link
Contributor

I'm not entirely sold on the usefulness of a semi-polished, but slightly manual, and still non-lossy middle ground between full-on serialization and debugging.

I don’t understand how serialization is applicable here. How can I use Encodable to serialize not to something like JSON, but to string (or stream of text) in a format determined by the implementer of the Encodable trait? E.g. an URL is something like http://example.com, not {"scheme": "http", "host": "example.com", ...}

@alexcrichton
Copy link
Member

I would imagine that if you know that your type can be perfectly encoded as a string then you invoke encode_str and decode_str with your own parsing as appropriate. With @aturon's multidispatch proposal, and depending on the design of Encodable, you could specify a different method of encoding per-encoder (if the encoder remains a type parameter on the trait).

@SimonSapin
Copy link
Contributor

It would also be useful to have a "debugging" serialization (e.g. used in assert_eq!) of String and &str that is not just the string itself. Currently, println!("{}", vec!["a", "b, c", ""]) returns [a, b, c, ] which can be confusing.

@Gankra
Copy link
Contributor

Gankra commented Sep 27, 2014

We have BTreeMap and BTreeSet impling all the common things. So I think this issue in now just "do #17534". Can be closed?

@vks
Copy link
Contributor

vks commented Sep 27, 2014

What about EnumSet?

@Gankra
Copy link
Contributor

Gankra commented Sep 27, 2014

What's it missing? Ord/PartialOrd?

@Gankra
Copy link
Contributor

Gankra commented Sep 29, 2014

Ok. EnumSet is Ord+PartialOrd now. Anything else?

@vks
Copy link
Contributor

vks commented Sep 30, 2014

I think this is all and this can be closed.

@ftxqxd
Copy link
Contributor

ftxqxd commented Sep 30, 2014

Almost no iterators (Map, FlatMap, MoveItems, etc.) implement Clone, which can be annoying. I’m not sure how much sense this makes, but I’d like to be able to clone iterators sometimes.

@thestinger
Copy link
Contributor

Iterators like Map can't implement Clone because they use the traditional boxed closures.

@Gankra
Copy link
Contributor

Gankra commented Sep 30, 2014

It would be completely unsound for iter_mut to be cloneable. Cloning an into_iterator also seems vaguely unsound to me. It couldn't be lazy, at very least. It would have to completely clone the underlying structure, at which point... what have we accomplished over just cloning before/after the iteration? That only leaves iterators, which could probably be cloned soundly. Although honestly this seems like more of an argument for providing prev and prev_back on such iterators.

Can you provide some concrete usecases?

@thestinger
Copy link
Contributor

It's not possible to provide prev to most iterators without adding overhead. It also wouldn't be safe on mutable iterators. Cloning iterators is useful and is how cycle is implemented, but for the closure-based adaptors it would require moving to unboxed closures.

@Gankra
Copy link
Contributor

Gankra commented Sep 30, 2014

@thestinger I was only suggesting adding prev for immutable iterators over collections. Almost all of them could provide this trivially, as far as I can tell. You just increment nelems and move the pointer in the other direction (which you know how to do if you're double-ended). You do have to add some extra logic for seeing if you've prev'd off the end of the structure, but that should only add complexity for prev.

@thestinger
Copy link
Contributor

No, double-ended doesn't imply the ability to move the iterator in the reverse direction. A vector iterator has a pointer to either end of the remaining range and would need 4 pointers if it could be expanded again via prev. The same thing can be accomplished without overhead by cloning the iterator before advancing it and then restoring the state, as cycle does.

@huonw
Copy link
Member Author

huonw commented Sep 30, 2014

Move-iterators are safe to clone, e.g. Vec<T>.into_iter() would clone by non-destructively traversing to get &s (which is actually something we could expose externally) and pushing onto a new Vec and into_iter-ing that.

(I'm sure it would be difficult to write clone implementations for some move-iterators, but the common ones probably aren't so bad.)

@Gankra
Copy link
Contributor

Gankra commented Sep 30, 2014

@thestinger Ah, I mistakenly assumed the Vec iterator had a reference to the source slice.

@aturon
Copy link
Member

aturon commented Oct 2, 2014

Note: I've created a discuss topic to address the questions around Show raised in this thread. I'd like to write an RFC to nail this down, but first wanted to solicit community feelings on the topic.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 17, 2014
closes rust-lang#12677 (cc @Valloric)
cc rust-lang#15294

r? @aturon / @alexcrichton

(Because of rust-lang#19358 I had to move the struct bounds from the `where` clause into the parameter list)
japaric pushed a commit to japaric/rust that referenced this issue Jan 19, 2015
barosl added a commit to barosl/rust that referenced this issue Jan 20, 2015
closes rust-lang#21402
cc rust-lang#15294

r? @alexcrichton or @aturon 
cc @ExpHP (btw, this only covers arrays with arity up to 32)
@steveklabnik
Copy link
Member

Given that this issue is vague, and it seems like all of the specific things mentioned here have been done, I'm giving this a close. If anyone knows of any other things that are missing, let's add them, but I don't find this ticket to be super actionable right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests