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

feat(lint): add new js lint: useTopLevelRegex #2794

Merged

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 9, 2024

Summary

This rule detects regex literals that are defined inside functions. This can result in performance problems if a function like this is called often:

function foo(someString) {
	return /[a-Z]*/.test(someString)
}

Instead, biome will now encourage users to place regex literals at the top level of a module.

const REGEX = /[a-Z]*/;

function foo(someString) {
	return REGEX.test(someString)
}

It's a little unclear to me if it would be preferable to implement this using a Visitor instead.

closes #2148

Test Plan

Added snapshot tests

cargo test -p biome_js_analyze use_top_level_regex

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog labels May 9, 2024
@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch 2 times, most recently from 30617fe to d8f298f Compare May 9, 2024 17:28
@dyc3 dyc3 marked this pull request as ready for review May 9, 2024 17:28
Copy link

codspeed-hq bot commented May 9, 2024

CodSpeed Performance Report

Merging #2794 will not alter performance

Comparing dyc3:05-09-feat_lint_add_new_js_lint_usetoplevelregex_ (c13ff04) with main (9c920a1)

Summary

✅ 99 untouched benchmarks

@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch from d8f298f to f2411cd Compare May 9, 2024 18:03
@dyc3

This comment was marked as resolved.

@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch 3 times, most recently from 02dd38f to 0822e10 Compare May 10, 2024 17:26
@dyc3 dyc3 requested a review from ematipico May 10, 2024 17:27
@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch 5 times, most recently from 0874f71 to 99c8a74 Compare May 13, 2024 11:52
@ematipico ematipico requested a review from Conaclos May 13, 2024 12:26
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

Calling test/exec have side effects when a regex is set with the y and/or g flag. I think we should ignore regexes if one of its flags is set.

@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch from 99c8a74 to 3b190fc Compare May 13, 2024 14:30
@dyc3
Copy link
Contributor Author

dyc3 commented May 13, 2024

Currently, we don't parse the regex expression and flags as separate syntax nodes, so resolving that would require either string parsing (which is probably an antipattern here) or updating the grammar and to add those nodes. I think that would be better to put in a separate PR or 2. I can open issues for those once this is merged.

@dyc3 dyc3 force-pushed the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch from 3b190fc to c13ff04 Compare May 14, 2024 16:54
@Conaclos
Copy link
Member

Currently, we don't parse the regex expression and flags as separate syntax nodes, so resolving that would require either string parsing (which is probably an antipattern here) or updating the grammar and to add those nodes. I think that would be better to put in a separate PR or 2. I can open issues for those once this is merged.

I remembered that we have a utility for that: you can call decompose() on a JsRegexLiteralExpression. This will return a pair that contains the regex and its flags.

@dyc3
Copy link
Contributor Author

dyc3 commented May 15, 2024

Ah, I see, thanks for pointing that out. I could go ahead and do that in this PR if you like, but I would rather get this merged as is so that I don't have to keep rebasing this branch.

@Conaclos Conaclos merged commit b33c9df into biomejs:main May 15, 2024
12 checks passed
@dyc3 dyc3 deleted the 05-09-feat_lint_add_new_js_lint_usetoplevelregex_ branch May 15, 2024 13:41
@dyc3
Copy link
Contributor Author

dyc3 commented May 15, 2024

For the record, do we want to update the grammar to add a node for the regex flags?

@Conaclos
Copy link
Member

For the record, do we want to update the grammar to add a node for the regex flags?

I don't think so.
However, we could implement a parser for regex literals.
Regex could be parsed using a Visitor. Only rule requiring the parsed form of regex literals could subscribe to the visitor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement rule useTopLevelRegex
3 participants