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

refactor the BorrowckErrors trait to take fn(self) #48783

Closed
nikomatsakis opened this issue Mar 6, 2018 · 3 comments
Closed

refactor the BorrowckErrors trait to take fn(self) #48783

nikomatsakis opened this issue Mar 6, 2018 · 3 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@nikomatsakis
Copy link
Contributor

The [BorrowckErrors trait] is used to report errors from both HIR and MIR-based borrowck:

pub trait BorrowckErrors {

It contains various methods currently defined to take &self:

fn cannot_move_when_borrowed(&self, span: Span, desc: &str, o: Origin)
-> DiagnosticBuilder<'_>
{
let err = struct_span_err!(self, span, E0505,
"cannot move out of `{}` because it is borrowed{OGN}",
desc, OGN=o);
self.cancel_if_wrong_origin(err, o)
}

However, it is implemented on the TyCtxt type:

impl<'b, 'gcx, 'tcx> BorrowckErrors for TyCtxt<'b, 'gcx, 'tcx> {

This type is actually an alias for an &-reference, so that means that the final self winds up passing a double reference. This is not bad in and of itself, but @spastorino encountered in #48682 that the current setup causes conflicts around the tcx when using an &mut self method. In particular, if you do something like this:

let err = self.tcx.cannot_use_while_mutably_borrowed(...);
..

this will borrow &self.tcx for as long as err is in use, preventing us from invoking other &mut self methods.

For now, we worked around this problem by storing the tcx into a local variable:

let tcx = self.tcx;

But if the methods were defined instead as fn(self), then it would have worked fine the original way.

Making this transformation would involve a few changes though. First off, right now the returned DiagnosticBuilder values carry the lifetime of the incoming &self reference. We would want to change that to the lifetime of the TyCtxt, so we would need to modify the trait to carry a lifetime parameter:

/// This trait is expected to be implemented on a reference.
/// The `'r` is the lifetime of that reference.
trait BorrowckErrors<'cx> {
    ...
    fn cannot_use_when_mutably_borrowed(
        self, // changed to self
        span: Span,
        ...
    ) -> DiagnosticBuilder<'cx> {
        ... // as before
    }
    ...
}

Then it would be implememented for TyCtxt like:

impl<'cx, 'gcx, 'tcx> BorrowckErrors<'cx> for TyCtxt<'cx, 'gcx, 'tcx> { ... }

And we would have to modify the other impl like so:

impl<'a, b, 'tcx: 'b> BorrowckErrors<'a> for &'a BorrowckCtxt<'b, 'tcx> {
@nikomatsakis nikomatsakis added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Mar 6, 2018
@nikomatsakis
Copy link
Contributor Author

cc @rust-lang/wg-compiler-nll -- little refactoring issue related to NLL, contains mentoring instructions

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 6, 2018
@csmoe
Copy link
Member

csmoe commented Mar 7, 2018

I'll try this.


sorry for missing the cc, may I take this issue?

@nikomatsakis
Copy link
Contributor Author

@csmoe but of course! (I can also add you to the NLL WG)

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Mar 12, 2018
…lf, r=petrochenkov

refactor the `BorrowckErrors` trait to take `fn(self)`

Fixes rust-lang#48783
kennytm added a commit to kennytm/rust that referenced this issue Mar 19, 2018
…lf, r=nikomatsakis

refactor the `BorrowckErrors` trait to take `fn(self)`

Fixes rust-lang#48783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

2 participants