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

for initial release, limit two phase borrows to method-call autorefs #46747

Closed
pnkfelix opened this issue Dec 15, 2017 · 4 comments
Closed

for initial release, limit two phase borrows to method-call autorefs #46747

pnkfelix opened this issue Dec 15, 2017 · 4 comments
Assignees
Labels
A-NLL Area: Non Lexical Lifetimes (NLL)

Comments

@pnkfelix
Copy link
Member

@nikomatsakis is concerned that two-phased borrows are a hazard. In particular, they are coupled to MIR code-generation, which means if we are too aggressive in how much code we accept today, it could mean that a later change to MIR construction will inject a regression.

Niko has suggested a simple strategy for fixing this that we should deploy before we stabilize two-phase borrows: We should thread through information about whether a borrow was introduced via method-call autoref, and then only do the two-phase borrow system for those autorefs.

That way we will be somewhat conservative in what code is deemed acceptable by two-phase borrows today, and therefore give us more breathing room in the future as we refine the system.

@pnkfelix pnkfelix added A-NLL Area: Non Lexical Lifetimes (NLL) WG-compiler-nll labels Dec 15, 2017
@pnkfelix
Copy link
Member Author

cc #46037

@pnkfelix
Copy link
Member Author

To be clear, we do not think that this aspect of two-phase borrows needs to block the initial release of #[feature(nll)]; that is why this issue is not on the NLL prototype milestone.

@pnkfelix
Copy link
Member Author

In my work on #46908, it seems like we have a choice:

Either:

  1. we modify a (relatively small...) number of our tests in slightly inscrutable ways in order to force a two-phase borrow to activate ... or
  2. we implement this before trying to land -Z borrowck=migrate (and thus this does belong on a NLL milestone.

at this point we have a series of NLL milestones on the path to stabilization, so we should decide where this falls in that sequence anyway.

@nikomatsakis
Copy link
Contributor

Given the large number of tests affected, it seems pretty clear to me that we ought to make this change if we are going to make it. That is, unless we plan to make two-phase borrowing part of the model we expect to teach users -- which might be ok but seemed like something we wanted to be cautious about -- then I feel confident we need to impose restrictions, or else people will be stumbling across it all the time. In other words, I kind of think we should do this first so that we don't have to edit a bunch of tests.

bors added a commit that referenced this issue Feb 9, 2018
NLL: Limit two-phase borrows to autoref-introduced borrows

This imposes a restriction on two-phase borrows so that it only applies to autoref-introduced borrows.

The goal is to ensure that our initial deployment of two-phase borrows is very conservative. We want it to still cover the `v.push(v.len());` example, but we do not want it to cover cases like `let imm = &v; let mu = &mut v; mu.push(imm.len());`

(Why do we want it to be conservative? Because when you are not conservative, then the results you get, at least with the current analysis, are tightly coupled to details of the MIR construction that we would rather remain invisible to the end user.)

Fix #46747

I decided, for this PR, to add a debug-flag `-Z two-phase-beyond-autoref`, to re-enable the more general approach. But my intention here is *not* that we would eventually turn on that debugflag by default; the main reason I added it was that I thought it was useful for writing tests to be able to write source that looks like desugared MIR.
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)
Projects
None yet
Development

No branches or pull requests

2 participants