limit the length of types in monomorphization #37789

arielb1 · 2016-11-15T21:28:15Z

This adds the new insta-stable #![type_size_limit] crate attribute to control
the limit, and is obviously a [breaking-change] fixable by that.

Fixes #37311.

r? @nikomatsakis

durka · 2016-11-15T22:18:52Z

Why not #![recursion_limit]?

arielb1 · 2016-11-15T23:15:34Z

@durka

Because recursion_limit is way too small.

durka · 2016-11-15T23:18:57Z

I agree, it is :)

durka · 2016-11-15T23:19:21Z

The type size limit could be a multiplier of the recursion limit, or something.

Mark-Simulacrum · 2016-11-16T00:20:23Z

Is there a reason we can't increase the recursion limit itself? @jseyfried mentioned here that this is probably something that would be reasonable to do: #36214 (comment).

brson · 2016-11-16T01:28:32Z

Please explain why this is insta-stable.

arielb1 · 2016-11-16T10:31:26Z

@brson

type_length_limit is insta-stable because I want to provide people that exceed the maximum type length limit some workaround.

Maybe it would be better to just make this a rate-limited warning instead of an error to avoid the worries, but then testing it would be difficult and the compiler would still hang.

nikomatsakis · 2016-11-16T23:25:55Z

@arielb1 the workaround could certainly be to use nightly for some time, if they need to lift the limit?

nikomatsakis · 2016-11-16T23:26:01Z

We should run crater here, though

nikomatsakis · 2016-11-16T23:28:30Z

src/librustc_trans/collector.rs

@@ -433,6 +434,40 @@ fn check_recursion_limit<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
    (instance.def, recursion_depth)
 }

+fn check_type_length_limit<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,


This seems good; I'm wondering if this is the only place we have to check it. I was originally considering checking this directly in the type interner (i.e., we just couldn't create a type that is too big). What do you think about that?

Checking it in other places would basically require us to ~~intern~~cache "type sizes" in types, which would bump our memory usage for no very good reason.

I actually expected this value to be stored inside each TyS - seems cheaper that way?

@eddyb

That's what I meant. Looks like a bump in memory use, and there's no killer case for that.

Sorry, I wrote my message before seeing yours. Any chance this can be done with recursion, to avoid the memory allocations of the walker? Or is it amortized and otherwise not a problem?
Could we reuse the walker?

bors · 2016-11-21T17:30:24Z

☔ The latest upstream changes (presumably #37824) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-11-22T01:44:42Z

Has anyone run crater on this?

nikomatsakis · 2016-11-22T01:45:01Z

I'm satisfied with @arielb1's reason to limit it to monomorphization. We can always expand to more things later if needed.

nikomatsakis · 2016-11-22T01:46:07Z

Started a crater run.

nikomatsakis · 2016-11-22T14:55:10Z

Crater results: https://gist.github.com/nikomatsakis/731f7057c3bb9443a0744449d2a328a3

6626 crates tested: 4652 working / 1616 broken / 1 regressed / 3 fixed / 354 unknown.

Things to investigate:

rangetree-0.1.1 (before) (after) -- false positive

nikomatsakis · 2016-11-22T14:59:10Z

Looks good. The only question then is perma-stability. But this doesn't bother me. I think there is an inherent need for this sort of threshold, given that we have adopted a monomorphization strategy without any fixed limits to prevent infinite recursion. And of course if this threshold became unnecessary in the future, we could always make it a no-op, or use it to inform the value for a newer threshold.

cc @rust-lang/compiler @rust-lang/lang -- I plan to r+ this commit, which adds an attribute that limits the maximum size (not just depth) of types we construct during monomorphization. This is to give nice errors for programs that construct types whose sizes grows exponentially with depth. It augments the existing recursion counter. People ok with this?

Another option would be to somehow try to drive this limit as some function of the recursion depth, or to make a meta-attribute like #[compilation_limits(recursion_depth = "2", type_size = "4")].

Yet another option is to make this option unstable. Given the zero impact on crater, it seems like nobody needs to use this option to compile today, so I guess there isn't much reason to stabilize it just yet. But I'd want to act quickly, so that when we do find people who need it, they have a stable option available to them. Thoughts, @arielb1 ?

aturon · 2016-11-22T16:40:37Z

@nikomatsakis Sounds like a nice improvement. I'm personally ambivalent about how precisely we express the limit -- it should be rare enough that it doesn't matter that much.

arielb1 · 2016-11-22T18:26:38Z

Won't a compilation limit break the recursion depth code? Or should we have it over the normal recursion depth?

I'm not sure whether to feature-gate the type size limit. I don't expect anyone to encounter it, but this could really be a problem to someone. What do you prefer?

aturon · 2016-11-22T18:33:17Z

See @nikomatsakis's comment for the summary. The biggest question right now is whether we're OK making the crate attribute insta-stable.

@rfcbot fcp merge

rfcbot · 2016-11-22T18:34:35Z

nikomatsakis · 2016-11-22T20:34:59Z

@arielb1

Won't a compilation limit break the recursion depth code?

I don't follow. I just meant that we would deprecate the existing attribute and prefer a new, unified syntax. I don't care enough to do that deprecation though. Can always do it later (and deprecate the two attributes).

Or did you mean, would it break code if we based this limit on the recursion depth? Potentially, I think is the answer.

withoutboats · 2016-11-22T21:39:02Z

@rfcbot reviewed

bluss · 2016-11-23T23:34:49Z

As a sidenote (I saw it from this crater report), crater doesn't build crates that use workspaces correctly. For example libc, macro-attr, and others are listed as broken for this reason.

nikomatsakis · 2016-11-29T16:31:18Z

@bluss good catch. @brson, how's that crater replacement coming along?

nikomatsakis · 2016-11-29T16:31:25Z

Nonetheless, I wouldn't expect a lot of regressions here.

nikomatsakis · 2016-11-29T16:31:45Z

@bors r+

bors · 2016-11-29T16:31:50Z

📌 Commit 050b9b5 has been approved by nikomatsakis

bors · 2016-11-29T22:03:09Z

⌛ Testing commit 050b9b5 with merge 0274e60...

bors · 2016-11-29T23:26:59Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

nikomatsakis · 2016-11-30T19:36:04Z

@arielb1

Looks like you have to run the script to update the test output:

error: reached the type-length limit while instantiating `<T as Foo><(&(&(&(&(&(&(&(&(&(&(&(&(&(&(&(&(&(&(&(), &()), &(&()...`
  --> C:\bot\slave\auto-win-msvc-64-opt-rustbuild\build\src/test\ui\issue-37311-type-length-limit\issue-37311.rs:23:5
   |
23 |       fn recurse(&self) {
   |  _____^ starting here...
24 | |         (self, self).recurse();
25 | |     }
   | |_____^ ...ending here
   |
   = note: consider adding a `#![type_length_limit="2097152"]` attribute to your crate

error: aborting due to previous error

This adds the new insta-stable `#![type_size_limit]` crate attribute to control the limit, and is obviously a [breaking-change] fixable by that.

arielb1 · 2016-12-01T22:54:49Z

@bors r=nikomatsakis

bors · 2016-12-01T22:54:51Z

📌 Commit 242cd7e has been approved by nikomatsakis

bors · 2016-12-02T00:15:18Z

⌛ Testing commit 242cd7e with merge 1077149...

@nikomatsakis

limit the length of types in monomorphization This adds the new insta-stable `#![type_size_limit]` crate attribute to control the limit, and is obviously a [breaking-change] fixable by that. Fixes #37311. r? @nikomatsakis

bors · 2016-12-02T04:39:24Z

…=lcnr Re-implement a type-size based limit r? lcnr This PR reintroduces the type length limit added in rust-lang#37789, which was accidentally made practically useless by the caching changes to `Ty::walk` in rust-lang#72412, which caused the `walk` function to no longer walk over identical elements. Hitting this length limit is not fatal unless we are in codegen -- so it shouldn't affect passes like the mir inliner which creates potentially very large types (which we observed, for example, when the new trait solver compiles `itertools` in `--release` mode). This also increases the type length limit from `1048576 == 2 ** 20` to `2 ** 24`, which covers all of the code that can be reached with craterbot-check. Individual crates can increase the length limit further if desired. Perf regression is mild and I think we should accept it -- reinstating this limit is important for the new trait solver and to make sure we don't accidentally hit more type-size related regressions in the future. Fixes rust-lang#125460

rust-highfive assigned nikomatsakis Nov 15, 2016

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 16, 2016

nikomatsakis added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 16, 2016

nikomatsakis reviewed Nov 16, 2016

View reviewed changes

arielb1 force-pushed the length-limit branch from 2b923f5 to 050b9b5 Compare November 21, 2016 20:05

aturon added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2016

brson mentioned this pull request Dec 1, 2016

Compiler hangs after 'write metadata stage' #37311

Closed

limit the length of types in monomorphization

242cd7e

This adds the new insta-stable `#![type_size_limit]` crate attribute to control the limit, and is obviously a [breaking-change] fixable by that.

arielb1 force-pushed the length-limit branch from 050b9b5 to 242cd7e Compare December 1, 2016 22:54

bors merged commit 242cd7e into rust-lang:master Dec 2, 2016

bluss removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Dec 6, 2016

ehuss mentioned this pull request Mar 8, 2019

Undocumented Attributes rust-lang/reference#532

Closed

6 tasks

eddyb mentioned this pull request Sep 15, 2020

[experiment] rustc_mir: remove #![type_length_limit] checks. #76772

Closed

compiler-errors mentioned this pull request Jun 12, 2024

Re-implement a type-size based limit #125507

Merged

limit the length of types in monomorphization #37789

limit the length of types in monomorphization #37789

Conversation

arielb1 commented Nov 15, 2016

durka commented Nov 15, 2016

arielb1 commented Nov 15, 2016

durka commented Nov 15, 2016

durka commented Nov 15, 2016

Mark-Simulacrum commented Nov 16, 2016

brson commented Nov 16, 2016

arielb1 commented Nov 16, 2016 • edited Loading

nikomatsakis commented Nov 16, 2016

nikomatsakis commented Nov 16, 2016

nikomatsakis Nov 16, 2016

Choose a reason for hiding this comment

arielb1 Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

eddyb Nov 17, 2016

Choose a reason for hiding this comment

arielb1 Nov 17, 2016

Choose a reason for hiding this comment

eddyb Nov 17, 2016

Choose a reason for hiding this comment

bors commented Nov 21, 2016

nikomatsakis commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016 • edited Loading

nikomatsakis commented Nov 22, 2016

aturon commented Nov 22, 2016

arielb1 commented Nov 22, 2016 • edited Loading

aturon commented Nov 22, 2016

rfcbot commented Nov 22, 2016 • edited by michaelwoerister Loading

nikomatsakis commented Nov 22, 2016

withoutboats commented Nov 22, 2016

bluss commented Nov 23, 2016

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

bors commented Nov 29, 2016

bors commented Nov 29, 2016

bors commented Nov 29, 2016

nikomatsakis commented Nov 30, 2016

arielb1 commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 2, 2016

bors commented Dec 2, 2016

arielb1 commented Nov 16, 2016 •

edited

Loading

arielb1 Nov 17, 2016 •

edited

Loading

nikomatsakis commented Nov 22, 2016 •

edited

Loading

arielb1 commented Nov 22, 2016 •

edited

Loading

rfcbot commented Nov 22, 2016 •

edited by michaelwoerister

Loading