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

Remove -Zborrowck=compare #59193

Closed
4 of 8 tasks
matthewjasper opened this issue Mar 14, 2019 · 7 comments · Fixed by #62629
Closed
4 of 8 tasks

Remove -Zborrowck=compare #59193

matthewjasper opened this issue Mar 14, 2019 · 7 comments · Fixed by #62629
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-medium Medium priority

Comments

@matthewjasper
Copy link
Contributor

matthewjasper commented Mar 14, 2019 •
edited
Loading

This flag is mostly unused and adds complexity. This may be best to split across multiple PRs.

  • Remove the flag (Remove -Z borrowck=compare flag #60513)
  • Remove from tests (Remove -Z borrowck=compare flag #60513)
  • Remove error reporting from AST borrow check
  • Remove librustc_borrowck dependency on librustc_mir
  • Remove error codes that are only emitted by AST borrow check
  • Remove the Origin enum
  • Remove BorrowckErrors trait or make it pub(crate)
  • Move borrowck_errors and suggest_ref_mut into the borrow_check module
@matthewjasper matthewjasper added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-NLL Area: Non-lexical lifetimes (NLL) labels Mar 14, 2019
@pnkfelix
Copy link
Member

NLL triage: prioritizing as P-medium.

@pnkfelix pnkfelix added the P-medium Medium priority label Mar 20, 2019
@chrisvittal
Copy link
Contributor

I'm willing to start poking at this. Removing the flag is easy enough. But what should I be doing with the tests that have the flag?

@matthewjasper
Copy link
Contributor Author

matthewjasper commented Mar 25, 2019 •
edited
Loading

You should be removing the flag, and updating the test annotations. This should be done after #59114, so that the migrate mode output is being tested.

@matthewjasper matthewjasper mentioned this issue Mar 27, 2019
Merged
bors added a commit that referenced this issue Apr 22, 2019
@bors
Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
bf0ea60 
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
@bors
Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
c21fbfe 
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
@matthewjasper
Copy link
Contributor Author

@chrisvittal do you still want to work on this?

@chrisvittal
Copy link
Contributor

I do. I’ll have a PR soon.

@chrisvittal chrisvittal mentioned this issue May 3, 2019
Merged
Centril added a commit to Centril/rust that referenced this issue May 4, 2019
@Centril
Rollup merge of rust-lang#60513 - chrisvittal:remove-borrowck-compare…
ffc33b3 
…, r=matthewjasper

Remove -Z borrowck=compare flag

This is the start of the work that needs to be done on rust-lang#59193. It just removes the flag and updates
the tests.

r? @matthewjasper
@matthewjasper
Copy link
Contributor Author

@chrisvittal are you working on the rest of this?

@chrisvittal
Copy link
Contributor

No Sorry. I should have told you sooner.

@matthewjasper matthewjasper mentioned this issue Jun 6, 2019
Merged
bors added a commit that referenced this issue Jul 12, 2019
@bors
Auto merge of #61590 - matthewjasper:remove-borrowck-mir-dependency, …
1b1b538 
…r=pnkfelix

Remove rustc_mir dependency from rustc_borrowck

Also renames `rustc_borrowck` to `rustc_ast_borrowck` and removes all error reporting from it.

cc #59193
@matthewjasper matthewjasper mentioned this issue Jul 12, 2019
Merged
bors added a commit that referenced this issue Jul 15, 2019
@bors
Auto merge of #62629 - matthewjasper:cleanup-borrowck-errors, r=petro…
5480b47 
…chenkov

Cleanup borrowck errors

This removes some of the unnecessary code that allowed sharing error reporting between two borrow checkers.

closes #59193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-medium Medium priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Cleanup borrowck errors matthewjasper/rust
3 participants
@pnkfelix @chrisvittal @matthewjasper