Update cyclic requires RFC - #237
Conversation
| A problem that a developer might run into is that, if their application consists of a very large SCC (their whole application, perhaps\!), their incremental typechecking performance will be very bad: Luau will have to recheck all files whenever any file in the SCC has changed. | ||
|
|
||
| To mitigate this and put some soft pressure on the developer, we'll report a warning when we encounter an SCC that consists of too many modules. This warning will explain that large clusters of cyclic modules can cause typechecking performance to degrade badly. We'll allow this limit to be configured via `FrontendOptions`. | ||
| To mitigate this and put some soft pressure on the developer, we'll report a warning when we encounter an SCC that consists of too many modules. This warning will explain that large clusters of cyclic modules can cause typechecking performance to degrade badly. We'll allow this limit to be configured via `FrontendOptions`, defaulting to 4 modules. The warning will list the names of the modules in the SCC, up to a configurable limit (default 10). |
There was a problem hiding this comment.
what is the approach users should take to mitigating this warning? to me it seems non-obvious where such a warning would span and how people would go about fixing it if they've deliberately ignored it or cranked the limits in the past.
There was a problem hiding this comment.
The mitigation would be to break the cycle via an interface module or restructuring the dependency graph, something users today would be doing anyways. For ex, if you have A->B->C->D->A, you could extract the shared types/interfaces into a separate non-cyclic modules that the others all require, which can reduce the SCC or even remove a cycle entirely.
Regular type errors are still reported in each module, but SCC size warning currently attaches to the first module in the cycle, though we're still working on improving the error/warning process in SCCs
There was a problem hiding this comment.
The mitigation would be to break the cycle via an interface module or restructuring the dependency graph, something users today would be doing anyways. For ex, if you have
A->B->C->D->A, you could extract the shared types/interfaces into a separate non-cyclic modules that the others all require, which can reduce the SCC or even remove a cycle entirely.
this doesn't really address my question. not every cycle is a sparse graph, it might be
A->B
A->C
C->A
C->B
B->A
B->C
A->D
D->A
D->B
...etc. My question was mostly about what we're actually telling users to do if their graphs start looking like that and they want their graphs to stop looking like that. Four also seems a tad low to me
Regular type errors are still reported in each module, but SCC size warning currently attaches to the first module in the cycle, though we're still working on improving the error/warning process in SCCs
right, but i think attaching a warning to potentially dozens of files with no clear starting point (literally any one of them might be removable entirely and you could still have the warning in 11 other files) will just lead users to crank the limits with no clear path to refactor it without starting from scratch. the solver isn't aware of how your code evolved, it can only see that there's a problem. and there might be a problem for... hundreds of files, which goes unnoticed or ignored, and now it's impossible to really find the source of it
| 3. Pre-allocate a `BlockedType` placeholder for each module in the SCC. This allows the typechecker to resolve references to types from other modules in the SCC, even if they haven't been fully defined yet. | ||
| 4. Run constraint generation for each module in the SCC. Each module in the SCC gets its own scope and data flow graph, but all constraints flow into a single shared `ConstraintGraph`. After constraint generation for each module, the `BlockedType` placeholder is bound to the actual inferred return type. | ||
| 5. Run constraint solver once over the combined constraint graph for the entire SCC. This enables cross-cycle type inference, so constraints from `foo` that depend on types from `bar`, for example, are solved together. |
There was a problem hiding this comment.
Pre-allocate a
BlockedTypeplaceholder for each module in the SCC.
After constraint generation for each module, the
BlockedTypeplaceholder is bound to the actual inferred return type.
This enables cross-cycle type inference, so constraints from
foothat depend on types frombar, for example, are solved together.
would this have a lot of side effects? it seems like changing the entire module to be populated with a blocked type would have untold implications on constraint generation
There was a problem hiding this comment.
The BlockedType isn't replacing the module's internal types, it's just the return type seen by other modules in the SCC cycle during just constraint generation. Each module's local scope is still generated normally with concrete types. The BlockedType is just the placeholder to require(otherModuleInSCC) before we've solved it. So once constraint generation finishes for a module, we bind the BlockedType to the actual inferred return type, and then the solver resolves everything together.
There was a problem hiding this comment.
i'm asking if things will behave in visibly different ways to users - it seems necessary to me that they would, but i can't picture how that would be side effect free. If I took, say, a 500,000 line luau codebase, got my depgraph to form the entire thing into an scc, and we ignore all of the solver's time limits and whatnot - would typechecking behave the same as before? on a smaller scale, can modules influence the types of modules they require if they're in the same scc? i don't totally see why not (i mean, wouldn't you want that for some things--like two datastructures or functions depending on eachother?) or, what about cases like this:
would the type solver still be able to infer number here if meow's value were a blocked type? my hope would be yes because meow.mrrp would infer to index<meow, "mrrp">, but i don't really know that (and currently if i reproduce the same type function with a generic, i get an error about Cannot call a value of type index<value, "mrrp">):
to say i have limited experience would be an overstatement of my skill-set, so apologies if i have the wrong idea entirely or am missing something important. "each module will get a blocked type placeholder" just sounds scary to me with only a vague idea of what a blocked type actually is beyond "something we can't know yet"
| 1. `main.luau` requires `folder.luau`. A placeholder is created for `folder` and it begins executing. | ||
| 2. `folder.luau` requires `file.luau`. A placeholder is created for `file` and it begins executing. | ||
| 3. `file.luau` requires `folder.luau`. Cycle detected — `folder`'s locked placeholder is returned immediately. | ||
| 4. `file.luau` attempts to access `folder.Folder`. The placeholder is still incomplete and therefore has the `CyclicDependencyError` metatable attached to it. We tell the developer that they `"Cannot access the exported field 'Folder' because it has a cyclic dependency on its requiring module"` and raise an exception. The developer can use the stack trace to understand the cycle. |
There was a problem hiding this comment.
The developer can use the stack trace to understand the cycle.
can they? a really long stack (or several of them!) could be really annoying to sift through.
it's not as simple as ModuleA -> ModuleB -> ModuleC -> ModuleD -> ModuleA. A stack might contain several traces from the same module, might be very long, might contain leading traces unrelated to the main cycle, it might even contain mutually recursive traces, etc...
There was a problem hiding this comment.
Ah that's a fair point, it could get very noisy, and we can note this as a future improvement needed, like somehow including the module cycle path in error messages...
There was a problem hiding this comment.
that seems okay, but i think it should be addressed in the drawbacks section
3 participants
Rendered
Updating after some implementation iterations and discussions with the team! We found this approach to work best, and added some more technical details