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

Pre-populate/duplicate check class definitions (new solver) #1493

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

checkraisefold
Copy link
Contributor

@checkraisefold checkraisefold commented Oct 28, 2024

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:

std::optional<TypeFun> binding;
if (auto it = scope->exportedTypeBindings.find(className); it != scope->exportedTypeBindings.end())
binding = it->second;
// This class definition must have been `prototype()`d first.
if (!binding)
ice("Class not predeclared");

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.

@checkraisefold checkraisefold changed the title Pre-populate class definitions, duplicate checking for class definitions Pre-populate class definitions, duplicate checking for class definitions (new solver) Oct 28, 2024
@checkraisefold checkraisefold changed the title Pre-populate class definitions, duplicate checking for class definitions (new solver) Pre-populate/duplicate check class definitions (new solver) Oct 28, 2024
@vegorov-rbx
Copy link
Collaborator

This means silent error-types popping up everywhere because types are used before they are declared.

This is another bug btw, error reporting is missing in ConstraintGenerator::resolveReferenceType

@checkraisefold
Copy link
Contributor Author

This means silent error-types popping up everywhere because types are used before they are declared.

This is another bug btw, error reporting is missing in ConstraintGenerator::resolveReferenceType

Yeah, that's what I noticed.

@checkraisefold
Copy link
Contributor Author

checkraisefold commented Oct 28, 2024

There's a test for this (class_definitions_reference_other_classes) which wasn't tripped by the new solver because of the resolveReferenceType lack of error reporting, and it only checks success. I'll edit the test later today to explicitly check the types of the objects to catch any later regression if resolveReferenceType isn't fixed yet or the (future) error reporting in it breaks

@checkraisefold
Copy link
Contributor Author

image
Test updated, here is the result of it without these changes.

Copy link
Collaborator

@aatxe aatxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be flagged, but otherwise looks good.

@checkraisefold
Copy link
Contributor Author

flag gated
used scoped flag in the test because this test has no cases where it will succeed in the new solver without this flag enabled

@checkraisefold checkraisefold requested a review from aatxe October 30, 2024 18:50
Comment on lines +758 to +759
if (!FFlag::LuauNewSolverPrePopulateClasses)
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't usually flag it this way, but honestly, it's kind of nice in this case.

@aatxe aatxe merged commit f1d4621 into luau-lang:master Nov 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Types cannot be used before declaration in declaration files (new solver)
3 participants