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

Regression: overflow while adding drop-check rules #29844

Closed
Manishearth opened this issue Nov 15, 2015 · 13 comments
Closed

Regression: overflow while adding drop-check rules #29844

Manishearth opened this issue Nov 15, 2015 · 13 comments
Assignees
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

https://users.rust-lang.org/t/drop-check-regression-overflow-while-adding-drop-check-rules-for-struct/3617

use std::sync::Arc;

pub struct DescriptorSet<'a> {
    pub slots: Vec<AttachInfo<'a, Resources>>
}

pub trait ResourcesTrait<'r>: Sized {
    type DescriptorSet: 'r;
}

pub struct Resources;

impl<'a> ResourcesTrait<'a> for Resources {
    type DescriptorSet = DescriptorSet<'a>;
}

pub enum AttachInfo<'a, R: ResourcesTrait<'a>> {
    NextDescriptorSet(Arc<R::DescriptorSet>)
}

fn main() {
    let _x = DescriptorSet {slots: Vec::new()};
}

playpen

gives the error



<anon>:22:9: 22:11 error: overflow while adding drop-check rules for DescriptorSet<'_> [E0320]
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                  ^~
<anon>:22:9: 22:11 note: overflowed on enum AttachInfo variant NextDescriptorSet field #0 type: alloc::arc::Arc<DescriptorSet<'_>>
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                  ^~
<anon>:22:14: 22:47 error: overflow while adding drop-check rules for DescriptorSet<'_> [E0320]
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:22:14: 22:47 note: overflowed on enum AttachInfo variant NextDescriptorSet field #0 type: alloc::arc::Arc<DescriptorSet<'_>>
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:22:36: 22:46 error: overflow while adding drop-check rules for collections::vec::Vec<AttachInfo<'_, Resources>> [E0320]
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                                             ^~~~~~~~~~
<anon>:22:36: 22:46 note: overflowed on struct alloc::arc::Arc field `_ptr` type: core::ptr::Shared<alloc::arc::ArcInner<DescriptorSet<'_>>>
<anon>:22     let _x = DescriptorSet {slots: Vec::new()};
                                             ^~~~~~~~~~
error: aborting due to 3 previous error

cc @pnkfelix

@Manishearth Manishearth added I-nominated regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Nov 15, 2015
@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2015

Trait matching creates a region that is equated with our current region each time it runs, but our recursion-detection can't handle this. Annoying.

I think we should have a unification system for regions to deal with this (ping @nikomatsakis).

@neon64
Copy link

neon64 commented Nov 20, 2015

Hi, I discovered this regression as I was fiddling around with one of my projects, and therefore I'm pretty motivated to try and fix it as it currently prevents me from continuing. I took a look at where the error was reported inside dropck.rs and was quickly overwhelmed by the logic and complexity, however I would like to have a crack at some sort of (hacky) fix.

@arielb1 the 'unification system for regions' sounds pretty complex, but if it is possible to explain the workings of that to a mere mortal like me then I'd be happy to do the 'plumbing' work on any implementation.

Also I find I solve problems a lot by visualising the data etc, and I was wondering how you handle debugging. I've recently switched from 'managed' languages like Java and I really miss the complete runtime inspection capabilities of something like the IntelliJ idea debugger. Is it possible to use gdb for that or do you just resort to a bunch of println!("{:?}", foo); statements everywhere?

@Manishearth
Copy link
Member Author

gdb works with rust. You need to compile rust in debug mode (I think make RUSTFLAGS='-g' would work? not sure) though.

@arielb1
Copy link
Contributor

arielb1 commented Nov 20, 2015

@neon64

A unification system for regions isn't something I would want a beginner to do - either me or @nikomatsakis should do it.

I do most of my rustc debugging by using the logs - RUST_LOG=rustc::middle::traits and look at the output. Debuggers have a tendency of not being useful with the complex data structures and functions rustc uses.

@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 20, 2015
@nikomatsakis
Copy link
Contributor

@arielb1 yes, I've encountered this problem, but I am not sure what answer I prefer. A unification system for regions is one thing I considered, along with an alternate scheme for inferring substitutions. I had the latter up and working (I can't find the branch just now) but was dissatisfied with it in some ways that I don't remember now -- the main thing was that instead of creating fresh type variables for every impl parameter, it would first try to deduce the values for the type variables by looking at the trait reference. e.g., if we had impl<T> Foo<T> for i32, and the trait reference was Foo<u32> for i32, we would not make a variable $T that gets unified with u32, we'd just compute a substitution T=u32 first off. In some cases, we had to fall back to unification variables (e.g., projections).

@arielb1
Copy link
Contributor

arielb1 commented Dec 1, 2015

@nikomatsakis

I was also thinking of some kind of "trait IR" to speed up trait matching, but the full set of required operations requires some kind of unification, so we will not completely eliminate the problem. Maybe I should be working on region unification?

@nikomatsakis
Copy link
Contributor

triage: P-high

This is a regression, so giving P-high, but given that the fix may involve changing some fundamental infrastructure we have to be a bit wary.

@nikomatsakis nikomatsakis self-assigned this Dec 3, 2015
@arielb1 arielb1 assigned arielb1 and unassigned nikomatsakis Dec 3, 2015
arielb1 pushed a commit to arielb1/rust that referenced this issue Dec 7, 2015
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 8, 2015
out what types/regions we should use just by matching up the trait
reference from the impl with the trait reference we need. Fixes rust-lang#29844.
@nikomatsakis
Copy link
Contributor

@arielb1 I found that branch I was talking about (which required running git grep across every revision in every rust directory I have, but that's another story). This is it:

https://github.com/nikomatsakis/rust/tree/dropck-infinite-recursion

It does make this test case pass. I am running all tests now to see if the rest pass. I've only rebased the code but not deeply re-reviewed it. In particular, I don't remember what I didn't like about it.

@brson
Copy link
Contributor

brson commented Dec 9, 2015

Looks like this will hit 1.5 stable tomorrow?

@brson brson added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 9, 2015
@brson
Copy link
Contributor

brson commented Dec 11, 2015

Possibly worth considering a point release.

bors added a commit that referenced this issue Dec 12, 2015
Fixes #29844

I would prefer to
(a) make some performance measurements
(b) use the unification table in a few more places
before committing further, but this is probably good enough for beta.

r? @nikomatsakis
@neon64
Copy link

neon64 commented Dec 12, 2015

Thanks so much guys for being able to fix this. It is great to be part of a community that is willing to help others, and I realise now that I wouldn't have been able to fix it myself.

@arielb1
Copy link
Contributor

arielb1 commented Dec 12, 2015

Sorry for the mess. My patch got a little delayed, and then I had a cold (I'm better now).

@arielb1
Copy link
Contributor

arielb1 commented Dec 12, 2015

Our team should try to be better with things slipping than that, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants