Turn the RFC1592 warnings into hard errors

[arielb1]

The warnings have already reached stable, and I want to improve the trait error reporting code.

Turning warnings into errors, this is obviously a [breaking-change].

r? @nikomatsakis

cc @rust-lang/compiler

[nikomatsakis]

Started a crater run comparing HEAD vs HEAD^.

[nikomatsakis]

Crater run yielded 43 regressions, but it seems probably many are false positives (haven't investigated yet): https://gist.github.com/nikomatsakis/9006df8ddc4cee518d38732cacfe8e7c

[eddyb]

Most of them are timeouts / SSL errors.
8 of them are trait methods that return Self: aurum-numeric, comonoid, jobsteal, lense, ndarray, regex_dfa, rendarray, vectormath.
The other one, shapir, uses the type Box<std::error::Error + Sized>, which must be an accident.

[nikomatsakis]

I will try to do another crater run since the timeout / SSL problems should be less bad now.

In the meantime: cc @Daggerbot @srijs @rphmeier @james-darkfox @bluss @jneem @davll @workanator, crates of yours might be affected by this upcoming bugfix! The changes needed though are pretty small for the most part.

[rphmeier]

@nikomatsakis thanks for the heads up

(fixed in jobsteal 0.5.1)

[bluss]

Thanks for the heads up, ndarray was fixed in version 0.5.1 (which was released in april 2016).

[nikomatsakis]

(My next attempt at a crater run failed again, btw, and I'm not entirely sure why.)

[brson]

I'll give crater a shot.

[nikomatsakis]

I re-ran crater and got this report: https://gist.github.com/nikomatsakis/41f3a69b6b37a65a2575eba69c716aed

[nikomatsakis]

That report shows 12 root regressions and 18 total. However, those root regressions are not actually root regressions -- many of them seem to be from dependent crates, and I think that most of those are outdated versions where a fix is available. Here is a categorization:

• vectormath v0.1.1 (returns (Self, Self)) (davll/vectormath-rs#6)
• ndarray v0.3.1, v0.4.9 (returns (Self, Self::Elem)) (fixed in v0.5.1)
• shapir v0.0.8 (using Sized as an object) (#34982 (comment))
• num v0.1.34 (returns (Self, Self::Elem)) (no longer seems to be true on master)
• jobsteal v0.4.3 (returns (Self, Self)) (acknowledged)
• comonoid v0.2.1 (returns (Self, Self)) (srijs/rust-comonoid#2)
• lense v0.1.2 (returns (Self, ...)) (#34982 (comment))
• regex_dfa v0.4.0 (returns (Self, Vec<_>)) (problem seems to be fixed in v0.5.0)
• aurum_numeric v0.1.2 (returns (Self, Self)) (#34982 (comment))

One non-data-point:

• parse-macros-0.1.0 -- time out

[nikomatsakis]

I would like feedback from @rust-lang/core / @rust-lang/compiler members. Basically these are two warnings that were made for oversights in the type system. Most crates seem to work but there are some regressions. More homework is needed, I think, to verify which of these are outdated crates, and to make sure all owners are contacted. Other than that, I think we should go ahead and convert these warnings into errors, particularly since the infrastructure for them is painful to maintain.

We could however take intermediate steps instead -- convert to deny-by-default?

I'm trying to get a feeling for when it is appropriate to "throw the switch".

[nikomatsakis]

More homework is needed, I think, to verify which of these are outdated crates, and to make sure all owners are contacted.

Along these lines, it's really tedious to comb through crater results right now. There has to be a way to make this more automated.

[workanator]

@nikomatsakis thank you for letting know. Will fix my crate.

[workanator]

@nikomatsakis I get rid of Sized in my crate (shapir) and published the new version. I hope that will help.

[nikomatsakis]

@workanator great! :)

[nikomatsakis]

OK, so I decided that we should land this, but only once the crates we identified are all either fixed or have pending PRs. To that end, I opened up for vectormath davll/vectormath-rs#6, but I didn't get to the others yet.

One weird thing I found is that the num crate doesn't seem to get any warnings locally -- is this a bug in the warning system? Are they being squashed somehow? Or was it already fixed locally? Haven't had time to figure it out yet.

[alexcrichton]

@nikomatsakis just to recap, these warnings have hit stable, right? If so, how many releases?

We have a new release coming out a week from today, so it may be worth waiting a week to miss the beta train as well perhaps? (would give an extra cycle with these as stable warnings).

Given the low impact, ease of fixing, burden on the compiler, and analysis, I'd personally be fine turning into hard errors.

[bors]

☔️ The latest upstream changes (presumably #35592) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

@alexcrichton

just to recap, these warnings have hit stable, right? If so, how many releases?

I believe so, but I'm not sure how many releases. Would be good to verify.

Given the low impact, ease of fixing, burden on the compiler, and analysis, I'd personally be fine turning into hard errors.

Makes sense; I'd still prefer if we have PRs up. It's not that many repos. Though it takes a surprisingly long time to "context switch" around. I'm still a bit confused why I don't get any warnings for the num crate.

[nikomatsakis]

I tried to make a PR for lense but the current master has a bunch of (unrelated) compilation errors, from what I can see.

cc @trustless-fox -- warning that we will be converting some future-compat warnings that may or may not affect your current code into hard errors. =)

[nikomatsakis]

I could not find the repository for aurum-numeric anymore. cc @Daggerbot -- the existing version of this crate on crates.io may be affected by this bugfix here, which will be converted soon from a warning into an error. See #33242 for details! :)

[nikomatsakis]

OK, I checked off all the broken crates, but @arielb1 it looks like we need a rebase here. If you rebase, r=me! :)

[nikomatsakis]

@bors r+

[bors]

📌 Commit 86f41e8 has been approved by nikomatsakis

[bors]

⌛️ Testing commit 86f41e8 with merge 1072f84...

[bors]

💔 Test failed - auto-mac-64-opt

[arielb1]

I managed to revert an LLVM upgrade. Un-revert it.

@bors r=nikomatsakis

[bors]

📌 Commit 7b92d05 has been approved by nikomatsakis

[bors]

⌛️ Testing commit 7b92d05 with merge 147371f...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: nikomatsakis
Pushing 147371f to master...

[james-darkfox]

@nikomatsakis Please remove lense from your tests. It was a PoC prototype that has since been succeeded; the [unpublished] successor is crystal. Thanks in advance.

[nikomatsakis]

@james-darkfox very good. I can't easily remove from the tests, but I'll try to remember not to bother you. ;)

[james-darkfox]

@nikomatsakis Does it iterate over crates.io ? I could send a dummy crate. If it checks git, then that repo will be removed soon anyway. Thanks; and keep sure to bother me if anything else breaks! :-)