Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Feature] Add action to autorequire undefined globals #2177

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

sewbacca
Copy link
Contributor

I attempted to implement #1938.
This is how it looks in action:
autorequire-rewrite
How to make this less of a hack?

-- TODO: Is there a better way to detect undefined-global?
for _, diag in ipairs(diagnostics) do
if diag.code == 'undefined-global' then
for _, potglobal in ipairs(potentialGlobals) do
if diag.message:find(potglobal.name) then
addPotentialRequires(potglobal)
end
end
end
end

How to add tests? I suppose they should go somewhere here:
-- TODO: Add some tests for ACTION_AUTOREQUIRE

But how do I create the stubs for other modules?

@sewbacca sewbacca changed the title Added concept of action autorequire [Feature] Add action to autorequire undefined globals Jun 23, 2023
@sumneko
Copy link
Collaborator

sumneko commented Jun 25, 2023

But how do I create the stubs for other modules?

See test/crossfile

Improved eq error message in test/code_action/init.lua
Uses core.diagnostics.undefined-global
@sewbacca
Copy link
Contributor Author

I added a testcase, and used the test/crossfile directory as a reference (though the actual test lies inside the code_action/init.lua file). I implemented a custom TEST_CROSSFILE function to setup the unit test. I have also modified the eq function, mainly to return a more friendly error message, showing exactly the keypath that does not match.

I also don't use diagnostic messages anymore to determine if a global is undefined, so that I can add the refactor.rewrite command. Instead I use the core.diagnostic.undefined-global function to report the undefined globals back to me and double check, if the requested position from the user is actually undefined. It still feels a bit of a hack, but it should be much more robust now, since I don't use a diagnostic message as a reference. Is this function called when the user asks for code fixes or more often? Because as far as I understand, the function checks the whole ast, not just only the part in which we are interested. Is there maybe a function, which can check if a given variable is an undefined global directly?

The undefined-global function is marked as async, but the code-action function is not. Can I safely add a ---@async statement to remove the warning, since the tests pass?

@sumneko
Copy link
Collaborator

sumneko commented Jun 28, 2023

I have also modified the eq function, mainly to return a more friendly error message, showing exactly the keypath that does not match.

Greate, thank you!

Is this function called when the user asks for code fixes or more often? Because as far as I understand, the function checks the whole ast, not just only the part in which we are interested. Is there maybe a function, which can check if a given variable is an undefined global directly?

There is currently no such function. You can do the following:

  1. Extract the check function in undefined-global into a separate function.
  2. Use guide.eachSourceBetween to only check the interested part.
  3. Cache the results in node.
---@class vm.node
---@field package _undefined_global? boolean

function api.isUndefinedGlobal(src)
    local node = vm.compileNode(src)
    if node._undefined_global == nil then
        node._undefined_global = checkIsUndefinedGlobal(src)
    end
    return node._undefined_global
end

The undefined-global function is marked as async, but the code-action function is not. Can I safely add a ---@async statement to remove the warning, since the tests pass?

Yes, all code is executed within a coroutine during testing.

(Translated by ChatGPT)

@sewbacca
Copy link
Contributor Author

sewbacca commented Jul 17, 2023

I've moved the undefined global check into script/vm and rewrote code-action and diagnostics/undefined-global to use the vm instead. I also added another test, if an init file is present. I believe it is now ready for review.

@sewbacca sewbacca marked this pull request as ready for review July 17, 2023 12:57
Resolve member naming
@sewbacca
Copy link
Contributor Author

sewbacca commented Jul 17, 2023

Fixed formatting (I dont know how that happened)
Renamed _undefined_global to undefinedGlobal

@sumneko
Copy link
Collaborator

sumneko commented Jul 18, 2023

Nice work, thank you!

@sumneko sumneko merged commit 9e1e7cc into LuaLS:master Jul 18, 2023
@sewbacca sewbacca deleted the feature/shortcut-autorequire branch July 18, 2023 20:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants