RFC: Support cyclic requires.

[andyfriesen]

Rendered

We augment the require() function, the module interface, and the type checker to allow modules to cyclically require one another in many cases.

[hgoldstein]

Are there any backwards compatibility issues here?

[karl-police]

how would hot-reloading a module affect everything, since it talks about cache

[hgoldstein]

Food for thought, I think this would get tricker with a (sketched out) import syntax:

--- A.luau

import ClassBOne from B

export class ClassAOne extends ClassBOne ... end
export class ClassATwo ... end

--- B.luau

import ClassATwo from A

class ClassBOne ... end
class ClassBTwo extends ClassATwo ... end

... if we're considering this a serious drawback, this seems like the real danger.

[andyfriesen]

Does this materially change the problem? Such a syntax would probably desugar like so:

local _unnamed = require("./B')
local ClassBOne = _unnamed.ClassBOne
type ClassBOne = _unnamed.ClassBOne

It certainly fails in exactly the same way as the example in the RFC, but I think that's acceptable. You'd get a readable error message, after all.

[alexmccord]

You could also keep them separate, and instead dispatch one constraint solver in the same component, and dispatch the next one if that one is stuck, and so on, until either all of them completes or all of them are stuck.

This would avoid a constraint from module A making its way into some random metavariable's bounds in module B.

[MagmaBurnsV]

The ceremony required to make modules that don't use static exports participate in cycles seems silly to me. As the RFC mentions, cycles are already rare for projects prior to classes, and since classes go hand-in-hand with static exports, I see no sense in adding an escape hatch for this edge case.

Just support cycles only for modules that use static exports. That way you don't even have to worry about if a module returns a different value since it's impossible.

[karl-police]

So, this is GETVARARGS. Now, the thing I know about it, is that it's often used within function bodies.

Doing local var = ... would mean that it's taking 1 element from the var arg alone.

If I do local a,b,c = ... it would take 3 and not more.

What would the design have to change here? Or what would this syntax mean if it is within function bodies?
Or does nothing change about GETVARARGS and it will stay 1 element, it's just to accept the export table

[karl-police]

Will there be a new kind of type?

I know there's modules that do the following

-- now we could require("Main.luau") for the varargs?
--- A.luau
return function (varargs)
   print(varargs.OurEnvironment) -- inheritance from upper module, I guess.
end

but I guess cyclic requires would solve this too, e.g. for Require Tracer?

-- Main.luau
local PortionA = require("A.luau")

local Main = {
  OurEnvironment = {}
}

PortionA(Main)

[VegetationBush]

Wouldn't it be better if a separate keyword was used instead (i.e., requireDeferred, lazyRequire)? This allows require to stay pure instead of throwing new kinds of errors, or worrying about a workaround to capture tuples after returning the cyclic metatable.

A second benefit to this is that it handles the case where A immediately requires B but B does not immediately require A (which I believe is an edge case that wasn't considered in this RFC).

[dcope-rbx]

I may not be following all the details here correctly, but would it be problematic if the ModuleScript tries to set a metatable on the export table? i.e. would it clobber the metatable that was already set by the system?

[andyfriesen]

Yes! This is a pretty bad situation to be in, however: In order to trigger this, you would have to set the metatable on some other module different from the current one. eg

local A = require("./A")
setmetatable(A, ...) -- !!!

This sounds like extremely bad practice to me!

I can look into what it would take to disallow this.

[dcope-rbx]

I was actually thinking of the case where a ModuleScript sets its own export table's metatable:

local exports = ...
setmetatable(exports, {})

My understanding is that this would then replace any metatable that the system had already set on the table passed to the ModuleScript.

[MagmaBurnsV]

This is why I think cycles should only be supported between modules that use the static export system. This issue isn't possible with static exports since the export table isn't directly accessible until after the module has returned.

[andyfriesen]

Got it.

This isn't good programming style at all, but the adaptation is simple enough: Instead of setting and unsetting the metatable, we'll just save and restore whatever metatable it had.

[WheretIB]

This is why I think cycles should only be supported between modules that use the static export system. This issue isn't possible with static exports since the export table isn't directly accessible until after the module has returned.

I don't think the export table should be any kind of secret.
You can ask module B to require you and pass your export table back to you.

My understanding is that this would then replace any metatable that the system had already set on the table passed to the ModuleScript.

You can lock the metatable so that attempt will fail.

[MagmaBurnsV]

I don't think the export table should be any kind of secret.
You can ask module B to require you and pass your export table back to you.

Since that's only possible after module A has frozen and returned the table, that's fine. It's only a problem if the module can access the export table while it's being populated, since that allows user code to clobber it like showcased.

My thought was that the metatable attached to the pre-populated table would have a __metatable that locks the requiring module from setting a new metatable, which is then stripped by the runtime when that table is passed to the required module to populate. Would this not work?

[WheretIB]

It will not be frozen, in A -> B -> A chain, B receives incomplete table of A that it can return to A for the same observation and modification.

[andyfriesen]

The ceremony required to make modules that don't use static exports participate in cycles seems silly to me. As the RFC mentions, cycles are already rare for projects prior to classes, and since classes go hand-in-hand with static exports, I see no sense in adding an escape hatch for this edge case.

Just support cycles only for modules that use static exports. That way you don't even have to worry about if a module returns a different value since it's impossible.

This is another possibility that we talked about, but the implementation of this design is very simple and doesn't really compromise the export experience in any way.

I'll add it to the RFC as an alternative.

[andyfriesen]

Wouldn't it be better if a separate keyword was used instead (i.e., requireDeferred, lazyRequire)? This allows require to stay pure instead of throwing new kinds of errors, or worrying about a workaround to capture tuples after returning the cyclic metatable.

This proposal doesn't propose any errors that would not be raised today. It simply proposes a different mechanism for deciding when to raise that error.

A second benefit to this is that it handles the case where A immediately requires B but B does not immediately require A (which I believe is an edge case that wasn't considered in this RFC).

I don't see how this would be a problem under this proposal. If B requires A later, after it has finished evaluating, everything should work just fine.

[MagmaBurnsV]

@andyfriesen My concern with supporting non-static-export scenarios is that it means the system has to assume the worst case and do things like save metatables, double check returned values, etc. With the limitations of static exports, these cases aren't possible and the resolution steps can be simplified.

Also, I just think onboarding for learners is going to suck with these mechanics. Nobody is going to understand "you have to grab a proxy table from ... within both modules, unless this is done internally with export" but it will be easier to explain that "only modules that use static exports can be cyclic."

[WheretIB]

I don't see a point of bifurcating how modules can perform import and export and adding extra checks on what kind of module is being required. Feels largely unnecessary.
By treating evaluation semantics the same, it simplifies implementation.