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

Fix crash for overlapping injections #7621

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Jul 13, 2023

Fixes #7434
fixes #3816
fixes nix-community/tree-sitter-nix#39
fixes #7578
fixes #3283
fixes #7615

The crash was caused by creating multiple overlapping injections (one for bash and another one for the shebang). Having two overlapping injections in the same layer doesn't make much sense. Therefore, I added a simple check that skips any injection that intersects with a previous one.

This works because queries are visited in ascending order (start never decreases). However, we had two queries for injections: one for combined injections and normal injections. That meant that overlaps between combined and normal injections weren't caught which could lead to similar bugs.

To fix this I would have to sort injections which would be inefficient for large files with many injections Luckily it was fairly easy to merge the queries for combined and injection queries. This is also a small optimization as it means we run one less query (on the entire file) during parsing.

Marked for next since this crash is a regression (its technically an existing bub but normally we never have overlapping injections)

@pascalkuthe pascalkuthe added this to the next milestone Jul 13, 2023
@pascalkuthe pascalkuthe added C-bug Category: This is a bug A-tree-sitter Area: Tree-sitter E-medium Call for participation: Experience needed to fix: Medium / intermediate A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jul 13, 2023
@pascalkuthe pascalkuthe force-pushed the fix_overlapping_injections branch from 89596f9 to 209814f Compare July 13, 2023 23:11
@pascalkuthe pascalkuthe force-pushed the fix_overlapping_injections branch from 209814f to 11ac7b9 Compare July 13, 2023 23:21
@gabydd
Copy link
Member

gabydd commented Jul 14, 2023

My laptop currently doesnt have internet so I can't test this pr out but would this also fix #3816

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Jul 14, 2023

I haven't tested this and wknt get a chance fir a whike but assuming your comments there are accurate then this would ended fix that bug too. You werent too far off there with your fix. This essentially does the same thing as deduplication but also handles cases where ranges overlap (instead of being exactly the same) and handles all overlaps (not just thise of a singke combined injection). I just wasnt aware if that issue/your comments there :D

If you could confirm once you habe time to test we can links that issue to this PR

@pascalkuthe
Copy link
Member Author

I suspect there are more issues that are fixed by this @the-mikedavis mentioned he wanted to test if this fixes some issues

@gabydd
Copy link
Member

gabydd commented Jul 14, 2023

I can confirm this fixes #3816 and fixes nix-community/tree-sitter-nix#39 which was the same issue

and it also fixes #7578

edit #3283 as well
and #7615

@pascalkuthe
Copy link
Member Author

Thanks for going trough all those issues! That's more than I expected

In the past we used two separate queries for combined and normal injections. There was no real reason for this (except historical/slightly easier implementation). Instead, we now use a single query and simply check if an injection corresponds to a combined injection or not.
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a bit of time to properly review, but it's great that a single fix closes so many issues! Thanks @gabydd for combing through the issue tracker and retesting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements A-tree-sitter Area: Tree-sitter C-bug Category: This is a bug E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
3 participants