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

integrate MIR type-checker with NLL inference #45825

Merged
merged 65 commits into from
Nov 16, 2017

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 7, 2017

This branch refactors NLL type inference so that it uses the MIR type-checker to gather constraints. Along the way, it also refactors how region constraints are gathered in the normal inference context mildly. The new setup is like this:

  • What used to be region_inference is split into two parts:
    • region_constraints, which just collects up sets of constraints
    • lexical_region_resolve, which does the iterative, lexical region resolution
  • When resolve_regions_and_report_errors is invoked, the inference engine converts the constraints into final values.
  • In the MIR type checker, however, we do not invoke this method, but instead periodically take the region constraints and package them up for the NLL solver to use later.
    • This allows us to track when and where those constraints were incurred.
    • We also remove the central fulfillment context from the MIR type checker, instead instantiating new fulfillment contexts at each point. This allows us to capture the set of obligations that occurred at a particular point, and also to ensure that if the same obligation arises at two points, we will enforce the region constraints at both locations.
  • The MIR type checker is also enhanced to instantiate late-bound-regions with fresh variables and handle a few other corner cases that arose.
  • I also extracted some of the 'outlives' logic from the regionck, which will be needed later (see future work) to handle the type-outlives relationships.

One concern I have with this branch: since the MIR type checker is used even without the -Znll switch, I'm not sure if it will impact performance. One simple fix here would be to only enable the MIR type-checker if debug-assertions are enabled, since it just serves to validate the MIR. Longer term I hope to address this by improving the interface to the trait solver to be more query-based (ongoing work).

There is plenty of future work left. Here are two things that leap to mind:

  • Type-region outlives. Currently, the NLL solver will ICE if it is required to handle a constraint like T: 'a. Fixing this will require a small amount of refactoring to extract the implied bounds code. I plan to follow a file-up bug on this (hopefully with mentoring instructions).
  • Testing. It's a good idea to enumerate some of the tricky scenarios that need testing, but I think it'd be nice to try and parallelize some of the actual test writing (and resulting bug fixing):
    • Same obligation occurring at two points.
    • Well-formedness and trait obligations of various kinds (which are not all processed by the current MIR type-checker).
    • More tests for how subtyping and region inferencing interact.
    • More suggestions welcome!

r? @arielb1

@nikomatsakis
Copy link
Contributor Author

(Seems like this doesn't bootstrap, I had only been building with --stage 1. Will investigate. Marking as [WIP] for now.)

@nikomatsakis nikomatsakis changed the title Integrate MIR type-checker with NLL inference [WIP] integrate MIR type-checker with NLL inference Nov 7, 2017
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2017
@nikomatsakis nikomatsakis changed the title [WIP] integrate MIR type-checker with NLL inference integrate MIR type-checker with NLL inference Nov 7, 2017
@nikomatsakis
Copy link
Contributor Author

Fixed the problem that was preventing bootstrapping in the final commit. Removing the [WIP] tag.

@bors
Copy link
Contributor

bors commented Nov 9, 2017

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

@nikomatsakis
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Nov 11, 2017

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

@nikomatsakis
Copy link
Contributor Author

@arielb1 so -- there's a lot of code here I know -- would it be helpful if I broke it up into smaller PRs that you can review independently?

(For the time being, I've created a "shadow master" on my own repo to allow NLL work to continue, since it seems like we're not landing things on master fast enough to do all the work there.)

@arielb1
Copy link
Contributor

arielb1 commented Nov 12, 2017

@nikomatsakis

Nope, I just missed this not being WIP.

@nikomatsakis
Copy link
Contributor Author

One thing I was thinking. This PR made some changes to how the MIR type-checker works -- in particular, it tries to process all obligations fully. This could have an impact on performance. I was thinking of making those changes conditional on -Znll, although it'd be nice to measure the impact just to know what it is.

Slightly longer term, I have a branch trying to convert trait selection into a standalone query, which would (I believe) address this issue, by allowing us to cache the results independent of where they occurred. But that's still experimental at this point.

@arielb1
Copy link
Contributor

arielb1 commented Nov 12, 2017

@nikomatsakis

Monomorphic trait evaluation is already very cached, so I don't think this should be much of a problem.

//! type-check the closure body (e.g., here it informs us that `T`
//! outlives the late-bound region `'a`).
//!
//! > That said, in writing this, I have come to wonder: this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have an for<'a> |x: &'a mut (&'a u8, ?0)| closure, and later on you have ?0 = &'b u8, then the closure gains an 'b: 'a implied bound, and you can do x.0 = x.1;, e.g. in:

fn foo<U, F: for<'a> FnMut(&'a mut (&'a u8, U))>(_f: F) {}
fn main() {
    foo(|x| { x.0 = x.1; });
}

Whether this is important in some practical case, I have no idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, maybe that was the sort of case that made me push it later. Quite likely. I remember a lot of these sorts of things arising during the move to unboxed closures. Anyway it seems fine to do it this way.

Copy link
Contributor

@arielb1 arielb1 Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just fix the comment to not "wonder"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will do.

///
/// Not legal during a snapshot.
pub fn take_and_reset_data(&mut self) -> RegionConstraintData<'tcx> {
mem::replace(&mut self.data, RegionConstraintData::default())
Copy link
Contributor

@arielb1 arielb1 Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also reset the other fields (e.g. lubs and glbs sound important), to avoid leakage? Or maybe these are just not used in MIR regionck?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think you are correct about lubs and glbs. At first I think they were ok, but I guess then we would be caching a LUB from some other locale that may not be a LUB here. Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To be clear: We're not actually using LUB/GLB in NLL, but it seems good to be thorough anyhow.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. I also was forgetting about unification_table. =)

// properly, and that code is currently largely confined to
// regionck (though I made some efforts to extract it
// out). -nmatsakis
let _ = infcx.ignore_region_obligations(body_id);
Copy link
Contributor

@arielb1 arielb1 Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think wfcheck already checks that these obligations hold (that's it, that the param env is WF if its predicates hold), and this code should be going away anyway with lazy normalization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is why I didn't try to fix this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@nikomatsakis
Copy link
Contributor Author

Monomorphic trait evaluation is already very cached, so I don't think this should be much of a problem.

Note that we don't have any guarantees that these trait evaluations are monomorphic. This stuff is occuring on the MIR, right?

variable to an empty value. We then process each outlives constraint
repeatedly, growing region variables until a fixed-point is reached.
Region variables can be grown using a least-upper-bound relation on
the region lattice in a fairly straight-forward fashion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -1026,6 +1023,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
let var_name = self.tcx.hir.name(var_node_id);
format!(" for capture of `{}` by closure", var_name)
}
infer::NLL(..) => bug!("NLL variable found in lexical phase"),
};

struct_span_err!(self.tcx.sess, var_origin.span(), E0495,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

See the [general inference README](../README.md) for an overview of
how lexical-region-solving fits into the bigger picture.

Region constraint collect uses a somewhat more involved algorithm than
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is lexical_region_resolve/README.md, why does it talk about "Region constraint collect"?

I think just s/Region constraint collect/Region inference/ works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

debug!("resolve_var({:?}) = {:?}", rid, result);
result
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

// regionck to be sure that it has found *all* the region
// obligations (otherwise, it's easy to fail to walk to a
// particular node-id).
region_obligations: RefCell<NodeMap<Vec<RegionObligation<'tcx>>>>,
Copy link
Contributor

@arielb1 arielb1 Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this handle snapshots correctly - i.e. are region obligations that were created in a "child" fulfillcx in a probe ignored? type_known_to_meet_bound creates a "child" fulfillment context, and I'm not sure it can't be called from typeck snapshots.

If you think nobody should be registering region obligations in a snapshot, which should eventually be the case with MIR regionck, it's a good idea to add an assertion to prevent things blowing up.

@nikomatsakis
Copy link
Contributor Author

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Nov 16, 2017

📌 Commit 8c109f5 has been approved by arielb1

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2017
@bors
Copy link
Contributor

bors commented Nov 16, 2017

⌛ Testing commit 8c109f5 with merge d0f8e29...

bors added a commit that referenced this pull request Nov 16, 2017
…ielb1

integrate MIR type-checker with NLL inference

This branch refactors NLL type inference so that it uses the MIR type-checker to gather constraints. Along the way, it also refactors how region constraints are gathered in the normal inference context mildly. The new setup is like this:

- What used to be `region_inference` is split into two parts:
    - `region_constraints`, which just collects up sets of constraints
    - `lexical_region_resolve`, which does the iterative, lexical region resolution
- When `resolve_regions_and_report_errors` is invoked, the inference engine converts the constraints into final values.
- In the MIR type checker, however, we do not invoke this method, but instead periodically take the region constraints and package them up for the NLL solver to use later.
    - This allows us to track when and where those constraints were incurred.
    - We also remove the central fulfillment context from the MIR type checker, instead instantiating new fulfillment contexts at each point. This allows us to capture the set of obligations that occurred at a particular point, and also to ensure that if the same obligation arises at two points, we will enforce the region constraints at both locations.
- The MIR type checker is also enhanced to instantiate late-bound-regions with fresh variables and handle a few other corner cases that arose.
- I also extracted some of the 'outlives' logic from the regionck, which will be needed later (see future work) to handle the type-outlives relationships.

One concern I have with this branch: since the MIR type checker is used even without the `-Znll` switch, I'm not sure if it will impact performance. One simple fix here would be to only enable the MIR type-checker if debug-assertions are enabled, since it just serves to validate the MIR. Longer term I hope to address this by improving the interface to the trait solver to be more query-based (ongoing work).

There is plenty of future work left. Here are two things that leap to mind:

- **Type-region outlives.** Currently, the NLL solver will ICE if it is required to handle a constraint like `T: 'a`. Fixing this will require a small amount of refactoring to extract the implied bounds code. I plan to follow a file-up bug on this (hopefully with mentoring instructions).
- **Testing.** It's a good idea to enumerate some of the tricky scenarios that need testing, but I think it'd be nice to try and parallelize some of the actual test writing (and resulting bug fixing):
    - Same obligation occurring at two points.
    - Well-formedness and trait obligations of various kinds (which are not all processed by the current MIR type-checker).
    - More tests for how subtyping and region inferencing interact.
    - More suggestions welcome!

r? @arielb1
@bors
Copy link
Contributor

bors commented Nov 16, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing d0f8e29 to master...

@bors bors merged commit 8c109f5 into rust-lang:master Nov 16, 2017
bors added a commit that referenced this pull request Nov 23, 2017
…ielb1

typeck aggregate rvalues in MIR type checker

This branch is an attempt to land content by @spastorino and @Nashenas88 that was initially landed on nll-master while we waited for #45825 to land.

The biggest change it contains is that it extends the MIR type-checker to also type-check MIR aggregate rvalues (at least partially). Specifically, it checks that the operands provided for each field have the right type.

It does not yet check that their well-formedness predicates are met. That is #45827. It also does not check other kinds of rvalues (that is #45959). @spastorino is working on those issues now.

r? @arielb1
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2022
…matsakis

Remove migrate borrowck mode

Closes rust-lang#58781
Closes rust-lang#43234

# Stabilization proposal

This PR proposes the stabilization of `#![feature(nll)]` and the removal of `-Z borrowck`. Current borrow checking behavior of item bodies is currently done by first infering regions *lexically* and reporting any errors during HIR type checking. If there *are* any errors, then MIR borrowck (NLL) never occurs. If there *aren't* any errors, then MIR borrowck happens and any errors there would be reported. This PR removes the lexical region check of item bodies entirely and only uses MIR borrowck. Because MIR borrowck could never *not* be run for a compiled program, this should not break any programs. It does, however, change diagnostics significantly and allows a slightly larger set of programs to compile.

Tracking issue: rust-lang#43234
RFC: https://github.com/rust-lang/rfcs/blob/master/text/2094-nll.md
Version: 1.63 (2022-06-30 => beta, 2022-08-11 => stable).

## Motivation

Over time, the Rust borrow checker has become "smarter" and thus allowed more programs to compile. There have been three different implementations: AST borrowck, MIR borrowck, and polonius (well, in progress). Additionally, there is the "lexical region resolver", which (roughly) solves the constraints generated through HIR typeck. It is not a full borrow checker, but does emit some errors.

The AST borrowck was the original implementation of the borrow checker and was part of the initially stabilized Rust 1.0. In mid 2017, work began to implement the current MIR borrow checker and that effort ompleted by the end of 2017, for the most part. During 2018, efforts were made to migrate away from the AST borrow checker to the MIR borrow checker - eventually culminating into "migrate" mode - where HIR typeck with lexical region resolving following by MIR borrow checking - being active by default in the 2018 edition.

In early 2019, migrate mode was turned on by default in the 2015 edition as well, but with MIR borrowck errors emitted as warnings. By late 2019, these warnings were upgraded to full errors. This was followed by the complete removal of the AST borrow checker.

In the period since, various errors emitted by the MIR borrow checker have been improved to the point that they are mostly the same or better than those emitted by the lexical region resolver.

While there do remain some degradations in errors (tracked under the [NLL-diagnostics tag](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-diagnostics), those are sufficiently small and rare enough that increased flexibility of MIR borrow check-only is now a worthwhile tradeoff.

## What is stabilized

As said previously, this does not fundamentally change the landscape of accepted programs. However, there are a [few](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-fixed-by-NLL) cases where programs can compile under `feature(nll)`, but not otherwise.

There are two notable patterns that are "fixed" by this stabilization. First, the `scoped_threads` feature, which is a continutation of a pre-1.0 API, can sometimes emit a [weird lifetime error](rust-lang#95527) without NLL. Second, actually seen in the standard library. In the `Extend` impl for `HashMap`, there is an implied bound of `K: 'a` that is available with NLL on but not without - this is utilized in the impl.

As mentioned before, there are a large number of diagnostic differences. Most of them are better, but some are worse. None are serious or happen often enough to need to block this PR. The biggest change is the loss of error code for a number of lifetime errors in favor of more general "lifetime may not live long enough" error. While this may *seem* bad, the former error codes were just attempts to somewhat-arbitrarily bin together lifetime errors of the same type; however, on paper, they end up being roughly the same with roughly the same kinds of solutions.

## What isn't stabilized

This PR does not completely remove the lexical region resolver. In the future, it may be possible to remove that (while still keeping HIR typeck) or to remove it together with HIR typeck.

## Tests

Many test outputs get updated by this PR. However, there are number of tests specifically geared towards NLL under `src/test/ui/nll`

## History

* On 2017-07-14, [tracking issue opened](rust-lang#43234)
* On 2017-07-20, [initial empty MIR pass added](rust-lang#43271)
* On 2017-08-29, [RFC opened](rust-lang/rfcs#2094)
* On 2017-11-16, [Integrate MIR type-checker with NLL](rust-lang#45825)
* On 2017-12-20, [NLL feature complete](rust-lang#46862)
* On 2018-07-07, [Don't run AST borrowck on mir mode](rust-lang#52083)
* On 2018-07-27, [Add migrate mode](rust-lang#52681)
* On 2019-04-22, [Enable migrate mode on 2015 edition](rust-lang#59114)
* On 2019-08-26, [Don't downgrade errors on 2015 edition](rust-lang#64221)
* On 2019-08-27, [Remove AST borrowck](rust-lang#64790)
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 16, 2022
Remove migrate borrowck mode

Closes #58781
Closes #43234

# Stabilization proposal

This PR proposes the stabilization of `#![feature(nll)]` and the removal of `-Z borrowck`. Current borrow checking behavior of item bodies is currently done by first infering regions *lexically* and reporting any errors during HIR type checking. If there *are* any errors, then MIR borrowck (NLL) never occurs. If there *aren't* any errors, then MIR borrowck happens and any errors there would be reported. This PR removes the lexical region check of item bodies entirely and only uses MIR borrowck. Because MIR borrowck could never *not* be run for a compiled program, this should not break any programs. It does, however, change diagnostics significantly and allows a slightly larger set of programs to compile.

Tracking issue: #43234
RFC: https://github.com/rust-lang/rfcs/blob/master/text/2094-nll.md
Version: 1.63 (2022-06-30 => beta, 2022-08-11 => stable).

## Motivation

Over time, the Rust borrow checker has become "smarter" and thus allowed more programs to compile. There have been three different implementations: AST borrowck, MIR borrowck, and polonius (well, in progress). Additionally, there is the "lexical region resolver", which (roughly) solves the constraints generated through HIR typeck. It is not a full borrow checker, but does emit some errors.

The AST borrowck was the original implementation of the borrow checker and was part of the initially stabilized Rust 1.0. In mid 2017, work began to implement the current MIR borrow checker and that effort ompleted by the end of 2017, for the most part. During 2018, efforts were made to migrate away from the AST borrow checker to the MIR borrow checker - eventually culminating into "migrate" mode - where HIR typeck with lexical region resolving following by MIR borrow checking - being active by default in the 2018 edition.

In early 2019, migrate mode was turned on by default in the 2015 edition as well, but with MIR borrowck errors emitted as warnings. By late 2019, these warnings were upgraded to full errors. This was followed by the complete removal of the AST borrow checker.

In the period since, various errors emitted by the MIR borrow checker have been improved to the point that they are mostly the same or better than those emitted by the lexical region resolver.

While there do remain some degradations in errors (tracked under the [NLL-diagnostics tag](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-diagnostics), those are sufficiently small and rare enough that increased flexibility of MIR borrow check-only is now a worthwhile tradeoff.

## What is stabilized

As said previously, this does not fundamentally change the landscape of accepted programs. However, there are a [few](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-fixed-by-NLL) cases where programs can compile under `feature(nll)`, but not otherwise.

There are two notable patterns that are "fixed" by this stabilization. First, the `scoped_threads` feature, which is a continutation of a pre-1.0 API, can sometimes emit a [weird lifetime error](rust-lang/rust#95527) without NLL. Second, actually seen in the standard library. In the `Extend` impl for `HashMap`, there is an implied bound of `K: 'a` that is available with NLL on but not without - this is utilized in the impl.

As mentioned before, there are a large number of diagnostic differences. Most of them are better, but some are worse. None are serious or happen often enough to need to block this PR. The biggest change is the loss of error code for a number of lifetime errors in favor of more general "lifetime may not live long enough" error. While this may *seem* bad, the former error codes were just attempts to somewhat-arbitrarily bin together lifetime errors of the same type; however, on paper, they end up being roughly the same with roughly the same kinds of solutions.

## What isn't stabilized

This PR does not completely remove the lexical region resolver. In the future, it may be possible to remove that (while still keeping HIR typeck) or to remove it together with HIR typeck.

## Tests

Many test outputs get updated by this PR. However, there are number of tests specifically geared towards NLL under `src/test/ui/nll`

## History

* On 2017-07-14, [tracking issue opened](rust-lang/rust#43234)
* On 2017-07-20, [initial empty MIR pass added](rust-lang/rust#43271)
* On 2017-08-29, [RFC opened](rust-lang/rfcs#2094)
* On 2017-11-16, [Integrate MIR type-checker with NLL](rust-lang/rust#45825)
* On 2017-12-20, [NLL feature complete](rust-lang/rust#46862)
* On 2018-07-07, [Don't run AST borrowck on mir mode](rust-lang/rust#52083)
* On 2018-07-27, [Add migrate mode](rust-lang/rust#52681)
* On 2019-04-22, [Enable migrate mode on 2015 edition](rust-lang/rust#59114)
* On 2019-08-26, [Don't downgrade errors on 2015 edition](rust-lang/rust#64221)
* On 2019-08-27, [Remove AST borrowck](rust-lang/rust#64790)
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
Remove migrate borrowck mode

Closes #58781
Closes #43234

# Stabilization proposal

This PR proposes the stabilization of `#![feature(nll)]` and the removal of `-Z borrowck`. Current borrow checking behavior of item bodies is currently done by first infering regions *lexically* and reporting any errors during HIR type checking. If there *are* any errors, then MIR borrowck (NLL) never occurs. If there *aren't* any errors, then MIR borrowck happens and any errors there would be reported. This PR removes the lexical region check of item bodies entirely and only uses MIR borrowck. Because MIR borrowck could never *not* be run for a compiled program, this should not break any programs. It does, however, change diagnostics significantly and allows a slightly larger set of programs to compile.

Tracking issue: #43234
RFC: https://github.com/rust-lang/rfcs/blob/master/text/2094-nll.md
Version: 1.63 (2022-06-30 => beta, 2022-08-11 => stable).

## Motivation

Over time, the Rust borrow checker has become "smarter" and thus allowed more programs to compile. There have been three different implementations: AST borrowck, MIR borrowck, and polonius (well, in progress). Additionally, there is the "lexical region resolver", which (roughly) solves the constraints generated through HIR typeck. It is not a full borrow checker, but does emit some errors.

The AST borrowck was the original implementation of the borrow checker and was part of the initially stabilized Rust 1.0. In mid 2017, work began to implement the current MIR borrow checker and that effort ompleted by the end of 2017, for the most part. During 2018, efforts were made to migrate away from the AST borrow checker to the MIR borrow checker - eventually culminating into "migrate" mode - where HIR typeck with lexical region resolving following by MIR borrow checking - being active by default in the 2018 edition.

In early 2019, migrate mode was turned on by default in the 2015 edition as well, but with MIR borrowck errors emitted as warnings. By late 2019, these warnings were upgraded to full errors. This was followed by the complete removal of the AST borrow checker.

In the period since, various errors emitted by the MIR borrow checker have been improved to the point that they are mostly the same or better than those emitted by the lexical region resolver.

While there do remain some degradations in errors (tracked under the [NLL-diagnostics tag](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-diagnostics), those are sufficiently small and rare enough that increased flexibility of MIR borrow check-only is now a worthwhile tradeoff.

## What is stabilized

As said previously, this does not fundamentally change the landscape of accepted programs. However, there are a [few](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-fixed-by-NLL) cases where programs can compile under `feature(nll)`, but not otherwise.

There are two notable patterns that are "fixed" by this stabilization. First, the `scoped_threads` feature, which is a continutation of a pre-1.0 API, can sometimes emit a [weird lifetime error](rust-lang/rust#95527) without NLL. Second, actually seen in the standard library. In the `Extend` impl for `HashMap`, there is an implied bound of `K: 'a` that is available with NLL on but not without - this is utilized in the impl.

As mentioned before, there are a large number of diagnostic differences. Most of them are better, but some are worse. None are serious or happen often enough to need to block this PR. The biggest change is the loss of error code for a number of lifetime errors in favor of more general "lifetime may not live long enough" error. While this may *seem* bad, the former error codes were just attempts to somewhat-arbitrarily bin together lifetime errors of the same type; however, on paper, they end up being roughly the same with roughly the same kinds of solutions.

## What isn't stabilized

This PR does not completely remove the lexical region resolver. In the future, it may be possible to remove that (while still keeping HIR typeck) or to remove it together with HIR typeck.

## Tests

Many test outputs get updated by this PR. However, there are number of tests specifically geared towards NLL under `src/test/ui/nll`

## History

* On 2017-07-14, [tracking issue opened](rust-lang/rust#43234)
* On 2017-07-20, [initial empty MIR pass added](rust-lang/rust#43271)
* On 2017-08-29, [RFC opened](rust-lang/rfcs#2094)
* On 2017-11-16, [Integrate MIR type-checker with NLL](rust-lang/rust#45825)
* On 2017-12-20, [NLL feature complete](rust-lang/rust#46862)
* On 2018-07-07, [Don't run AST borrowck on mir mode](rust-lang/rust#52083)
* On 2018-07-27, [Add migrate mode](rust-lang/rust#52681)
* On 2019-04-22, [Enable migrate mode on 2015 edition](rust-lang/rust#59114)
* On 2019-08-26, [Don't downgrade errors on 2015 edition](rust-lang/rust#64221)
* On 2019-08-27, [Remove AST borrowck](rust-lang/rust#64790)
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
Remove migrate borrowck mode

Closes #58781
Closes #43234

# Stabilization proposal

This PR proposes the stabilization of `#![feature(nll)]` and the removal of `-Z borrowck`. Current borrow checking behavior of item bodies is currently done by first infering regions *lexically* and reporting any errors during HIR type checking. If there *are* any errors, then MIR borrowck (NLL) never occurs. If there *aren't* any errors, then MIR borrowck happens and any errors there would be reported. This PR removes the lexical region check of item bodies entirely and only uses MIR borrowck. Because MIR borrowck could never *not* be run for a compiled program, this should not break any programs. It does, however, change diagnostics significantly and allows a slightly larger set of programs to compile.

Tracking issue: #43234
RFC: https://github.com/rust-lang/rfcs/blob/master/text/2094-nll.md
Version: 1.63 (2022-06-30 => beta, 2022-08-11 => stable).

## Motivation

Over time, the Rust borrow checker has become "smarter" and thus allowed more programs to compile. There have been three different implementations: AST borrowck, MIR borrowck, and polonius (well, in progress). Additionally, there is the "lexical region resolver", which (roughly) solves the constraints generated through HIR typeck. It is not a full borrow checker, but does emit some errors.

The AST borrowck was the original implementation of the borrow checker and was part of the initially stabilized Rust 1.0. In mid 2017, work began to implement the current MIR borrow checker and that effort ompleted by the end of 2017, for the most part. During 2018, efforts were made to migrate away from the AST borrow checker to the MIR borrow checker - eventually culminating into "migrate" mode - where HIR typeck with lexical region resolving following by MIR borrow checking - being active by default in the 2018 edition.

In early 2019, migrate mode was turned on by default in the 2015 edition as well, but with MIR borrowck errors emitted as warnings. By late 2019, these warnings were upgraded to full errors. This was followed by the complete removal of the AST borrow checker.

In the period since, various errors emitted by the MIR borrow checker have been improved to the point that they are mostly the same or better than those emitted by the lexical region resolver.

While there do remain some degradations in errors (tracked under the [NLL-diagnostics tag](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-diagnostics), those are sufficiently small and rare enough that increased flexibility of MIR borrow check-only is now a worthwhile tradeoff.

## What is stabilized

As said previously, this does not fundamentally change the landscape of accepted programs. However, there are a [few](https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-fixed-by-NLL) cases where programs can compile under `feature(nll)`, but not otherwise.

There are two notable patterns that are "fixed" by this stabilization. First, the `scoped_threads` feature, which is a continutation of a pre-1.0 API, can sometimes emit a [weird lifetime error](rust-lang/rust#95527) without NLL. Second, actually seen in the standard library. In the `Extend` impl for `HashMap`, there is an implied bound of `K: 'a` that is available with NLL on but not without - this is utilized in the impl.

As mentioned before, there are a large number of diagnostic differences. Most of them are better, but some are worse. None are serious or happen often enough to need to block this PR. The biggest change is the loss of error code for a number of lifetime errors in favor of more general "lifetime may not live long enough" error. While this may *seem* bad, the former error codes were just attempts to somewhat-arbitrarily bin together lifetime errors of the same type; however, on paper, they end up being roughly the same with roughly the same kinds of solutions.

## What isn't stabilized

This PR does not completely remove the lexical region resolver. In the future, it may be possible to remove that (while still keeping HIR typeck) or to remove it together with HIR typeck.

## Tests

Many test outputs get updated by this PR. However, there are number of tests specifically geared towards NLL under `src/test/ui/nll`

## History

* On 2017-07-14, [tracking issue opened](rust-lang/rust#43234)
* On 2017-07-20, [initial empty MIR pass added](rust-lang/rust#43271)
* On 2017-08-29, [RFC opened](rust-lang/rfcs#2094)
* On 2017-11-16, [Integrate MIR type-checker with NLL](rust-lang/rust#45825)
* On 2017-12-20, [NLL feature complete](rust-lang/rust#46862)
* On 2018-07-07, [Don't run AST borrowck on mir mode](rust-lang/rust#52083)
* On 2018-07-27, [Add migrate mode](rust-lang/rust#52681)
* On 2019-04-22, [Enable migrate mode on 2015 edition](rust-lang/rust#59114)
* On 2019-08-26, [Don't downgrade errors on 2015 edition](rust-lang/rust#64221)
* On 2019-08-27, [Remove AST borrowck](rust-lang/rust#64790)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants