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

Remove all optional positions from the checker #144

Closed PatrickLaflamme closed 1 month ago

PatrickLaflamme commented 1 month ago

As mentioned here, we want to remove all optional SpanWithSource references and make positional data required across the checker. This PR attempts to accomplish this.

@kaleidawave - still getting comfortable with the codebase and test framework. Might need a bit of help verifying this change doesn't have any unintended side-effects.

kaleidawave commented 1 month ago

Nice! This should help prevent against future cases that use this.

With regards to testing positions of diagnostics (errors, warnings etc), there isn't any testing to where they are emitted. I could add something to specification.md but that would mean that tests would have to be written more cumbersomely

- Invalid parameter (line 2, column 4 to column 10)

As long as tests in the CI are green then it should be good. I have looked at the position logic and there isn't too much that can go wrong.


This looks good to merge. However: I don't know if you want to have a look at whether Event::Conditionally and Event::CreateObject could have their position from Option<SpanWithSource> to SpanWithSource

https://github.com/PatrickLaflamme/ezno/blob/1ca23d4970ac693f231a344075cbc22ac66265cf/checker/src/events/mod.rs#L93

https://github.com/PatrickLaflamme/ezno/blob/1ca23d4970ac693f231a344075cbc22ac66265cf/checker/src/events/mod.rs#L126

They might be hard to add positions to. For example: I know that every function x() {} starts with a side effect of Event::Condition { condition: *called with new*, truthy: [Event::CreateObject { *** }] } to support calling new on a function. In those cases there isn't an AST position to map to. You could try and use the function position?

Alternatively if there really isn't a possible you could as last resort SpanWithSource::NULL and put a big comment about why there isn't a accessible position to mark this with.

PatrickLaflamme commented 1 month ago

I ended up using the function position and it seems to have worked well.

The one piece of weirdness I found was here, which felt super un-ergonomic, but given we couldn't open the file, there isn't really much else we could do. In the end I figure that is an extreme corner case where an entry point is defined in the js configs but the file doesn't exist or the user doesn't have permissions to read it.

kaleidawave commented 1 month ago

Awesome, I will have at look at that edge case later. Except from that looks ready to merge

kaleidawave commented 1 month ago

Thanks for the changes that should help with things I want to do with events in the future.

I have reverted back to Option for the CannotFindFile error. While most of the time it has a position pointing to a import statement that it can't read from, there is an edge case where it is thrown when the input path to the checker is non existent. It works ATM by creating a diagnostic::global rather than one with a position so should be all fine (no unwraps).