unused function lint for recursive functions by nnullcolumn · Pull Request #2553 · luau-lang/luau

nnullcolumn · 2026-07-14T02:19:41Z

updates code and tests so that functions which are used inside of their own body but unused outside of it get linted
closes unused function lint fails for recursive function #2550
100% human authored

hgoldstein

This needs to be flagged, but otherwise I like the direction.

hgoldstein · 2026-08-06T16:02:06Z

        l.function = true;

-        return true;
+        l.scopeDepth++;


If I write:

local function foobar() -- Let's call this foobar_0 local function foobar() -- And this foobar_1 return foobar() end end return foobar()

... foobar_0 and foobar_1 are different AstLocal*s. Luau's parser handles this sorta thing for you. I don't think you need to track scope depth here at all.

scope depth was the chosen mechanism for detecting whether the reference was inside the body of the function or not. i think i could change this to use the Location of the function's body, instead? at the time it seemed the most intuitive to me (i think it wasn't meant to get around anything in the local case, i more or less use the depth like a boolean here but that seems less intuitive than using the span of the body now)

You could probably get away with a single boolean flag is my point, there shouldn't be a way to see a scope depth that isn't either 0 or 1.

sorry i don't think i communicated very well; yeah, i could, but there's a rare case i'd actually probably like to add tests for. it might break if someone wrote speculative code at runtime that changes the definition:

local function fun() if cond then -- same local function fun() end -- exits scope else -- same local function fun() end -- exists scope end -- function considered used return fun() end -- sets the scope to false even though it already left

admittedly im sure this pattern is pretty rare in user code, but as a general thing i often prefer counters or spans over straight up booleans when traversing things because it makes the worstcase of "things are stepping over eachother in ways we didn't expect or that this code wasnt originally written for" much less catastrophic. this might also be more likely e.g. if someone updated the lint to support forward declaration

Oh I see what you mean: if you'd like to support this, sure.

hgoldstein · 2026-08-06T17:23:46Z

+        bool function = false;
+        bool import = false;
+        bool used = false;
+        bool softUsed = false;


Maybe usedAsRecursiveCall instead?

i've updated it to usedOutsideSelf and usedRecursively respectively if that's alright.

hgoldstein · 2026-08-06T17:25:03Z

-        Location location;
-        bool function;
-        bool used;
+        unsigned int scopeDepth = 0;


You might need scope depth for globals?

i think i'd prefer to keep the global handling simple if there aren't any apparent issues with it

nnullcolumn · 2026-08-08T12:54:20Z

This needs to be flagged, but otherwise I like the direction.

i flagged it

nnullcolumn · 2026-08-08T12:55:15Z

idk why it auto requested review from vegorov sry that may have been a misclick or something? dunno

EDIT: oh theres a new CODEOWNERS in the repo thats kinda cool

nnullcolumn · 2026-08-08T13:08:25Z

unification_runs_a_limited_number_of_iterations_before_stopping_subtyping failed on windows

lmfao

nnullcolumn · 2026-08-09T03:19:30Z

+end
+)");
+
+    REQUIRE(0 == result.warnings.size());


this test case was accidentally duplicated / incomplete. whoops

hgoldstein

This still looks like a bunch of the changes are not flagged? I would also advise against mixing refactors like renaming variables or providing defaults to struct members with other changes.

nnullcolumn · 2026-08-13T01:01:41Z

This still looks like a bunch of the changes are not flagged?

I'll look over it again, i guess i was a bit sleep deprived at the time

I would also advise against mixing refactors like renaming variables or providing defaults to struct members with other changes.

can you point to a specific commit hash? the three commits since your last review split it up (there was a dedicated commit for the renames)

nnullcolumn · 2026-08-13T01:12:40Z

This still looks like a bunch of the changes are not flagged?

I'll look over it again, i guess i was a bit sleep deprived at the time

took a look now. there are two places where usedRecursively is set = true, so the flag code i added line 905 and 1034 just fall back to setting a 'external' reference (effectively disabling the change because that was the old behavior)

i think adding the flag in other places would obscure the control flow a bit but i can do that if it's wanted

hgoldstein

i think adding the flag in other places would obscure the control flow a bit but i can do that if it's wanted

When we flag code, we try to make sure that the code path is exactly the same when the flag has been set to off. There are some cases where we fudge things slightly, but it's meant to be extremely conservative. For example, even adding fields to LintLocalHygiene::Local like you've done is technically "unflagged." This maps to our internal process for gauging risk: things that are entirely flagged are minimally risky, less likely to cause issues on rollout, require extensive QA, etc.

That's part of why I ask not to mix refactoring and other changes. Mixing the two makes it really hard to ensure that the flag-off path is exactly the same.

hgoldstein · 2026-08-14T19:37:08Z

+        bool import = false;
+        bool usedOutsideSelf = false;
+        bool usedRecursively = false;
+        bool arg = false;


This is what I mean by "don't mix refactoring and other changes." Optimally this would be:

AstNode* defined = nullptr; bool function; bool import; bool used; bool arg; bool usedRecursively = false; unsigned int scopeDepth = 0;

Only make the changes you need, don't mix in renaming fields or adding defaults.

Just add struct fields at the end rather than also changing the order of fields.

Just add struct fields at the end rather than also changing the order of fields

sorry, ocd probably. i'll try to do that in the future

Only make the changes you need, don't mix in renaming fields or adding defaults.

the defaults were added aggressively here after i experienced a bug w uninitialized values:

fefc694

~~the renames were done here, in their own dedicated commit:~~

~~d7d2be8~~

~~are you suggesting that my changes as a whole shouldn't rename things in the linter? if so, that seems silly - the previous names didn't make as much sense with new fields added, so i changed them~~

EDIT: silly me, i missed part of the review comment when i wrote that

hgoldstein · 2026-08-14T19:38:23Z

        l.function = true;

-        return true;
+        l.scopeDepth++;


Oh I see what you mean: if you'd like to support this, sure.

nnullcolumn · 2026-08-15T00:27:03Z

i think adding the flag in other places would obscure the control flow a bit but i can do that if it's wanted

When we flag code, we try to make sure that the code path is exactly the same when the flag has been set to off. There are some cases where we fudge things slightly, but it's meant to be extremely conservative. For example, even adding fields to LintLocalHygiene::Local like you've done is technically "unflagged." This maps to our internal process for gauging risk: things that are entirely flagged are minimally risky, less likely to cause issues on rollout, require extensive QA, etc.

That's part of why I ask not to mix refactoring and other changes. Mixing the two makes it really hard to ensure that the flag-off path is exactly the same.

that makes complete sense, yeah. i'll adjust some things to make the original code more clearly specialized with the flag. Surely keeping the off-path the same isn't entirely realistic all the time though, is it? i feel like to do it properly i'd need to make a second copy of every field which i use differently

hgoldstein · 2026-08-21T15:56:02Z

Surely keeping the off-path the same isn't entirely realistic all the time though, is it? i feel like to do it properly i'd need to make a second copy of every field which i use differently

It's not, no. Any time you're adding a field to an existing struct, for example, there's a mild risk. The one the Luau team has run into is that even "entirely flagged" blocks in function calls have an effect on the stack size for that function. If you look in Subtyping.cpp there are a bunch of places where we use std::unique_ptr<SubtypingResult> on the stack. Part of the reasoning there was that a previous flagged change caused us to stack overflow more often.

nnullcolumn · 2026-08-21T21:27:49Z

Any time you're adding a field to an existing struct, for example, there's a mild risk. The one the Luau team has run into is that even "entirely flagged" blocks in function calls have an effect on the stack size for that function.

that's terrifying and fascinating, i can't imagine what kinds of niche failure cases y'all have hit. my approach to flagging this was a bit of a shortcut and i knew it. it can be easy to lose sight of why it's important and view stuff like that as a chore that you can get away with sweeping under the rug, but seeing a team that cares is a breath of fresh air in a world full of LGTMs and LLMs. i'll do my best to match that patience, care, and attention to detail with contributions going forward.

nnullcolumn added 5 commits July 14, 2026 02:17

code + tests

9c087a2

improvements

1a105a6

aggressively initialize

fefc694

cleanup

1d210f3

lol

a1c4937

hgoldstein self-assigned this Aug 4, 2026

hgoldstein requested changes Aug 6, 2026

View reviewed changes

nnullcolumn added 2 commits August 8, 2026 12:49

flag this change

c612b29

add tests discussed in pr

7763014

nnullcolumn requested a review from a team as a code owner August 8, 2026 12:49

nnullcolumn requested a review from vegorov-rbx August 8, 2026 12:49

renames

d7d2be8

Merge branch 'master' into enhance-unused-function-lint

797b5ed

nnullcolumn commented Aug 9, 2026

View reviewed changes

MabMabMabMabMab mentioned this pull request Aug 10, 2026

Luau/type error message improvements luwu-community/luwu#20

Merged

hgoldstein reviewed Aug 12, 2026

View reviewed changes

hgoldstein reviewed Aug 14, 2026

View reviewed changes

Uh oh!

Conversation

nnullcolumn commented Jul 14, 2026

Uh oh!

hgoldstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnullcolumn Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnullcolumn commented Aug 8, 2026

Uh oh!

nnullcolumn commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnullcolumn commented Aug 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hgoldstein left a comment

Choose a reason for hiding this comment

Uh oh!

nnullcolumn commented Aug 13, 2026

Uh oh!

nnullcolumn commented Aug 13, 2026

Uh oh!

hgoldstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnullcolumn Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnullcolumn commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hgoldstein commented Aug 21, 2026

Uh oh!

nnullcolumn commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nnullcolumn Aug 7, 2026 •

edited

Loading

nnullcolumn commented Aug 8, 2026 •

edited

Loading

nnullcolumn Aug 15, 2026 •

edited

Loading

nnullcolumn commented Aug 15, 2026 •

edited

Loading