I am the author of a crate (GitHub - phillord/pretty_rdf) that currently uses AsRef<str> as a bound on most of its methods. I am debating whether this is the right choice, and think that it probably should be Borrow instead.
I have read the documentation of the two carefully but still find it all rather opaque. My justification for the switch would be that I do make use of both Hash and Equality of the generic type and that this is a good basis for choosing between the two.
Am I right? Is this why I would choose one over the other?
Borrow<str> essentially means that your generic type is a transparent wrapper around, or container of, a string slice, and essentially anything you do with the string slice should behave identically to the equivalent operation on the wrapper type.
If you rely upon this property, then you should require Borrow.
I looked briefly at the code and don't understand what you're going for. Here you have a struct where you've derived PartialOrd and Ord:
/// An RDF IRI
#[derive(Ord, PartialOrd, Clone)]
pub struct PNamedNode<A: AsRef<str>> {
pub iri: A,
// true if we have previously split iri
position_cache: RefCell<bool>,
// position at which the fragment occurs
position_split: RefCell<Option<usize>>,
}
Then here you implement Hash, PartialEq, and Eq in terms of str:
Why should Ord and Eq be different? You also have structs that derive all 5, and structs that derive them but implement comparisons with other types in terms of str. You can definitely make these work with AsRef<str>, but you need to always use it.
In case you're not aware, the derive macros for these traits put bounds on all the generics in a type and they don't use as_ref. So the struct above has a PartialOrd impl that looks like this:
If it s a transparent wrapper then surely that is Deref<item = str>?
The point of the generic in this case is to allow me to cache the strings somewhere -- basically I need to wrap Rc and Arc. They are the only two obvious implementations at this point.
It's a good question one that I don't have a ready answer to, but I expect is historical. The Ord and PartialOrd derive implementation is simply wrong, but harmlessly so as it is also redundant.
The two RefCell fields are a perfomance cache implementation. So the Hash and Eq implementation is correct to ignore them. I guess by the terms of the AsRef documentation, I am free just not to implement Ord; the Borrow documentation confuses me still and I am undecided whether it implies that Ord must match Eq and must be implemented, or whether Borrow is okay without a Ord implementation.
Borrow without Ord is fine (this is how HashSet uses it). The main thing Borrow requires over AsRef is that x.borrow() == y.borrow() is always the same as x == y. If you never do x == y then you don't need Borrow, you can just use AsRef and always do x.as_ref() == y.as_ref().
Well, I don't do x == y but I do x.borrow() == y. More specifically, I need to do x.borrow() == "some_string". The interface documentation does not say much about this, but I think that this is the naturalistic interpretation.
x.borrow() == "some_string"isx.borrow() == y.borrow(), except you already have the borrowed version. Borrow doesn't care about things like Arc<str> == &str.
Unless you can guarantee what @drewtato quoted, then you should use AsRef. This requirement is clear in the Borrow doc. The doc does not leave it open to interpretation.
Unfortunately, this is backward. My library is giving a type bound. It is not a question of whether I can provide guarantees but whether I depend on them. It is a little unclear to me how I work out whether I do or not; I think I probably do not, but think that any type likely to use the library would probably be behaving unintuitiively if x.borrow() == y .borrow() is different from x == y.
All of which is leading me to suspect that my original AsRef implementation is, indeed, wrong and that Borrow would be better.
The derived implementation of Ord being different from only comparing str-wise can be harmful for things like BTreeMap similar to how it can be for Hash/Eq in a HashMap.
My rule of thumb for my own structs is that if you're Borrow<str>, write all your comparison traits to defer to strs implementations. It's okay to not implement comparison traits you don't need. And if this is onerous, you can have a newtype that does this instead (for use cases like HashMap and BTreeMap).
But apparently your situation is more about what you're requiring of others? Like, others supply the A parameter of PNamedNode<A>, and A alone should drive the PartialEq implementation? AsRef<str> would be more general in that case; you can use documentation to highlight that PartialEq and friends rely on the str representation only.
If you're uncomfortable with that and go with Borrow, downstream can still work around it with their own newtypes (e.g. if they have a type that implements AsRef<str> but not Borrow<str>).
Indeed. This difference in Ord and Eq is clearly problematic for its potential to introduce difficult to debug errors which would occur if I used BTreeMap. I will remove it.
As you say, mostly I am concerned with the constraints I put on others. The library is generic over A which is bound by AsRef<str>. It does provide some structs which are currently AsRef but simply follow the bounds on A; so they would switch over and use Borrow. Currently PNamedNode fulfils the AsRef contract but breaks Borrow; a bug I need to fix rather than by design.