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

implement setters support as described in #39 #138

Closed PatrickLaflamme closed 1 month ago

PatrickLaflamme commented 1 month ago

This PR implements setters support as described in #39.

Note: In run_setter_on_object, we do an expect on the optional setter_position field, as the FunctionType.call method requires a SpanWithSource. I looked into refactoring to change the setter_position from an Option but it looked like it'd have wide consequences. @kaleidawave - would love your thoughts on this one.

kaleidawave commented 1 month ago

Awesome! Yep I believe the event positions were added in #37. There were some difficult cases so just decided it to make it Option and use None in those cases. If it's too difficult at the moment then fine to skip.

Tests look good and are passing. Will double check and merge when I back week after next!

PatrickLaflamme commented 1 month ago

Makes sense. Simulating the function call currently does not accept an Option for the SpanWithSource. I chose to fail closed here, panicking if a setter had no SpanWithSource because failing open would mean we would silently fail to evaluate the side effects of the setter function.

If I understood your comment correctly, are you suggesting we update the FunctionType.call to accept an Option?

kaleidawave commented 1 month ago

If I understood your comment correctly, are you suggesting we update the FunctionType.call to accept an Option?

Actually the opposite, I think that Event::Setters position field should be SpanWithSource. I think at the moment there are a few creations that don't have access to a position (but could!) such as Environment::delete_property and some of the class field registration. Those kind of set events won't trigger a setter so I think the setter_position.expect("Setter position is required!"); is fine for the moment (and the tests still run). If you would like to improve that so that every event has a SpanWithSource position then let me know and I can look into it further and write up a issue explaining the cases :)

kaleidawave commented 1 month ago

As for the rest, PR is great! Thanks for the contribution + adding tests for it!

kaleidawave commented 1 month ago

Hey, again just added some additional tests. One thing that was missing in the PR is reporting the assignment type mismatch with the assigned value. It was being checked in .call (by regular function parameter checking), just wasn't being surfaced as the result was stored into let _ =. Not to worry have added some basic typing report it.

https://github.com/kaleidawave/ezno/blob/main/checker/specification/specification.md#setter-assignment-type

https://github.com/kaleidawave/ezno/blob/c4ada6d3e52e0fad5a4eadcc513b3bfd31d2d0ed/checker/src/types/properties.rs#L743