Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crater run to estimate impact of full NLL transition #60680

Open
nikomatsakis opened this issue May 9, 2019 · 13 comments

Comments

Projects
None yet
5 participants
@nikomatsakis
Copy link
Contributor

commented May 9, 2019

In the @rust-lang/lang meeting today, we were saying it'd be a good idea to do a crater run against full NLL just to get a rough idea of how many affected crates there will be.

There are two ways we could do this and both have some value:

  • change the "warning lints" to "deny" and then do the crater run. This has the benefit that if crate X fails to compile, dependencies of crate X will still be tested, giving a more accurate overall picture.
  • change default borrowck mode to NLL. This has the advantage that some errors (notably regionck errors) that are no longer relevant are disabled. However, I think we will overall get an underestimate this way, plus there are still (maybe) some soundness holes.
@Centril

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I would suggest that we do both of the two ways since it's an important ticket.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 16, 2019

cc #43234

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 16, 2019

triage: this is an important task, but not one that I find should be prioritized over the various bugs currently plaguing the compiler.

Marking as P-medium.

I'm not sure why @Centril nominated it. Its possible that they nominated it solely for prioritization. Or its possible they wanted it to get attention from the compiler team. I'm going to leave the nomination tag on for now, with the understanding, that if we do discuss it at the T-compiler meeting and manage to assign it to someone, then we can remove the nomination tag.

@pnkfelix pnkfelix added the P-medium label May 16, 2019

@pnkfelix pnkfelix self-assigned this May 16, 2019

@pnkfelix pnkfelix removed the I-nominated label May 16, 2019

@lqd

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

I will try to help @pnkfelix with this.

@lqd lqd self-assigned this May 16, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@lqd looks like the easiest way to go is to open two distinct PR's corresponding to the two avenues outlined in the description, right?

@lqd

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

(we talked about this in this Zulip thread) yes and I opened #60911 and #60914 for these 2 avenues

bors added a commit that referenced this issue May 17, 2019

Auto merge of #60911 - lqd:nll_crater1, r=<try>
[DO NOT MERGE] NLL crater run 1: switch default borrowck mode from migrate to full NLLs

This switches the default borrowck mode from `migrate` to `mir` for one of the crater runs needed for #60680.

For the previous runs, we also specified crater to `cap-lints=warn`, we might want this again this time ?

r? @ghost

cc @pnkfelix, @Centril

bors added a commit that referenced this issue May 17, 2019

Auto merge of #60914 - lqd:nll_crater2, r=<try>
[DO NOT MERGE] NLL crater run 2: deny NLL warning lints in migrate mode

This keeps the borrowck migrate mode as the default (and thus does not suppress regionck errors like full NLLs would), but does not downgrade the errors ignored by AST borrowck into warnings, for the 2nd crater run needed for #60680.

For the previous runs, we also specified crater to cap-lints=warn, we might want this again this time ?

r? @ghost

cc @pnkfelix, @Centril, @matthewjasper
@lqd

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Copying triage results over from each PR:

#60914:

Of the 1877 regressions, these 87 crates caused 1859 crates to fail downstream, while these 54 crates themselves failed to build.

There is some overlap between these 2 categories: so this PR impacted around 130 root crates. I remember a lot of those from the previous crater runs, with legit errors — some of them fixed in more recent versions of the crates (including semver-compatible releases only requiring a cargo update to fix the NLL errors) .

#60911:

Here as well, of the 1998 regressions, these 99 crates caused 1892 failures downstream, while these 140 crates themselves failed to build.

That's 220 crates in total (all the 131 in #60914 + these 89 new ones).

@Centril

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Nominating for lang team (possibly also compiler team for execution) to discuss ^-- and how to proceed with that data.

@Centril

This comment has been minimized.

Copy link
Member

commented May 23, 2019

We did not get to discuss this on the language team meeting. Revisiting next time.

@Centril

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Sadly, we did not have enough time to discuss this on the language team meeting this week either, rescheduling for next time.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

discussed in lang team meeting.

The overall set of regressions was not a deal breaker for the people present in the meeting.

One trend we noted with the listed regressions is that a lot of the problems are coming from ... "old crates", and the main point will be "yep, someone needs to update their dependencies."

So, as part of that, is that we want to write a blog post discussing the overall plan here. in particular, we want to discuss the overall migration to NLL, stating that NLL is coming for every Rust edition, and then saying that the final transition (to hard errors) is fixing soundness bugs. So, part of that post will present a representative sample of real world code with bugs that are being detected by NLL.

(I am taking on the job of writing the aforementioned post; @nikomatsakis is going to provide guidance about where the post will end up being published; i.e. what forum is most appropriate for it. I plan to have the post ready in time for the next Rust release.)

@pnkfelix pnkfelix removed the I-nominated label Jun 20, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

(With my release team hat on, we will need to talk about NLL in 2015 in the 1.35.0 blog post in any case, and so linking to the aforementioned to-write blogpost would be a ideal).

@Stargateur

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Can you only list crate >= 1.0.0 ? Maybe sort by last update too.

I mean if I understand correctly, these crates "should" not compile, there are probably not maintains. Do we really have the ressource to keep them working for almost no gain ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.