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

Include module name for definitions files #861

Merged
merged 7 commits into from
Nov 21, 2023

Conversation

JohnnyMorganz
Copy link
Contributor

Closes #818

We set the moduleName of the source module to the provided packageName.
This then populates the relavent definitionModuleName's on CTV/FTV/TTVs, so it allows us to lookup where they originated from.

The one place I can see this having an impact inside of Luau code is the following:
https://github.com/Roblox/luau/blob/1fa8311a188a932233d2ce2bc6412e5bab37c035/Analysis/src/Error.cpp#L70-L90
Which will potentially change they way error messages are formatted.
Should there be a way to exclude definition files when calling getDefinitionModuleName, so we can preserve this behaviour?

@zeux
Copy link
Collaborator

zeux commented Oct 30, 2023

@JohnnyMorganz Please let us know if you're still interested in this PR (if so it would need to be rebased and the flag would need to default to false). Apologies that it was left unreviewed for so long.

@JohnnyMorganz
Copy link
Contributor Author

Yeah I'd still be interested in this. Will follow up on the weekend

@@ -165,6 +166,8 @@ LoadDefinitionFileResult Frontend::loadDefinitionFile(GlobalTypes& globals, Scop
LUAU_TIMETRACE_SCOPE("loadDefinitionFile", "Frontend");

Luau::SourceModule sourceModule;
if (FFlag::LuauDefinitionFileSetModuleName)
sourceModule.name = packageName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please set humanReadableName as well.

@vegorov-rbx vegorov-rbx self-requested a review November 20, 2023 16:44
@JohnnyMorganz
Copy link
Contributor Author

Turns out whilst i was adding the test case that AstStatDeclareFunction declare function foo() doesn't set the definition at all. maybe something else to fix

@vegorov-rbx vegorov-rbx merged commit ae53051 into luau-lang:master Nov 21, 2023
7 checks passed
@JohnnyMorganz JohnnyMorganz deleted the definition-module-name branch November 21, 2023 16:31
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.

Include a useful definitionModuleName for types originating from definitions file
3 participants