Skip to content

feat: treat array index as nil-able #371

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

Merged
merged 2 commits into from
Apr 18, 2025

Conversation

ribru17
Copy link
Contributor

@ribru17 ribru17 commented Apr 17, 2025

Expressions like this:

---@type integer[]
local arr = {1, 2, 3}
return arr[2]

should be considered as type integer|nil, since array accesses can always potentially return nil. This commit marks those expressions as such.

Related to #248

Expressions like this:

```lua
---@type integer[]
local arr = {1, 2, 3}
return arr[2]
```

should be considered as type `integer|nil`, since array accesses can
always *potentially* return `nil`. This commit marks those expressions
as such.
Comment on lines 669 to 674
return Ok(TypeOps::Union.apply(base, &LuaType::Nil));
} else if member_key.is_expr() {
let expr = member_key.get_expr().ok_or(InferFailReason::None)?;
let expr_type = infer_expr(db, cache, expr.clone())?;
if check_type_compact(db, &LuaType::Number, &expr_type).is_ok() {
return Ok(base.clone());
return Ok(TypeOps::Union.apply(base, &LuaType::Nil));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming this is also necessary?

@ribru17
Copy link
Contributor Author

ribru17 commented Apr 17, 2025

cc @lewis6991, will this be too annoying in practice?

@CppCXY CppCXY requested a review from Copilot April 17, 2025 06:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@CppCXY
Copy link
Member

CppCXY commented Apr 17, 2025

I think we need a configuration for strictly indexing arrays, because I noticed that even in Rust, array access operations do not return Option, and other languages, including TypeScript, do not return T | null.

@lewis6991
Copy link
Collaborator

#248

@ribru17
Copy link
Contributor Author

ribru17 commented Apr 17, 2025

I've gated this change behind a config option, defaulting to true. My reasoning for having this be opt-out is that there exists pairs and ipairs for safe array iteration, and indexing arrays manually (important: talking about arrays specifically, not a tuple like [integer, integer]) is not common enough that it warrants allowing this extra unsafety, in my opinion. Fine with having this as opt-in if there is another argument, of course

@CppCXY CppCXY merged commit 9413d94 into EmmyLuaLs:main Apr 18, 2025
14 checks passed
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

Successfully merging this pull request may close these issues.

3 participants