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

improve the missing-fields logic to be able to correctly handle classes defined several times #2770

Merged
merged 11 commits into from
Aug 1, 2024

Conversation

NeOzay
Copy link
Contributor

@NeOzay NeOzay commented Jul 23, 2024

linked to #2769.

this PR allows:

---@class Foo
---@field a number
---@field b number
---@field c number

---@class Foo

---@type Foo
local x = {
	a = 1,
	b = 2,
}

image

better display of missing fields

---@class Foo
---@field a number
---@field b number
---@field c number

---@class Foo
---@field d number

---@type Foo
local x = {
	a = 1,
	b = 2,
}

image

detects missing index fields

---@class A
---@field [1] string

---@type A
local t = {}

image

support class unions

---@class Foo
---@field a number
---@field b number
---@field c number

---@class Foo
---@field d number

---@class Bar
---@field ba number
---@field bb number
---@field bc number

---@class Bar
---@field bd number

---@type Foo|Bar
local x = {
	a = 1,
	b = 2,
}
---@type Foo|Bar
local y = {
	ba = 1,
	bb = 2,
	bc = 3,
	bd = 4,
}

image

changelog.md Outdated Show resolved Hide resolved
script/core/diagnostics/missing-fields.lua Outdated Show resolved Hide resolved
Comment on lines 49 to 53
local myKeys = {}
for _, field in ipairs(src) do
local key = vm.getKeyName(field)
local key = vm.getKeyName(field) or field.tindex
if key then
myKeys[key] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

The computation of myKeys seems independent with the current nested forloop. It only depends on src, which is a function argument in the callback.

I suppose this can be optimized to execute only once using lazy initialization 😄

  • first define local myKeys outside of current nested forloop
  • then inside the loop, only compute it if myKeys == nil
local myKeys
for _, samedefs in pairs(sortedDefs) do
  ...
  for _, def in ipairs(samedefs) do
    ...
    -- build myKeys lookup table once
    if myKeys == nil then
      myKeys = {}
      for _, field in ipairs(src) do
        -- same logic
      end
    end

NeOzay and others added 3 commits July 28, 2024 18:19
Co-authored-by: Tom Lau <tomandfatboy@gmail.com>
Co-authored-by: Tom Lau <tomandfatboy@gmail.com>
@lewis6991
Copy link
Contributor

---@type Foo|Bar
local x = {
a = 1,
b = 2,
}

The diagnostic for this looks wrong to me. Foo|Bar doesn't mean it is both types, it means it is one of those types, just as string|integer is not both a string and an integer.

I'm not sure what the correct diagnostic should be, maybe it should be missing c since it has most of Foo.

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 29, 2024

What I can do is only list the Classes that include all the specified fields.

@CppCXY CppCXY self-requested a review July 31, 2024 11:13
@CppCXY
Copy link
Collaborator

CppCXY commented Jul 31, 2024

Please resolve the conflict.

@max397574
Copy link
Contributor

What I can do is only list the Classes that include all the specified fields.

I think that would make more sense (if there is such a class)
Because in the example if you say missing fields for Bar there would also have to be a diagnostic because additional fields which aren't defined for Bar where added

@sumneko
Copy link
Collaborator

sumneko commented Aug 1, 2024

Thank you!

@sumneko sumneko merged commit bbe49f6 into LuaLS:master Aug 1, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants