lua-language-server icon indicating copy to clipboard operation
lua-language-server copied to clipboard

How to make a third-party member function as deprecated

Open ChouUn opened this issue 1 year ago • 11 comments

How are you using the lua-language-server?

Visual Studio Code Extension (sumneko.lua)

Which OS are you using?

Windows

What is the issue affecting?

Diagnostics/Syntax Checking

Expected Behaviour

---@deprecated
M.add = M.add

The left one should have marked as deprecated immediately when used such as ~u:add~

Actual Behaviour

  1. it not works first. QQ_1722841266627
  2. and works unexpectedly. QQ_1722841307997

Reproduction steps

  1. Write the code
-- File A

---@class A
local A = {}

---@param attr string
function A.add(attr)
end

-- File B

---@class A
local B = A

---@deprecated
B.add = A.add

B.add('some attr')

We'll find that B.add havn't marked as deprecated 2. Keep write the followed

-- File C

---@class A
---@field add fun(attr: string)
local C = A

Additional Notes

My original intention was to mark a third-party member function as deprecated. As there was no straightforward way, I think, to do it, I had to use a lot of tricks, which led to more subsequent problems.

Log File

No response

ChouUn avatar Aug 05 '24 07:08 ChouUn

I found a way which works sometimes. QQ_1722842414718 but just sometimes .. It will led to incorrect marking. QQ_1722842341509

ChouUn avatar Aug 05 '24 07:08 ChouUn

I believe the deprecated logic is done by this diagnostic: https://github.com/LuaLS/lua-language-server/blob/26a7b690c7eeb1a209b1b600886b2ac6691c5d2e/script/core/diagnostics/deprecated.lua#L37-L40

It calls the function vm.getDeprecated(src, deep=true) which is defined here: https://github.com/LuaLS/lua-language-server/blob/26a7b690c7eeb1a209b1b600886b2ac6691c5d2e/script/vm/doc.lua#L162-L182

Looking at this code, the forloop logic seems to be that:

  • if any definition of the field is NOT deprecated
    • => return not deprecated immediately

I have no idea why it is written like this 😕 maybe for performance reason? Anyway with this logic I don't think there is a way to redefine an existing function as deprecated, because there always exist a function definition that is not deprecated => which makes the above forloop return nil

If I change the above logic to below then it will work:

deprecated = getDeprecated(def)
if deprecated then
    return deprecated
end

But again I think this may have performance consideration behind, because ---@deprecated is rarely used in codebase. If every time we loop all definitions to check if a field have been mark deprecated or not, it may have very high performance impact ☹️

tomlau10 avatar Aug 05 '24 08:08 tomlau10

I think I have found an improvement to your workaround When you reassign the function value, you should use different reference:

---@class M
local m = {}

function m.a() end


---@class B
local b = m

---@deprecated
b.a = m.a  -- don't write `b.a = b.a`, write it as `b.a = m.a`

b.a()   --> deprecated
m.a()   --> but this will still be no warning :(

edit: opps... seems you already found it out 😂

tomlau10 avatar Aug 05 '24 08:08 tomlau10

---@class B
local b = m

---@deprecated
b.a = m.a  -- don't write `b.a = b.a`, write it as `b.a = m.a`

A well discovery! However, the third-party library still returns type M, so this can only be used in the newly implemented functions.

ChouUn avatar Aug 05 '24 08:08 ChouUn

I just tested the proposed fix above and benchmarked it in my project which has 500+ lua files on my macbook pro 2016. I set deprecated to have Error severity level and then use --checklevel=Error to only check for this kind of issues.

    "diagnostics.severity": {
        "deprecated": "Error!"
    },

Seems that it doesn't cause too much performance impact 😄

  • original code: 20.087s
  • patched code: 20.455s

I think @ChouUn you can test the above changes and open a PR for it 👀


edit: seems my fix doesn't work when there is a @field definition ☹️

  • not work
-----
-- emulate library file

---@class y3
local y3 = {}

---@class y3.unit
---@field add_attr fun(self)
y3.unit = {}
function y3.unit:add_attr() end

-----
-- your file

---@deprecated
y3.unit.add_attr = y3.unit.add_attr

---@type y3.unit
local unit
unit:add_attr()  --> no warning
  • works
-----
-- emulate library file

---@class y3
local y3 = {}

---@class y3.unit
y3.unit = {}
function y3.unit:add_attr() end

-----
-- your file

---@deprecated
y3.unit.add_attr = y3.unit.add_attr

---@type y3.unit
local unit
unit:add_attr()  --> deprecated

tomlau10 avatar Aug 05 '24 09:08 tomlau10

did you mean ? If that's the case, you can directly open an issue in y3lib. image

CppCXY avatar Aug 05 '24 11:08 CppCXY

did you mean ? If that's the case, you can directly open an issue in y3lib.

Although I am exactly creating a polyfill for y3lib to prevent other collaborators from calling certain APIs.
I hope to continue using y3lib's type definition "DDD" rather than aliasing it to "AB," as the latter might break some declarations.
This should fall under the category of diagnostics / feature for LLS, I think, rather than y3lib.

because ---@deprecated is rarely used in codebase

Certainly, using deprecated is a widespread need, but marking functions of third-party libraries as deprecated is quite rare.
Lua's flexible syntax makes this possible, and I hope to make full use of this feature 😄.

ChouUn avatar Aug 05 '24 12:08 ChouUn

这是一个两难的问题。目前的策略是当对象有多个定义时,只要有一个定义未标记为弃用,那么整个对象都不是弃用。

这个是为了用户可以重新实现已被弃用的内置函数如 table.getlen

sumneko avatar Aug 15 '24 07:08 sumneko

这是一个两难的问题。目前的策略是当对象有多个定义时,只要有一个定义未标记为弃用,那么整个对象都不是弃用。

我能想到的,是 @deprecated 支援1個 attr 來表達 強力模式,比如說 ---@deprecated (final)

  • 那麼在檢查 deprecated 時,如果其中1個 deprecated 是帶 final => early return true
  • 否則 loop 全部,並在過程中記錄 是否至少有1個未標記為 deprecate
    • 是 => false (即現有效果,允許用戶重新實現被 mark deprecate 的內置函數
    • 否 => true (全部都是 mark 了 deprecate)

這裡的大前題,是 final 只應該被 user 使用 內置的 meta 不應該隨意用 final 否則 user 就不能取消 deprecate 🤔

tomlau10 avatar Aug 15 '24 13:08 tomlau10

这是一个两难的问题。目前的策略是当对象有多个定义时,只要有一个定义未标记为弃用,那么整个对象都不是弃用。

我能想到的,是 @deprecated 支援1個 attr 來表達 強力模式,比如說 ---@deprecated (final)

  • 那麼在檢查 deprecated 時,如果其中1個 deprecated 是帶 final => early return true

  • 否則 loop 全部,並在過程中記錄 是否至少有1個未標記為 deprecate

    • 是 => false (即現有效果,允許用戶重新實現被 mark deprecate 的內置函數
    • 否 => true (全部都是 mark 了 deprecate)

這裡的大前題,是 final 只應該被 user 使用 內置的 meta 不應該隨意用 final 否則 user 就不能取消 deprecate 🤔

感觉变成了 “隐身” -> “反隐” -> “无视反隐的隐身” 的套娃游戏

sumneko avatar Aug 15 '24 15:08 sumneko

有点像 css 的 !important

ChouUn avatar Aug 16 '24 09:08 ChouUn