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

More restrictive 2 phase borrows - take 2 #58739

Open
wants to merge 6 commits into
base: master
from

Conversation

@matthewjasper
Copy link
Contributor

matthewjasper commented Feb 25, 2019

Signal lint diagnostic mutable_borrow_reservation_conflict when borrow-check finds a 2-phase borrow's reservation overlapping with a shared borrow.

(pnkfelix updated description)

cc #56254 , #59159

r? @pnkfelix

cc @RalfJung @nikomatsakis

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 25, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 25, 2019

⌛️ Trying commit 58d9280 with merge d7cd765...

bors added a commit that referenced this pull request Feb 25, 2019

Auto merge of #58739 - matthewjasper:more-restrictive-tpb, r=<try>
More restrictive 2 phase borrows - take 2

Another try at this. Currently changes to a hard error, but we probably want to change it to a lint instead.

cc #56254

r? @pnkfelix

cc @RalfJung @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2019

💥 Test timed out

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2019

⌛️ Trying commit 58d9280 with merge 684d1bd...

bors added a commit that referenced this pull request Feb 26, 2019

Auto merge of #58739 - matthewjasper:more-restrictive-tpb, r=<try>
More restrictive 2 phase borrows - take 2

Another try at this. Currently changes to a hard error, but we probably want to change it to a lint instead.

cc #56254

r? @pnkfelix

cc @RalfJung @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@rust-lang/infra can this have a check only crater run?

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Feb 26, 2019

You have the perms to request one yourself now 🎉

@matthewjasper

This comment was marked as outdated.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@craterbot run start=master#b57fe74a27590289fd657614b8ad1f3eac8a7ad2
try#d7cd76545183ac4a130654fd5c03b5d3ef1287b2 mode=check-only

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Feb 26, 2019

🚨 Error: missing end toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@matthewjasper

This comment was marked as outdated.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@craterbot run start=master#b57fe74a27590289fd657614b8ad1f3eac8a7ad2 end=try#d7cd76545183ac4a130654fd5c03b5d3ef1287b2 mode=check-only

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Feb 26, 2019

👌 Experiment pr-58739 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@matthewjasper

This comment was marked as outdated.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@craterbot abort pr-58739

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Feb 26, 2019

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@matthewjasper

This comment was marked as outdated.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@craterbot abort name=pr-58739

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Feb 26, 2019

🗑 Experiment pr-58739 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2019

@craterbot run start=master#fb162e69449b423c5aed0d9c39f6c046fa300c30 end=try#684d1bdaca93437778609f08a04ce6e7e30a894f mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Feb 26, 2019

👌 Experiment pr-58739 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 13, 2019

Still I think there is a broader question here about whether we need to stick to as strict as path as @joshtriplett is describing, in terms of migration of code over time.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 13, 2019

I do not know if the formal reasoning efforts would be satisfied if their model technically only applied to editions >= 2021

That'd be rather unsatisfying. I'd value consistency (across editions) stronger than easier modeling of some of the editions.

The reasoning for this change was not to take away this feature forever, but allow a more deliberate introduction of more powerful kinds of 2PB. At this point, it may already be too late for that. I can only hope that we move more deliberately with such extensions in the future, and become better at taking into account concerns such as feasibility and complexity of a formal model.
(I am not meaning to blame anyone here; 2PB happened before the inception of Stacked Borrows. It is also my own fault that I delayed integrating 2PB into Stacked Borrows, as a consequence of which I only realized one week before the release that 2PB are much more powerful than I thought they would be.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 13, 2019

Some thoughts:

In retrospect, perhaps we should have shipped NLL without two-phase borrows. I'm not sure if that would have been technically feasible. I don't think we really considered it at the time and obviously we can't change it now. But it's a good think to keep in mind for the future. Obviously, I would much rather that the title of this PR would be "enable two-phase borrows!" than "restrict two-phase borrows".

Of course, we did ship two-phase borrows and stabilize NLL. At the time, I at least thought the consensus was that we would add this restriction (perhaps I was mistaken), for the usual reasons: we don't yet know precisely how much behavior to expect, so it's better to start small and add new behavior than to start large and regret it. (Basically, same argument as the previous paragraph, but with the caveat of doing it in the future.)

Procedurally, how I thought of it was that we were classifying the current 2PB behavior as a "known bug" that we decided to ship with because we estimated that the overall impact of fixing it would be small. I think the crater results largely bear out the assumption, though that's a matter of perspective. That said, it took us a bit longer to prepare this PR than we would have hoped, but oh well, that's life for you.

It may be worth nothing that shifting from warning to error is another decision point. For major changes like this, we usually do those shifts with crater results in hand, so as that point comes closer, we would be able to tell how much people have observed these warnings. I'm not advocating we revisit this decision then -- I would not want to -- but it seems like an important caveat. In particular, if we focus on 2-phase borrows, it could be that we resolve our concern with 2PB before then, though given @RalfJung's need to graduate, that may be unlikely. =)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 13, 2019

In retrospect, perhaps we should have shipped NLL without two-phase borrows.

For my part I think in retrospect I think we should not have shipped NLL at all as part of Rust 2018 or delayed the edition until we could have implemented the restriction in this PR.

Of course, we did ship two-phase borrows and stabilize NLL. At the time, I at least thought the consensus was that we would add this restriction (perhaps I was mistaken), for the usual reasons: we don't yet know precisely how much behavior to expect, so it's better to start small and add new behavior than to start large and regret it. (Basically, same argument as the previous paragraph, but with the caveat of doing it in the future.)

Procedurally, how I thought of it was that we were classifying the current 2PB behavior as a "known bug" that we decided to ship with because we estimated that the overall impact of fixing it would be small. I think the crater results largely bear out the assumption, though that's a matter of perspective. That said, it took us a bit longer to prepare this PR than we would have hoped, but oh well, that's life for you.

I wholeheartedly agree with this. I definitely was under the impression that we would treat this as a bug, crater it, and then fix it. Otherwise I would not have consensed to shipping NLL with the edition in the first place.

That'd be rather unsatisfying. I'd value consistency (across editions) stronger than easier modeling of some of the editions.

Indeed. The goal of restricting would be to find an operational semantics that would fit with the type system. If we can find such an appropriate semantics then there's no value in not having it on all editions. And we must find some such semantics somehow for Rust 2018. Having any differences between the operational semantics of different editions seems therefore like a non-starter.

pnkfelix added some commits Mar 13, 2019

add mutable_borrow_reservation_conflict future-incompatibility lint.
Convert the new 2-phase reservation errors into instances of the lint
so that they will be controlled by that attribute.
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 13, 2019

Just to be clear: My original intent in making this a future-incompatibility lint was not that we would end up having differing static semantics between editions. (nor different dynamic semantics between editions; but even I have not gone so far as to suggest that here.)

My original intent was to instead treat this as a bug in our static semantics, and land the fix for that bug gradually, via warning-cycles, in a similar manner as we have done in the past when possible for unsoundness issues.

  • Namely, when I wrote my suggestion, I didn't intend for the change in semantics to be tied to any particular edition (current or future).
  • But I also didn't specify that it wouldn't be tied to a edition.

When @joshtriplett responded to my suggestion for staged breakage with a comment that assumed that the breakage would be tied to a future edition (and that we would simply allow the less-restricted two-phase borrows on edition 2018), I hadn't expected that response at all, and I decided to take it quite seriously. It seemed like it could provide a reasonable path to a compromise.

It seems like a multiple parties involved would not be satisfied with that compromise, though.


My next question: does @joshtriplett object to using a future compatibility lint here; a lint with no attached schedule for when the lint would be promoted to a hard error (potentially across all editions)?

My thinking here is:

  • maybe future research into alternative formalizations of the dynamic semantics will find one that works with more general two phase borrows, ...
  • but for now we want to push developers away from using that generalized form, ...
  • and a lint seems like a plausible way to do so without breaking any crates in the meantime?
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 13, 2019

Also, in case its not clear:

  • I've reviewed @matthewjasper 's work here and am willing to r=me it.
  • But I've also added on my own commits that handle the conversion of @matthewjasper 's diagnostic into a new lint (#59159); those need review (either by @matthewjasper, or anyone familiar with the lint system and NLL tests)
  • and of course there is the outstanding fcp merge request
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Mar 13, 2019

I'm happy with the commits to convert to a lint.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 14, 2019

So we discussed this in some depth at today's lang team meeting. I also went back and reviewed the logs from the original issue (#56254). I wanted to try and produce a summary of the various things that were said and a few other things besides. I will try to keep this objective and not color with my opinion.

First off, I feel the meeting left in an ambiguous state. There wasn't a clear consensus in that particular meeting. However, do see the history of the discussion section that follows.

History of the discussion

One point that we did not really discuss much in the meeting, but which I think is relevant, is that many of us (myself included) thought that this issue was already decided in favor of merging. Looking over the original issue (#56254), I see a number of comments to that effect. However, it also seems possible that there was some "talking past one another" and misunderstanding. Unfortunately, the record is not as clear as one might like.

For example, @pnkfelix writes that, in the NLL meeting, the NLL WG decided to make this change on master and risk some breakage. @scottmcm then later writes that we discussed in the lang-team and notes:

Everyone seems fine with prohibiting this for now, though generally the the code was considered reasonable. There was some concern about details of the stability promise here, and thus whether it's justifyable to disable it once it's in a stable release.

There were a few later comments where we reiterated the plan, though with some ambivalence -- basically the same ambivalence that came out in the meeting today. My memory (which seems to accord with e.g. this comment of mine is that we had decided to make this change, but I was vaguely reluctant and toying with the idea of advocating that we reverse this decision, before ultimately deciding that it was the prudent course.

Observations from the meeting

In the course of the meeting we made a few observations that I noted in the notes.

Everybody agrees it would be best if we had two concrete models to compare, one that permits this pattern of pre-existing borrows, and one that forbids it. Those advocating for landing this PR felt that this was the best way to buy us time to make such a comparison, since developing this second model will take time.

crater breakage is the tip of the iceberg. @joshtriplett raised the (clearly true) point that if we see breakage on a crater run, we can be sure that there is breakage we don't see. Real, stable code will be affected by this change (though not immediately, as it starts as a warning).

Everyone would like to accept this pattern eventually, all other things being equal. However, not everyone agrees on the amount of complexity they would be willing to add to the underlying UB model -- or the amount of optimizations they would forego -- in order to accept this pattern.

There are two RFCs that justify landing this PR. First, RFC 1122 which defined the semver guarantees for the Rust language, explicitly states that we can land changes that affect extant code for soundness fixes, compiler bugs, or underspecified language semantics. This RFC specifies that for a "small number" of affected crates, we can simply land the change, but recommends a warning period. I think that there was general agreement that accepting this pattern was considered a bug in 2PB, albeit a difficult one to fix. Second, RFC 2405 defined that "accidentally stabilized features are not stable".

No crates will stop compiling immediately (i.e., as a direct result of this PR). This is because of the warning period. The affected crates, if not updated, would stop compiling if the warning transitions to a hard error. It is possible, of course, that we will find a model we are happy with before the warning period is over, in which case we could simply remove the warning -- but we should not expect that, as designing and investigating models is complex and time-consuming research work.

However, crates may wind up rewriting code that would later be accepted (which is unfortunate). We might wind up making this pattern a hard-error and then deciding that the ultimate model we want is able to gracefully accommodate this pattern -- or, perhaps, we will be able to accommodate some instances of this pattern but not all.

The real-world breakage here falls within ordinary parameters. We regularly fix bugs that affect code in the wild. The absolute number of affected crates here is quite small.

Specific proposals

I believe we ended with a specific proposal to issue a future compatibility lint and, in the associated issue, explain that we this code may ultimately wind up being accepted but we are trying to settle the underlying rules for UB in this case and hence are opting to take a conservative approach.

Moreover, we will do a kind of retrospective to try and formulate rules going forward that prevent us from getting into this situation again. (I for one think it would be useful to collect the rules from various RFCs into a "procedures" manual in the lang-team repo, for example.)

Alternate proposal

Right at the end of the meeting, there was an alternate proposal that we might take time to study both artifacts before reaching a decision. I discuss this proposal in the comment below.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 15, 2019

No crates will be immediately affected by this PR. This is because of the warning period. Indeed, it may be that no crates are EVER affected by this PR. We might find a model that we are happy with before we convert the future compat warning into a hard error.

I'm not sure if by "crate" you just mean libraries failing to build, but future compat warnings do impact people in that they feel pretty strongly pressured to rewrite their code to avoid them. Since it seems that the consensus is we will probably ultimately allow this code, this seems like a rather unfortunate outcome.

But I'm not very involved in any of the related groups, so I've checked my box. I just think we should remember to think about user experience of stability more holistically than whether or not code keeps building.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 15, 2019

@withoutboats

I'm not sure if by "crate" you just mean libraries failing to build, but future compat warnings do impact people in that they feel pretty strongly pressured to rewrite their code to avoid them. Since it seems that the consensus is we will probably ultimately allow this code, this seems like a rather unfortunate outcome.

Thanks for the correction. When I wrote affected, I really meant "stop compiling". I will update the summary to make it more precise. I will also add a bit more text covering the downside of people being asked to rewrite code that (may be) later accepted.

Since it seems that the consensus is we will probably ultimately allow this code, this seems like a rather unfortunate outcome.

I think the consensus is that we would like to allow this code but not that we will probably do so. Specifically, it depends on what kind of model we are able to find. I think that if we all felt confident we would find a model that accepts this code and permits plenty of optimizations and is easy to explain and understand, we would not have this PR open right now.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 15, 2019

I realized that there was something I left out of my summary, in a sense. I'm going to add it here as a separate comment. This comment is more of a "personal opinion", in that the following thoughts are not things we discussed in the meeting, but I am still attempting to make them fairly objective (feel free to object and I will try to clarify the language or strike points that seem wholly incorrect).

Alternate proposal

Near the end of the meeting, as we running out of time, I believe @joshtriplett was suggesting that perhaps we should try to wait on this decision until we have time to compare a model that accepts this pattern with one that does not, so we can better judge the tradeoffs involved (e.g., how much more complex is the permissive model? What kinds of optimizations does it rule out?).

This does sound nice. The difficulty however is that:

  • As time passes, more and more crates will be relying on this pattern, and we wind up without the freedom to change it.
  • We don't have the two models now and we don't have anyone who presently has time to work on them.
  • There aren't really two models anyway -- there are a variety of ways one might accommodate this pattern, resulting in distinct trade-offs, and ideally we would like time to explore the space more fully.

One option could be to elaborate on some of the repercussions of the "most expressive model" that @RalfJung described here under the heading "Extreme Proposal", but it's not clear that we have the time to do even that very well.

The fact of the matter is that even the base model of stacked borrows itself is still an experimental artifact whose consequences (e.g., on optimization) are not fully understood, despite @RalfJung having been working on it for months. Figuring out those consequences in a way that @RalfJung or @arielb1 understands is ongoing work. Being able to explain those consequences in a way that the rest of us will understand will be harder still. I would not want to try and do it on a rushed schedule.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 16, 2019

@withoutboats

I'm not sure if by "crate" you just mean libraries failing to build, but future compat warnings do impact people in that they feel pretty strongly pressured to rewrite their code to avoid them. Since it seems that the consensus is we will probably ultimately allow this code, this seems like a rather unfortunate outcome.

Agreed entirely.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 19, 2019

I haven't seen a lot of further discussion here. I think we should be aiming to reach a decision shortly, because it would be good to beta uplift this PR, if we are going to make this change at all. In short, time is of the essence here.

Apart from @withoutboats's clarification (which I included), I also haven't seen objections to my summary (or my addendum concerning the alternate proposal), so I'm going to assume we all agree on those assessments of the situation. Please feel free to let me know if you disagree. =)

So the key factors we have to weigh seem to be the following:

  • The cost of existing, stable crates having to migrate, due to getting warnings (and later, likely, errors).
  • The benefit of having time to explore the interaction of stacked borrows and 2PB at our leisure.
  • The danger, if we don't take this step, of winding up with a model that doesn't give us the optimizations we want or is distinctly more complex to explain and implement.

Other factors that I'm not quite sure how to weigh in include expectations. Multiple people (including at least centril and myself) were under the impression that this was already decided. On the other hand, clearly that was not universally agreed upon, and of course the crate authors and other people using stable were under the expectation that their code would keep working. =) I'm going to call this a wash and just say that we should take this situation into account when faced with future decisions of this kind.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 19, 2019

I think the consensus is that we would like to allow this code but not that we will probably do so. Specifically, it depends on what kind of model we are able to find. I think that if we all felt confident we would find a model that accepts this code and permits plenty of optimizations and is easy to explain and understand, we would not have this PR open right now.

One thing that I think might help understand the motivation here is more specifics about what kind of optimizations might be incompatible with allowing this code; unless I've missed a comment, right now they've been alluded to without a lot of explanation. The motivation of this warning feelings very theoretical right now to me and I think also to @joshtriplett; what is the practical impact of losing out on these optimizations?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 19, 2019

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 19, 2019

In response to the "history of the discussion" point, digging through the previous issue, I think the summary of "Everyone seems fine with prohibiting this for now, though generally the the code was considered reasonable." was not accurate, because I remember raising exactly these same objections around that time (note my comments right around that time consistent with that). The subsequent summary comment of "There was some concern about details of the stability promise here, and thus whether it's justifyable to disable it once it's in a stable release." also contradicts the idea that "everyone seems fine".

I can somewhat understand why, based on comments in the github issues, it seemed like this had previously reached a semblance of consensus. Going based on my own memory, I don't recall this reaching consensus, and I do recall this having the same level of discussion and concern then as it does now.

If the tradeoff here was between "don't break stable code/users" and "enable this specific incredible optimization opportunity that will make many people happy", that would still be a hard decision, and I'd be asking whether we could provide an opt-in, or alternatively detect this case and specifically warn "if you do this, that's OK (in this edition at least), but you're missing out on an optimization".

But right now, the tradeoff seems to be between "don't break stable code/users" and "make borrows easier to model, which might unlock some future optimizations but we don't yet know". To me, personally, that feels like a far, far easier decision. This doesn't fall under "it's OK to break stable if the thing we're breaking is unsound", because the code in question is sound. It doesn't feel right to me to redefine "sound" with a new model and then use that new definition to justify breaking stable code.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Mar 19, 2019

As a casual observer following by email, I’m a bit troubled that this conversation exists at all. It signifies a process issue somewhere. I would humbly propose that the process issue was to push on getting something stable too fast; a time-sensitive decision was made where the consequences were not widely understood.

It seems that the same thing is happening again here: we have a need to make a decision fast to make up for the previous fast decision. And it could be possible to need another fast decision if we missed something here.

Regarding the specific discussion, I’m a big fan of Ralf’s modeling work, but I think rust is at a point where for the sake of maturity, we need to live with language’s existing warts and do our best even if the result is less elegant. Making a fast decision to undo a previous fast decision and then uplifting the results to beta seems like a bad precedent to set.

Also, I will note that having a model that works for only edition 2021+ still has immense practical value for increasing confidence in code written from then on. We can still have a lint or a warning about violations on older editions.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 19, 2019

Is landing a lint a fast decision?

Or is choosing not to land a lint a fast decision?

(In other words: i can see the lint as buying us time.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 19, 2019

Do we have reason to expect a deluge of new users of this pattern prior to the point where we can make a considered decision between fleshed-out models and optimization opportunities?

Past experience certainly suggests that the number of users can grow rapidly. More importantly, it is actually often somewhat hard to get people to migrate their code -- it takes time. Sometimes crates are not well maintained. If we have (for example) a problem in version 0.3.0 of a crate, it may get fixed as part of a move to 0.4.0 -- but then we sometimes find that there are downstream crates still relying on 0.3.0, and hence we need to try and get a 0.3.1 issued in order to fix those crates, or encourage them to upgrade. In general, the sooner we can act, the better. (In addition to this PR, I would prefer to submit corrective PRs to the affected crates as well, for example.)

But right now, the tradeoff seems to be between "don't break stable code/users" and "make borrows easier to model, which might unlock some future optimizations but we don't yet know".

The tradeoff is not just about optimizations, but also about the ease of reasoning about unsafe code. That is, the more complex our underlying alias model, the harder it will be for unsafe code authors to understand it. The same holds for compiler authors and the rest. We are certainly aiming to give ourselves freedom here.

Also, I want to push back on "don't break stable code" -- that is, I don't think this is a "binary issue". Certainly we try to avoid affecting stable code, but (as documented in my comment) we reserve the right to fix bugs and clarify semantics. There are a lot of practical considerations that go into any particular change. One of them of course is the sheer amount of code -- if there were 100 crates, or 1000, I don't know if we'd be having this conversation. Another is how hard it is to fix the affected code. And a final one is what the "upgrade path" is like. From my POV, this change represents more or less the gold star:

  • there are not a lot of affected crates
  • you get warnings, not immediate errors, giving time for the transition
  • the actual fix is fairly easy to do

Likewise. I had the same question in the last lang team meeting: what optimizations could we not do with the more expansive model that would permit this code?

I agree, it'd be great if we could be more precise. However, this is challenging to do, in part because there are still quite a number of unknowns.

Let me try to give a feeling. Hopefully I get the details right, I haven't thought stacked borrows very deeply recently. The key feature of the current model that we are trying to preserve is that, when you take an &mut borrow (2-phase or not) and produce a reference r, that reference is exclusive for as long as it is used. So for example if we have a snippet of code like so:

let x = &mut *p;
... // XXX
use(*x);

then we know that any access to *x that occurs in the span XXX came from some reference that was derived from x (perhaps via x itself, or perhaps by reborrowing x). This allows us to do various kinds of code reordering, because we can easily identify potential conflicts. In particular, any access to a pointer that we know "pre-existing" the &mut *p borrow that created x cannot overlap with x.

But with 2PB as presently implemented, that property is no longer true. One could do reads from references that alias x, as long as they occur before x is "activated". At minimum this might disallow moving the activation point earlier.

However, this is where the question of the model comes in. If we adopted some of the more general models that Ralf proposed, in order to accommodate 2PB, that might have more dramatic effects, leading to all kinds of reorderings that would be disallowed but which would otherwise be beneficial. OTOH, if we adopted a more complex model, it might be more tailored to what we presently accept, but might in turn be harder to understand.

(I don't think it's entirely obvious at this point how to weight the simplicity of the model, optimization power, and other factors, fwiw, it's exactly one of the things I would like to be able to take some time and consider.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Mar 19, 2019

Seems like an easy decision to me as well - the breakage is within normal limits, so we can make at least a deprecation lint if there's any chance that the more conservative rule will be useful in the future.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 19, 2019

@joshtriplett

I don't consider it acceptable for us to break existing users in order to change the language semantics to make it easier to model.

We don't break existing users of Rust.

I appreciate where these sentiments are coming from, but I think this is an overly black-and-white take, and such strong statements make it hard to advance discussion. We have generally avoided such "hard and fast" rules, in favor of carefully thinking about outcomes any time a decision is needed; sometimes things go one way, sometimes another.

The fact of the matter is that we do break stable code in practice, when we strongly believe that other benefits outweigh this cost (which also depends on analysis of the cost of breakage). It's a matter of looking at the global picture -- the net effect on all users.

I think labeling this as a question of making the semantics "easier to model" is missing the point; the ease of modeling reflects the ease of reasoning about unsafe code, which I think is another important goal.

In other words, there's a conflict between two values here:

  • We value stability of Rust
  • We value the ability of Rust programmers to reason about their code, especially when using unsafe

If we cling to a hard-and-fast rule around one value, we risk losing the other. That's why the nuanced analysis in this thread -- in terms of the migration experience, etc -- is crucial. And I personally agree with the conclusion of others on the team, that in this particular case it makes sense to go through the migration process when all the factors are weighed.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 19, 2019

@nikomatsakis

(I don't think it's entirely obvious at this point how to weight the simplicity of the model, optimization power, and other factors, fwiw, it's exactly one of the things I would like to be able to take some time and consider.)

That is exactly my concern as well, and this decision feels rushed in a way that prevents taking that time. I get that you're suggesting this lint in the hopes of buying such time, but conversely, "rustc is telling me not to do this" is a kind of breakage for which I'd want that time to consider. (In particular, in a world in which we advise Rust developers to use deny(warnings).)

In the interests of trying to find a path forward here (though I don't yet feel comfortable going forward with this), here are three concrete concerns I have:

  • Are we committing to not turning any potential lint into anything more than a warning until after we've been able to make this decision at our leisure? I wouldn't want to find this lint turned into an error on the basis of "well, it's been a while, we don't see any crates using it, and the inexorable path of future-compatibility lints is towards becoming errors", without us first deciding on a two-phase-borrows model and semantics.
  • Can we find language for this that does not come across as "stop doing this", but rather "please go read this github issue before introducing new instances of this, and please let us know if this is a pattern you care about"? As @withoutboats said, I feel that a future-compatibility lint comes across as "stop doing this", which doesn't seem ideal for something we may well want to allow.
  • What went wrong here, and how do we stop it from happening again? A retrospective would help.
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 19, 2019

@aturon You're right; both of those sentiments could have been phrased differently, and in particular I should have made it explicit (rather than implicit) that they were "I think we should not" statements rather than "we do not" statements.

For the record, my position is not "we should not under any circumstances do this", as an absolute rule. Rather, my position is "I feel like we're being rushed into doing something without time to think about it", together with a substantial amount of recent memories of how such rushing resulted in poor outcomes.

I also feel that people see a lint as a lightweight nudge, even though we encourage people to use deny(warnings) which turns lints into "you must change your code immediately before you can do anything else".

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 19, 2019

"I feel like we're being rushed into doing something without time to think about it", together with a substantial amount of recent memories of how such rushing resulted in poor outcomes.

I think the poor outcome is the one that we are trying to correct here. In other words, we opted not to rush this PR to arrive before Rust 2018 nor to disable NLL -- in retrospect, as I wrote, I think we should have disabled two-phase borrows, which I suspect we could have done, at least outside of match statements, but honestly we did not think of it. Instead, we opted to allow NLL to ship with what was effectively a known bug (there are others, though they are increasingly obscure), and to correct it later. (At least, that was my perspective, I realize now it was not universally shared.)

I also feel that people see a lint as a lightweight nudge

I do not view the lint as a lightweight nudge. I view it as a fairly firm statement that people should rewrite their code.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 21, 2019

I'm going to transcribe here (with some light edits) an insight that I previously circulated via an email to T-lang team:

While reviewing a (out-of-date?) list of previous breaking changes (bitrust), I was struck by another breaking change we made: we disallowed floating point literals in patterns (and this is currently enforced via the lint illegal_floating_point_literal_pattern ).

I claim that illegal_floating_point_literal_pattern is a similar case to what we are now discussing, but with far far far broader impact (e.g. it broke servo; at least in its initial form).

Why is it similar?

Because the intent of RFC 1445, as far as I remember, was not about removing a soundness concern; it was about restricting the Rust language to support just the cases that everyone can agree on for now.

We could have chosen to stick with the semantics as implemented, but we chose to land a lint because we didn't want to commit to that semantics.

And that, to me, sounds the same as trying to accommodate a restricted semantic model, even if it means disrupting the customers of the stable release channel.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Mar 21, 2019

Or is choosing not to land a lint a fast decision?

IMHO, changing behavior is usually more dangerous than not changing behavior, so the comparison seems a bit unfair. If we make the change, what prevents us from wanting to make more changes?

While reviewing a (out-of-date?) list of previous breaking changes (bitrust)...

Thanks for doing this research! I think an additional thing to keep in mind is that it might be appropriate to be more strict as the language matures; it does not follow that we did X before, so we can do X’ now. IMHO, as Rust matures we should be more careful about breaking changes. Even if they break a small percentage of crates, the absolute number of crates is increasing.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 21, 2019

Let me try to give a feeling. Hopefully I get the details right, I haven't thought stacked borrows very deeply recently. The key feature of the current model that we are trying to preserve is that, when you take an &mut borrow (2-phase or not) and produce a reference r, that reference is exclusive for as long as it is used.

Indeed. The key property that is lost by the more permissive two-phase borrows is the property that when a mutable reference is created (as a two-phase borrow), we know it is unique. Instead, it only becomes unique once it is written to the first time (that would be the "activation point", I guess).

Note that (somewhat unintuitively) this loss of analytic power affects all two-phase borrows, unless the compiler can prove that no other aliases get used. So it's not just the few instances that get rejected by a stricter borrow checker that get pessimized. After all, there could always be a raw pointer hidden somewhere that constitutes another alias. Basically, the moment unknown or hard-to-analyze code gets invoked, we'll have to assume the worst and assume aliases get used until the reference is written through the first time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.