kaleidawave / ezno

A JavaScript compiler and TypeScript checker written in Rust with a focus on static analysis and runtime performance
https://kaleidawave.github.io/posts/introducing-ezno/
MIT License
2.3k stars 42 forks source link

Bug: Unsound checking of function calls with array parameters #134

Closed CharlesTaylor7 closed 2 months ago

CharlesTaylor7 commented 2 months ago

There's some bugginess with functions that declare array parameters.

Bugs are listed with a comment ❌ .

function callWithNum(a: number) {}
function callWithArray(a: number[]) {}
function callWithArrays(...args: number[][]) {}

callWithNum(1) // ✅ type checks
callWithNum([1]) // ✅ does not type check

callWithArray(1) // ❌ type checks but shouldn't
callWithArray([1]) // ✅ type checks

callWithArrays(1, 2, 3) // ❌ type checks but shouldn't
callWithArrays([1], [2], [3]) // ✅ type checks

I'm not sure what's going on here. I initially thought the bug had to do with just the implementation of rest parameter constraint checking, but realized the issue applies to regular parameters too.

kaleidawave commented 2 months ago

Ooof looks like a issue in subtyping logic. The type_is_subtype function seems to be returning that 3 has all the properties to be passed as a Array<number>, which isn't true (3 does not have a length property for example so isn't the same as Array). The issue is in type_is_subtype as it also affects other places where subtyping is checked (there are some differences specified by SubTypeBehavior and modes like BasicEquality but are only to do with inferring function generics, rarely the is subtype decision).

image

kaleidawave commented 2 months ago

Running with $Env:EZNO_DEBUG=1 environment variable prints some tracing information I have manually added using the notify! macro.

image

I can see that subtyping is run 4 times for the above snippet. Each time checking whether structure generic (SG) on Array with T (TypeId(11)) = number (TypeId(4) is a supertype of literal number type 3.

I also see Here being logged from src\types\subtyping.rs:551 directly after. Hmm think I have found the culprit

https://github.com/kaleidawave/ezno/blob/24e35b98da5a6c705be3cce7eb56dc17bfcb32a4/checker/src/types/subtyping.rs#L551-L552

I think when I added that code I wasn't quite sure what should happen on that branch and so just default to saying it was fine (you have found a good case where it definitely is not).

Just modified it to return SubTypeResult::IsNotSubType(NonEqualityReason::Mismatch) instead (as well as on the line a few above) and still passing all the tests. Will put in a PR to fix tomorrow!

Aside this should have been caught but it seems isn't currently tested in specification.md. Should work out where to and how to test...? Or maybe I should make a new unit test specifically for type to type subtyping. Would you be interesting on working on something like that?

kaleidawave commented 2 months ago

Fixed in #135 (using above process). Not the best test but it now exists: https://github.com/kaleidawave/ezno/blob/main/checker/specification/specification.md#array-property-checking