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(lint/noBlankTarget): don't hang when applying the code fix #2687

Merged
merged 1 commit into from
May 3, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented May 2, 2024

Summary

Fix #2675

The code fix created a JSX string with a JSX_IDENT token.
inner_string_text is called to retrieve the content of the attribute.
inner_string_text removes quotes only when the token is a JSX_STRING_LITERAL.
Thus, on first run, the quotes were well removed, while on the following run, the quotes are not removed.
This led to a never fixed lint rule and thus, an infinite loop.

I also fixed a similar issue for noUselessFragments which was not observed yet.

To avoid this kind of issue we could:

  • Add debug_assert in node factories (this should be generated by the grammar) to guarantee that every leaf node has the correct token kind.
  • Avoid the infinite loop by setting a limit. We can expect to have some conflicting rules in the future.
    We should ensure that this doesn't lead to an infinite loop.

Test Plan

I manually tested the code.
We could add a test in biome_cli however I think it is not worth it because this could require to add a test for every rule that create jsx/js/css strings.

@Conaclos Conaclos marked this pull request as ready for review May 2, 2024 21:51
@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels May 2, 2024
@Conaclos Conaclos force-pushed the conaclos/noBlankTarget/2675 branch 2 times, most recently from d020ef0 to db69dcf Compare May 2, 2024 22:09
Copy link

codspeed-hq bot commented May 2, 2024

CodSpeed Performance Report

Merging #2687 will not alter performance

Comparing conaclos/noBlankTarget/2675 (db69dcf) with main (afa5004)

Summary

✅ 87 untouched benchmarks

@Conaclos Conaclos merged commit 43525a3 into main May 3, 2024
15 checks passed
@Conaclos Conaclos deleted the conaclos/noBlankTarget/2675 branch May 3, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Biome --apply Command Hangs Due to Incorrect rel Attribute Recommendation
1 participant