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

What does compare-mode=nll do these days? #78915

Closed
guswynn opened this issue Nov 10, 2020 · 7 comments
Closed

What does compare-mode=nll do these days? #78915

guswynn opened this issue Nov 10, 2020 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@guswynn
Copy link
Contributor

guswynn commented Nov 10, 2020

See #76765 (comment)

what does compare-mode=nll actually represent these days? I thought nll was long-since stable

cc @tmandry

@guswynn
Copy link
Contributor Author

guswynn commented Nov 10, 2020

@rustbot modify labels: +A-diagnostics +T-compiler

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 10, 2020
@camelid camelid changed the title Make it more clear what an about async fn's returns when referring to what it returns in compare-mode=nll What does compare-mode=nll do these days? Nov 10, 2020
@camelid camelid added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Nov 10, 2020
@bjorn3
Copy link
Member

bjorn3 commented Nov 10, 2020

I could find two differences:

$ rg "BorrowckMode::Mir|\.migrate|suppress_errors" compiler
compiler/rustc_middle/src/ty/context.rs
1336:        self.borrowck_mode().migrate()
1365:            return BorrowckMode::Mir;

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs
45:        RegionckMode::Erase { suppress_errors: false } => {
56:        RegionckMode::Erase { suppress_errors: true } => {

compiler/rustc_infer/src/infer/mod.rs
105:        suppress_errors: bool,
124:            BorrowckMode::Migrate => RegionckMode::Erase { suppress_errors: false },
127:            BorrowckMode::Mir => RegionckMode::Erase { suppress_errors: true },

compiler/rustc_mir/src/borrow_check/mod.rs
1118:                ) if { tcx.migrate_borrowck() && this.borrow_set.contains(&location) } => {

compiler/rustc_data_structures/src/sso/map.rs
528:        self.ssomap.migrate_if_full();

compiler/rustc_session/src/config.rs
483:            BorrowckMode::Mir => false,
1568:        "mir" => BorrowckMode::Mir,

First regionck being skipped instead of given the opportunity to report errors:

RegionckMode::Erase { suppress_errors: false } => {
// Do real inference to get errors, then erase the results.
let mut values = resolver.infer_variable_values(&mut errors);
let re_erased = region_rels.tcx.lifetimes.re_erased;
values.values.iter_mut().for_each(|v| match *v {
VarValue::Value(ref mut r) => *r = re_erased,
VarValue::ErrorValue => {}
});
(values, errors)
}
RegionckMode::Erase { suppress_errors: true } => {
// Skip region inference entirely.
(resolver.erased_data(region_rels.tcx), Vec::new())
}

Second a fix for #56254 ([NLL] prohibit "two-phase borrows" with existing borrows?)

) if { tcx.migrate_borrowck() && this.borrow_set.contains(&location) } => {
let bi = this.borrow_set.get_index_of(&location).unwrap();
debug!(
"recording invalid reservation of place: {:?} with \
borrow index {:?} as warning",
place_span.0, bi,
);
// rust-lang/rust#56254 - This was previously permitted on
// the 2018 edition so we emit it as a warning. We buffer
// these sepately so that we only emit a warning if borrow
// checking was otherwise successful.
this.reservation_warnings
.insert(bi, (place_span.0, place_span.1, location, bk, borrow.clone()));
// Don't suppress actual errors.
Control::Continue
}

@guswynn
Copy link
Contributor Author

guswynn commented Nov 11, 2020

@bjorn3 interesting, in #76765, src/test/ui/issues/issue-76547.nll.stderr is materially different (and worse!) output in 'mir' instead of 'migrate'. I am inclined to think that, because nll is quite old at this point, and migrate is the default, we should perhaps get rid of the 2 modes and just use whatever codepaths are triggered by Migrate?(well, compare-mode=polonious usually sets -Zpolonious AND -borrowck=mir so perhaps we can just trigger the non-migrate paths with -Zpolonius?)

@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2020

Regionck is an artifact of the fact that the old ast borrowck used lexical lifetimes. With polonius it doesn't even make sense to talk about regions anymore.

@guswynn
Copy link
Contributor Author

guswynn commented Nov 11, 2020

@bjorn3 should we get rid of the distinction between "migrate" and "nll", it may say ci time and also will eventually be entirely replaced by polonious, as far as i can tell?

@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2020

Migrate is the current behavior and nll is what it should become I think.

@jackh726
Copy link
Member

jackh726 commented May 5, 2022

Going to close this since NLL is tracked elsewhere.

@jackh726 jackh726 closed this as completed May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants