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

Borrow check does not detect moves in overloaded operators #3387

Closed
nikomatsakis opened this issue Sep 5, 2012 · 3 comments
Closed

Borrow check does not detect moves in overloaded operators #3387

nikomatsakis opened this issue Sep 5, 2012 · 3 comments
Assignees
Labels
A-lifetimes Area: Lifetimes / regions A-type-system Area: Type system I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.

Comments

@nikomatsakis
Copy link
Contributor

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.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Sep 5, 2012
@nikomatsakis
Copy link
Contributor Author

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 added a commit that referenced this issue Sep 6, 2012
Also:
- report illegal move/ref combos whether or not ref comes first
- commented out fix for #3387, too restrictive and causes an ICE
@ghost ghost assigned nikomatsakis Sep 6, 2012
@nikomatsakis
Copy link
Contributor Author

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:

    foo(&a, &b)
        ^^   ^^
    ^~~~~~~~~~~^

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.

bors added a commit that referenced this issue May 7, 2013
…omatsakis

This rather sprawling branch refactors the borrow checker and much of the region code, addressing a number of outstanding issues. I will close them manually after validating that there are test cases for each one, but here is a (probably partial) list:

  - #4903: Flow sensitivity
  - #3387: Moves in overloaded operators
  - #3850: Region granularity
  - #4666: Odd loaning errors
  - #6021: borrow check errors with hashmaps
  - #5910: @mut broken

cc #5047

(take 5)
@nikomatsakis
Copy link
Contributor Author

Fixed now.

compile-fail tests:

borrowck-loan-in-overloaded-op.rs
borrowck-loan-rcvr-overloaded-op.rs

RalfJung pushed a commit to RalfJung/rust that referenced this issue Mar 23, 2024
make 'invalidate' benchmark shorter

This is currently by far the slowest benchmark in our suite, taking >9s, when the second slowest takes 2.7s. So let's speed this up to 2.3s, making it still the second-slowest in the benchmark suite.

`@saethlin` any objections? Also, why is this called "invalidate"? It got added in rust-lang/miri#3083 but I can't figure out the point of that name even after looking at the PR.^^ There should be a comment in the benchmark explaining what it is testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions A-type-system Area: Type system I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.
Projects
None yet
Development

No branches or pull requests

1 participant