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

Add maxInstantiationDepth to compiler optionDeclarations #29602

Conversation

AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Jan 26, 2019

  • There is an associated issue that is labeled
    'Bug' or 'help wanted' or is in the Community milestone
  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally (Had to run jake baseline-accept)
  • You've signed the CLA
  • There are new or updated unit tests validating the change

Fixes #29511

image

The first two make sense to me, since I added a new field to the CompilerOptions interface in types.ts

The last one isn't so obvious to me and I'm not sure how I'd go about correcting this.

[EDIT]
Oh, I think it's just that I have to accept the new baseline file under baselines/local/showConfig/Shows tsconfig for single option/maxInstantiationDepth/tsconfig.json

{
    "compilerOptions": {
        "maxInstantiationDepth": 0
    }
}

I looked at the baseline changes and they seem okay to me. Nothing wildly different. So, I went and accepted them.

I know it's just a test with a dummy value but setting maxInstantiationDepth to 0 is a bad idea =P
[/EDIT]


The issue is only marked as Needs Investigation but I figured I'd take a stab at adding this compiler option. Being able to increase the maximum instantiation depth from 50 to an arbitrary value is useful for me because I seem to work with deeply nested, non-recursive, types a lot.

I'm not sure how to write unit tests to test this compiler option yet but I'll try and figure it out. I figured I'd just make the PR first.

I'm not sure what description to give this compiler option, and what code it would use.

Do I need to add maxInstantiationDepth to protocol.ts > CompilerOptions, too?

I'm also unsure what else I might be missing.


While jake runtests fails and I haven't written a unit test for this compiler option, I've tested it against a project I'm working on and it seems to work.

Without setting maxInstantiationDepth, compiling my project fails, setting maxInstantiationDepth to 74 lets it compile successfully.


I realize this PR has a bunch of problems but I'm out of my depth (No pun intended).

@sandersn
Copy link
Member

sandersn commented Feb 3, 2020

maxInstantiationDepth is an intentional limitation of the compiler to prevent slow compiles, so we're not going to remove it. Even if #32611 allows much more nested types to compile, it's likely that we'll have a different kind of limiter in place. Because that future limiter will be named something different, I don't think PR will apply to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experiment A fork with an experimental idea which might not make it into master
Projects
None yet
3 participants