http broken in latest nightly

[ZoeyR]

building http in the latest nightly of rust (2018-3-27 as of this writing) fails. A selection of the errors is below:

error: use of deprecated item 'std::ascii::AsciiExt': use inherent methods instead
  --> src\uri\mod.rs:32:5
   |
32 | use std::ascii::AsciiExt;
   |     ^^^^^^^^^^^^^^^^^^^^
   |
note: lint level defined here
  --> src\lib.rs:160:9
   |
160| #![deny(warnings, missing_docs, missing_debug_implementations)]
   |         ^^^^^^^^
   = note: #[deny(deprecated)] implied by #[deny(warnings)]

error[E0034]: multiple applicable items in scope
   --> src\method.rs:325:9
    |
325 |         Method::try_from(t.as_bytes())
    |         ^^^^^^^^^^^^^^^^ multiple `try_from` found
    |
    = note: candidate #1 is defined in an impl of the trait `std::convert::TryFrom` for the type `_`
note: candidate #2 is defined in the trait `convert::HttpTryFrom`
   --> src\convert.rs:22:5
    |
22  |     fn try_from(t: T) -> Result<Self, Self::Error>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: to disambiguate the method call, write `convert::HttpTryFrom::try_from(...)` instead

It would seem the recent stabilization of TryFrom as well as the deprecation of AsciiExt have broken http.

Side note: Is Rust allowed to cause breakage like this with new versions of the compiler? I would think that it would be problematic to do so. TryFrom seems to be an issue since it is included in the prelude.

[carllerche]

I don't know. I filed rust-lang/rust#49436.

[seanmonstar]

We can likely fix this internally. I doubt the rust team would consider this a breaking change.

[sfackler]

This is one of the things that's "expected breakage" but if it ends up being overly traumatic to the ecosystem we might back out.

[carllerche]

@sfackler define "overly traumatic". My understanding was that these sorts of "expected breakage" would go through an ecosystem "crater" (or whatever it is called) and if it broke things it wouldn't be applied.

I understand the problem that one must be able to add new fns / traits, but if breaking the http crate is considered "expected breakage", that seems to fail at Rust's promise for backwards compatibility as this will cascade down do everyone that depends on http as well as production Rust users.

The "fix" seems to be to not include TryFrom in std's prelude until the next Rust edition, which is coming soon.

[sfackler]

Yeah, we'll be looking at the crater report that runs for the first beta.

but if breaking the http crate is considered "expected breakage"

Are you arguing that the http crate is inherently special and must never be broken?

The "fix" seems to be to not include TryFrom in std's prelude until the next Rust edition, which is coming soon.

Yeah, that's probably what we'd do if we end up deciding to back the prelude part out. I don't think prelude changes were formally covered in the epoch RFC but it definitely seems like something we would be able to do.

[carllerche]

@sfackler

Are you arguing that the http crate is inherently special and must never be broken?

Um, i'm not sure I understand this statement, but what I was saying is that there are already many users of the http crate including in large scale production deployments.

I would hope that the Rust team considers backwards incompatible changes that impact production users as "special", no matter the crate.

[sfackler]

Almost any change to the API of any existing item has the ability to break third party code. We do not think it's reasonable to never be able to touch anything after it's first been added, so we say that we're allowed to make these changes anyway. At that point, some amount of downstream code may or may not break. If that's happened, there needs to be a change either in the standard library or in those downstream libraries. Depending on how many of those there are, we make a decision of how to move forward. Crates used by "production users" have been broken before in this kind of way, and they will be broken in the future.

Production users on stable have 6 weeks before this affects them in any way - this is why we have release trains. Production users on nightly should (hopefully) be pinning to a specific nightly to avoid being randomly broken whenever something lands that affects them. Cutting an http release with #194 will fix those production users. Removing TryFrom from the prelude will also fix them.

[seanmonstar]

I agree that there actually many many things that could break compiling (adding an inherent method to an item could break someone, if they implemented a trait for that item and there's now a name conflict).

In this case, we certainly can (and likely will, to at least prepare for the future) merge #194. However, the problem exists that anyone with a binary that upgrades compiler version, but doesn't want to upgrade their Cargo.lock, will suddenly see things break. A 0.1.5 release of http should be super easy to apply, but it's an unfortunate user experience.

Especially since they can't really opt-out, other than to not upgrade their compiler. That's because this trait is in the prelude.

[sfackler]

What opting out would be possible with other library changes that broke third party crates?

However, the problem exists that anyone with a binary that upgrades compiler version, but doesn't want to upgrade their Cargo.lock, will suddenly see things break.

Could you go into more detail about why people are in that situation?

[seanmonstar]

What opting out would be possible with other library changes that broke third party crates?

In that case, possibly none. However, many people likely hold the compiler and std to a higher standard, and std is special in that use std::prelude::* is magically injected into every module.

Could you go into more detail about why people are in that situation?

I cannot. I'm talking in theoreticals. The situation still exists, and while probably rare and sounds contrived, it's really not: with Rust's stability guarantees, people should feel free to upgrade their compiler whenever. If they do so, and suddenly see their app doesn't compile, they'll be less trusting of upgrading.

[carllerche]

@sfackler It's mostly that the stable version of Rustc is not included in Cargo.lock. It has been my understanding that pinning a project to a stable version of rustc should not be needed.

The issue arises when you clone a Rust repo, run cargo build, it should work.

If I am mistaken, then it should become recommended as good practice to include a rustup toolchain file (https://github.com/rust-lang-nursery/rustup.rs/#the-toolchain-file) in projects.

For example, Conduit transitively depends on http and has all the versions locked. Releases are frozen in time and we would like them to be able to be compiled reliably even in the future.

Up until today, we have not really pinned the Rustc version in the repo, so if the TryFrom change lands, it will break our past release builds. Maybe this is a bad assumption and we can’t rely on rustc stability. It would be good to figure out best practices

[sfackler]

with Rust's stability guarantees, people should feel free to upgrade their compiler whenever.

I don't understand what stability guarantees you're referring to. We have added 14 methods to Iterator and 2 methods to Ord, both traits in the prelude, since 1.0 and those additions have broken crates multiple times (for_each being a recent notable case). We identify these cases explicitly as minor changes in our stability guarantees. The prelude isn't mentioned in that RFC, but adding a new trait to the prelude is IMO roughly equivalent to adding new methods to existing traits in terms of the types of breakage it can produce.

[carllerche]

@sfackler

Regarding the section you linked:

All that said, it is incumbent on library authors to ensure that such "minor" changes are in fact minor in practice: if a conflict like t.foo() is likely to arise at all often in downstream code, it would be advisable to explore a different choice of names. More guidelines for the standard library are given later on.

The main point is that the Rust team has advertised stability as a feature. To downstream users, it doesn't actually matter if a change is minor or not, it matters if their build will break. If the Rust team is not reasonably able to provide a guarantee of stability, that is fine but should be advertised and downstream users can adjust best practices by, for example, pinning the stable version of Rust.

Again if this change lands on Rust stable, it will break the existing frozen Conduit builds due to the fact that we did not pin the version of Rust but dependencies are pinned.

[tirkarthi]

Seems the change was reverted for now : rust-lang/rust#49518

[carllerche]

Thanks