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(grit): fix matching multiple args #3797

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Sep 5, 2024

Summary

We had an issue with matching function arguments in Grit queries, that prevented metavariables from matching multiple arguments. In short, the metavariable was consumed by the JsFormalParameter and stored inside one of its slots. This wasn't a problem if you only tried to match a single argument with no other syntax, but if you wanted the metavariable to match anything more, it wouldn't work.

To fix it, I made sure JsMetavariable can be accepted in our grammar next to JsFormalParameter and updated the JS parser so it's accepted there.

I also extended our list handling in the bindings a bit to be more in line with how Grit's reference implementation does it. The change in GritBinding::singleton() seems it's actually necessary, but some of the others may not be. It's probably better this way, but we don't have the test coverage yet to say for sure 😅

Test Plan

Added a test case.

@github-actions github-actions bot added A-Linter Area: linter A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages labels Sep 5, 2024
Copy link

codspeed-hq bot commented Sep 5, 2024

CodSpeed Performance Report

Merging #3797 will degrade performances by 9.14%

Comparing arendjr:fix-matching-multiple-args (adfb11a) with main (3f1790e)

Summary

❌ 1 regressions
✅ 106 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main arendjr:fix-matching-multiple-args Change
db_17847247775464589309.json[cached] 12.9 ms 14.2 ms -9.14%

@arendjr arendjr merged commit b1d55ff into biomejs:main Sep 5, 2024
12 of 13 checks passed
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants