Skip to content

: are not typed checked properly #2842

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

Open
Rathoz opened this issue Sep 9, 2024 · 13 comments
Open

: are not typed checked properly #2842

Rathoz opened this issue Sep 9, 2024 · 13 comments

Comments

@Rathoz
Copy link
Contributor

Rathoz commented Sep 9, 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

Reproduction steps

---@param a number|string
function Foo.Bar(a)
	return a:lower() -- Should warn as the code will error on a number input
end

Additional Notes

#2708 (comment)

Log File

No response

@tomlau10
Copy link
Contributor

tomlau10 commented Sep 9, 2024

I think in the reproduction code snippet, that should be @param instead of @type 👀
=> ---@param a number|string

@ChrisKader
Copy link

Well, string.lower can actually take a number as the first parameter (string.lower(1)) will not result in an error.

---@param a number|string
function Foo.Bar(a)
	return a:char()
end

This will result in a warning.

@ChrisKader
Copy link

ChrisKader commented Oct 10, 2024

This is a bit more "verbose", but it is what I prefer.

---@param a number|string
function Foo.Bar(a)
    return (type(a) ~= "string" and tostring(a) or a):lower()
end

@Rathoz
Copy link
Contributor Author

Rathoz commented Oct 10, 2024

Whether or not string.lower() accepts a number or not should not be relevant here.
The point is there should be a type warning, as there will be a runtime error, if a numbers input is supplied. The point of the type checking is to minimize the risk of mistakes becoming runtime errors

@ChrisKader
Copy link

It is valid here as you are giving LuaLS the option of a being a string or a number. As such, it will not warn you since as one of the types you provided is valid (string).

While LuaLS helps in a lot of ways, this is one of those situations that it does not. If your function expects a certain type of variable, you should be doing runtime type checking (type(a) == "string") and not just assuming the variable type is correct because it "passed" LuaLS type checking.

string is the only "basic" type class in LuaLS that is extended by a library, and I assume the cause of, this "issue". If you are not doing any runtime type checking, though you should be, the best solution is probably to call string.lower(a) instead of assuming the variable is a string that has "stringlib" methods on it.

@tomlau10
Copy link
Contributor

it will not warn you since as one of the types you provided is valid (string).

Um... I think this statement contradicts with the a:char() example above. In a:char() case, one of the types provided is also valid (string), but luals can detect it and show warnings. So definitely one of the types provided is valid is not an argument for not showing warnings for the case a:lower().

not just assuming the variable type is correct because it "passed" LuaLS type checking.

You are right that luals cannot detect all the runtime errors, and we all know that. And that's the reason why issues exist: they are used to track these edge cases, letting maintainers know they exist, and hoping that they will be fixed by maintainers or contributors 😄 .

The edge case described in this issue

When luals tries to link a a:b() function call syntax (colon notation) to a Class.b(a) function definition (dot notation), it should make sure that all union types in a can be casted to Class, otherwise it should show errors. This is the missing piece 👀.

Furthermore, I know there is a config type.weakUnionCheck: https://luals.github.io/wiki/settings/#typeweakunioncheck
Maybe when this is set to true, then no checking for the edge case mentioned in this issue. Just like when turning on type.weakUnionCheck, the a:char() case will NOT show errors.

@ChrisKader
Copy link

Um... I think this statement contradicts with the a:char() example above. In a:char() case, one of the types provided is also valid (string), but luals can detect it and show warnings. So definitely one of the types provided is valid is not an argument for not showing warnings for the case a:lower().

It is valid here as you are giving LuaLS the option of a being a string or a number. As such, it will not warn you since as one of the types you provided is valid (string) and a valid parameter for the function being called.

@tomlau10
Copy link
Contributor

yeah~ Don't know why I overlooked just now 😕 and you are correct on that both string|number should not match the 1st param of string.char. But when I write:

---@number
local n
local s = string.char(n) --< this doesn't throw error?
  • number can be casted as integer??
  • seems another bug 🙈 ?

@ChrisKader
Copy link

---@type number
local n

local s = string.char(n) --< this doesn't throw error?

?

@ChrisKader
Copy link

numbers and integers are inter-changable assuming you have the setting toggled.

@tomlau10
Copy link
Contributor

tomlau10 commented Oct 10, 2024

numbers and integers are inter-changable assuming you have the setting toggled.

Ah~ I see that type.castNumberToInteger setting, and it is default to true. I haven't changed it so it is true in my case.

Now with that, how come a:char() case will throw error? 🤔
Since number can be casted to integer, so

  • a:char() => string.char(a)
  • => a has number type <=> integer type
  • => it also matches the 1st param of string.char?

By your a valid parameter for the function being called argument, luals should not throw error?
In the same wordings: it will (should) not warn you since as one of the types you provided is valid (number) and a valid parameter for the function being called.

@tomlau10
Copy link
Contributor

As such, it will not warn you since as one of the types you provided is valid (string) and a valid parameter for the function being called.

Here is an example snippet showing that LuaLS doesn't work this way: 👀

---@class A
local A

function A:test() end

---@class B
local B

---@type A|B
local c
c:test() -- param-type-mismatch: Cannot assign `A|B` to parameter `A`. - `B` cannot match `A` - Type `B` cannot match `A`
  • warning will be shown on the line c:test(), even though one of the types A can match A:test()
  • that's because when luals links c:test() to A:test() => A.test(A), it requires all union types (A|B) in c to match the 1st param type A, instead of just allowing one of the type matches
  • this strict checking behavior can be loosen by setting type.weakUnionCheck = true, where its purpose is exactly to allow one of the type matches. When you set it to true, then this code snippet will not throw param-type-mismatch error 😄

(off topic)
However the above code snippet will not throw warnings when I write local A = {} and local B = {}

---@class A
local A = {}

function A:test() end

---@class B
local B = {}

---@type A|B
local c
c:test() -- no warnings no matter `type.weakUnionCheck = true/false`
  • I guess there maybe something wrong when checking the casting between classes, if the class variable has a table value 😕
  • Maybe this should be tracked in another issue

@Rathoz
Copy link
Contributor Author

Rathoz commented Mar 11, 2025

Hey sorry, missed the replies

Not sure I fully follow. So going back to the basics

Let's take the example of

---@param b string
local function Baz(b)
end

---@param a number|string
function Foo.Bar(a)
	if type(a) == 'string' then
		return Baz(a) -- 100% fine
	end
	return Baz(a) -- Cannot assign `string|number` to parameter `string`. - `number` cannot match `string` - Type `number` cannot match `string` Lua Diagnostics.(param-type-mismatch)
end 

It requires that all types that haven't been narrowed are allowed

---@param a number|string
function Foo.Bar(a)
	local c
	c = string.lower(a) -- OK, string.lower() handles numbers and strings
	c = number.lower(a) -- Undefined global `number`
	c = integer.lower(a) -- Undefined global `integer`
	c = a.lower(a) -- Does not error, but should. As a number/integer is not a table (unlike a string)
	c = a:lower() -- Does not error, but should. 
end 

a:lower() is just sugar for a.lower(a) (not string.lower(a))
Here it does not check that all types are allowed

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

No branches or pull requests

3 participants