NLL: turn off migration mode #58781
Comments
|
Explicitly marking this P-medium for now, to reflect the fact that transitioning the 2015 edition to migrate mode is higher priority for the short term. |
pnkfelix
added
the
C-tracking-issue
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
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
|
Created #60680 as an issue to do a first crater run to get a rough idea. |
|
Is this the right place for comments/questions about the migration warning? I got this warning: The warning is good, my code definitely has UB. I found that I can't upgrade the warning to an error with This code compiles and has UB on Rust 1.0, I thought NLL is only supposed to relax things? Are you still fixing any bugs in the old borrow checker? |
|
@jethrogb no, NLL was not meant to solely relax things. Such relaxation is the primary selling point of NLL, but we always knew it was going to introduce breakage. The shift to NLL is coupled with a change in the underlying borrow-checker implementation to analyze MIR (instead of analyzing the higher-level AST, which is what the old borrow checker did). In order to avoid injecting breaking changes that people could not work around in the short term, we currently have NLL in a migration mode. That was added in PR #52681. In hindsight, it may have been nice to leverage the lint system here to let people upgrade these warnings to errors. (One reason we didn't do that is that we didn't want people to |
This is one of the behaviors we no longer allow in NLL. Since it can lead to undefined behavior, I think it's definitely worth making it a hard error without waiting to turn off migration mode (rust-lang#58781). Closes rust-lang#60450. My ulterior motive here is making it impossible to leave variables partially initialized across a yield (see discussion at rust-lang#63035), so tests are included for that.
…ized, r=Centril Make use of possibly uninitialized data [E0381] a hard error This is one of the behaviors we no longer allow in NLL. Since it can lead to undefined behavior, I think it's definitely worth making it a hard error without waiting to turn off migration mode (rust-lang#58781). Closes rust-lang#60450. My ulterior motive here is making it impossible to leave variables partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so tests are included for that. cc rust-lang#54987 --- I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me. r? @Centril cc @cramertj @nikomatsakis @pnkfelix @RalfJung
…ized, r=Centril Make use of possibly uninitialized data [E0381] a hard error This is one of the behaviors we no longer allow in NLL. Since it can lead to undefined behavior, I think it's definitely worth making it a hard error without waiting to turn off migration mode (rust-lang#58781). Closes rust-lang#60450. My ulterior motive here is making it impossible to leave variables partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so tests are included for that. cc rust-lang#54987 --- I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me. r? @Centril cc @cramertj @nikomatsakis @pnkfelix @RalfJung
…ized, r=Centril Make use of possibly uninitialized data [E0381] a hard error This is one of the behaviors we no longer allow in NLL. Since it can lead to undefined behavior, I think it's definitely worth making it a hard error without waiting to turn off migration mode (rust-lang#58781). Closes rust-lang#60450. My ulterior motive here is making it impossible to leave variables partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so tests are included for that. cc rust-lang#54987 --- I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me. r? @Centril cc @cramertj @nikomatsakis @pnkfelix @RalfJung
|
@matthewjasper Could you either a) close this issue or b) update it with the current status? I think we've fully dropped migration mode now and even cleaned up some of the leftover AST borrowck-related bits, but there is some stuff left that is not yet fully migrated I believe. |
|
Migrate mode still exists, although it's not really migrating anything. I've updated the description for the current status. |
crlf0710
added
the
T-compiler
|
The lint is orthogonal from migration mode, in any case. |
|
Presumably fully transitioning to NLL would also cause the "NLL" mode tests in the UI test suite ( |
|
Yes, this would remove |
|
What's the current status of this issue? |
Add variance-related information to lifetime error messages
This PR adds a basic framework for displaying variance-related information in error messages. For example:
```
error: lifetime may not live long enough
--> $DIR/type-check-pointer-comparisons.rs:12:5
|
LL | fn compare_mut<'a, 'b>(x: *mut &'a i32, y: *mut &'b i32) {
| -- -- lifetime `'b` defined here
| |
| lifetime `'a` defined here
LL | x == y;
| ^ requires that `'a` must outlive `'b`
|
= help: consider adding the following bound: `'a: 'b`
= note: requirement occurs because of a mutable pointer to &i32
= note: mutable pointers are invariant over their type parameter
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
```
The last three lines are new.
This is accomplished by adding a new struct `VarianceDiagInfo`, and passing it along through the various relation methods. When relating types that change the variance (e.g. `&mut T` or `*mut T`), we pass a more specific `VarianceDiagInfo` storing information about the cause of the variance change. When an error, we use the `VarianceDiagInfo` to add additional information to the error message.
This PR doesn't change any variance-related computation or behavior - only diagnostic messages. Therefore, the implementation is quite incomplete - more detailed error messages can be filled in in subsequent PRs.
Limitations:
* We only attempt to deal with invariance - since it's at the bottom of the 'variance lattice', our variance will never change again after it becomes invariant. Handling contravariance would be trickier, since we can change between contravariance and covariance multiple times (e.g. `fn(fn(&'static u8))`). Since contravariance (AFAIK) is only used for function arguments, we can probably get away without a very fancy message for cases involving contravariance.
* `VarianceDiagInfo` currently only handles mutable pointers/references. However, user-defined types (structs, enums, and unions) have the variance of their type parameters inferred, so it would be good to eventually display information about that. We'll want to try to find a balance between displaying too much and too little information about how the variance was inferred.
* The improved error messages are only displayed when `#![feature(nll)]` / `-Z borrowck=mir` is enabled. If issue rust-lang#58781 is not resolved relatively soon, then we might want to duplicate some of this logic in the 'current' (non-NLL) region/outlives handling code.
|
@matthewjasper: Are you still working on #57374? If not, I'd be interested in working on it |
|
I made some good progress (see https://github.com/matthewjasper/rust/tree/nll-hrtb-errors) but haven't had a chance to finish it. Specifically I wanted to avoid the code duplication between |
|
@matthewjasper would it make sense to try to merge this great progress you've already completed ? or would you rather wait for the 2 tasks you mentioned to be completed before doing so ? (I thought the branch was older than it actually was, and that I'd try to help by rebasing it over here but it probably wasn't necessary) |
|
You could try that. The branch is pretty old, but hopefully touches parts of the codebase that move pretty slowly. |
|
Sorry, I meant I had already rebased your branch in the link above: it was indeed in slow-moving parts of the compiler, with only a few simple conflicts, and only one compile error. Very straightforward to update overall. Your improvements to these error messages are already looking great. I can open a PR and help land them, if @nikomatsakis and @Aaron1011 want to have a look ? |
|
Now that higher-ranked subtype errors are improved for stable code (#57374 (comment)), is #73154 the only blocker? |
|
A few ideas:
|
|
Regarding the leak check: I believe we do have to reproduce some of that functionality. |
|
Is there something actionable that a small contributor like me could help with? It would be very pleasant to finally stabilize NLL 🥳 |
|
@marmeladema The diagnostic issues mentioned above could probably use triage to see if they're still relevant: https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3ANLL-diagnostics . If they are, then resolving those may be a good place to start. You can also try hopping into #t-compiler or #t-compiler/help and asking there. |
|
I opened a zulip threads in the wg-nll group about those: https://rust-lang.zulipchat.com/#narrow/stream/122657-t-compiler.2Fwg-nll/topic/nll.20diagnostics.20issues TL;DR is that there are actually 4 and we believe they can all probably be closed. |
jackh726
added
the
S-tracking-impl-incomplete
|
I've updated the op to include the latest potential blockers and added a tracking label to reflect that some impl work needs to be re. #73154. |
…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)
Transition fully to NLL (instead of the migration mode currently used).
#![feature(nll)]or-Z borrowck=mirNLL accepts higher-ranked subtype that non-NLL rejects #57642 (needs test for now)Two-phase borrows (2PB) question: [NLL] prohibit "two-phase borrows" with existing borrows? #56254Must have crater run showing no major crates fail in practice (crater run to estimate impact of full NLL transition #60680)Investigate [NLL] Lifetimes errors in a function can mask later errors in same function #96331The text was updated successfully, but these errors were encountered: