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

bad Diagnostics. (inject-field) #2746

Closed
NeOzay opened this issue Jul 7, 2024 · 12 comments · Fixed by #2747
Closed

bad Diagnostics. (inject-field) #2746

NeOzay opened this issue Jul 7, 2024 · 12 comments · Fixed by #2747

Comments

@NeOzay
Copy link
Contributor

NeOzay commented Jul 7, 2024

How are you using the lua-language-server?

Visual Studio Code Extension (sumneko.lua)

Which OS are you using?

Windows

What is the issue affecting?

Diagnostics/Syntax Checking

Expected Behaviour

the language server correctly deduces the type of the variable but indicates which is not declared.
only occurs when variables are declared at object creation.

image
image

Actual Behaviour

nothing

Reproduction steps

---@class test
---@field position number
local test = {
      position = 0,
      started = false,
}
  
---@param animation test
local function f1(animation)
  animation.position = 1 -- ok
  animation.started = false -- Diagnostics. (inject-field)
end

Additional Notes

No response

Log File

No response

@tomlau10
Copy link
Contributor

tomlau10 commented Jul 8, 2024

I just did some debugging and it seems that the inject-field diagnostic logic is missing a case for def.type == 'tablefield', which causes the false positive (?) case in this issue 😕 .
https://github.com/LuaLS/lua-language-server/blob/master/script/core/diagnostics/inject-field.lua#L58-L70

        for _, def in ipairs(vm.getDefs(src)) do
            local dnode = def.node
            if dnode
            and not isExact
            and vm.getDefinedClass(uri, dnode) then
                return
            end
            if def.type == 'doc.type.field' then
                return
            end
            if def.type == 'doc.field' then
                return
            end
-->>>>> missing the case below
            if def.type == 'tablefield' then
                return
            end
--<<<<<
        end

However I am not 100% sure if the current behavior is by design. Maybe @NeOzay you can open a PR after testing the above code, and then ask maintainers about it?

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

I'm going to try that

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

after testing, your snippet does not seem to correct the problem.

@tomlau10
Copy link
Contributor

tomlau10 commented Jul 8, 2024

after testing, your snippet does not seem to correct the problem.

How did you test the change? It works in my computer 😕

I test it by changing the file inside the vscode extension folder, in my case it's the C:\Users\TomLau\.vscode\extensions\sumneko.lua-3.9.3-win32-x64\server\script\core\diagnostics\inject-field.lua. After that you need to reload the extension, you can use ctrl+shift+p > reload window cmd to reload vscode in order to take effect.

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

my bad, finally it works I'm going to make a PR

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

the change does not pass the test suite.
for this case:

---@class (exact) Class1
---@field x number
local m = {
	x = 1, -- OK
	y = 2, -- Warning
}

m.x = 1 -- OK
m.y = 2 -- Warning (no warning)

function m:init() -- OK
	self.x = 1 -- OK
	self.y = 2 -- Warning (no warning)
	function self:xx() -- Warning
	end
end

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

how can we know if the class is in exact mode?

if def.type == 'tablefield' and not exact then
    return
end

@tomlau10
Copy link
Contributor

tomlau10 commented Jul 8, 2024

Sorry but I don't know much about the LuaLS codebase ☹️
I've only tried to optimize some performance in undefined-field diagnostic before.
Maybe you need to ask help from maintainers in your PR.

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 8, 2024

problem resolved, the PR passes the test suite.

@C3pa
Copy link
Contributor

C3pa commented Jul 8, 2024

This issue might be the same as #2341 and possibly even #2528.

@mycroftjr
Copy link

Why not just add --- @param started boolean?

@tomlau10
Copy link
Contributor

tomlau10 commented Jul 9, 2024

Why not just add ---@param started boolean ?

I think you are talking about adding ---@field ? This is just another way to annotate the fields, and requires manually defining the line.

And I am sure that the current issue is a really a 🐛 , that's because if started is declared via setfield syntax, no inject-field warning will be generated:

---@class test
---@field position number
local test = {}
test.position = 0
test.started = false

---@param animation test
local function f1(animation)
    animation.position = 1 -- ok
    animation.started = false -- no warning here as expected
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants