Make port: None equal to port: default_port #133

untitaker · 2015-11-02T20:39:45Z

untitaker · 2015-11-02T20:39:51Z

untitaker · 2015-11-02T20:42:05Z

Will add tests if you agree with the concept.

eefriedman · 2015-11-02T20:43:46Z

src/lib.rs

@@ -226,7 +226,7 @@ pub enum SchemeData {
 }

 /// Components for URLs in a *relative* scheme such as HTTP.
-#[derive(PartialEq, Eq, Clone, Debug, Hash, PartialOrd, Ord)]
+#[derive(Clone, Debug, Hash, PartialOrd, Ord)]


If you're not deriving PartialEq, you can't derive Hash, PartialOrd, or Ord.

Should rustc catch that?

It should, yes.

Hash is fine, it's the Ord traits which are problematic.

Probably... please file a bug (https://github.com/rust-lang/rust/issues/new).

Err, how is Hash not a problem. From https://doc.rust-lang.org/nightly/std/hash/trait.Hash.html: "if two keys are equal, their hashes should also be equal".

Can rustc derive a correct Eq impl from a custom Hash impl?

Nope. It doesn't, and it's not possible to either, since there isn't enough information on what choices to make. In general an unsolvable problem.

Why not? Return hash(self) == hash(other) from eq method.

Anyway, thanks for the quick review, I'll get to work.

On Mon, Nov 02, 2015 at 01:15:37PM -0800, Manish Goregaokar wrote:

@@ -226,7 +226,7 @@ pub enum SchemeData {
}

/// Components for URLs in a relative scheme such as HTTP.
-#[derive(PartialEq, Eq, Clone, Debug, Hash, PartialOrd, Ord)]
+#[derive(Clone, Debug, Hash, PartialOrd, Ord)]

Nope. It doesn't, and it's not possible to either, since there isn't enough information on what choices to make. In general an unsolvable problem.

Reply to this email directly or view it on GitHub:
https://github.com/servo/rust-url/pull/133/files#r43683364

Hashes may have collisions. Equal keys imply equal hashes, not the other way around.

BTW I've filed the derive(Hash) issue against rustc: rust-lang/rust#29758


        Make port: None equal to port: default_port

Fix #132

untitaker · 2015-11-04T00:17:38Z

I've updated the PR with custom Hash and Eq impls.

Should I do the same for Ord? Do we even need to order URLs?

untitaker · 2015-11-10T18:48:50Z

Any update on this? Is there something left to do?

Manishearth · 2015-11-10T19:22:35Z

LGTM, but I'll wait for Simon to review it

SimonSapin · 2015-11-10T19:57:51Z

Yes, Ord and PartialOrd should be consistent with Eq and PartialEq. Please change them to use the identity key as well, and add those tests you mentioned :)

I also think that ordering URLs doesn't make sense, but Cargo wants to sort things that contain them: #114


        Manually impl Ord, PartialOrd


        Add tests

untitaker · 2015-11-10T20:41:18Z

Added some tests for equality.
I'm not sure how ordering should look like. Do we want the same exact ordering behavior as before, i.e. do we need extensive tests? Or is breakage fine as long as it doesn't happen too often? It's "obvious" that this new impl is deterministic. cc @alexcrichton
The hashing tests are ugly, check_eq was supposed to be a macro but then I somehow hit a ICE

untitaker · 2015-11-10T21:13:39Z

src/lib.rs

+}
+
+
+impl PartialEq for RelativeSchemeData {


I'd be interested into factoring out all these boilerplate impls into a new crate. Would that be something useful, and would it be worth adding as dependency?

I haven't decided on an API yet, but perhaps a macro that impls PartialEq, Hash, etc given a type and a function like get_identity_key.

SimonSapin · 2015-11-10T22:44:34Z

@bors-servo r+

Looks good, thanks!

It’s not important that the ordering is the same as before. Only that it’s consistent (with the usual properties: total, anti-symmetric, transitive) within one execution of a program. The current impls are fine. I don’t think ordering tests are important here, the impls are very straightforward.

Yes, the impls are boilerplate-ish, but they’re only one line each. I’m skeptical that it’s useful to have a crate just for this, at least until there’s more than one thing that would use it.

bors-servo · 2015-11-10T22:44:36Z

📌 Commit ab7f6fc has been approved by SimonSapin

bors-servo · 2015-11-10T22:46:12Z

⌛ Testing commit ab7f6fc with merge fedf144...


        Auto merge of #133 - untitaker:issue-132-port-eq, r=SimonSapin

Make port: None equal to port: default_port Fix #132  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/133)

bors-servo · 2015-11-10T22:50:56Z

☀️ Test successful - travis

untitaker · 2015-11-10T22:51:04Z

within one execution of a program

I don't exactly know where Cargo uses them, but I thought that it'd produce massive diffs for e.g. .lock-files when upgrading cargo and then running cargo update. I guess that's not a real issue though.

SimonSapin · 2015-11-11T16:01:15Z

@alexcrichton, how do you feel about the ordering of URLs changing?

alexcrichton · 2015-11-11T18:54:42Z

Does this radically change how URLs are ordered, or is it basically in just some niche cases the ordering gets "more correct" ?

SimonSapin · 2015-11-11T19:03:53Z

URLs that do not specify a port number now sort as if they specified the scheme’s default port number. If none of the URLs in a given set specify it (which is the common case), I think the relative order is is unchanged.

alexcrichton · 2015-11-11T19:10:44Z

Eh that seems fine. The ordering of URLs is already a little ambiguous anyway I think, so tweaking it a bit should be fine.

eefriedman reviewed Nov 2, 2015
View changes

Make port: None equal to port: default_port

Loading status checks…

b999b7d

Fix #132

untitaker force-pushed the untitaker:issue-132-port-eq branch from 355375f to b999b7d Nov 3, 2015

metajack assigned SimonSapin Nov 3, 2015

untitaker added 2 commits Nov 10, 2015

Manually impl Ord, PartialOrd

Loading status checks…

3e72c19

Add tests

Loading status checks…

ab7f6fc

untitaker force-pushed the untitaker:issue-132-port-eq branch from 4a20858 to ab7f6fc Nov 10, 2015

untitaker reviewed Nov 10, 2015
View changes

bors-servo merged commit ab7f6fc into servo:master Nov 10, 2015
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

untitaker deleted the untitaker:issue-132-port-eq branch Nov 11, 2015

untitaker mentioned this pull request Feb 13, 2016

Lint: Warn if only one of {Hash, PartialEq} is derived, the other manually implemented rust-lang/rfcs#1499

Closed

servo / rust-url

Make port: None equal to port: default_port #133

Make port: None equal to port: default_port #133

untitaker commented Nov 2, 2015

untitaker commented Nov 2, 2015

untitaker commented Nov 2, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

untitaker commented Nov 4, 2015

untitaker commented Nov 10, 2015

Manishearth commented Nov 10, 2015

SimonSapin commented Nov 10, 2015

untitaker commented Nov 10, 2015

This comment has been minimized.

SimonSapin commented Nov 10, 2015

bors-servo commented Nov 10, 2015

bors-servo commented Nov 10, 2015

bors-servo commented Nov 10, 2015

untitaker commented Nov 10, 2015

SimonSapin commented Nov 11, 2015

alexcrichton commented Nov 11, 2015

SimonSapin commented Nov 11, 2015

alexcrichton commented Nov 11, 2015

servo / rust-url

Join GitHub today

Make port: None equal to port: default_port #133

Make port: None equal to port: default_port #133

Conversation

untitaker commented Nov 2, 2015

untitaker commented Nov 2, 2015

untitaker commented Nov 2, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

untitaker commented Nov 4, 2015

untitaker commented Nov 10, 2015

Manishearth commented Nov 10, 2015

SimonSapin commented Nov 10, 2015

untitaker commented Nov 10, 2015

This comment has been minimized.

SimonSapin commented Nov 10, 2015

bors-servo commented Nov 10, 2015

bors-servo commented Nov 10, 2015

bors-servo commented Nov 10, 2015

untitaker commented Nov 10, 2015

SimonSapin commented Nov 11, 2015

alexcrichton commented Nov 11, 2015

SimonSapin commented Nov 11, 2015

alexcrichton commented Nov 11, 2015