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

if not x then return end should exclude falsy types after the block #356

Closed
kirdaaa opened this issue Feb 11, 2022 · 7 comments · Fixed by #865
Closed

if not x then return end should exclude falsy types after the block #356

kirdaaa opened this issue Feb 11, 2022 · 7 comments · Fixed by #865
Assignees
Labels
enhancement New feature or request roadmap This is on our long-term roadmap

Comments

@kirdaaa
Copy link

kirdaaa commented Feb 11, 2022

local function foo(x: string?): string
    if not x then
        return "nil"
    end

    return "\"" .. x .. "\""
end

Code like this is correct, after the if statement, x cannot be falsy, but analyzer still warns that concatenation is not allowed:

TypeError: Type 'string?' could not be converted into 'number | string'

@kirdaaa kirdaaa added the bug Something isn't working label Feb 11, 2022
@zeux
Copy link
Collaborator

zeux commented Feb 11, 2022

Yup, this is a current limitation of refinements - we don't implement control flow analysis so we don't propagate the inverse of the if condition to the code after the if..then..else statement. This is on our roadmap to solve this year - will keep this bug open for now. The current workarounds include either putting the rest of the code in the else block, or redundantly asserting assert(x) after the block.

@LoganDark
Copy link
Contributor

redundantly asserting assert(x) after the block.

I find myself doing this all the time now. Thankfully since assert(true) can be fastcalled, it should be super fast to execute.

@hoontee
Copy link

hoontee commented Feb 26, 2023

Is this still on the roadmap? Myself and my team are getting this type error a bunch.

@alexmccord
Copy link
Contributor

Yes, it's still on the roadmap. The current focus is on the new type checking architecture, but implementing control flow analysis is very high on my priority queue.

@perkcalamity
Copy link

Super happy to see that this is on the road map! I utilize guard clauses quite a lot to reduce nested if blocks. I use the assert(var) workaround for the time being. Love, love, loveee Luau!

@ambergamefam
Copy link

ambergamefam commented Mar 1, 2023

Glad this is being addressed.

Issues such as this one (as well as bugs like #533, #830 and #535) are affecting usability for Luau to the point that nobody in my institution wants to adopt Luau or strict mode in any of their code, and I have a hard time arguing against them or recommending that people use types even though I always personally write typesafe/strict code. This also affects engineering decisions made in my company, such as what tooling we use; our engineers then tend to still opt for using outdated tools that undermine the ability to use Luau features / typesafe code at all.
So I just want to plead that fixing bugs with type refinement should be TOP priority.

Thanks for your hard work 🙏

@alexmccord
Copy link
Contributor

alexmccord commented Mar 10, 2023

I was able to implement a smaller version of control flow analysis to help alleviate this pain point. It'll make it into the next Luau release cycle.

vegorov-rbx added a commit that referenced this issue Mar 17, 2023
* A small subset of control-flow refinements have been added to
recognize type options that are unreachable after a
conditional/unconditional code block. (Fixes
#356).

Some examples:
```lua
local function f(x: string?)
    if not x then return end

    -- x is 'string' here
end
```
Throwing calls like `error` or `assert(false)` instead of 'return' are
also recognized.
Existing complex refinements like type/typeof and tagged union checks
are expected to work, among others.

To enable this feature, `LuauTinyControlFlowAnalysis` exclusion has to
be removed from `ExperimentalFlags.h`.
If will become enabled unconditionally in the near future.

* Linter has been integrated into the typechecker analysis so that
type-aware lint warnings can work in any mode
`Frontend::lint` methods were deprecated, `Frontend::check` has to be
used instead with `runLintChecks` option set.
Resulting lint warning are located inside `CheckResult`.

* Fixed large performance drop and increased memory consumption when
array is filled at an offset (Fixes
#590)
* Part of [Type error suppression
RFC](https://github.com/Roblox/luau/blob/master/rfcs/type-error-suppression.md)
was implemented making subtyping checks with `any` type transitive.

---
In our work on the new type-solver:
* `--!nocheck` mode no longer reports type errors
* New solver will not be used for `--!nonstrict` modules until all
issues with strict mode typechecking are fixed
* Added control-flow aware type refinements mentioned earlier

In native code generation:
* `LOP_NAMECALL` has been translated to IR
* `type` and `typeof` builtin fastcalls have been translated to
IR/assembly
* Additional steps were taken towards arm64 support
RomanKhafizianov pushed a commit to RomanKhafizianov/luau that referenced this issue Nov 27, 2023
* A small subset of control-flow refinements have been added to
recognize type options that are unreachable after a
conditional/unconditional code block. (Fixes
luau-lang/luau#356).

Some examples:
```lua
local function f(x: string?)
    if not x then return end

    -- x is 'string' here
end
```
Throwing calls like `error` or `assert(false)` instead of 'return' are
also recognized.
Existing complex refinements like type/typeof and tagged union checks
are expected to work, among others.

To enable this feature, `LuauTinyControlFlowAnalysis` exclusion has to
be removed from `ExperimentalFlags.h`.
If will become enabled unconditionally in the near future.

* Linter has been integrated into the typechecker analysis so that
type-aware lint warnings can work in any mode
`Frontend::lint` methods were deprecated, `Frontend::check` has to be
used instead with `runLintChecks` option set.
Resulting lint warning are located inside `CheckResult`.

* Fixed large performance drop and increased memory consumption when
array is filled at an offset (Fixes
luau-lang/luau#590)
* Part of [Type error suppression
RFC](https://github.com/Roblox/luau/blob/master/rfcs/type-error-suppression.md)
was implemented making subtyping checks with `any` type transitive.

---
In our work on the new type-solver:
* `--!nocheck` mode no longer reports type errors
* New solver will not be used for `--!nonstrict` modules until all
issues with strict mode typechecking are fixed
* Added control-flow aware type refinements mentioned earlier

In native code generation:
* `LOP_NAMECALL` has been translated to IR
* `type` and `typeof` builtin fastcalls have been translated to
IR/assembly
* Additional steps were taken towards arm64 support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request roadmap This is on our long-term roadmap
Development

Successfully merging a pull request may close this issue.

7 participants