Skip to content
This repository has been archived by the owner on Jun 12, 2022. It is now read-only.

Add options for internal limit flags #12

Merged
merged 6 commits into from
Apr 17, 2022

Conversation

Corecii
Copy link
Contributor

@Corecii Corecii commented Apr 14, 2022

This PR adds options for internal limits. Raising these limits may allow the typechecker to work on code that usually errors with Internal error: Code is too complex to typecheck!.

This PR introduces a new format for flags so that we can potentially be forward-compatible with general flag commands that allow setting any flag to any value. This new format contains the internal flag name in the commandline option so that we won't be stuck eternally hard-coding kebab-case options to PascalCase flags.

This new format looks like: --flag:LuauFlagName=value

I don't have good documentation on what these flags do. I just know that they're typically all listed together.

Closes #11

README.md Outdated Show resolved Hide resolved
@Corecii
Copy link
Contributor Author

Corecii commented Apr 14, 2022

The output of the new help options with defaults looks like the following right now:

Available limit flags:
  These options affect internal limits of the Luau typechecker.
  Changing these may permit the typechecker to work on complex code at the cost of performance.
  --flag:LuauTypeInferRecursionLimit=INT:            default 500
  --flag:LuauTypeInferIterationLimit=INT:            default 2000
  --flag:LuauTypeInferTypePackLoopLimit=INT:         default 5000
  --flag:LuauCheckRecursionLimit=INT:                default 500
  --flag:LuauTarjanChildLimit=INT:                   default 1000
  --flag:LuauTableTypeMaximumStringifierLength=INT:  default 0

I chose not to add these defaults to the readme as they may change over time.

@Corecii
Copy link
Contributor Author

Corecii commented Apr 14, 2022

^ I missed a few issues which I've since fixed. I've been using this for a few hours now without any issues, so the PR is ready when you are! 👍

@JohnnyMorganz
Copy link
Owner

Thanks! One question, whats the impact of this is one of the FFlags is later removed? Do we need to continuously keep up to date with which fflags are available and modifiable?

@JohnnyMorganz
Copy link
Owner

Theres this code (from the original luau-analyze CLI tool) which finds all the FFlag bool values and flips them on at the beginning: https://github.com/JohnnyMorganz/luau-analyze-rojo/blob/master/main.cpp#L376-L378

Could we potentially make the --flag:<FLAG_NAME>=<FLAG_VALUE> code dynamic here? Instead of hardcoding which flags we use, a user can just pass a --flag:NAME=VALUE argument and we lookup the flag with NAME and set it to VALUE?

Note: I have no idea how the FFlag system actually works :P, I don't know if this is actually feasible or not

@Corecii
Copy link
Contributor Author

Corecii commented Apr 17, 2022

That looks possible, and something along those lines was my idea of what the "next step" would be and that's why I formatted the flag options in the way that I did.

I'll have a go at it this coming week. If all works out then it'll be possible to assign flags dynamically and print out all flags with their default values.

@JohnnyMorganz
Copy link
Owner

That looks possible, and something along those lines was my idea of what the "next step" would be and that's why I formatted the flag options in the way that I did.

I'll have a go at it this coming week. If all works out then it'll be possible to assign flags dynamically and print out all flags with their default values.

Whoops, I completely missed you mentioning this in the OP, sorry!

I'm happy to merge this PR in as it is right now though, I have no particular preference

@Corecii
Copy link
Contributor Author

Corecii commented Apr 17, 2022

I wasn't originally planning to figure out generalized flags right away because I didn't know how! I'm happy you've given me direction here since I think I can figure it out now. If I'd known about this before I'd likely have put it in the original PR!

I'm not particularly picky about if we merge this PR as it is or if we wait for generalized flag options either. Merging this PR as is is probably a good idea though. No reason to wait when this feature is ready now, and forward compatible with future plans, right?

@JohnnyMorganz
Copy link
Owner

Cool, I'll merge this in now, thanks!

No rush on the more general version btw, I'm also happy to look into it at a later date if you're busy

@JohnnyMorganz JohnnyMorganz merged commit ebcd844 into JohnnyMorganz:master Apr 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to raise internal limits
2 participants