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

non-lexical lifetimes (NLL) tracking issue #43234

Open
nikomatsakis opened this issue Jul 14, 2017 · 61 comments

Comments

@nikomatsakis
Copy link
Contributor

commented Jul 14, 2017

This issue tracks the status of the transition to non-lexical lifetimes and a MIR-based borrow-checker. Both of these are jargon-y terms for compiler authors: the effect of these features on end-users is, simply put, that the compiler accepts a wider range of code with fewer bugs (and, hopefully, better error messages). We will refer to the combination of the above as NLL.

Key facts for getting involved

Implementation plan

  • Initial prototype: features available on nightly builds with #![feature(nll)] for experimentation.
  • Valid code works: (issues labeled with NLL-complete)
    • all run-pass tests pass
    • bootstrap builds with nll enabled: see #51823.
      • summary: We have bootstrapped at least once. We need to integrate such bootstrapping into the CI to ensure it stops breaking.
    • crater run with nll enabled passes without regressions
      • see #52217 for the set of ICEs from recent crater run.
  • Invalid code gets errors: (issues labeled with NLL-sound)
    • compile-fail/ui tests generate errors in the right places (possibly with bad diagnostics)
      • see #52663 as evidence that we generate errors in all cases where expected for the ui test suite. (Current plan for compile-fail suite is to port majority of those tests to ui)
  • Performance is good (issues labeled with NLL-performant)
    • see this dropbox paper which tracks profiling information
    • see also the NLL perf dashboard which compares running cargo check without and with NLL turned on, so that we can see a summary of the current performance impact on the compiler's static analyses in isolation.
  • Future compatibility warnings on stable: Once crater runs are clean, we can enable the future-compatibility warnings on stable: see #46908 and #52681
    • currently, due to #52681, we are issuing compatibility warnings on the 2018 edition.
    • next step is to migrate the 2015 edition in the same manner (#57804)
  • Diagnostic parity: (issues labeled with NLL-diagnostics) Ensure that all diagnostics are "as good or better" than before.
  • transition from migration mode to full NLL
    • NLL warnings (downgraded from errors) become errors again
    • as an intermediate step, potentially use a deny-by-default lint before making these hard errors
  • Feature stabilized: NLL and MIR-based borrow checker enabled by default, AST-based borrow checker removed: #57895

Many of the work items for unchecked bullets above are gathered into one place on #57895

How to join the working group

If you'd like to help, please join the NLL working group -- just leave a comment below or ping @nikomatsakis on gitter and you will be added to the @rust-lang/wg-compiler-nll team. You will then get occasional pings (e.g., when new mentoring instructions are available or when looking for help), as well as being eligible to be assigned to issues and so forth. We discuss things on the WG-compiler-nll channel on Gitter.

How to find an issue to work on

To find important issues, use one of the following queries:

You may also wish to look for E-mentor issues, which means that they have mentoring instructions. Also, if an issue is assigned, then someone is supposed to be working on it, but it's worth checking if they have made progress and pinging them -- people often get busy with other things.

Issues tagged with NLL-deferred are low priority right now, but if one strikes your fancy, feel free to tackle it!

How are issues are organized

All issues related to NLL are tagged with WG-compiler-nll. They are further tagged with a NLL-foo label to indicate a subcategory. Issues that have no NLL-label are considered "untriaged" and need to be sorted. Issues tagged with NLL-deferred are low priority right now.

Finally, you can always take a look at the full list of NLL-related issues.

In particular, issues tagged with E-mentor are those that contain mentoring instructions that can help you get started.

Issues (and pull requests) tagged with I-nominated are meant to be reviewed by the WG-compiler-nll at each weekly meeting. Here's the current nominated list.

If you can't find anything, reach out to @nikomatsakis on gitter.

Other issues

This section tracks related issues and notes.

Bugs in AST borrow check fixed in MIR borrowck

#47366 tracks known bugs from the AST borrow checker that were fixed in the MIR borrow checker. For each such bug, we have added a test to the repo with #![feature(nll)] and then closed the relevant issue (obviously, though, the bug will still be reproducable until the MIR-based borrow checker is stabilized, presuming one uses the AST-based borrow checker). You can also search for things tagged with NLL-fixed-by-NLL.

Questions to be resolved before stabilization
  • #46036: infinite loop false edges
  • NLL should identify and respect the lifetime annotations that the user wrote #47184
Possible extensions
  • refine liveness with maybe-initialized at greater resolution than a single variable
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

I've written up a gist with the initial draft of a plan. It lays out the first few steps anyway and some of the initial vision. I'll just copy-and-paste the end here, which lays out some of the PRs I think would make sense:

(list moved to header)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

@Nashenas88 has expressed some interest in doing this! It occurs to me that this project may be big enough for multiple people, too.

cc @rust-lang/compiler

bors added a commit that referenced this issue Jul 17, 2017

Auto merge of #43271 - Nashenas88:nll, r=nikomatsakis
Add empty MIR pass for non-lexical lifetimes

This is the first step for #43234.

bors added a commit that referenced this issue Jul 18, 2017

Auto merge of #43271 - Nashenas88:nll, r=nikomatsakis
Add empty MIR pass for non-lexical lifetimes

This is the first step for #43234.

bors added a commit that referenced this issue Jul 20, 2017

Auto merge of #43271 - Nashenas88:nll, r=nikomatsakis
Add empty MIR pass for non-lexical lifetimes

This is the first step for #43234.

bors added a commit that referenced this issue Jul 28, 2017

Auto merge of #43324 - Nashenas88:visit_locations, r=arielb1
Provide positional information when visiting ty, substs and closure_substs in MIR

This will enable the region renumbering portion of #43234 (non-lexical lifetimes). @nikomatsakis's current plan [here](https://gist.github.com/nikomatsakis/dfc27b28cd024eb25054b52bb11082f2) shows that we need spans of the original code to create new region variables, e.g. `self.infcx.next_region_var(infer::MiscVariable(span))`. The current visitor impls did not pass positional information (`Location` in some, `Span` and `SourceInfo` for others) for all types. I did not expand this to all visits, just the ones necessary for the above-mentioned plan.

bors added a commit that referenced this issue Aug 10, 2017

Auto merge of #43559 - Nashenas88:nll-region-renumberer, r=arielb1
Non-lexical lifetimes region renumberer

Regenerates region variables for all regions in a cloned MIR in the nll mir pass. This is part of the work for #43234.

@pnkfelix pnkfelix added this to the impl period milestone Sep 15, 2017

@pnkfelix pnkfelix added the E-mentor label Sep 15, 2017

@aturon aturon removed this from the impl period milestone Sep 15, 2017

@spastorino

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

have talked with @nikomatsakis and I'm joining this group 😄

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 29, 2017

Rollup merge of rust-lang#44845 - SimonSapin:nll_mod_rs, r=nikomatsakis
Move src/librustc_mir/transform/nll.rs to a subdirectory

CC rust-lang#43234
@chrisvittal

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2017

With #45013 pretty much set. I'm willing to work on testing infrastructure. I just need to know what we want and what parts of the code I need to look at.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

For those who will hack on NLL: right now, the "tip" of NLL development has not yet landed in master. In order to keep things moving, we're adopting the following strategy.

There is a nll-master branch on my fork of Rust. This contains the NLL work that is considered "done". You can open PRs against that branch on my repository.

In terms of rebasing etc, this branch works like Rust master: that is, until your PR lands, you have to rebase it against nll-master. But once a PR lands there, you are done -- I will take care of rebasing it and keeping it up to date.

This branch is not intended to live permanently far from master -- I will rebase it as needed, and I will always be trying to land the next logical chunk from it. However, this removes main Rust master as the central "gate" for Rust development.

EDIT: This is no longer true.

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@mark-i-m

First, currently we are doing NLL borrow-checking and AST region inference, we would expect things to get faster when AST region inference is done.

Second, there are probably some performance optimizations to be done, but I think that NLL is just doing more things than AST, and so will be slower for any given amount of optimization effort.

@rfcbot

This comment has been minimized.

Copy link

commented Sep 7, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

commented Sep 17, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

hannobraun added a commit to hannobraun/rust-dwm1001 that referenced this issue Sep 27, 2018

Update dependency on `dw1000` crate
This commit enables the `nll` feature in the two examples that receive
data. I think this is justified, because:
- I don't know how to work around the issue, except by introducing
  excessive copying, and increasing memory usage in `dw1000`.
- This crate is nightly-only anyway, since something enables the
  `const-fn` feature in `bare-metal`.
- None-lexical lifetime are up for stabilization soon, if I understand
  correctly. The tracking issue[1] has completed final comment period
  with a disposition to merge.

[1] rust-lang/rust#43234

@pnkfelix pnkfelix added metabug and removed A-metadata labels Oct 2, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 10, 2018

Rollup merge of rust-lang#55801 - pnkfelix:update-box-insensitivity-t…
…est-for-nll, r=davidtwco

NLL: Update box insensitivity test

This is just keeping one of our tests honest with respect to NLL, in two ways:

 1. Adds uses of borrows that would otherwise be too short to observe the error that we would have expected to see...
 2. ... I say "would have expected" because all of the errors in this file are part of the reversion of rust-lang/rfcs#130 that is attached to NLL (you can see more discussion of this here rust-lang#43234 (comment) )

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 10, 2018

Rollup merge of rust-lang#55801 - pnkfelix:update-box-insensitivity-t…
…est-for-nll, r=davidtwco

NLL: Update box insensitivity test

This is just keeping one of our tests honest with respect to NLL, in two ways:

 1. Adds uses of borrows that would otherwise be too short to observe the error that we would have expected to see...
 2. ... I say "would have expected" because all of the errors in this file are part of the reversion of rust-lang/rfcs#130 that is attached to NLL (you can see more discussion of this here rust-lang#43234 (comment) )
@orium

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Anyone knows what are the conditions for removing the old AST-based borrow checker? I'm curious about the performance gain it will cause 🙂

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@orium Steps remaining, from what I remember/understand

  1. turn on the borrow-checker's migrate (to NLL) mode for the 2015 edition (#57804).
  2. switch the migrate mode so that it uses NLL's region inference algorithm rather than the old AST-based region inference.
  3. update both 2018+2015 editions to issue hard errors from NLL rather than the soft-warnings generated by migrate mode.

(Issue #58781 captures both points 2 and 3 above.)

Once we do those things, then I think we'll be in a good position to actually remove the AST-borrowck code.


However: I do not expect this shift to yield a serious performance improvement. Or at least, not the one you might be expecting, based on how I interpret your comment. Removing the AST-borrowck code will only improve performance for code that is being rejected by the NLL checker (and thus falling back on running the AST-borrowck for determining if the NLL errors should be downgraded to warnings).

We do not run the AST-borrowck code (or at least not the bulk of it) when your code is successfully passing the NLL borrow checker. Thus, if you do not see any warnings or errors from your code, then you probably won't see a performance improvement from the eventual removal of the AST-borrowck code.

(The main exception I might imagine is whether there may be an impact from when we get rid of the AST-based region inference code and replace it with the NLL based one. But I don't know offhand if there is any serious performance impact associated with the AST-based region inference pass.)

@orium

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Thanks for clarifying that @pnkfelix. I was under the impression the old borrow checker was somehow more involved.

bors added a commit that referenced this issue Mar 11, 2019

Auto merge of #59114 - matthewjasper:enable-migate-2015, r=<try>
Enable NLL migrate mode on the 2015 edition

Blocked on #58739

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable the `-Ztwo-phase-borrows` flag on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests
* Remove the `-Zborrowck=compare` option
* Remove the `-Ztwo-phase-borrows` flag. It's kept so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll

bors added a commit that referenced this issue Apr 22, 2019

Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll

bors added a commit that referenced this issue Apr 22, 2019

Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll

bors added a commit that referenced this issue Apr 22, 2019

Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
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.