Repository navigation
Borrow check does not detect moves in overloaded operators #3387
Description
This test crashes:
enum foo = ~uint;
impl foo: add<foo, foo> {
pure fn add(f: foo) -> foo {
foo(~(**self + **f))
}
}
fn main() {
let x = foo(~3);
let y = x + move x;
}
The fix is already in my trans refactor branch, just wanted to make a note of it here. The error is that the region hierarchy tables don't consider overloadable operators as possible scopes.
Activity
WIP: Massive refactoring of trans
nikomatsakis commentedon Sep 6, 2012
The current fix for this, unfortunately, makes a lot of otherwise reasonable programs fail to borrow check and also causes an ICE in one case. Nonetheless, I plan to land it as part of the trans rewrite.
I will leave this issue open, though, and I have xfail-test'd a few tests and marked them with the issue #. Immediately prior to landing the rewrite I'll try to get a proper fix and then close this issue.
The proper fix, I think, is to take a better approach to rvalue lifetimes, per recent discussions with Graydon. The reason we get error messages is that the current rvalue lifetimes are too short in many cases. The current plan is two fold: first, I will adjust how cleanups occur in trans so that they are grouped at statements rather than being arbitrarily tight (for example, bounded around calls). This will help with the ICE, which occurs because borrowck decides that something should be preserved for a very small lifetime and trans doesn't install an appropriate cleanup scope.
However, the second part is then to use inference to decide how long a particular rvalue should live based on whether its contents are borrowed. It seems that there is no simple rule that accommodates all the use cases we'd like, so we are going to have borrowck make the decision. The rule will be something like: "temporaries live at least as long as the enclosing statement, but no longer than the innermost loop or fn body". If you want a precise lifetime, create a variable.
nikomatsakis commentedon Oct 24, 2012
In the end I opted not to land that patch, but this and related issues are cropping up again. The basic problem I am encountering now—which is related—is that our current region hierarchy is too shallow. That is, we allow call expressions, blocks, fn bodies, and loops to act as parent nodes, and arrange all of other expression nodes under them. We certainly wish to include operators (binary, index, etc) as possible parents—and perhaps simply want to mirror the AST in its entirety. The latter is often how I think of it in my head.
This requires some caution. I know we are getting a bit of mileage now out of the shallowness of our tree and we have to be sure we don't introduce some unsoundness here or there. An example:
If we are not careful, the lifetimes of &a and &b could be inferred just to those two expressions (as shown) and not to the call to foo as a whole. Actually I guess this can already be a problem today, it's just mildly exacerbated by introducing more nodes. So basically the shallowness of our tree may be hiding some bugs (I don't know that it is).
To fix this, and related errors, we just have to be very careful in the regionck.rs pass and elsewhere that the lower bounds we place on lifetimes is not too short. Unfortunately this reasoning is currently a bit scattered between the type checker and regionck. Probably it is best to move that reasoning into regionck as much as possible, because it runs after the type inferencer (but not the region inferencer) has acquired all the data it will ever have.
nikomatsakis commentedon May 7, 2013
Fixed now.
compile-fail tests:
borrowck-loan-in-overloaded-op.rs
borrowck-loan-rcvr-overloaded-op.rs
Auto merge of rust-lang#3387 - RalfJung:bench, r=RalfJung
Merge rust-lang#3387