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

Reduce effective maximum constraint depth for instances of the same conditional #31880

Closed
wants to merge 1 commit into from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jun 12, 2019

Fixes #31823

We track how many instances of the same conditional root we've seen as we're generating constraints and now bail when we've seen the same root 10 (effectively 5) times, similar to what we do with symbols during assignability checking. This will prevent us from generating an absurdly large conditional with too many branches to reasonably check when a conditional infinitely expands while generating constraint types.

@weswigham
Copy link
Member Author

@typescript-bot pack this

@falsandtru when the bot gets back with a URL to the build, do you want to validate that this prevents your (much larger, I'm sure) type structure from hanging?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 12, 2019

Heya @weswigham, I've started to run the tarball bundle task on this PR at a0e77ae. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/33137/artifacts?artifactName=tgz&fileId=F5A0D46456AF503953598D39BFA041FFACFAE25B5246E5787A9D73F589BEAB6202&fileName=/typescript-3.6.0-insiders.20190612.tgz"
    }
}

and then running npm install.

@falsandtru
Copy link
Contributor

do you want to validate that this prevents your (much larger, I'm sure) type structure from hanging?

I'm not sure but looks like that package works well with my code (error is made correctly) and improvements are probably good if they make no breaking changes.

@shaunluttin
Copy link

shaunluttin commented Jun 14, 2019

We tried running the installable tgz against a project that was hanging with 3.5.2, and we now receive a new error: Type instantiation is excessively deep and possibly infinite. A quick look at the location of the error didn't show any evidence on the surface of a really deep type. There is no such error when running 3.4.5.

@ahejlsberg
Copy link
Member

I have two concerns with the fix implemented here. First, I think it will break real world code (as seems to be the case in the comment above). Second, per-type instantiation depth isn't really the best metric because some types are deep but narrow where others are shallow but wide. I'd love to find a better measure of the "complexity cost" of a type so we can be a bit more selective.

if (t.flags & TypeFlags.Conditional) {
const base = getNodeLinks((t as ConditionalType).root.node).resolvedType!;
const id = "" + getTypeId(base);
perInstanceDepth.set(id, (perInstanceDepth.get(id) || 0) + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all the extra indirections here? Couldn't you just store the counter as a property in the resolved type?

@ahejlsberg
Copy link
Member

I'm experimenting with a type instantiation count limiter in #32079. This could potentially be a way of catching compiler hangs caused by "shallow but wide" types.

@sandersn sandersn added this to Gallery in Pall Mall Jan 28, 2020
@sandersn sandersn moved this from Gallery to Limbo in Pall Mall Jan 28, 2020
@sandersn
Copy link
Member

sandersn commented Feb 3, 2020

I'm cleaning up our PR backlog, so I'm going to close this PR because it looks like #32079 fixes the bug also.

@sandersn sandersn closed this Feb 3, 2020
@sandersn sandersn removed this from Limbo in Pall Mall Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler hangs
6 participants