RFC 1214 Regression Report -- where to go from here?

[nikomatsakis]

I did a crater run to compute the regressions that are still out there from RFC 1214. This converted RFC 1214 warnings into hard errors. It also took advantage of the cap-lints flag to try to better distinguish root failures from other failures: in particular, warnings were only converted to errors for the main crate being compiled. I think that this means that in principle we care about both root and non-root regressions, though the difference here is not large (and indeed the one non-root regression I investigated seemed to be a true non-root regression, where the error was from some other crate, I’m not sure why that is, perhaps my cap-lints code didn’t work properly). I haven’t really dug into the results in detail but the few spot checks I did looked mostly legit.

https://gist.github.com/nikomatsakis/02c89fe605e644fc14d8

The interesting question then is what is the best strategy to go forward. There are still some bugs to fix related to RFC 1214, and we still have other soundness fixes to come (e.g., #25860). I have had the thought of trying to actively submit fixes to these projects to try and move things forward. I’m also not sure when the RFC 1214 work hit stable, I think perhaps just in this most recent release.

Another related problem that I noticed: the RFC 1214 warnings are hardcoded to be warnings because I didn’t want people to be able to suppress them without fixing the problem. But this also means that they can be overlooked! For example, I noticed a warning had appeared in rustc (turned out to be due to a bug), but we didn’t notice it because it only occurred in stage1/stage2, and since it was hard-coded to a warning, the #[deny(warnings)] flag that triggers in those stages didn’t cause it to be an error. One thing we might do to increase visibility of RFC 1214 warnings is to have them respect that flag.

Anyway, I see a few options:

1. Switch the warnings to hard errors.
2. Switch the warnings to hard errors, but include some way to temporarily revert them back to warnings if you opt-in. This mechanism would be removed in a future release.
3. Switch the warnings to hard errors, unless the “cap-lints” option is in use (e.g., more-or-less what I tested). This would mean you get errors, but only for the project you are actively compiling.
4. Leave them as warnings for now.

Somewhat orthogonally, we can choose to do active outreach to the affected projects:

1. Simply send emails.
2. Try and submit PRs or other fixes.

Thoughts?

cc @brson @aturon @nrc @pnkfelix @huon

[huon]

I’m inclined to say that we ratchet up the pressure to fix without making it a hard error given there’s quite a few regressions still around. Making it obey deny(warnings) seems like a good step in this direction (I’d even consider having this backported to beta so it gets into the biggest wild as soon as possible?), and then after a release of that, we switch to 2 or 3, and then finally 1 at some point in the future.

I’m also not sure when the RFC 1214 work hit stable, I think perhaps just in this most recent release.

It seems to be in beta, but not stable yet: http://is.gd/EIjHMm

[DanielKeep]

What about having some way of flagging crates on crates.io as having potential problems with an up-coming compiler? Perhaps a badge that shows up in the listings, and a banner that appears for any logged-in user whose account is associated with flagged crates?

cargo could also print out a warning if the user publishes any crates while there are flagged crates on their account.

To prevent crates “falling through the cracks” by being updated right after a crater run, have the warning be sticky until the user goes to their dashboard and specifically does something to clear the warning.

That way, the Rust team has another way to let crate owners know that Bad Things™ may be about to happen, even if they don’t read the forum or the subreddit.

[bluss]

I think we have successfully converted a lot of people to Rust’s stable releases. Since the warnings are not part of Rust 1.3, it makes sense that many authors have yet to discover this issue.

If it looks like this is going to be simple, almost all authors will fix their crates, then the breaks can be turned on the immediately following release (Rust 1.5). That doesn’t sound too bad. Is it realistic for 70-odd crates to update during 6 weeks? If we doubt it, break in Rust 1.6 instead.

[eefriedman]

You might want to double-check that these are actually all RFC1214 regressions. I just reduced https://github.com/rust-lang/rust/issues/29048 from https://github.com/jgraham/webdriver-rust/ .

[nikomatsakis]

You might want to double-check that these are actually all RFC1214 regressions

Yes, I do plan to go through them.

[brson]

Quick ideas.

Wait until all pending soundness fixes are ready to go and turn them on together to limit the repeating pain.

Whitelist these exact crates, leaving them as warnings, turning errors on everywhere else, to give the broken crates more time.

Submit patches upstream right now so that when the error hits stable in 6-12 weeks or whatever they will hopefully be redeployed. Although this is a lot of breakage, we could probably get a big chunk of the patches done in a day. I’d be happy to help with this.

[ahmedcharles]

Whitelist these exact crates, leaving them as warnings, turning errors on everywhere else, to give the broken crates more time.

This solution and others which prioritize the experience for crates.io above private code seem to have lower value or may be perceived as making crates.io a first class citizen with other code bases being second class. I don’t really have any important private code myself, but I do think this should be a consideration.

Other than that, I think the first step would be fixing the warning so it respects #[deny(warnings)], including in the beta if possible, because that just seems like a bug.

[nikomatsakis]

On the topic of #[deny(warnings)], the easiest solution here would be to make a new lint, called either “unsound” or “future_incompatible”. This lint might even default to error to give it some more oomph. Using a true lint means it would give us the “cap lints” behavior for free, which is cool.

One question is whether it’s worth ensuring that you ALWAYS get some sort of warning for a “future_incompatible”, or if we should allow people to do `#[allow(future_incompatible)]". I guess it’s not a big thing to ensure you always get a warning for this particular lint.

I would then rewrite all the calls to warn to be calls to add-lint.

[yongqli]

Hi all,

I have some code that breaks because of RFC 1214. How can I fix the errors?

Consider the code:

pub fn use_r<I, R>(xs: I, r: &R) {
    unimplemented!()
}

fn test<'a, R>(r: &'a mut R) {
    let a = |r: &'a mut R| {
        [(|| use_r(vec![0.].into_iter(), r))()]
    };
    a(r);
    // a(r);
}

This compiles on nightly, but I can’t call a(r); twice.

This version:

fn test2<R>(r: &mut R) {
    let a = |r: &mut R| {
        [(|| use_r(vec![0.].into_iter(), r))()]
    };
    a(r);
    a(r);
}

will no longer compile on nightly but is what I want.

Rust playground link

I have a weird closure inside a closure that I immediately invoke, but this is in fact what I want. What happens is that I have a macro that inlines invocations of a expression passed in as a closure. Is there a way to get this to work or will it no longer be supported?

[nikomatsakis]

Hmm, oddly removing the type annotations makes that build. Seems like this may be a bug. I’ll try to investigate further in a bit, but it would be helpful for tracking purposes if you filed an issue (and cc’d me, thanks).

[yongqli]

Sure thing: https://github.com/rust-lang/rust/issues/30829

[nikomatsakis]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.