Version 1.0, rewrite the data structure and API

[SimonSapin]

Do not merge yet, I’d like to get feedback on API design.

What’s changing

This is a large rewrite of the data structures and API that aims to eventually become version 1.0.0. Motivations are:

• I designed rust-url when I was relatively new to Rust, coming from Python where memory allocation and ownership are not concerns. The URL Standard also writes algorithms to optimize for human comprehension, not computer resources. As a result, each component of the Url struct is a String or a Vec<String>.
• At the same time, I tried to use enums to enforce some invariants in the type system. For example, “non-relative” like data: don’t have a host or port number. In practice I think this makes the API a pain to use more than it’s worth.
• The spec has changed a lot (notably around dealing with protocols/schemes outside the small list it knows about), and I’m not sure the API can be adapted backward-compatibly anyway.

I think the API can be more "rusty", and a bunch of long-standing issues be fixed along the way. The big idea is that the Url struct now contains a single String of the URL’s serialization (the only heap memory allocation), plus some indices into it (and some other meta-data). The indices allow accessor methods for URL components to return &str in O(1) time.

The fields of Url are now private, as they need to maintain a number of invariants. Getting invariants wrong is not a memory-safety hazard (e.g. string indexing is still checked) but can cause non-sensical results.

Rather than multiple ad-hoc methods like serialize_authority and serialize_without_fragment, Url implements indexing with ranges of Position, which is an enum indicating a position between URL components. For example, serialize_without_fragment is now &url[..Position::AfterQuery], takes O(1) time, and returns a &str slice.

While we’re making breaking changes, this is an opportunity to clean up APIs that didn’t turn out so great:

• Drop UrlParser. The builder pattern works well for std::process::Command, but for URLs in practice almost only base_url is ever used. v0.5.2 already has base_url.join(str), which stays the same.
• Drop all the lossy_percent_decode_* methods. They don’t seem to be used at all. Maybe accessors could return a PercentEncodedStr wrapper that dereferences to &str and has a percent_decode() method?
• In the percent_encoding module, there was “duplicated” APIs that returned a new String or Vec<u8> for convenience, or pushed to an existing &mut String or &mut Vec<u8> for flexibility. This was a bit awkward, so I’ve changed it to return iterators of char or u8 which can be used with .extend(…) or .collect(). I’m not sure this an improvement and might revert it. Feedback appreciated.

Downsides

This new design is great for accessing components, but not so much for modifying them. We need to make changes in the middle of the string, and with percent-encoding we don’t know the encoded size of the new thing until we’ve encoded it. Then we need to fix up indices. As a result, the set_* methods have lots of fiddly code where it’s easy to make subtle mistakes. This code should be well-tested before this PR is merged, but it’s not at all yet.

Docs

To see what the new API looks like more easily than by reading the code, you can find the output of cargo doc at https://simonsapin.github.io/rust-url-1.0-docs/url/

Trying it out

Too see what the API usage is like in practice, I’ve updated some crates that use rust-url for this PR:

• https://github.com/alexcrichton/git2-rs/pull/115
• Update to rust-url 1.0 rust-lang/cargo#2428
• Update to rust-url 1.0 rwf2/cookie-rs#42
• Update to rust-url 1.0 hyperium/hyper#740
• https://github.com/cyderize/rust-websocket/pull/70
• Update to rust-url 1.0 servo#9840

Since this PR isn’t merged yet, to do the same in your crates you’ll need a clone of the git repository at the right branch:

git clone https://github.com/servo/rust-url --branch 1.0

… and a Cargo path override in the directory of your crate:

.cargo/config

path = [
    "../rust-url",
]

Feedback, please

Since we’re making breaking changes, this is the opportunity to make more (or different) breaking changes before version 1.0 is published and we commit to this new API.

Please comment here to say what are your pain points with the 0.5.x API, what could be improved, what I’ve done in this PR that doesn’t seem like a good idea, etc.

Thanks!

[Ms2ger]

WebIdl is a curious name; maybe DomApi?

[asajeffrey]

Would we be better off calling this version 0.7.0 and waiting for it to bake in before calling it 1.0.0?

[asajeffrey]

Is the copyright statement correct? IANAL but I think it should be copyrighted to Mozilla, at least for the work done as an employee.

[asajeffrey]

Since rust-url isn't on reviewable, do you want line comments via github?

[alexcrichton]

Wow, thanks for all this @SimonSapin! (especially the updates to some crates)

These changes all sound great to me from an API perspective, should certainly allow for a good amount of flexibility in rust-url after 1.0 has happened (without making a new major release).

I might recommend what @asajeffrey was mentioned, however, of holding off on 1.0 for now until there's some experience with this rewrite. Similarly libc avoided going to 1.0 initially and instead went to 0.2, and now that we're confident we'll move it to 1.0 when we can. That being said it's up to you. You've done a lot of the update work so you've probably got quite a good handle on what the API is like :)

[SimonSapin]

I’ve added a "Downsides" sections to the initial message. Setters are ugly.

@Ms2ger I’m ok with renaming it, though https://url.spec.whatwg.org/ doesn’t use "DOM" for this at all.

@asajeffrey rust-url is on reviewable, but I hid the link since I thought we might want to hold a full code review (it’s kinda big) until the API is settled. But feel free to use https://reviewable.io/reviews/servo/rust-url/176 if you prefer.

Re copyright, I have no idea. I don’t mind changing it if someone can provide legal advice. I vaguely remember that "authorship" (~copyright) can not be transferred in French law. (Though usage rights can be licensed I think?)

For the version number, I don’t mind making it 0.6 or something. 1.0 seemed appropriate to signal that it is "more breaking" than 0.2 -> 0.3 -> 0.4 -> 0.5 have been so far, and that crate is starting to be more "mature". I don’t think it matters a lot if future semver-breaking changes as needed are 0.6 -> 0.7 -> 0.8 or 1.0 -> 2.0 -> 3.0. However, I’d rather not make a semver-breaking release later gratuitously just to add a "not 0.x" stamp.

[seanmonstar]

Humongous 👍 from me.

If I could ask for 1 additional getter, it would be authority(), which is used in the HTTP 1.1 spec.

[SimonSapin]

@seanmonstar would &url[Position::BeforeUsername..Position::AfterPort] work? (Note that e.g. Position::BeforeUsername is well-defined even if there is no username.)

[seanmonstar]

@SimonSapin I believe that works. Is there desire to not add an authority() getter?

[asajeffrey]

Making sure this doesn't get lost in IRC... It might be nice to have:

impl Url { fn query_params<'a>(&'a self) -> QueryParams<'a> { ... } }
impl<'a> Iterator<(Cow<'a,str>,Cow<'a,str>)> for QueryParams<'a> { ... }

so people who want to can, e,g, build a hash table of the query parameters.

[SimonSapin]

@seanmonstar the idea is to have one generic mechanism (indexing) rather than a proliferation of ad-hoc methods for every combinations that various use cases might require.

[valenting]

I haven't gone through all the changes, but the old scheme_type_mapper was quite useful for Gecko integration, as chrome:// resource:// and a bunch of others work just like regular special protocols.
Is there a way of parsing these special schemes in the 1.0 version?

[blaenk]

I think this is great overall!

I think the indexing mechanism is interesting, and I think there is value in having it be general to this level, but I fear that not having some getters/helpers may be a negative impact on ergonomics. At least for the common components such as those currently delineated by the Position enum. People will wonder why they should have to type &url[Position::BeforeFragment..Position::AfterFragment] where "anywhere else" they could just do url.get_fragment().

Instead of thinking about what component someone wants and just using it, they'll have to know/lookup the "indexing language" in order to determine which indexing positions are appropriate, which may not always be immediately obvious for more obscure components. Sure, the positions are pretty self-explanatory, but I still think it incurs a context-switch.

What do you think about having something like a Component enum which could include some common ones like Path, Fragment, etc. which can simply be translated to the appropriate Position range. Then we could impl Index<Component> for Url so we can do &url[Componet::Fragment]. Now you don't have to worry about ad-hoc method blow up, it's all confined to Component, built atop the more general Position foundation.

Failing that, I think a good compromise would be to continue to have some getters/helpers for the most basic/common components, while still being able to access pretty much any component thanks to the more general indexing mechanic you introduced.

We could define these in a separate module as free functions or in some kind of UrlComponentGetters trait which someone can import to have access to the various helpers, with the latter perhaps being more ergonomic than the former.

[SimonSapin]

@valenting With whatwg/url@b266a43, the parsing algorithm in the spec changed to better support unknown schemes like chrome::

• If the scheme is followed by :// (colon and two slashes), the URL has all the components that http: URLs have (username, password, host, port number, path, query, fragment)
• Otherwise, if the scheme is followed by :/ (colon and a single slash) the URL has no "authority" (username, password, host, or port) but it has a path made of slash-separated segments (and query and fragment) and can still be used as a base URL to resolve input like ../foo.
• Otherwise (no slash after :), the URL is non-relative and can not be used as a base URL

The spec still has a concept of special scheme which is just http, https, ws, wss, ftp, gopher, and nothing else. But the only thing being "special" does is trigger some legacy parsing quirks like backslashes being accepted instead of slashes, and http:../foo being equivalent to ../foo in the base URL has the same scheme. I don’t think there’s a reason to want to opt into that.

[SimonSapin]

@blaenk

People will wonder why they should have to type &url[Position::BeforeFragment..Position::AfterFragment] where "anywhere else" they could just do url.get_fragment().

They don’t! Sorry this was unclear, Url has a bunch of getter methods for accessing one component at a time. (They just don’t have the get_ prefix.)

impl Url {
    pub fn scheme(&self) -> &str { … }
    pub fn username(&self) -> &str { … }
    pub fn password(&self) -> Option<&str> { … }
    pub fn host_str(&self) -> Option<&str> { … }
    pub fn port(&self) -> Option<u16> { … }
    pub fn path(&self) -> &str { … }
    pub fn query(&self) -> Option<&str> { … }
    pub fn fragment(&self) -> Option<&str> { … }
}

(There are corresponding set_* methods too.)

Indexing is for when you want more than one consecutive component, like "authority" (username, password, host, and port) or "path and query". You can get a &str slice cheaply rather than doing partial serialization yourself.

[blaenk]

@SimonSapin My apologies! I skimmed through the changes and completely missed that :(

Great work!

[valenting]

@SimonSapin That's great then! I think it shouldn't be too hard to update the C API.
I'm not super happy that we need to keep indexes around, and the setter code is very verbose. However, optimizing parsing and getters is the right call. We can probably build an abstraction to make the code more concise later on.
Great job!

[SimonSapin]

@valenting Yes, setters have some common patterns that probably can be refactored to share code. What alternative would there be to keeping indexes around?

[cmbrandenburg]

It fails to build for me on Windows.

$ cargo run
   Compiling matches v0.1.2
   Compiling unicode-normalization v0.1.2
   Compiling unicode-bidi v0.2.3
   Compiling url v1.0.0-dev (https://github.com/servo/rust-url.git?branch=1.0#d5ca46a7)
C:\Users\cb1033\.cargo\git\checkouts\rust-url-21e599028fd381de\1.0\src\lib.rs:1145:42: 1145:46 error: unresolved name `path` [E0425]
C:\Users\cb1033\.cargo\git\checkouts\rust-url-21e599028fd381de\1.0\src\lib.rs:1145     file_url_segments_to_pathbuf_windows(path)
                                                                                                                            ^~~~
C:\Users\cb1033\.cargo\git\checkouts\rust-url-21e599028fd381de\1.0\src\lib.rs:1145:42: 1145:46 help: run `rustc --explain E0425` to see a detailed explanation
error: aborting due to previous error
Could not compile `url`.

To learn more, run the command again with --verbose.

Here's my relevant Cargo.toml:

[dependencies]
url = { git = "https://github.com/servo/rust-url.git", branch = "1.0"}

[valenting]

@SimonSapin The only alternative would be keeping the segments as separate objects, which means more allocations, and worse getter performance, so this is probably the best option.
I wonder if we could abstract over the indexes in a way that hides them all together. Index math is very error prone.

PS. The bits in slicing.rs are simply wonderful!

[asajeffrey]

As a bit of data for the position API, the uses of it in Servo are:

url[Position::BeforePath..Position::AfterQuery]
url[..Position::AfterPath]
url[Position::BeforePath..]
url[..Position::BeforeFragment]

and:

Url::parse(&load_data.url[Position::BeforeUsername..])

The last one is a bit odd because the URL is being serialized then parsed, which seems like wasted effort.

Perhaps an API like:

Url::before_foo() -> Url
Url::after_foo() -> Url

would do the trick? So url[Position::Foo..Position::Bar] would become url.after_foo().before_bar().as_str(). Some thought might be needed about how to handle the separator chars, but in most cases the could just be dropped I suspect.

[asajeffrey]

PS: In order to avoid lots of copying, the above API would need us to change the backing store from a String to a Cow<'a,str>.

[nox]

PS: In order to avoid lots of copying, the above API would need us to change the backing store from a String to a Cow<'a, str>.

Say no to Cow, and yes to T: Borrow<str>, which allows more variations.

[SimonSapin]

@bors-servo r=asajeffrey

(Again after hitting "Synchronize" on http://build.servo.org/homu/queue/rust-url)

[bors-servo]

📌 Commit 4a59d93 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit 4a59d93 with merge e33f72f...

[bors-servo]

☀️ Test successful - travis

[SimonSapin]

🎊 🎊 🎊 🎊

Thanks everyone!

Please don’t cargo publish this yet, I’ll go through the open issues tomorrow to see if there’s a last-minute breaking change we want to make beforehand.

[sfackler]

@SimonSapin did the issues with push_path_component and an empty path get resolved?

[SimonSapin]

@sfackler That’s part of what I’ll look at tomorrow before publishing.

[SimonSapin]

Aaaaaand we’re live! https://crates.io/crates/url/1.0.0

[frewsxcv]

Awesome work @SimonSapin! 🎊

[flying-sheep]

aah, so serialize() is as_str() now!