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

Adds table field checks #65

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

arichard4
Copy link

@arichard4 arichard4 commented Apr 29, 2022

Closes #39

The basic design here:

  • Start tracking on a table assignment of the form $var = {x, y, z}. This automatically excludes anything with a metatable.
  • End tracking of all tracked vars on any scope changes, from control flow (for, while, if, etc) or from function calls. This automatically stops tracking on the usage of functions like pairs/ipairs/table.insert/table.remove, so we don't need any special case handling for them.
  • Track which variables are a reference to which tables. Not sure how useful this actually is; if you'd prefer, I can just flatly stop tracking the table in that case.
  • Track which table fields have been set or have been accessed. Unknown sets/accesses are tracked as potentially setting/accessing every table field.
  • On a table field access, warn if that field has been set. If the field access key is unknown (a variable or a complex type like "1 + 1"), only warn if no fields at all have been set. If an unknown set has occurred, don't warn.
  • On a table field set, check if it's overwriting a previous set that hasn't possibly been accessed.
  • When the table goes out of scope, check if there are any sets that have no accesses, accounting for unknown key accesses.

If you check my fork of luacheck, on the table_fields branch, I wrote a far more complicated version last November that tried to handle control flow (except for break statements) and function calls as well. Unfortunately that got unreviewably complicated; I may submit bits of that in later reviews, but I think that this version is more comprehensible to start with.

@UncombedCoconut
Copy link

The branched luacheck remains very fast on a ~100K line code base, with no false positives. No true positives either, and it looks like this initial level of caution is pretty limiting. For example, it's not allowed to catch problems here because print is a function:

local x = {}
print(x.oops)

So it'd be interesting to see if other projects hit these cautious warnings, and (if the implementation is well received) how some bits which extend the tracking boundaries look.

@arichard4
Copy link
Author

arichard4 commented Apr 30, 2022

I’ve run it across 12 largeish lua GitHub projects; it found issues in 3.

sile-typesetter/sile#1286

This version catches the typo’d field name, but not the duplicate assignment.

It also finds non-functionality-affecting minor issues in two other codebases:

In Kong/Kong, there are four places where env.plugins is checked on an empty env tables, eg line 20 here: https://github.com/Kong/kong/blob/911385466571e4f72999fa43419daafae08159d0/spec/02-integration/03-db/11-db_transformations_spec.lua

In LunarVim, some of the colors (red2, orange, gray3) are unused here: https://github.com/LunarVim/onedarker.nvim/blob/master/lua/lualine/themes/onedarker.lua


Making analysis continue through function calls involves a lot of code and cases, but is conceptually straightforward- we need to track which variables are externally accessible; which are passed to the function directly; and add special cases for builtins we care about like table.insert.

Adding support for continuing analysis through control flow is too complicated to be feasible without a different setup- this is currently looping over the linearized code from linearize.lua, which adapts poorly to non-linear control flow.

@arichard4 arichard4 requested a review from alerque April 30, 2022 04:04
@arichard4
Copy link
Author

I don't usually use Github, so I'm not familiar with the standard workflow- would it be standard for me to add the (conceptually separate/additional) support for continued execution through function calls to this PR, open a separate PR (on top of this branch or something? not sure how) or something else?

@alerque
Copy link
Member

alerque commented May 11, 2022

If it is conceptually a different feature but dependent on this one my suggestion is to start a new branch based on this one and open a new PR in draft mode. It will show all the commits in this branch too until this PR is merged, then after this is merged it will show just the new commits in that branch and can be taken out of draft mode for review. There are different workflows to handle this but that's the one I usually reach for. If early review of the unique commits is important you can open a PR merging into this branch, then change the target later, but I don't feel that is necessary for this case.

@alerque
Copy link
Member

alerque commented May 18, 2024

I'm sorry I never got a chance to give this a good test drive. I'm considering whether I should move ahead with a new release soon to get the new builtins out the door or hang back for a bit and try to work out whether this is ready to go in. What's your take on how ready this is?

@alerque
Copy link
Member

alerque commented May 18, 2024

I tried merging master into this locally. The merge conflicts are only in the total number of warnings/errors/files output in some tests, so that's a no-brainer to fix. However the tests don't seem to to output results at all, a half dozen or so actually crash with Lua errors for output, not lint results.

@alerque alerque changed the title Adds table field checks for #39 Adds table field checks Aug 29, 2024
@@ -437,7 +437,8 @@ local item_callbacks = {
Cjump = ClosureState.handle_jump,
Eval = ClosureState.handle_eval,
Local = ClosureState.handle_local_or_set_item,
Set = ClosureState.handle_local_or_set_item
Set = ClosureState.handle_local_or_set_item,
OpSet = ClosureState.handle_local_or_set_item,
Copy link
Member

Choose a reason for hiding this comment

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

@arichard4 I took a stab at both fixing the Lua error and integrating the test result changes from the last few releases into this branch. I wasn't 100% sure about this fix though, does it look sane to you?

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.

Analyze unused or undefined table fields
3 participants