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

[STAL-1960] Address various bugs #408

Merged
merged 6 commits into from
Jun 26, 2024
Merged

[STAL-1960] Address various bugs #408

merged 6 commits into from
Jun 26, 2024

Conversation

jasonforal
Copy link
Collaborator

@jasonforal jasonforal commented Jun 24, 2024

What problem are you trying to solve?

Fix various bugs

What is your solution?

Pass JsRuntime into test helper function
(Sneaking this in) We need to pass in a reference to JsRuntime so we can later inspect it in tests.

Ensure the QueryMatch and Violation bridges are wiped clean, even after an execution error."
We previously returned early via ? on a scoped execution. This led to bridges getting in an inconsistent state after an error. For example, a script might have mutated a bridge by successfully pushing a Violation to it, and then it threw an error. This would've left a violation on the bridge, which would've been drained by the next successful execution, leading to unexpected results.

Update file context from set_file_context, not automatically from set_root_context.
There was a "bug" here that caused the go module query code to run for every file, regardless of whether it was a go file or not. I'm refactoring the bridges to have less logic within their individual functions, relying instead on the JsRuntime to trigger the necessary logic. This implicitly fixes the aforementioned bug.

Filter results from the tree_sitter::QueryCursor that have no captures. Additionally, add guard to return early
A tree_sitter::QueryCursor can return a vec of empty vecs (e.g. [ [], [], [], [] ] // ...), which would cause a v8 execution for each empty vec. This adds two guards against this and a test to ensure it can't happen.

Use Arc to pass around tree_sitter::Tree
It turns out when a tree_sitter::Tree is cloned, it doesn't point to the same root node as the tree being cloned. We need to use a reference counting pointer to get the behavior we desire (detecting effective equivalence of two trees)

Clear the TsNodeBridge when the tree_sitter::Tree changes on the RootContext
There was no bug in analysis behavior, however, before this commit, we were keeping all nodes ever serialized in the TsNodeBridge, meaning they wouldn't be garbage-collected. This clears the bridge whenever the tree changes.

Alternatives considered

What the reviewer should know

  • In my opinion, these are small enough to be able to be lumped into a single PR like this, but I can separate them if you prefer.

@jasonforal jasonforal requested a review from juli1 June 24, 2024 19:06
@jasonforal
Copy link
Collaborator Author

jasonforal commented Jun 25, 2024

Pushing one extra bug fix since this PR is already tackling multiple

@jasonforal jasonforal merged commit d4c1a68 into dev Jun 26, 2024
62 checks passed
@jasonforal jasonforal deleted the jf/STAL-1960_misc branch June 26, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants