Equality with build metadata ignored?

[alexcrichton]

I may be misunderstanding what pre is used for, but I'd expect this program to pass?

extern crate semver;                                       
                                                           
fn main() {                                                
    let a = semver::Version::parse("1.0.1+1.7.3").unwrap();
    let b = semver::Version::parse("1.0.1+1.7.5").unwrap();
    assert!(a != b);                                       
}                                                          

[alexcrichton]

(currently this panics)

[alexcrichton]

(feel free to close if this is expected)

[steveklabnik]

related to #107

[dwijnand]

I think this should be closed, as mentioned in #107 (comment):

+1.7.3 is not a pre-release version. It is build metadata. According to the semver spec,

Build metadata SHOULD be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence.

So those two versions are equivalent. It's more obvious if you use more obviously-looking build metadata like:

    let a = semver::Version::parse("1.0.1+built-at-1205").unwrap();
    let b = semver::Version::parse("1.0.1+built-at-1210").unwrap();
    assert!(a != b);                                       

[sgrif]

As I mentioned in #107 (comment), I think this is a misreading of the spec. Having the same precedence does not mean they are equal. Or to put it more specifically, I think it should be ignored in a PartialOrd impl, but not in a PartialEq impl. This also means that Version could implement Eq, but not Ord.

Precedence refers to how versions are compared to each other when ordered.

[dwijnand]

PartialOrd extends PartialEq and, if I'm not mistaken, if x.eq(&y) returns true and then you must make x.partial_cmp(&y) return Ordering::Equal.

In the docs it says:

Implementations of PartialEq, PartialOrd, and Ord must agree with each other.

[sgrif]

Nothing I said conflicts with that

[dwijnand]

I don't think you can or should have such an implementation of Eq that disallows Version from having an Ord impl.

"Can" because I think you could interpret the semver spec as saying that you must be allowed to query version precedence.

And "should" because I think it would be throwing the baby out with the bathwater to not be able to compare if a version is < or > another, just so that you can have build-metadata sensitive equality.

[sgrif]

The docs are specific about what this means:

That is, a.cmp(b) == Ordering::Equal if and only if a == b and Some(a.cmp(b)) == a.partial_cmp(b) for all a and b.

I have not said anything which disagrees with that (nor does any of it as written even apply if you're not implementing Ord, but I think it's implied that it also means "a.partial_cmp(b) == Some(Ordering::Equal) if and only if a == b", which I'm not contradicting)

[sgrif]

Ah, I think I understand what you're saying now. I'm saying that Version(1.0.0+a).partial_cmp(Version(1.0.0+b)) should return None, you're saying that it should return Some(Ordering::Equal), and the docs say that it can't unless Version(1.0.0+a) == Version(1.0.0+b). I think you're correct in that the most strict interpretation of the text of the spec (specifically the phrasing "have equal precedence") implies that, but I still stand by my previous statement (meaning that .partial_cmp on those values would return None), as I believe it still fits with the intention of the spec, and is much more useful. Having partial_cmp return None in that case is the only way to implement this while adhering to the constraints laid out in the docs (again, assuming the slightly different wording so they actually apply if Ord isn't implemented)

[dwijnand]

Ah, I had missed those important details in Ord. Thank you for laying out your POV so well.

I would assume that Version(1.0.0+a) == Version(1.0.0+b) must imply Version(1.0.0+a).partial_cmp(Version(1.0.0+b)) under "Implementations of PartialEq, PartialOrd, and Ord must agree with each other", but I wish it were more formal than "must agree".

Has anyone in the ecosystem encoded these laws so we can just test for our difference in interpretation (the trait laws, not a semver thing)?

Secondly, I'm not too familiar with Ord vs PartialOrd, what is lost by implementing only the latter?

[sgrif]

what is lost by implementing only the latter?

The ability to be used anywhere that requires total ordering, such as being a key in a BTreeMap. The most common example of a type that implements PartialOrd but not Ord is f32 or f64. Generally speaking, most functions which require ordering (such as sort) require PartialOrd not Ord, and most issues arising from NaN in floats come from the the lack of an Eq implementation, not the lack of an Ord implementation. I can't think of any cases that require Ord beyond BTreeMap off the top of my head, though I'm sure they exist.

EDIT: It appears I was mistaken, fn sort does require Ord in the standard library. Which is definitely unfortunate since my interpretation of the spec says that it definitively can't, though the current impl upholds the requirements defined in the docs (total, asymmetric, transitive)

[dtolnay]

This is fixed in 1.0.0.

fn main() {
    let a = semver::Version::parse("1.0.1+1.7.3").unwrap();
    let b = semver::Version::parse("1.0.1+1.7.5").unwrap();
    println!("{}", a == b);
}

false