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

Bogus error in beta and nightly: recursive type has infinite size #31299

Closed
jorendorff opened this issue Jan 30, 2016 · 18 comments
Closed

Bogus error in beta and nightly: recursive type has infinite size #31299

jorendorff opened this issue Jan 30, 2016 · 18 comments
Assignees
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jorendorff
Copy link
Contributor

trait Front {
    type Back;
}

impl<T> Front for Vec<T> {
    type Back = Vec<T>;
}

struct PtrBack<T: Front>(*mut T::Back);

struct M(PtrBack<Vec<M>>);

fn main() {
    println!("{}", std::mem::size_of::<M>());
}

This program prints 8 in Stable (1.5.0), but in Beta and Nightly compilation fails with:

<anon>:11:1: 11:27 error: recursive type `M` has infinite size [E0072]
<anon>:11 struct M(PtrBack<Vec<M>>);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:11:1: 11:27 help: see the detailed explanation for E0072
<anon>:11:1: 11:27 help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `M` representable
<anon>:11:1: 11:27 note: type `M` is embedded within `PtrBack<collections::vec::Vec<M>>`...
<anon>:11:1: 11:27 note: ...which in turn is embedded within `PtrBack<collections::vec::Vec<M>>`...
<anon>:11:1: 11:27 note: ...which in turn is embedded within `M`, completing the cycle.

From 8 bytes to infinity would seem to be a serious memory usage regression.

@jorendorff
Copy link
Contributor Author

Same error if I change the pointer to a Box.

@alexcrichton alexcrichton added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jan 30, 2016
@jonas-schievink
Copy link
Contributor

cc @nikomatsakis since I think he touched that code last

@arielb1
Copy link
Contributor

arielb1 commented Jan 31, 2016

Mini (no Vec needed - Vec is quite internally complex for some reason):

trait Front { type Back; }
impl<T> Front for T { type Back = T; }
struct PtrBack<T: Front>(*mut T::Back);
struct M(PtrBack<M>);

fn main() {
    println!("{}", std::mem::size_of::<M>());
}

@arielb1
Copy link
Contributor

arielb1 commented Jan 31, 2016

This is fallout from inductiveness

In the OC, the problem is that:

*) M: Sized if (sized)
*) PtrBack<M>: Sized if (sized)
*) *mut <M as Front>::Back: Sized if (sized)
*) M: Front if (impl)
*) M: Sized 

This could potentially be fixed by lazy normalization.

A harder case is:

trait Front { type Back; }
impl<T> Front for T { type Back = *mut T; /* note *mut is here */ }
struct PtrBack<T: Front>(T::Back);
struct M(PtrBack<M>);

fn main() {
    println!("{}", std::mem::size_of::<M>());
}

The cycle is similar:

*) M: Sized if (sized)
*) PtrBack<M>: Sized if (sized)
*) <M as Front>::Back: Sized if (sized)
*) M: Front if (impl)
*) M: Sized 

However, not even lazy normalization can save us here.

I admit the infinite type detection here is just plain wrong.

@Aatch
Copy link
Contributor

Aatch commented Feb 1, 2016

@arielb1 those sequences are kinda hard to understand. However, I'm not sure why Sized is that relevant here. Unless the "infinite sized type" error is just a red herring and it's mis-reporting the recursive dependency for Sized vs. non-Sized?

@nikomatsakis
Copy link
Contributor

@Aatch the message is basically a special-cased variant targeting the case where computing whether T: Sized is implemented requires that we recursively compute T: Sized. That usually (but not always) means an infinitely sized type, hence the message.

This is pretty related to how we can formalize auto traits. I'd been debating about a formalization that basically inserted one (automatically generated) impl for each auto trait, but those impls worked the same way as all other impls (this is not how it works today, but how it works today is unsound). However, I realized that this could fail, and my example was exactly the one that @jorendorff gives here -- one where the recursion arises through a projection. In the Obligation Forest PR, I considered whether to make Sized co-inductive, but @aturon and I decided we could avoid it. This may not have been correct.

So it may be that the correct fix is to have auto traits have a more complex, co-inductive proving semantics and -- since Sized is entirely builtin -- also give it those same semantics.

@Aatch
Copy link
Contributor

Aatch commented Feb 4, 2016

@nikomatsakis so to be clear, the error message is wrong here, the problem is that we have "T meets Sized if T meets Sized" being reported as "T has infinite size". Makes sense given that in the absence of projection the only way that recursion could happen is if T contained T without indirection. Just want to make sure there's a version of this problem written in language understandable to those not super-familiar with the compiler's type-system internals.

@nikomatsakis
Copy link
Contributor

@Aatch I agree the message is wrong, I didn't consider this case when phrasing it -- but I think ideally we wouldn't be reporting an error here at all. I guess the question is if there is a way to achieve that.

@arielb1
Copy link
Contributor

arielb1 commented Feb 4, 2016

auto trait = OIBIT right?

I'd been debating about a formalization that basically inserted one (automatically generated) impl for each auto trait

What's the difference between that and what we have today? That auto traits are coinductive?

If we forbid coinductive traits from having supertraits and associated types/constants we should be fine, as they can only be inputs of trait selection and not outputs.

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Feb 4, 2016
@nikomatsakis
Copy link
Contributor

It's not clear what the best fix here is. One option is to change how we process the Sized trait to make it coinductive -- this may cause an issue with adding an associated constant Size, but maybe not. At minimum we ought to improve the error message. But we should make some decisions, given that this is a regression, hence the P-high categorization.

@nikomatsakis
Copy link
Contributor

I think that for now the best thing here is to fix the error message, leaving ourselves the freedom to make this legal again in the future if we decide it won't undermine the type system. :)

@jorendorff
Copy link
Contributor Author

@areilb1 Would you mind explaining your *) notation?

I'm trying to understand what's causing the loop. It seemed to me like it might be

  • let's check if struct M(PtrBack<Vec<M>>); is a reasonable thing to say
  • that requires that M is a valid parameter to Vec<_>
  • that requires that M: Sized (implicit bound)
  • that requires that all M's fields have valid types
  • that requires that M is a valid parameter to Vec<_> (closing the loop)

But that can't be right, because struct M(Vec<M>); compiles just fine. The trait must be involved in the loop somehow. And I know you've already posted the answer. I just can't read it. :)

@nikomatsakis While we're here - I have feels about the wording "has infinite size" in error messages. It feels like it's attributing a deeply ridiculous intent to me, the user. Instead of saying "you did a typo" rustc is saying "oh, you want an infinitely large struct, I'm sorry, I can't do that". It must think I'm some kind of genius-level doofus. The message would be a little insulting, if the compiler were constructed by actual people who are actually smarter than me... waaaait a minute...

@Aatch
Copy link
Contributor

Aatch commented Feb 11, 2016

@jorendorff so the message you get about "infinite size" there is actually wrong, it's actually the issue that we have the requirement M is Sized if M is Sized. Though that's not technically true here, either, hence @arielb1 stating that the error goes away if you tweak when normalization (turning T::Foo into an actual type) happens. Though it doesn't solve the issue in all cases.

As for the general problem of "infinite size" in errors, I see your point, but the error has to explain why it's incorrect somehow. I mean, what would an alternative error message look like? "You appear to have made a mistake, trying adding indirection in this type"? What mistake? What's the issue? Why would adding indirection help?

@arielb1
Copy link
Contributor

arielb1 commented Feb 12, 2016

infinite size

The expectation was that you encounter this error when you try to make a recursive type (say a tree or something) without using a Box. In that case, the error message tells you exactly what went wrong, and how you fix it.

I think that's a general problem with error messages. When code with a typo attempts to do something silly, you will get an error message about the silly thing.

There's a bug here that we report this error message with no infinite-sized type in sight. @nikomatsakis is working on fixing this (I think). Its just a recursive trait evaluation error.

@jorendorff
Copy link
Contributor Author

Sorry I mentioned it. Filed #31683 for the pet peeve.

@nikomatsakis nikomatsakis added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Feb 25, 2016
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 8, 2016
This avoids normalizing for structural types, which kind of gives us a
poor man's lazy normalization. It's enough to fix issue rust-lang#31299, anyway,
though you can still get false failures if you try hard enough.

Fixes rust-lang#31299.
@nikomatsakis nikomatsakis assigned arielb1 and unassigned nikomatsakis May 6, 2016
@nikomatsakis
Copy link
Contributor

I'm assigning this to @arielb1 since I believe that his pending PR (#33138) will fix it.

@nikomatsakis
Copy link
Contributor

This is now fixed (just verified) by #33138. Closing.

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

Successfully merging a pull request may close this issue.

7 participants