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.

Sign up for GitHub

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

Jump to bottom

[nll] bootstrap rustc with NLL enabled #51823

Closed
nikomatsakis opened this issue Jun 26, 2018 · 8 comments
Closed

[nll] bootstrap rustc with NLL enabled #51823

nikomatsakis opened this issue Jun 26, 2018 · 8 comments
Labels
NLL-complete Working towards the "valid code works" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

As discussed in today's meeting, we need to get rustc bootstrapping with NLL enabled -- if there are problems, we should isolate them.

@lqd is going to try and push this through and report back.
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll NLL-complete Working towards the "valid code works" goal labels Jun 26, 2018
@lqd lqd self-assigned this Jun 26, 2018
@nikomatsakis nikomatsakis mentioned this issue Jun 26, 2018
Closed
@lqd
Copy link
Member

lqd commented Jun 29, 2018 •
edited
Loading

Here's a summary of this adventure :)

I'll ignore the cases where MIR borrowck correctly adds new "unused mut" lint warnings which were previously not found by the AST borrowck: while they are new warnings, the unused muts were easily removed from the rustc code in order to keep building, so I marked them as ok (but can produce the list of cases if need be).

I tested by manually enabling the NLL feature in crates, #![cfg_attr(not(stage0), feature(nll))], and building. This doesn't test the submodules, stdsimd, and tools.

Interesting cases

Cases of incorrect "unused mut" lint warnings

Crates without issues
  • liballoc_jemalloc: seems ok (unsure whether it's built when jemalloc is disabled in the config.toml)
  • liballoc_system: seems ok
  • libarena: ok
  • libfmt_macros: ok
  • libgraphviz: ok
  • libpanic_abort: ok
  • libpanic_unwind: ok
  • libproc_macro: ok
  • libprofiler_builtins: ok
  • librustc_allocator: ok
  • librustc_apfloat: ok
  • librustc_asan: ok
  • librustc_borrowck: ok
  • librustc_codegen_llvm: ok
  • librustc_codegen_utils: ok
  • librustc_cratesio_shim: ok, but it's just 2 extern crates
  • librustc_errors: ok
  • librustc_incremental: ok
  • librustc_llvm: ok
  • librustc_lsan: ok
  • librustc_metadata: ok
  • librustc_mir: used to ICE with region_obligations not empty (known issue region_obligations not empty #51649). Now ok.
  • librustc_msan: ok
  • librustc_platform_intrinsics: ok
  • librustc_plugin: ok
  • librustc_privacy: ok
  • librustc_resolve: ok
  • librustc_save_analysis: ok
  • librustc_target: ok
  • librustc_tsan: ok
  • librustdoc: ok
  • libstd: ok
  • libstd_unicode: the little of what is there is ok :)
  • libsyntax_pos: ok
  • libterm: ok
  • libtest: ok
  • libunwind: ok
  • rustc: if it is built by x.py build, then it is ok.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jun 29, 2018 •
edited by matthewjasper
Loading

Tagging this meta bug as NLL-deferred until the following issues are fixed:

This was referenced Jul 29, 2018
Merged
Merged
@matthewjasper
Copy link
Contributor

Now that all of those issues are fixed, I've added #52671 to the list since it is also blocking bootstrap.

With that fix bootstrapping still breaks due to the unused mut lint and a case where two phase borrows don't track activation through unsized coercions. I've created PRs for those.

@matthewjasper matthewjasper mentioned this issue Jul 29, 2018
Merged
bors added a commit that referenced this issue Jul 30, 2018
@bors
Auto merge of #52830 - matthewjasper:bootstrap-prep, r=matthewjasper
e437841 
[NLL] Fix some things for bootstrap

Some changes that are required when bootstrapping rustc with NLL enabled.

* Remove a bunch of unused `mut`s that aren't needed, but the existing lint doesn't catch.
* Rewrite a function call to satisfy NLL borrowck. Note that the borrow is two-phase, but gets activated immediately by an unsizing coercion.

cc #51823
pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 1, 2018
@pietroalbini
Rollup merge of rust-lang#52834 - matthewjasper:allow-zst-conflicts, …
d5fcd27 
…r=pnkfelix

[NLL] Allow conflicting borrows of promoted length zero arrays

This is currently overkill as there's no way to create two conflicting borrows of any promoted.
It is possible that the following code might not fail due to const eval in the future (@oli-obk?). In which case either the array marked needs to not be promoted, or to be checked for conflicts

```rust
static mut A: () = {
    let mut y = None;
    let z;
    let mut done_y = false;
    loop {
        let x = &mut [1];  // < this array
        if done_y {
            z = x;
            break;
        }
        y = Some(x);
        done_y = true;
    }
    some_const_fn(y, z); // some_const_fn expects that y to not alias z.
};
```

r? @pnkfelix  @nikomatsakis

closes rust-lang#52671
cc rust-lang#51823
@matthewjasper
Copy link
Contributor

matthewjasper commented Aug 2, 2018 •
edited
Loading

Ok so rustc almost bootstraps, but changes since my last comment prevent it from doing so (more unused mut that AST misses, an issue with incorrect lifetime annotations). At this point I think that this will have to be checked with CI.

@Mark-Simulacrum @nikomatsakis @pnkfelix was there a decision on how we want to move rustc/CI towards nll?

@Mark-Simulacrum
Copy link
Member

I sort of presume that we can enable -Zborrowck=mir across all compilations in rust-lang/rust as soon as we believe the performance is good enough and the compiler works with it.

@nikomatsakis nikomatsakis mentioned this issue Aug 3, 2018
Closed
23 tasks
@pnkfelix
Copy link
Member

pnkfelix commented Aug 3, 2018

Or at least -Z borrowck=migrate -Z two-phase-borrows. I'm not sure about going all the way to -Z borrowck=mir -Z two-phase-borrows in our first step here.

@pnkfelix
Copy link
Member

pnkfelix commented Aug 3, 2018

Although, what am I talking about, we control our own code, we don't need to use the migration path.

@matthewjasper
Copy link
Contributor

Closing in favor of #53172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NLL-complete Working towards the "valid code works" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nikomatsakis @pnkfelix @lqd @Mark-Simulacrum @matthewjasper