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

Attempt to implement generic referencing

Open sewbacca opened this issue 3 years ago • 8 comments

It is an attempt to implement generic references inside strings. e.g.


---@generic T
---@param f T
local function test(f)

end

---@class Test

f "Test" -- This is a reference to class 'Test'

I am not really interested in the feature, as a use case has yet to arise (though I could imagine it arising), it is mainly an attempt to understand the language server better. And especially because I am modifying the source data inside getRefs, I wouldn't recommend blind merging. Any feedback would gladly be appretiated. I am trying my best to understand this awesome project :)

sewbacca avatar Jun 11 '22 21:06 sewbacca

Thank you for your PR. I have read your implementation. The overall idea is OK, but there are still many boundary conditions that need to be handled to pass the unit test, such as: https://github.com/sumneko/lua-language-server/blob/2dc94335203311d87ecf501b34fa9b973b4b8cb0/test/references/common.lua#L62-L67

In addition, I suggest you implement unit tests first. For example, strings are expressed in many ways, but it seems that you only implement the expression of double quotation marks.

sumneko avatar Jun 12 '22 16:06 sumneko

Thanks for your reply. So it is okay to mark generic Arguments as genArg = true? It felt like a quick hack, but if the ast is generated over and over, maybe it doesn't pose a problem. https://github.com/sumneko/lua-language-server/blob/37cf09ad81a337e4378e39b61a8c52d65e45cfa7/script/vm/ref.lua#L169-L176

sewbacca avatar Jun 12 '22 16:06 sewbacca

Thanks for your reply. So it is okay to mark generic Arguments as genArg = true? It felt like a quick hack, but if the ast is generated over and over, maybe it doesn't pose a problem.

There is no problem caching data on the ast itself, because whether it is genArg or not is determined by the current file itself. The modification of other files cannot be affected, and the ast will be recompiled when the current file is modified.

However, after careful consideration, it is not necessary to add the genArg attribute.

  1. Implement the definition witch supports finding ---@class TEST by f "TEST". This feature should not need to modify the ast.
  2. When searching reference of ---@class TEST, scan all files and use guide.eachSourceType(root, 'string') to find out all strings "TEST", then just use vm.getDefs for them to check if they are defined by ---@class TEST. You should call searchInAllFiles for it.
  3. Then vm.getRefs can found out correct results, so you don't need to check it again in rename

EDIT: I'm not right. The function caller and the function definer can be in different files. The genArg is defined at the call, but the semantics are determined by the definer. Therefore, you cannot modify the ast at the caller.

You can use vm.compileNode(source) - > node, and then add attributes on node. The node will be reset when any state changes.

sumneko avatar Jun 12 '22 16:06 sumneko

Hello again, now I've got some time on my hands to look into this subject again. Some questions occured to me: About the syntax in the tests: Do I understand correctly, that:

TEST [[
t[<~'a'~>] = 1
print(t.<!a!>)
]]

means, if I want to get the reference of t['a'] I expect to find it at print(t.a)?

And what does the call to findSource mean (in core.reference)? I assume it trys to find out, what the example is, by which to find all references?

Edit: What does the defMap parameter mean in the switch inside vm.ref?

sewbacca avatar Jul 23 '22 11:07 sewbacca

means, if I want to get the reference of t['a'] I expect to find it at print(t.a)?

<?xxx?> is request positions and <!xxx!> is response positions. <~~> is equal to <!<?xxx?>!>.

And what does the call to findSource mean (in core.reference)? I assume it trys to find out, what the example is, by which to find all references?

findSource is to convert position to parser.object ({line = 3, charater = 10} -> {type = 'getfield', ...})

What does the defMap parameter mean in the switch inside vm.ref?

  1. find refercens of thisCat:getName()
  2. find definitions of thisCat:getName() -> animal:getName()
  3. save animal:getName() into defMap
  4. search all xxx.getName in the workspace.
  5. test if the definitions of each xxx.getName matchs the defMap, if yes, add them into reference results.

sumneko avatar Jul 23 '22 13:07 sumneko

So, I think I got it working, but I haven't tested it thoroughly (see unit tests). I wanted to play arround with it, but I forgot how to compile and launch the server in a seperate VSCode Window.

Changes: I added array searching functions in utility. I added vm.isArgumentGenericLiteral() in vm.generic. I modified defs, definition, refs and reference to find references and definitions, as you layed it out for me:

1. Implement the definition witch supports finding `---@class TEST` by `f "TEST"`. This feature should not need to modify the ast.

2. When searching reference of `---@class TEST`, scan all files and use `guide.eachSourceType(root, 'string')` to find out all strings `"TEST"`, then just use `vm.getDefs` for them to check if they are defined by `---@class TEST`. You should call `searchInAllFiles` for it.

3. Then `vm.getRefs` can found out correct results, so you don't need to check it again in `rename`

Though I think I do need to check if the given reference is a generic literal argument in rename (not with genArg stored inside the node though, but with vm.isArgumentGenericLiteral()), because in a debug session, when figuring out what to do, there were other references inside another test case, unrelated to what I did.

I copied the searchInAllFiles into defs, I don't think this is a good design, because I would rather just put it somewhere else to import, but I didn't do that, because I don't even know if it is necessary or if there is another solution.

Any feedback would be appretiated.

sewbacca avatar Jul 23 '22 14:07 sewbacca

I have not had time recently, and it will take a few days to review.

sumneko avatar Jul 24 '22 13:07 sumneko

Please don't feel pressured by me, I appretiate that this repo is activly maintained by someone =)

sewbacca avatar Jul 24 '22 19:07 sewbacca

I am closing this pull request because I no longer require the proposed feature at this time. I'll leave the linked discussion here for reference, in case I or someone else comes back to it in the future.

sewbacca avatar Nov 04 '23 14:11 sewbacca