Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable MIR-borrowck-only output #46097

Closed
nikomatsakis opened this issue Nov 19, 2017 · 6 comments · Fixed by #46106
Closed

Enable MIR-borrowck-only output #46097

nikomatsakis opened this issue Nov 19, 2017 · 6 comments · Fixed by #46106
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@nikomatsakis
Copy link
Contributor

Currently, if you pass -Zborrowck-mir, we output both the AST output (tagged with (Ast)) and the MIR output (tagged with (Mir)).

Example:

> cd src/test/compile-fail
> rustc -Zborrowck-mir borrowck/borrowck-unary-move.rs
error[E0505]: cannot move out of `x` because it is borrowed (Ast)
  --> borrowck/borrowck-unary-move.rs:17:10
   |
16 |     let y = &*x;
   |              -- borrow of `*x` occurs here
17 |     free(x); //[ast]~ ERROR cannot move out of `x` because it is borrowed
   |          ^ move out of `x` occurs here

error[E0505]: cannot move out of `x` because it is borrowed (Mir)
  --> borrowck/borrowck-unary-move.rs:17:10
   |
16 |     let y = &*x;
   |             --- borrow of `(*x)` occurs here
17 |     free(x); //[ast]~ ERROR cannot move out of `x` because it is borrowed
   |          ^ move out of `x` occurs here

error: aborting due to 2 previous errors

I would like to alter the switch to be one where there are three mores:

-Zborrowck=ast -- the default, emits only AST
-Zborrowck=mir -- emits only MIR errors (and doesn't write `(Mir)`)
-Zborrowck=compare -- emits both, like `-Zborrowck-mir` does today.

This will be useful as we progress towards our yearly goal of having a working "demo", since the demo isn't very good if people are seeing the old and new errors. It will also be less annoying for editing the unit tests, since we won't need three copies of each error. And if we want the current output when running by hand, it is still available.

@nikomatsakis nikomatsakis added E-needs-mentor T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 19, 2017
@nikomatsakis nikomatsakis added this to the NLL prototype milestone Nov 19, 2017
@nikomatsakis
Copy link
Contributor Author

Mentoring instructions

This is where the -Zborrowck-mir flag is defined:

borrowck_mir: bool = (false, parse_bool, [UNTRACKED],
"implicitly treat functions as if they have `#[rustc_mir_borrowck]` attribute"),

That is part of a macro that ultimately defines the DebuggingOpts struct. We could change that line to something like this:

    borrowck: bool = (false, parse_opt_string, [UNTRACKED],
        "select which borrowck is used (`ast`, `mir`, or `compare`)"),

This will change from a borrowck_mir: bool field to a borrowck: Option<String> field. One problem is that this will also accept nonsense like -Zborrowck=foo. I think what we ought to do is to add an enum BorrowckMode comparable to PrintRequest and the others that exist already:

#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
enum BorrowckMode {
    Ast,
    Mir,
    Compare,
}

Then we will add a field in the Options struct by inserting a line around here like this:

/// Determines which borrow checker(s) to run. This is the parsed, sanitized
/// version of `debugging_opts.borrowck`, which is just a plain string.
borrowck_mode: BorrowckMode,

(The setup here is that there is an Options struct, which we just edited, which contains a debugging_opts: DebuggingOpts field that has the "raw" debugging options; that DebuggingOpts struct is the one that is auto-generated by the macro above.)

Then we can add some code around here that will parse the debugging_opts.borrowck data and create a BorrowckMode enum, or else report an error (using a call to early_error like so). The incremental field is sort of roughly the model we want, except that it doesn't report any errors:

OK, once we've done that, we should get a few compilation errors because the field tcx.sess.opts.debugging_opts.borrowck_mir no longer exists. You can convert those to examine the tcx.sess.opts.borrowck_mode field. I think I would add some methods to BorrowckMode:

impl BorrowckMode {
    /// Should the AST-based borrow checker execute at all?
    pub fn use_ast(self) -> bool {
        match self {
            BorrowckMode::Ast => true,
            BorrowckMode::Compare => true,
            BorrowckMode::Mir => false,
        }
    }
    ...
}

and so on so that we can convert to tcx.sess.opts.borrowck_mode.use_ast() or whatever.

Finally, we have to make the AST-based borrow checker not execute (or at least not report errors) unless we are in the right mode. It's actually easier to just make it not report errors: this is because the AST-based borrowck is still used for giving "unused mut" warnings, so if we just skip it altogether we'll get a bunch of weird warnings. To make it not report errors, though, we just have to modify the functions in this file, having them check the origin and just do nothing if it is wrong.

@nikomatsakis
Copy link
Contributor Author

Oh, one last thing. We have to edit all the tests! They can be changed from having -Zborrowck-mir at the top to -Zborrowck=mir, and then removing the extra, duplicate comments from the [mir] revision.

@arielb1 arielb1 added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Nov 19, 2017
@est31
Copy link
Member

est31 commented Nov 19, 2017

I want to work on this!

@est31
Copy link
Member

est31 commented Nov 19, 2017

@LooMaclin there seems to be a conflict here!

@est31
Copy link
Member

est31 commented Nov 19, 2017

Especially, I'm almost done with my PR, the only thing that remains is updating the tests which is a bit time intensive...

@LooMaclin
Copy link
Contributor

@est31 no problem. im stop working on that

@TimNN TimNN added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Nov 21, 2017
bors added a commit that referenced this issue Nov 26, 2017
Add a MIR-borrowck-only output mode

Removes the `-Z borrowck-mir` flag in favour of a `-Z borrowck=mode` flag where mode can be `mir`, `ast`, or `compare`.

* The `ast` mode represents the current default, passing `-Z borrowck=ast` is equivalent to not passing it at all.
* The `compare` mode outputs both the output of the MIR borrow checker and the AST borrow checker, each error with `(Ast)` and `(Mir)` appended. This mode has the same behaviour as `-Z borrowck-mir` had before this commit.
* The `mir` mode only outputs the results of the MIR borrow checker, while suppressing the errors of the ast borrow checker

The PR also updates the tests to use the new flags.

closes  #46097
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants