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

Remove all optional positions from the checker #144

Conversation

PatrickLaflamme
Copy link
Contributor

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
Copy link
Owner

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.

@kaleidawave kaleidawave added the checking Issues around checking label May 20, 2024
@PatrickLaflamme PatrickLaflamme force-pushed the fix/remove-optional-position-from-checker branch from cf9c2cc to d3d6823 Compare May 26, 2024 01:06
@PatrickLaflamme
Copy link
Contributor Author

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.

@PatrickLaflamme PatrickLaflamme force-pushed the fix/remove-optional-position-from-checker branch 2 times, most recently from 8b0293f to eab2d57 Compare May 26, 2024 01:54
@kaleidawave
Copy link
Owner

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

remove last vestiges of Option<SpanWithSource> from the checker

update cache

fix validity check failure

clippy fixes
@PatrickLaflamme PatrickLaflamme force-pushed the fix/remove-optional-position-from-checker branch from eab2d57 to a2a192a Compare May 26, 2024 16:26
@kaleidawave
Copy link
Owner

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).

@kaleidawave kaleidawave merged commit e1a7b5e into kaleidawave:main May 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checking Issues around checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants