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

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

Closed
SereneRuby12 opened this issue Nov 26, 2024 · 6 comments · Fixed by #2970
Closed

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

SereneRuby12 opened this issue Nov 26, 2024 · 6 comments · Fixed by #2970
Labels
duplicate This issue or pull request already exists

Comments

@SereneRuby12
Copy link
Contributor

SereneRuby12 commented Nov 26, 2024

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

@SereneRuby12 SereneRuby12 changed the title Passing a table to a function param that's a class type doesn't check for inherited fields missing missing-fields diagnostic doesn't check for fields of inherited classes Nov 26, 2024
@tomlau10
Copy link
Contributor

@SereneRuby12
Copy link
Contributor Author

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

@SereneRuby12
Copy link
Contributor Author

Do I close this?

@CppCXY CppCXY added the duplicate This issue or pull request already exists label Nov 26, 2024
@SereneRuby12
Copy link
Contributor Author

SereneRuby12 commented Nov 27, 2024

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
Copy link
Contributor

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)

for className, samedefs in pairs(sortedDefs) do
local missedKeys = {}
for _, def in ipairs(samedefs) do
if not def.fields or #def.fields == 0 then

  • 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:
                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 👍

@SereneRuby12
Copy link
Contributor Author

SereneRuby12 commented Nov 28, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
3 participants