Skip to content

Improvements to variable arguments and returns #1207

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

Closed
sumneko opened this issue Jun 14, 2022 Discussed in #1206 · 26 comments
Closed

Improvements to variable arguments and returns #1207

sumneko opened this issue Jun 14, 2022 Discussed in #1206 · 26 comments
Labels
enhancement New feature or request feat/LuaCats Annotations Related to Lua Language Server Annotations (LuaCats)
Milestone

Comments

@sumneko
Copy link
Collaborator

sumneko commented Jun 14, 2022

Discussed in #1206

Originally posted by carsakiller June 14, 2022

Introduction

Currently, there are multiple ways to document a variable number of parameters passed to a function but I think they are causing confusion and conflicting with each other.

There is also only one way to document a variable number of return values, which I think is even more problematic.

Problems

  1. @vararg vs @param
  2. @return varargs
  3. Ambiguous enums

@vararg vs @param

You have two options when documenting a variable number of parameters passed to a function:

Use @vararg

This means it can have a type, but no description

Example
---Concat strings together
---@vararg string The strings to concat
function concat(...) end

image

Use @param

This means it can have a name (...), type, and description

Example
---Concat strings together
---@param ... string The strings to concat
function concat(...) end

image

The obvious choice is to use @param - which makes @vararg completely useless.

@return varargs

When returning a variable number of values from a function, there are two options:

Follow standard usage

The standard usage of @return is @return <type> [name] [description]. However, this results in the name ... not being parsed correctly.

Example
---@alias color
---| '"red"'
---| '"blue"'

---@alias hex string A hexadecimal value representing a color e.g. #ffffff

---Convert color name to hex value
---@param ... color The names of the colors to convert
---@return hex ... The colors converted to hexadecimal
function colorToHex(...) 
    return ...
end

image

This doesn't look great and it only keeps track of the type for the first return value:
image
image

Inverted usage

Using @return <name> <type> <description> is a terrible idea... but it does at least parse ... correctly...

Example
---@alias color
---| '"red"'
---| '"blue"'

---@alias hex string A hexadecimal value representing a color e.g. #ffffff

---Convert color name to hex value
---@param ... color The names of the colors to convert
---@return ... hex The colors converted to hexadecimal
function colorToHex(...)
    return ...
end

image

This comes at the cost of completely ruining any kind of type tracking:
image

Ambiguous enums

Should your function both receive and return a variable number of values and it uses an enum, it becomes a little ambiguous as to what the provided documentation is referring to.
image
I am aware that these values are also listed at the top next to the parameter, but (and especially with how variable returns are currently displayed) it appears that the enum at the bottom is also referring to the return values.

Proposed Solutions

Solution to @vararg vs @param

I think that @vararg should just be removed in favor of using @param as it is able to do a better job at documenting variable parameters. It is confusing to have a tag for documenting parameters but then another tag to document multiple parameters and I feel it is more intuitive to only use @param, like so:

---@param ... string The strings to concat together
function concat(...) end

Solution to @return varargs

The current syntax of @return <type> [name] [description] should be kept and ... should be a valid return name that gets parsed as any other name does. This should result in:
image

Solution to Ambiguous enums

As for this problem, I am not sure how it can best be solved, please do offer some ideas.

Maybe the enum reference appears immediately after the param/return that references it. This would probably require splitting the annotation that the user is providing in order to insert this info between @ tags. This would definitely make it clear but it could lead to lots of spacing between params/returns should the enum have a lot of options.
image
This seems like it could cause lots of problems.

Another possible solution could be to automatically label the parameter vararg ... (param) and the return one ... (return) just so they cannot be confused.

@sumneko sumneko added enhancement New feature or request feat/LuaCats Annotations Related to Lua Language Server Annotations (LuaCats) labels Jun 14, 2022
@carsakiller
Copy link
Collaborator

carsakiller commented Jun 14, 2022

Hello 👋

For "fixing" the @vararg vs @param problem, keeping it for legacy reasons makes sense but it should then be better documented as such. As of right now, it is confusing.

I can edit the wiki to make it more clear that @param ... <type> [description] should be used for varargs, rather than the @vararg tag.

Is it possible to add descriptions for the tags here:
image
Like how parameter enums have this little nested window appear:
image
I have no problem writing descriptions for these, I am just not sure where to get started with the source code.

EDIT: I modified the wiki to make it more clear

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 14, 2022

I have added a sample, you can edit the locale/en-us/script.lua to add more.
Thank you for your help!

@carsakiller
Copy link
Collaborator

No problem!

How can I test my changes? I am not familiar with how to get set up for development for this project. I can't seem to find anything about development and testing on the wiki.

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 14, 2022

The fastest way is replacing your local files and restart server (F1 -> Reload Window in VSCode)

@carsakiller
Copy link
Collaborator

What is the purpose of the @nodiscard tag?

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 14, 2022

What is the purpose of the @nodiscard tag?

io.open('xxx') -- no
f = io.open('xxx') -- ok

@carsakiller
Copy link
Collaborator

Would a better name be @noignore?

It would then make more sense if the warning said:

The return values of this function cannot be ignored

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 14, 2022

Would a better name be @noignore?

It would then make more sense if the warning said:

The return values of this function cannot be ignored

This name is come from C++

@carsakiller
Copy link
Collaborator

carsakiller commented Jun 14, 2022

A few more questions:

  1. What effect does @async have? I don't see any in the context window. Nevermind, I see it now 🥴
  2. Does @see work? I can't seem to get it to appear in the context window.
  3. What does @cast do? Also doesn't seem to function. I am assuming it functions as described in Type cast and type narrow proposal #1030

@carsakiller
Copy link
Collaborator

Opened a PR to document all the tags and to better document the difference between @vararg and @param.

I guess next is to work on fixing variable return values. I am not sure where to get started with fixing the parsing and type checking for that...

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

2. Does @see work? I can't seem to get it to appear in the context window.

---@see iolib or ---@see iolib#open .
I don't know why using #

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

The current syntax of @return <type> [name] [description] should be kept and ... should be a valid return name that gets parsed as any other name does. This should result in:

Maybe we can only add a new syntax, such as ---@return ...:boolean [name] [description]

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

I guess next is to work on fixing variable return values. I am not sure where to get started with fixing the parsing and type checking for that...

"script\core\hover\description.lua:147" buildEnumChunk

@sumneko sumneko mentioned this issue Jun 15, 2022
@carsakiller
Copy link
Collaborator

  1. Does @see work? I can't seem to get it to appear in the context window.

---@see iolib or ---@see iolib#open . I don't know why using #

I still can't seem to get it to function. Can it only refer to libraries? Do you have some example code where it is used?

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

---@class myClass
local mt = {}

function mt:open()
    
end

---@see myClass#open
function mt:close()
    
end

@carsakiller
Copy link
Collaborator

It is able to pick up the comment I leave, but there is no mention of @see

image

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

I don't know the other usages of @see. I have never used it, and no one has complained.

@carsakiller
Copy link
Collaborator

carsakiller commented Jun 15, 2022

That's fair. I thought it would appear at the bottom so it could be used for linking to documentation or to other files.

This can be achieved with markdown
image

I just thought it would be highlighted and could be used for something like this where you link to another local file. (This errors as it can't be found - I guess it needs an absolute path)
image

I don't think it is really necessary though

EDIT: How I thought it would look:
image

@FlashHit
Copy link

Ye it would be nice to be able to refer to other functions or classes in the project.

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 15, 2022

It seems need to convert path to uri

local furi = require 'file-uri'
local ws   = require 'workspace'

local fullPath = ws.getAbsolutePath(fileUri, seePath)
if fullPath then
    local fullUri = furi.encode(fullPath)
end

@carsakiller
Copy link
Collaborator

Yeah that sounds like how it would need to be done

@sumneko
Copy link
Collaborator Author

sumneko commented Jun 21, 2022

  • ---@return ...:boolean [name] [description]
  • ---@return ...boolean [name] [description]
  • ---@return boolean... [name] [description]

@carsakiller
Copy link
Collaborator

carsakiller commented Jun 21, 2022

I think the third option would be the most intuitive

Is it not possible to fix the parsing so that the current syntax works?
---@return boolean ... Multiple booleans

I haven't had much time lately to take a deep dive of the language server and attempt fixing this, sorry

@carsakiller
Copy link
Collaborator

I can see a use case where you would like to name the variable returns, using one of the syntaxes you listed:

---@return string... nicknames The user's nicknames
function getNicknames() end

But I also think the name could be unnecessary as usually the returns are all very similar:

---@return string ... The types of thing
function getTypes() end

This way, the name is ... as the function name and description already let you know that the returned strings are all types. Or you could alias the type so instead of string you could do thingType.

sumneko added a commit that referenced this issue Jun 21, 2022
@flrgh
Copy link
Contributor

flrgh commented Jun 21, 2022

I agree w/ @carsakiller on supporting the standard format, where ... looks like a regular annotation name but gets some special treatment under the hood:

--- multiplies each number by a given factor
---
---@param c integer # the coefficient/factor 
---@param ... integer # numbers to be multiplied by `c`
---@return integer ...
function multiply(c, ...) end

I do think the : separator looks a little clunky in the call/return signature though:

function multiply(c: integer, ...: integer)
  -> ...: integer

Maybe the language server could handle this as a special case and drop the :. In my opinion, the following is a little more readable:

function multiply(c: integer, ...integer)
  -> ...integer

(this is a very small detail though)

@sumneko sumneko added this to the 3.4.0 milestone Jun 24, 2022
sumneko added a commit that referenced this issue Jun 25, 2022
sumneko added a commit that referenced this issue Jun 25, 2022
return names and parentheses can be used in `DocFunction`
sumneko added a commit that referenced this issue Jun 25, 2022
displayed in enums of hover
@sumneko
Copy link
Collaborator Author

sumneko commented Jun 25, 2022

It's all done except ---@see, please open a new issue about ---@see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feat/LuaCats Annotations Related to Lua Language Server Annotations (LuaCats)
Projects
None yet
Development

No branches or pull requests

4 participants