missing-fields diagnostic doesn't check for fields of inherited classes

estebanfer commented 3 days ago

How are you using the lua-language-server?

Other

Which OS are you using?

Windows

What is the issue affecting?

Type Checking, Diagnostics/Syntax Checking

Expected Behaviour

Initializing a class instance warns of missing fields from the inherited classes.

Actual Behaviour

Initializing a class instance only checks that the fields defined in the class are present, fields inherited from other classes aren't checked, so if one of them is missing, there's no warning.

Reproduction steps

---@class A
---@field a integer

---@class B: A
---@field b integer

---@type B
local b1 = { a = 1 } -- Missing required fields in type `B`: `b`
---@type B
local b2 = { b = 2 } -- No warning shown

---@param b B
local function func_b(b)
  return b
end

func_b({ a = 1 }) -- Missing required fields in type `B`: `b`
func_b({ b = 2 }) -- No warning shown

Additional Notes

No response

Log File

No response

tomlau10 commented 3 days ago

There are already many duplicated issues concerning this 😅

estebanfer commented 3 days ago

Oops I forgot to search issues again after realizing what was the actual bug

estebanfer commented 3 days ago

Do I close this?

estebanfer commented 2 days ago

This code fixes it locally, but I don't know what really changes from doing that. Should I open a PR? In file script\core\diagnostics\missing-fields.lua Line 59

-                for _, field in ipairs(def.fields) do
+                for _, field in ipairs(vm.getFields(def)) do

tomlau10 commented 2 days ago

I am not familiar with that part of the codebase as well, but just judging from the code inside script\core\diagnostics\missing-fields.lua, I think your fix is incomplete 🤔 (though it is no worse I believe) https://github.com/LuaLS/lua-language-server/blob/1bc01316af663092fa3ffa79e6a419a5b956e4e7/script/core/diagnostics/missing-fields.lua#L41-L44

There is an early break condition for #def.fields == 0
Thus when the inherited class has no defined fields, then the whole checking will be skipped
```
---@class A
---@field a integer
```

---@class B: A -- no extra defined fields for class B

---@type B local b1 = { a = 1 } -- No warning shown ---@type B local b2 = { b = 2 } -- No warning shown

- You may try to change this early break condition to something like:
```lua
                local fields = vm.getFields(def)
                if #fields == 0 then
                    goto continue
                end

then use for _, field in ipairs(fields) do below

In addition, I don't know if using vm.getFields inside the already nested forloop will cause performance issue or not 😕 . Anyway I think you can add more test cases to test/diagnostics/missing-fields.lua as well. If all of them passes without breaking existing ones, then I guess it is good to go 👍

estebanfer commented 1 day ago

Thanks for the extensive response! Good catch for that issue I'll fix that, start making more tests, and then open the PR

LuaLS / lua-language-server