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

Type warning when calling os.time with a table #2475

Open
Caton27 opened this issue Jan 10, 2024 · 7 comments
Open

Type warning when calling os.time with a table #2475

Caton27 opened this issue Jan 10, 2024 · 7 comments
Labels
feat/type check Related to the type checking feature

Comments

@Caton27
Copy link

Caton27 commented Jan 10, 2024

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?

Type Checking

Expected Behaviour

No warning

Actual Behaviour

The following warning is appearing:

Cannot assign string|osdate to parameter osdateparam?.

  • string cannot match osdateparam?
  • Type string cannot match nil
  • Type string cannot match osdateparam

Reproduction steps

  1. Enter the following code
local tDate = os.date("*t", os.time())
-- tDate is a table, e.g. {year=2024, month=1, day=10, hour=13, min=2, sec=24, yday=10, wday=4, isdst=false}
local newTime = os.time(tDate)
  1. Linter warning appears in the problems tab

Additional Notes

No response

Log File

No response

@C3pa
Copy link
Contributor

C3pa commented Jan 10, 2024

Interesting, I can't reproduce this issue.

@firas-assaad
Copy link
Contributor

I can reproduce it. It's because of the fix for this issue: #2235

os.time should probably be updated to accept osdate as well.

@C3pa
Copy link
Contributor

C3pa commented Jan 13, 2024

I also narrowed it down to that commit. Maybe @carsakiller can tell us why the osdate param type didn't work and needed to be duplicated as osdateparam?

@firas-assaad
Copy link
Contributor

It didn't work because of the introduction of the missing field diagnostic. osdate fields returned by os.date won't have nil values, but you can pass a partial osdate to os.time. Simply updating osdate to make the fields optional would've made it difficult to work with the value returned from os.date (you'd have to check for nil), so I can understand why a separate type was needed.

@carsakiller
Copy link
Collaborator

@firas-assaad is correct. It was necessary to prevent excessive nil checking. This seems to be a similar problem, unfortunately. os.date can return a string or table (depending on the argument) but the server is unable to narrow the resulting type, leading to this warning that it could be a string.

I'm not sure if there is anything we can do with just the definitions to fix the string issue, as the return types of os.date are correct and the argument type of os.time is correct. We can't add a string as an option for os.time and we can't remove it as a return type of os.date either.

If we add osdate like @firas-assaad said, we can do the following:

local tDate = os.date("*t", os.time())
---@cast tDate -string
local newTime = os.time(tDate)

@carsakiller carsakiller added the feat/type check Related to the type checking feature label Jan 13, 2024
@firas-assaad
Copy link
Contributor

firas-assaad commented Jan 13, 2024

I'm fine with having to cast to remove the string type. The current logic makes sense: a function may return a string, so if you pass the value to another function that only accepts tables, you'll have to add annotations to express your intent.

Now it'd be nice if we could override function definitions to say that the more specific '*t' argument returns a table whereas any other string argument returns a string, but that's probably a different story.

@carsakiller
Copy link
Collaborator

That is the problem talked about pretty extensively in #1456.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat/type check Related to the type checking feature
Projects
None yet
Development

No branches or pull requests

4 participants