Skip to content

Commit f1d4621

Browse files
Pre-populate/duplicate check class definitions (new solver) (#1493)
Closes #1492 Tested and working with the test case in the aforementioned issue, along with the full defs of luau-lsp with no issues or type errors In normal Luau files, you can use type aliases and type functions before they are declared. The same extends to declaration files, **except** in the new solver. The old solver perfectly allows this, and in fact intentionally adds it: https://github.com/luau-lang/luau/blob/db809395bf5739c895a24dc73960b9e9ab6468c5/Analysis/src/TypeInfer.cpp#L1711-L1717 This causes *much* headache and pain for external projects that make use of declaration files; namely, luau-lsp generates them from MaximumADHD's API dump, which is not ordered by dependency. This means silent error-types popping up everywhere because types are used before they are declared. The workaround would be to make code to manually reorder class definitions based on their dependencies with a bunch of code, but this is clearly not ideal, and won't work for classes dependent on each other/recursive. The solution used here is the same as is used for type aliases - the name binding for the class is given a blocked type before running the rest of constraint generation on the block. Questions remain: - Should the logic be split off of `checkAliases`? - Should a bound type be used, or should the (blocked) binding type be directly emplaced with the class type? What are the ramifications of emplacing with the bound versus the raw type? One ramification was initially ran into through an assertion because the class `superTy`/`parent` was bound, and several pieces of code assume it is not, so it had to be made followed. - Is folllowing `superTy` to set `parent` the correct workaround for the assertions thrown, or should the code expecting `parent` to be a ClassType without following it be modified instead to follow `parent`? - Should `scope->privateTypeBindings` also be checked for the duplicate error? I would presume so, since having a class with the same name as a private alias or type function should error as well? The extraneous whitespace changes are clang-format ones done automatically that should've been done in the last release - I can remove them if necessary and let another sync or OSS cleanup commit fix it.
1 parent 9a4bc6a commit f1d4621

File tree

2 files changed

+61
-13
lines changed

2 files changed

+61
-13
lines changed

Analysis/src/ConstraintGenerator.cpp

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ LUAU_DYNAMIC_FASTINT(LuauTypeSolverRelease)
3434
LUAU_FASTFLAG(LuauTypestateBuiltins2)
3535

3636
LUAU_FASTFLAGVARIABLE(LuauNewSolverVisitErrorExprLvalues)
37+
LUAU_FASTFLAGVARIABLE(LuauNewSolverPrePopulateClasses)
3738
LUAU_FASTFLAGVARIABLE(LuauNewSolverPopulateTableLocations)
3839

3940
namespace Luau
@@ -654,6 +655,7 @@ void ConstraintGenerator::applyRefinements(const ScopePtr& scope, Location locat
654655
void ConstraintGenerator::checkAliases(const ScopePtr& scope, AstStatBlock* block)
655656
{
656657
std::unordered_map<Name, Location> aliasDefinitionLocations;
658+
std::unordered_map<Name, Location> classDefinitionLocations;
657659

658660
// In order to enable mutually-recursive type aliases, we need to
659661
// populate the type bindings before we actually check any of the
@@ -751,6 +753,32 @@ void ConstraintGenerator::checkAliases(const ScopePtr& scope, AstStatBlock* bloc
751753
scope->privateTypeBindings[function->name.value] = std::move(typeFunction);
752754
aliasDefinitionLocations[function->name.value] = function->location;
753755
}
756+
else if (auto classDeclaration = stat->as<AstStatDeclareClass>())
757+
{
758+
if (!FFlag::LuauNewSolverPrePopulateClasses)
759+
continue;
760+
761+
if (scope->exportedTypeBindings.count(classDeclaration->name.value))
762+
{
763+
auto it = classDefinitionLocations.find(classDeclaration->name.value);
764+
LUAU_ASSERT(it != classDefinitionLocations.end());
765+
reportError(classDeclaration->location, DuplicateTypeDefinition{classDeclaration->name.value, it->second});
766+
continue;
767+
}
768+
769+
// A class might have no name if the code is syntactically
770+
// illegal. We mustn't prepopulate anything in this case.
771+
if (classDeclaration->name == kParseNameError)
772+
continue;
773+
774+
ScopePtr defnScope = childScope(classDeclaration, scope);
775+
776+
TypeId initialType = arena->addType(BlockedType{});
777+
TypeFun initialFun{initialType};
778+
scope->exportedTypeBindings[classDeclaration->name.value] = std::move(initialFun);
779+
780+
classDefinitionLocations[classDeclaration->name.value] = classDeclaration->location;
781+
}
754782
}
755783
}
756784

@@ -1646,6 +1674,11 @@ static bool isMetamethod(const Name& name)
16461674

16471675
ControlFlow ConstraintGenerator::visit(const ScopePtr& scope, AstStatDeclareClass* declaredClass)
16481676
{
1677+
// If a class with the same name was already defined, we skip over
1678+
auto bindingIt = scope->exportedTypeBindings.find(declaredClass->name.value);
1679+
if (FFlag::LuauNewSolverPrePopulateClasses && bindingIt == scope->exportedTypeBindings.end())
1680+
return ControlFlow::None;
1681+
16491682
std::optional<TypeId> superTy = std::make_optional(builtinTypes->classType);
16501683
if (declaredClass->superName)
16511684
{
@@ -1660,7 +1693,10 @@ ControlFlow ConstraintGenerator::visit(const ScopePtr& scope, AstStatDeclareClas
16601693

16611694
// We don't have generic classes, so this assertion _should_ never be hit.
16621695
LUAU_ASSERT(lookupType->typeParams.size() == 0 && lookupType->typePackParams.size() == 0);
1663-
superTy = lookupType->type;
1696+
if (FFlag::LuauNewSolverPrePopulateClasses)
1697+
superTy = follow(lookupType->type);
1698+
else
1699+
superTy = lookupType->type;
16641700

16651701
if (!get<ClassType>(follow(*superTy)))
16661702
{
@@ -1683,7 +1719,14 @@ ControlFlow ConstraintGenerator::visit(const ScopePtr& scope, AstStatDeclareClas
16831719

16841720
ctv->metatable = metaTy;
16851721

1686-
scope->exportedTypeBindings[className] = TypeFun{{}, classTy};
1722+
1723+
if (FFlag::LuauNewSolverPrePopulateClasses)
1724+
{
1725+
TypeId classBindTy = bindingIt->second.type;
1726+
emplaceType<BoundType>(asMutable(classBindTy), classTy);
1727+
}
1728+
else
1729+
scope->exportedTypeBindings[className] = TypeFun{{}, classTy};
16871730

16881731
if (declaredClass->indexer)
16891732
{

tests/TypeInfer.definitions.test.cpp

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
using namespace Luau;
1111

12+
LUAU_FASTFLAG(LuauNewSolverPrePopulateClasses)
13+
1214
TEST_SUITE_BEGIN("DefinitionTests");
1315

1416
TEST_CASE_FIXTURE(Fixture, "definition_file_simple")
@@ -492,11 +494,8 @@ TEST_CASE_FIXTURE(Fixture, "class_definition_indexer")
492494

493495
TEST_CASE_FIXTURE(Fixture, "class_definitions_reference_other_classes")
494496
{
495-
unfreeze(frontend.globals.globalTypes);
496-
LoadDefinitionFileResult result = frontend.loadDefinitionFile(
497-
frontend.globals,
498-
frontend.globals.globalScope,
499-
R"(
497+
ScopedFastFlag _{FFlag::LuauNewSolverPrePopulateClasses, true};
498+
loadDefinition(R"(
500499
declare class Channel
501500
Messages: { Message }
502501
OnMessage: (message: Message) -> ()
@@ -506,13 +505,19 @@ TEST_CASE_FIXTURE(Fixture, "class_definitions_reference_other_classes")
506505
Text: string
507506
Channel: Channel
508507
end
509-
)",
510-
"@test",
511-
/* captureComments */ false
512-
);
513-
freeze(frontend.globals.globalTypes);
508+
)");
514509

515-
REQUIRE(result.success);
510+
CheckResult result = check(R"(
511+
local a: Channel
512+
local b = a.Messages[1]
513+
local c = b.Channel
514+
)");
515+
516+
LUAU_REQUIRE_NO_ERRORS(result);
517+
518+
CHECK_EQ(toString(requireType("a")), "Channel");
519+
CHECK_EQ(toString(requireType("b")), "Message");
520+
CHECK_EQ(toString(requireType("c")), "Channel");
516521
}
517522

518523
TEST_CASE_FIXTURE(Fixture, "definition_file_has_source_module_name_set")

0 commit comments

Comments
 (0)