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

Fix table array infer #2924

Merged
merged 7 commits into from
Nov 5, 2024
Merged

Fix table array infer #2924

merged 7 commits into from
Nov 5, 2024

Conversation

Rcklos
Copy link
Contributor

@Rcklos Rcklos commented Nov 1, 2024

local t1 = {
    [1] = {
        x = 1
    },
    [2] = {
        x = "test"
    }
}

local t2 = {
    {
        x = 1
    },
    {
        x = "test"
    }
}

local idx = 2
local a = t1[idx]
local b = t2[idx]

让我们键入以上的代码,我认为t1和t2两个数组的结果是一样的,但是结果是变量a和变量b的提示并不一样。
变量a给我的代码提示是我能够接受的,
image
但是b给我的代码提示是错误的,始终把local b = t2[idx]等价解析为local b = t2[1],这并不完全正确,
image
通过阅读源码,我并不知道为什么针对tableexp类型还要加上field.tindex == 1这样的判断,但将其去除后代码提示就正确了。
image

@CppCXY
Copy link
Collaborator

CppCXY commented Nov 1, 2024

考虑到如果存在一个较大的配置表, 强行分析table元素的类型通常是无意义的

@CppCXY CppCXY changed the title Rcklos Fix table array infer Nov 1, 2024
@Rcklos
Copy link
Contributor Author

Rcklos commented Nov 1, 2024

考虑到如果存在一个较大的配置表, 强行分析table元素的类型通常是无意义的

能够理解你的意思,我接触到的项目中,配置表一般都用以下的方法去定义

local conf = {
    [1] = {id = 1,},
    [2] = {id = 2,},
    ...
}
return conf

如果真的确实很大,理论上上述的local a = t1[idx]也应该不必解析,通常情况下解析配置表没有意义,我们都会ignore掉这些文件

@CppCXY
Copy link
Collaborator

CppCXY commented Nov 1, 2024

以我的经验来看,不是所有人都愿意配置, 另外我更倾向于你的这个行为通过配置开关, 而不是直接启用

@tomlau10
Copy link
Contributor

tomlau10 commented Nov 1, 2024

👀 我 checkout 了這個 PR 來測試,發現還可以 fix 到 #2922 呢 👍

該 issue 是 generic ---@param tbl {[any]: V} 無法匹配 table expression {1, 'a', false} 中所有 type
只能 match 到第1個 param 的 type
原來是因為目前的 vm.getTableValue() 內限制了只在 field.tindex == 1 時做 infer


考虑到如果存在一个较大的配置表, 强行分析table元素的类型通常是无意义的
... 另外我更倾向于你的这个行为通过配置开关, 而不是直接启用

如果有個 config,並且 default 為關掉的話,可能大多數人一開始不會意識到是跟這 config 有關
反而會誤以為是 luals 的 bug? 🤔 就像 #2922

會否可以這樣呢:

  • 如果該 table 的 field 總數超過 N (舉例 10) => 維持原有的只 infer 第1個 field
    否則全部 field value 都 infer?
  • 又或者只 infer field.tindex <= 10 的 value

@Rcklos
Copy link
Contributor Author

Rcklos commented Nov 1, 2024

@CppCXY @tomlau10 结合了两位的思想,我认为也可以这样去实现:
定义一个config用于设定N的默认值,我们当然也可以初步定义N = 10,当table field的数量小于等于N时,可以将全部的value都infer,对于较大的table,也能够尽可能infer到field.tindex <=N的value。
我是初学者,如果都能够认可我这样的方案的话,我也可以尝试去实现

@tomlau10
Copy link
Contributor

tomlau10 commented Nov 2, 2024

定义一个config用于设定N的默认值,我们当然也可以初步定义N = 10,当table field的数量小于等于N时,可以将全部的value都infer,对于较大的table,也能够尽可能infer到field.tindex <=N的value。

我覺得可以 👍
不過我只是1個 contributor,最終決定權還是在 maintainer 😄

script/vm/type.lua Outdated Show resolved Hide resolved
@@ -655,9 +655,10 @@ function vm.getTableValue(uri, tnode, knode, inversion)
end
end
end
local inferSize = config.get(uri, "Lua.type.inferTableSize")
Copy link
Contributor

Choose a reason for hiding this comment

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

這個可以在最開首,forloop 外邊就獲取?
沒有必要每次 loop 都重新 get 1次 🤔

@sumneko sumneko merged commit 180a13b into LuaLS:master Nov 5, 2024
11 checks passed
@Rcklos Rcklos deleted the rcklos branch November 15, 2024 06:33
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.

4 participants