-
-
Notifications
You must be signed in to change notification settings - Fork 762
feat(js_analyze): implement useRegexpExec
#8034
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added the nursery rule [`useRegexpExec`](https://biomejs.dev/linter/rules/use-regexp-exec/). Enforce RegExp#exec over String#match if no global flag is provided. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| use crate::services::typed::Typed; | ||
| use biome_analyze::{ | ||
| Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, | ||
| }; | ||
| use biome_console::markup; | ||
| use biome_js_syntax::{AnyJsExpression, JsCallExpression}; | ||
| use biome_rowan::{AstNode, AstSeparatedList}; | ||
| use biome_rule_options::use_regexp_exec::UseRegexpExecOptions; | ||
|
|
||
| declare_lint_rule! { | ||
| /// Enforce RegExp#exec over String#match if no global flag is provided. | ||
| /// | ||
| /// String#match is defined to work the same as RegExp#exec when the regular expression does not include the g flag. | ||
| /// Keeping to consistently using one of the two can help improve code readability. | ||
| /// | ||
| /// RegExp#exec may also be slightly faster than String#match; this is the reason to choose it as the preferred usage. | ||
| /// | ||
| /// ## Examples | ||
| /// | ||
| /// ### Invalid | ||
| /// | ||
| /// ```ts,expect_diagnostic | ||
| /// 'something'.match(/thing/); | ||
| /// ``` | ||
| /// | ||
| /// ### Valid | ||
| /// | ||
| /// ```ts | ||
| /// /thing/.exec('something'); | ||
| /// ``` | ||
| /// | ||
| pub UseRegexpExec { | ||
| version: "next", | ||
| name: "useRegexpExec", | ||
| language: "js", | ||
| recommended: false, | ||
| sources: &[RuleSource::EslintTypeScript("prefer-regexp-exec").same(), RuleSource::EslintRegexp("prefer-regexp-exec").same()], | ||
| domains: &[RuleDomain::Project], | ||
| } | ||
| } | ||
|
|
||
| impl Rule for UseRegexpExec { | ||
| type Query = Typed<JsCallExpression>; | ||
| type State = (); | ||
| type Signals = Option<Self::State>; | ||
| type Options = UseRegexpExecOptions; | ||
|
|
||
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||
| let node = ctx.query(); | ||
|
|
||
| let binding = node.callee().ok()?.omit_parentheses(); | ||
| let callee = binding.as_js_static_member_expression()?; | ||
|
|
||
| let call_object = callee.object().ok()?; | ||
| if !ctx | ||
| .type_of_expression(&call_object) | ||
| .is_string_or_string_literal() | ||
| { | ||
| return None; | ||
| } | ||
|
|
||
| let call_name = callee.member().ok()?.as_js_name()?.to_trimmed_text(); | ||
| if call_name != "match" { | ||
| return None; | ||
| } | ||
|
|
||
| let args = node.arguments().ok()?.args(); | ||
| let first_arg = args.first()?.ok()?; | ||
| let express = first_arg.as_any_js_expression()?; | ||
|
|
||
| let regex_arg = match express { | ||
| AnyJsExpression::AnyJsLiteralExpression(express) => { | ||
| express.as_js_regex_literal_expression() | ||
| } | ||
| AnyJsExpression::JsIdentifierExpression(identifier) => { | ||
| // TODO: get static regexp value | ||
| return None; | ||
| } | ||
| _ => None, | ||
| }?; | ||
|
|
||
| let (_, flags) = regex_arg.decompose().unwrap(); | ||
| if flags.contains('g') { | ||
| return None; | ||
| } | ||
|
|
||
| Some(()) | ||
| } | ||
|
|
||
| fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { | ||
| let node = ctx.query(); | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| "Prefer "<Emphasis>"Regexp#exec()"</Emphasis>" over "<Emphasis>"String#match()"</Emphasis>" when searching within a string." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "This note will give you more information." | ||
| }), | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| 'something'.match(/thing/); | ||
|
|
||
| 'some things are just things'.match(/thing/); | ||
|
|
||
| const text = 'something'; | ||
| const search = /thing/; | ||
| text.match(search); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| /* should not generate diagnostics */ | ||
| /thing/.exec('something'); | ||
|
|
||
| 'some things are just things'.match(/thing/g); | ||
|
|
||
| const text = 'something'; | ||
| const search = /thing/; | ||
| search.exec(text); | ||
|
|
||
| const text1 = 'something'; | ||
| const search1 = /thing/g; | ||
| text1.match(search1); | ||
|
|
||
| const obj = { | ||
| match: () => { } | ||
| } | ||
| obj.match(/thing/) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| --- | ||
| source: crates/biome_js_analyze/tests/spec_tests.rs | ||
| expression: valid.js | ||
| --- | ||
| # Input | ||
| ```js | ||
| /* should not generate diagnostics */ | ||
| /thing/.exec('something'); | ||
|
|
||
| 'some things are just things'.match(/thing/g); | ||
|
|
||
| const text = 'something'; | ||
| const search = /thing/; | ||
| search.exec(text); | ||
|
|
||
| const text1 = 'something'; | ||
| const search1 = /thing/g; | ||
| text1.match(search1); | ||
|
|
||
| const obj = { | ||
| match: () => { } | ||
| } | ||
| obj.match(/thing/) | ||
|
|
||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| use biome_deserialize_macros::{Deserializable, Merge}; | ||
| use serde::{Deserialize, Serialize}; | ||
| #[derive(Default, Clone, Debug, Deserialize, Deserializable, Eq, Merge, PartialEq, Serialize)] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields, default)] | ||
| pub struct UseRegexpExecOptions {} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how how to resolve the value of a reference.
Could also use the type of the expression, then check the flags within the regex type? Not sure if that's valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the source rule do? https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/prefer-regexp-exec.ts
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to call an Eslint API to get the value of a given node if it can be decided statically. (not through types) But not sure if biome provides such API?
https://eslint-community.github.io/eslint-utils/api/ast-utils.html#getstaticvalue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the semantic model and the control flow graph, but iirc typed rules shouldn't be using those because they use the module graph. @arendjr, I'd appreciate your input here, you have more context on the type system and the module graph than I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typed rules can also use the semantic model and the control flow graph, but the semantic model is largely redundant with functionality that is already provided by the typed service, which provides similar things, but with a higher level of abstraction that includes types and cross-module inference.
So far the abstract way of looking at it :)
Unfortunately we don't really have an API yet that can determine static values (AFAIK), but the typed service actually does try to determine static values when it can determine them. Simple example:
The type system will be able to determine that the reference to
fooon the second line resolves to the concrete value1, because in TypeScript concrete values can be used as types as well and we try to infer as specific as we can. So yes, I think using the type system is probably the way to go here, but some more work will be necessary. I don't think we have any specific detection forRegExpyet, and I haven't looked yet into what flags need to be checked, but that might require some custom logic too?If you like, I might be interested to look into this next week.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be awesome. For now I only need to check on the global flag. But some API to the regexp type could be to check if it has any given flag could be nice