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: improve function type narrow by checking params' literal identical #2822

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

tomlau10
Copy link
Contributor

@tomlau10 tomlau10 commented Aug 24, 2024

The following related issues are not fixed by this PR, but I tested and they already work as of v3.10.5: #2509, #1343, #1146

edit: I originally thought #2509 is working, but NO. Just that the reproduction code is not complete, and I didn't reproduce it and think it is solved. I confirmed that it is still unsolved with a more complete example code.

The Problem

Originated from an example provided by @carsakiller: #1456 (comment), where the base signature contains a param type that is a superset of the other overload, type narrow feature will always includes the baseline one and cannot perform a more accurate type narrow.

A minimal example:

---@overload fun(a: string): string
---@overload fun(a: 'test'): integer
local function test(...) end

local a = test('')      -- string, good
local b = test('test')  -- string|integer, bad
local s = 'test'
local c = test(s)       -- string|integer, bad

Proposed Solution

I introduced a match score when trying to do a further type narrow. After the 1st pass checking isAllParamMatched in the existing logic, I tried to check if their arguments' literal have exact match:

  • If call args are literals, they will have a literal value like 123, "test"
  • If the overload params also specify a literal value, then they will have a literals map, we can then check if it contains the arg's literal value
  • When a function's params contain literal values of call args, I add bonus scores to it
  • Finally only keep functions that have the highest score

This is just an extra logic and should be no worse than current logic. Because when there are no identical view param, all functions just have the same score 0 => all will be kept as in the existing logic.

After the fix:

---@overload fun(a: string): string
---@overload fun(a: 'test'): integer
local function test(...) end

local a = test('')      -- string, good
local b = test('test')  -- integer, good
local s = 'test'
local c = test(s)       -- integer, good

中文版

長話短說,我嘗試在 vm.getExactMatchedFunctions 引入1個 計分制
在滿足原有的 isAllParamMatched 前題下

  • 如果 function param 的 literals map 包含 call arg 的 literal value => +1 分
    這樣能夠在 literal type 的 param exact match 時有較高分數
  • 最後只保留最高分數 (有可能多個) 的 functions
    達致更準確的 type narrow
  • 而由於這個 logic 是額外的,所以最差情況就只是跟現狀一樣
    => 所有 function 都是 0分 => 全部保留

我不確定有沒有更好改法
這個需要 @sumneko 來 review
(希望我沒有像上次一樣,fix 了1個問題卻又引起新 bug ... 🤦‍♂️ )

@tomlau10
Copy link
Contributor Author

tomlau10 commented Aug 24, 2024

Seems my latest strategy of checking param's literals map contains arg's literal is not working for carsakiller's example...
Because the base function signatures uses an @alias type, and the literals map of it will contains all enum literals value🤦‍♂️

So the logic will add bonus score for both the base function and the overload one
=> they have same bonus score
=> cannot do further type narrow ☹️


由於 @aliasliterals map 實際上是包含其定義中的所有 literal
所以我最新 logic 下的 檢查 function param literals map 包含 arg's liteval 值 對 carsakiller 原有例子沒效果。。。

---@alias ccTweaked.os.event
---| "alarm"
---| "char"
---| string

---@async
---@param event? ccTweaked.os.event
---@return any ...
---@overload fun(event: "alarm"): "alarm", integer
---@overload fun(event: "char"): "char", string
function os.pullEvent(event) end

local event, alarmID = os.pullEvent("alarm")
  --> any, any (expected: "alarm", integer)
local event, character = os.pullEvent("char")
  --> any, any (expected: "char", string)

要再想想先。。。

@tomlau10
Copy link
Contributor Author

想到了 🙂 計分制中不直接做 + 1,而是 + 1/{literals map 的 entry 總數}

  • 這樣當 overload param 定義是 單個 literal
    => 會 + 1/1 => + 1
  • 而如果 function param 是用 alias type (而 alias 通常是會有很多 literal)
    => 就只會 + 1/{大數} => 肯定少於上邊的 + 1

這樣就傾向選擇 param literal type 定義為較 narrow 的 function 了

---@alias ccTweaked.os.event
---| "alarm"
---| "char"
---| string

---@async
---@param event? ccTweaked.os.event
---@return any ...
---@overload fun(event: "alarm"): "alarm", integer
---@overload fun(event: "char"): "char", string
function os.pullEvent(event) end

local event, alarmID = os.pullEvent("alarm")
  --> "alarm", integer (good)
local event, character = os.pullEvent("char")
  --> "char", string (good)

@tomlau10
Copy link
Contributor Author

tomlau10 commented Aug 25, 2024

先 hold 一下,我突然又有些想法估計可以 fix #2509 🤔

  • 該 issue 的問題在於當 function 只有 @overload 定義 而沒有 @param 定義時
    base function param type 會從 第1個 overload 獲取 param type
  • 那麼可以考慮在做 type narrow 時,忽略這種 case 的 base function signature
    就是假定 如果只有 @overload 而沒有 @param
    => 該 function 只會按 @overload 的方式來調用

要些時間驗證一下


edit: 還是分開另外處理吧
不然出 bug 了都不知是跟哪個改動有關 😅

@tomlau10 tomlau10 marked this pull request as draft August 25, 2024 10:54
@tomlau10 tomlau10 marked this pull request as ready for review August 25, 2024 11:15
@sumneko
Copy link
Collaborator

sumneko commented Aug 28, 2024

我没有看代码,但是看描述我觉得无论什么时候都不应该用计分。如果无法确定如何narrow,那就不做narrow。

@tomlau10
Copy link
Contributor Author

tomlau10 commented Aug 28, 2024

无法确定如何narrow,那就不做narrow

我這裡的計分,最初其實是 計算 exact literal match 的 param count
(count 是數量,也就可理解成 1個 score)
並且是在 肯定的時候加分,否則就當 0分 🤔

  • 如果所有 signature 都不肯定 => 就會全是 0分
  • 全是 0分,也就是所有 signature 分數相同 => 不做 narrow
  • 也就是出現 score > 0 時,肯定是 true positive
    而 score = 0 是無從判斷 case,但由於 no worse 所以這些 case 也不 care 了

讓我具體舉一些例子

例子1

---@overload fun(a: string): string
---@overload fun(a: 'test'): integer
local function f(...) end
  • 這裡任何對 f() 的調用,都會找出上述2個 overload 來做判斷
  • f('') => 本身做 isAllParamMatched() 只能 match 出 fun(a: string): string
    • 新的計分制對他無任何影響
  • f('test') 在原有 isAllParamMatched() 情況下 2個 overload 都匹配的
    • 'test' canCastType 成 string,同時 'test' 也 canCastType 成 'test' => 原有機制無法進一步 narrow
    • 但實際上,由於 'test' 這個 literal 值跟第2個 overload 中的 a: 'test'完全相同 => 是可以做 narrow
    • 這裡加 1分 給他 => 最終 matchScores = [ 0, 1 ] => 只選 第2個 overload
      => f('test') -> integer 而不是原有的 string|integer

例子2

---@alias ccTweaked.os.event
---| "alarm"
---| "char"
---| string

---@async
---@param event? ccTweaked.os.event
---@return any ...
---@overload fun(event: "alarm"): "alarm", integer
---@overload fun(event: "char"): "char", string
function os.pullEvent(event) end
  • 這裡 os.pullEvent 其實有 3種 infer type
    1. fun(event?: ccTweaked.os.event): any ...
    2. fun(event: "alarm"): "alarm", integer
    3. fun(event: "char"): "char", string
  • 當調用 os.pullEvent('alarm') 時,按原機制 isAllParamMatched() 會 match 出 第1 & 第2個
    • 因為 'alarm' canCastType 成 ccTweaked.os.event,也 canCastType 成 'alarm'
    • 但按理是想 narrow 成 第2個 infer type,因為是 exact literal match
  • 按原有做法,我以為判斷 2者有任意1個 exact literal match 即可
    問題是 ccTweaked.os.event 這個 infer node 只是個 alias,實際上他又有幾個 literal 的解讀
    • 'alarm'
    • 'char'
    • 如果用 任意1個 exact literal match => +1分,就導致這個 case 也有 score=1 => 無法 filter 走
  • 所以這裡的計分制,簡單改成 + 1 / {infer 中支持的 literal 總數}
    => + 1/2
    => 使得他的 score 肯定 低於另外出現 exact match 的 function
    意義上是在符合 exact literal match (肯定的 true positive case) 大前題情況下,他的匹配率最高
    • 最終 os.pullEvent('alarm')matchScores = [ 0.5, 1, -1 ] => 就能 narrow 成第2個了

我大概理解你的擔心,是以為這計分制是 (胡亂) 計算什麼 similarity 之類?
但這裡不是,是在 肯定 (出現 exact literal value match) 的情況下做加分,其餘 case 都是 0 分
沒有 literal match 的情況下,不會給任何分數的 🤔
(而與期說分數,更像是 count 有多少 literal value match)
@sumneko

@sumneko
Copy link
Collaborator

sumneko commented Aug 28, 2024

明白了,这里的计分指的是narrow程度,那应该没有问题

@tomlau10
Copy link
Contributor Author

这里的计分指的是narrow程度

確實你這個說法更準確 👍

@sumneko sumneko merged commit c636fdd into LuaLS:master Sep 5, 2024
11 checks passed
@tomlau10 tomlau10 deleted the fix/type_narrow branch September 5, 2024 11:50
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.

2 participants