LocalUnused does not flag unused function parameters

[JohnnyMorganz]

local function createObj(id: number)
    return {
        id = 0, -- bug: this should've been "id" parameter, but no warning was flagged so it is missed
        data = "..."
    }
end

I flagged this as a bug, but it may be a feature request instead (as in, it seems there are separate lints for "LocalUnused", "ImportUnused" and "FunctionUnused", so instead of being part of LocalUnused it may warrant a new lint "ParameterUnused")

[zeux]

This is by design as unused parameter warnings tend to be much much much noisier than regular unused warnings.

[JohnnyMorganz]

I think I would personally have to disagree with the design - I've always found such a warning useful in other static analysis tools and never had a need to turn them off (selene/luacheck/eslint/...)

I guess the noisiness you refer to is when you're in the process of writing a new function? And it's already warning you about parameters that you intend to later use. I can see what you mean here. Again personally I wouldn't get annoyed by this, but I appreciate that the analysis tools must cater for a much wider audience. There may also be other cases which I'm missing.

The above example is based off a more subtle bug that we noticed in a larger codebase, which was initially just linted in Studio. The issue didn't have a direct impact so wasn't noticed until we exported the code and ran a lint pass using Selene.

[zeux]

To be clear, we could probably add a new warning for this that can be independently disabled, but in any existing codebase likely many local arguments are unused even if the explicit local assignments aren't.

There's plenty of cases where eg you pass a callback or declare a function that is expected to have a certain signature but then end up not using one of the arguments. Bundling the lack of use for a function argument in this case would require changing the name to use _ prefix, and while it can indicate a bug, it's much less likely to vs a local.

So the severity of an unused local (you may have forgotten to use it; if not, you're paying extra cost for computing the value and if it doesn't have side effects that's just redundant work; the fix is simply to remove local declaration, sometimes keeping an expression - it's code cleanup) is much larger than that of an unused parameter (you may have forgotten to use it but there are plenty of cases where it not being used is intentional; no specific cost, if you can remove the argument outright you'd need to update all callsites which is much more invasive).

[zeux]

Case in point: in lua-apps (our internal app code base), we have 705 instances of LocalUnused (they are turned off via a config file), and 3663 if you add an unused parameter warning. So this is would be a very substantial increase on existing complex codebase, something we probably couldn't deploy without a separate warning.

[JohnnyMorganz]

Thank you for the insight! I see what you mean wrt having unused parameters, especially in existing codebases if they haven't normally prefixed them with _

I imagine if this would have to be a new lint it would be disabled by default? I'm not sure if there is anyway right now to toggle whether a lint is enabled or not in Studio (either on a file-by-file comment or global config basis), so the uses will probably be quite low. (I know you can disable particular lints using !nolint NAME, but not sure about enable - unless you probably do want it enabled by default)

[zeux]

Coming back to this - yeah, I think we'd want to disable a new lint like this by default. LintOption::setDefaults is used for that - it used to set a pedantic shadow warning as disabled by default (we've since simply removed that one). Outside of Studio this would make it possible to enable this via .luaurc.