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

check that the shape of the table corresponds to the class #2768

Merged
merged 8 commits into from
Aug 15, 2024

Conversation

NeOzay
Copy link
Contributor

@NeOzay NeOzay commented Jul 21, 2024

Hi,
this PR add a correspondence check between the table and the class.
related to the problem #2734.
demonstration:

---@class Foo
---@field FooField integer
---@field afield? string

---@param FooParam Foo
local function test(FooParam) end

local x = {FooField = 1, afield = "hello"}
test({FooField = 1, afield = "hello"})
test(x)

local y = {FooField = 1, afield = 2}
test({FooField = 1, afield = 2})
test(y)

local z = {FooField = 1}
test({FooField = 1})
test(z)

local a = {afield = "hello"}
test({afield = "hello"})
test(a)

image
this reinforces the param-type-mismatch diagnosis, if the parameter is a table, it will need to correspond to the signature of the function.

this PR can still be greatly improved and does not pass all tests

@NeOzay
Copy link
Contributor Author

NeOzay commented Jul 21, 2024

it does not support classes in this form:

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

and array

@@ -328,7 +328,7 @@ function vm.isSubType(uri, child, parent, mark, errs)
for n in child:eachObject() do
local nodeName = vm.getNodeName(n)
if nodeName
and not (nodeName == 'nil' and weakNil)
and not (nodeName == 'nil' and weakNil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary space


for i, field in ipairs(set) do
local key = vm.getKeyName(field)
local node = vm.compileNode(field)
Copy link
Collaborator

Choose a reason for hiding this comment

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

bad indent

@@ -753,7 +796,7 @@ function vm.viewTypeErrorMessage(uri, errs)
lparams[paramName] = vm.viewKey(value, uri)
else
lparams[paramName] = vm.getInfer(value):view(uri)
or vm.getInfer(value):view(uri)
or vm.getInfer(value):view(uri)
Copy link
Collaborator

Choose a reason for hiding this comment

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

donot change here

end
end

-------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

donot add dashes

@NeOzay NeOzay marked this pull request as draft July 22, 2024 15:40
@tmillr
Copy link
Contributor

tmillr commented Jul 22, 2024

I thought this was the intended behavior, and a large part of the reason why (exact) was created and needed?

@NeOzay NeOzay requested a review from CppCXY July 28, 2024 16:31
@NeOzay NeOzay marked this pull request as ready for review July 28, 2024 20:44
@tmillr
Copy link
Contributor

tmillr commented Jul 28, 2024

---@class Foo
---@field FooField integer
---@field afield? string

---@param FooParam Foo
local function test(FooParam) end

local x = {FooField = 1, afield = "hello"}
test({FooField = 1, afield = "hello"})
test(x)

local y = {FooField = 1, afield = 2}
test({FooField = 1, afield = 2})
test(y)

local z = {FooField = 1}
test({FooField = 1})
test(z)

local a = {afield = "hello"}
test({afield = "hello"})
test(a)

I think you might need to change the example in your comment:

----@class Foo
+---@class (exact) Foo
 ---@field FooField integer
 ---@field afield? string
 
 ---@param FooParam Foo
 local function test(FooParam) end

Edit: nvm, (exact) is only used to disallow "injected fields" IIRC

@sumneko sumneko merged commit 7c9a24f into LuaLS:master Aug 15, 2024
11 checks passed
@sumneko
Copy link
Collaborator

sumneko commented Aug 16, 2024

I think this check is too strict after testing it with real projects. I will set an option for it and keep it off by default.

mikavilpas added a commit to mikavilpas/yazi.nvim that referenced this pull request Aug 17, 2024
It was released yesterday, but was toggled off by default.
The example in the pull request works nicely.

- pull request
  - LuaLS/lua-language-server#2768
- comment saying why it was toggled off
  - LuaLS/lua-language-server#2768 (comment)
- changelog with a little more info
  - https://github.com/LuaLS/lua-language-server/blob/d702a55715df19a219e963da496e6fb76db0aacd/changelog.md?plain=1#L8
mikavilpas added a commit to mikavilpas/yazi.nvim that referenced this pull request Aug 17, 2024
It was released yesterday, but was toggled off by default.
The example in the pull request works nicely.

- pull request
  - LuaLS/lua-language-server#2768
- comment saying why it was toggled off
  - LuaLS/lua-language-server#2768 (comment)
- changelog with a little more info
  - https://github.com/LuaLS/lua-language-server/blob/d702a55715df19a219e963da496e6fb76db0aacd/changelog.md?plain=1#L8
@Calandiel
Copy link

Perhaps it could be enabled by default on (exact) classes and disabled by default on others?

@SirzBenjie
Copy link

I think this check is too strict after testing it with real projects.

@sumneko Can you give examples where it seems too strict? I haven't looked at the change extensively, but my gut reaction is that the class definitions would be improper if this isn't passing.

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 this pull request may close these issues.

6 participants