Skip to content

Diagnostics for unnecessary assert #3128

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Apr 5, 2025
Merged

Conversation

ribru17
Copy link
Contributor

@ribru17 ribru17 commented Mar 22, 2025

image

@ribru17 ribru17 force-pushed the unnecessary_assert branch 2 times, most recently from ccb359f to d062eb5 Compare March 22, 2025 01:23
@tomlau10
Copy link
Contributor

Judging from the screenshot, the message expression of type `boolean` is always truthy from assert(true) seems a bit misleading 😕 .

  • false is also a boolean type, but assert(false) won't cause problem

@ribru17
Copy link
Contributor Author

ribru17 commented Mar 22, 2025

Ah true, not sure why the printed type evaluates to boolean instead of true. I'll investigate

@ribru17 ribru17 force-pushed the unnecessary_assert branch from d062eb5 to c2cd754 Compare March 22, 2025 03:00
@ribru17
Copy link
Contributor Author

ribru17 commented Mar 22, 2025

Opted to just not print the expression type for now

@CppCXY
Copy link
Collaborator

CppCXY commented Mar 24, 2025

is it test for multi return function?

---@return string?, string?
local function f() end

assert(f())

@CppCXY
Copy link
Collaborator

CppCXY commented Mar 24, 2025

I think that if it is always falsy is also an unnecessary assertion. right?

assert(nil and 5) -- always falsy

@ribru17 ribru17 force-pushed the unnecessary_assert branch from c2cd754 to 1c285e0 Compare March 24, 2025 03:44
@ribru17
Copy link
Contributor Author

ribru17 commented Mar 24, 2025

I think that if it is always falsy is also an unnecessary assertion. right?

assert(nil and 5) -- always falsy

True, but this PR only handles unnecessary if true. I'll add that as well, but wanted to keep the scope small at first.

is it test for multi return function?

---@return string?, string?
local function f() end

assert(f())

Works here as well, I added a test

@sumneko sumneko merged commit 1c02832 into LuaLS:master Apr 5, 2025
11 checks passed
@sumneko
Copy link
Collaborator

sumneko commented Apr 5, 2025

Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants