feat(linter): add noImpliedEval rule#8838
Conversation
🦋 Changeset detectedLatest commit: a4ccd4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-CLI
WalkthroughAdds a new JavaScript lint rule Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs`:
- Around line 211-231: is_global_object currently recurses for any
JsStaticMemberExpression, which flags expressions like window.foo.setTimeout
even if foo isn't a global; update the JsStaticMemberExpression branch in
is_global_object to only recurse when the member.object resolves to an actual
global root identifier: call global_identifier on the object (or traverse static
members until you get an identifier) and verify the identifier's text is in
GLOBAL_OBJECTS and model.binding(&reference).is_none() before returning
true/recursing; otherwise return false. Target the is_global_object function and
the JsStaticMemberExpression handling (using global_identifier, GLOBAL_OBJECTS,
and model.binding) to implement this stricter check.
🧹 Nitpick comments (1)
crates/biome_rule_options/src/no_implied_eval.rs (1)
1-6: Add a brief rustdoc for the options type.
This keeps rule options discoverable in generated docs. As per coding guidelines, please add a short doc comment.✏️ Suggested tweak
+/// Options for the `noImpliedEval` lint rule. pub struct NoImpliedEvalOptions {}
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs`:
- Around line 147-205: The is_eval_like_function helper misses optional-chaining
forms (e.g. window?.setTimeout or (0, setTimeout)?.()), so update
is_eval_like_function to detect and unwrap optional chains: for
AnyJsExpression::JsStaticMemberExpression and JsComputedMemberExpression check
member.is_optional_chain() (or unwrap their target expression) and proceed with
the same object/member checks, and for AnyJsExpression::JsCallExpression unwrap
optional call forms (check call.is_optional_chain() and inspect its callee
recursively) similar to how JsSequenceExpression and JsParenthesizedExpression
are handled; add unit tests covering window?.setTimeout(...),
window?.["setTimeout"](...), (0, setTimeout)?.(...), and parenthesized
optional-call cases to ensure coverage.
| fn is_eval_like_function( | ||
| callee: &AnyJsExpression, | ||
| model: &biome_js_semantic::SemanticModel, | ||
| ) -> bool { | ||
| match callee { | ||
| // Direct call: setTimeout(...) | ||
| AnyJsExpression::JsIdentifierExpression(_) => { | ||
| if let Some((reference, name)) = global_identifier(callee) { | ||
| let name_text = name.text(); | ||
| // Check if it's a global binding and one of the eval-like functions | ||
| return model.binding(&reference).is_none() | ||
| && EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Member access: window.setTimeout(...) or globalThis.setTimeout(...) | ||
| AnyJsExpression::JsStaticMemberExpression(member) => { | ||
| if let (Ok(object), Ok(member_name)) = (member.object(), member.member()) | ||
| && let Some(js_name) = member_name.as_js_name() | ||
| && let Ok(token) = js_name.value_token() | ||
| { | ||
| let name_text = token.text_trimmed(); | ||
| // Check if object is global and method is eval-like | ||
| return is_global_object(&object, model) | ||
| && EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Computed member: window["setTimeout"](...) | ||
| AnyJsExpression::JsComputedMemberExpression(member) => { | ||
| if let (Ok(object), Ok(member_expr)) = (member.object(), member.member()) | ||
| && is_global_object(&object, model) | ||
| && let Some(static_value) = member_expr.as_static_value() | ||
| && let Some(name_text) = static_value.as_string_constant() | ||
| { | ||
| return EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Sequence expression: (0, setTimeout)(...) | ||
| AnyJsExpression::JsSequenceExpression(sequence) => { | ||
| // Get the last expression in the sequence | ||
| if let Ok(right) = sequence.right() { | ||
| return is_eval_like_function(&right, model); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Parenthesized expression: may contain a sequence expression | ||
| AnyJsExpression::JsParenthesizedExpression(paren) => { | ||
| // Unwrap the parenthesized expression and check recursively | ||
| if let Ok(inner) = paren.expression() { | ||
| return is_eval_like_function(&inner, model); | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find optional chaining related types in the codebase
rg -n "Optional" crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Search for optional chaining types in the JS AST definitions
rg -n "Optional|optional" crates/biome_js_syntax -l | head -20Repository: biomejs/biome
Length of output: 407
🏁 Script executed:
# Look for optional chaining node types
rg -n "JsOptional" crates/ -t rs | head -30Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Check what types exist in the AST for member expressions and optional chaining
fd -e rs crates/biome_js_syntax/ -exec grep -l "Optional" {} \; | head -10Repository: biomejs/biome
Length of output: 300
🏁 Script executed:
# Look at the actual tests for this rule to see if optional chaining is tested
fd no_implied_eval -t f crates/biome_js_analyze/Repository: biomejs/biome
Length of output: 116
🏁 Script executed:
# Search for optional chaining types in expr_ext.rs
rg -A5 -B5 "optional" crates/biome_js_syntax/src/expr_ext.rs -iRepository: biomejs/biome
Length of output: 6401
🏁 Script executed:
# Look for all expression types that might be optional chaining
rg "JsOptionalChain|optional_chain|OptionalChain" crates/biome_js_syntax/src/generated/nodes.rs | head -20Repository: biomejs/biome
Length of output: 662
🏁 Script executed:
# Check the test file structure
head -100 crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rsRepository: biomejs/biome
Length of output: 3284
🏁 Script executed:
# Search for test files related to no_implied_eval
find crates -name "*no_implied_eval*" -type fRepository: biomejs/biome
Length of output: 165
🏁 Script executed:
# Check if there are test cases for optional chaining
rg "optional|\\?\\.\\(" crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs -A2 -B2Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at the test snapshots directory structure
find crates/biome_js_analyze -name "*no_implied_eval*" -type dRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check the snapshot test file
find crates/biome_js_analyze -path "*/no_implied_eval/*" -type f | head -20Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at test input files
find crates/biome_js_analyze -name "*no_implied_eval*" -type f | grep -E "(js|snap|test)"Repository: biomejs/biome
Length of output: 116
🏁 Script executed:
# Get the full file to see the test cases and complete implementation
wc -l crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rsRepository: biomejs/biome
Length of output: 120
🏁 Script executed:
# Read the entire file to understand the implementation and tests
cat crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rsRepository: biomejs/biome
Length of output: 9722
🏁 Script executed:
# Search for how optional chaining expressions are handled in other lints
rg "AnyJsOptionalChainExpression|is_optional_chain\(\)" crates/biome_js_analyze/src/lint/ | head -20Repository: biomejs/biome
Length of output: 2595
🏁 Script executed:
# Check if JsCallExpression can be optional and how it's accessed
rg "JsCallExpression.*optional|optional.*JsCallExpression" crates/biome_js_analyze/src/lint/ | head -20Repository: biomejs/biome
Length of output: 528
🏁 Script executed:
# Look for patterns where optional chaining callees are checked
rg "callee.*optional|optional.*callee" crates/biome_js_analyze/src/lint/ -A3 | head -40Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Verify how optional chaining at the call level is structured
# Check if optional chaining on a call expression's callee needs special handling
rg "optional_chain_token\|is_optional" crates/biome_js_syntax/src/expr_ext.rs | grep -A2 "JsCallExpression" | head -20Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at how no_non_null_asserted_optional_chain handles optional chain detection
cat crates/biome_js_analyze/src/lint/suspicious/no_non_null_asserted_optional_chain.rs | head -150Repository: biomejs/biome
Length of output: 3388
Add handling for optional-chained eval-like functions.
The rule currently doesn't unwrap optional chaining expressions. Patterns like window?.setTimeout("code") or (0, setTimeout)?.("code") would slip through. Add checks using .is_optional_chain() on JsStaticMemberExpression, JsComputedMemberExpression, and JsCallExpression, or unwrap them similarly to how sequence and parenthesised expressions are handled. Consider also adding test cases for optional chaining scenarios.
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs` around lines 147
- 205, The is_eval_like_function helper misses optional-chaining forms (e.g.
window?.setTimeout or (0, setTimeout)?.()), so update is_eval_like_function to
detect and unwrap optional chains: for AnyJsExpression::JsStaticMemberExpression
and JsComputedMemberExpression check member.is_optional_chain() (or unwrap their
target expression) and proceed with the same object/member checks, and for
AnyJsExpression::JsCallExpression unwrap optional call forms (check
call.is_optional_chain() and inspect its callee recursively) similar to how
JsSequenceExpression and JsParenthesizedExpression are handled; add unit tests
covering window?.setTimeout(...), window?.["setTimeout"](...), (0,
setTimeout)?.(...), and parenthesized optional-call cases to ensure coverage.
There was a problem hiding this comment.
Thanks for pointing this out. Optional chaining is already covered by the existing unwrapping logic, and I’ve added tests to make that behavior explicit.
|
@baeseokjae you said you plan to add support for HTML in another PR. This means this PR can't close the original issue. Also, use |
.changeset/add-no-implied-eval.md
Outdated
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added new lint rule [`noImpliedEval`](https://biomejs.dev/linter/rules/no-implied-eval/) to nursery. |
There was a problem hiding this comment.
| Added new lint rule [`noImpliedEval`](https://biomejs.dev/linter/rules/no-implied-eval/) to nursery. | |
| Added new lint nursery rule [`noImpliedEval`](https://biomejs.dev/linter/rules/no-implied-eval/) to nursery. |
| } | ||
|
|
||
| /// Checks if the expression is a global object (window, global, globalThis) | ||
| fn is_global_object(expr: &AnyJsExpression, model: &biome_js_semantic::SemanticModel) -> bool { |
There was a problem hiding this comment.
You won't probably need this anymore
|
|
||
| /// Checks if the argument is a string (literal, template, or concatenation) | ||
| fn is_string_argument(arg: &AnyJsExpression) -> bool { | ||
| use biome_js_syntax::AnyJsExpression::*; |
| fn is_eval_like_function( | ||
| callee: &AnyJsExpression, | ||
| model: &biome_js_semantic::SemanticModel, | ||
| ) -> bool { | ||
| match callee { | ||
| // Direct call: setTimeout(...) | ||
| AnyJsExpression::JsIdentifierExpression(_) => { | ||
| if let Some((reference, name)) = global_identifier(callee) { | ||
| let name_text = name.text(); | ||
| // Check if it's a global binding and one of the eval-like functions | ||
| return model.binding(&reference).is_none() | ||
| && EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Member access: window.setTimeout(...) or globalThis.setTimeout(...) | ||
| AnyJsExpression::JsStaticMemberExpression(member) => { | ||
| if let (Ok(object), Ok(member_name)) = (member.object(), member.member()) | ||
| && let Some(js_name) = member_name.as_js_name() | ||
| && let Ok(token) = js_name.value_token() | ||
| { | ||
| let name_text = token.text_trimmed(); | ||
| // Check if object is global and method is eval-like | ||
| return is_global_object(&object, model) | ||
| && EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Computed member: window["setTimeout"](...) | ||
| AnyJsExpression::JsComputedMemberExpression(member) => { | ||
| if let (Ok(object), Ok(member_expr)) = (member.object(), member.member()) | ||
| && is_global_object(&object, model) | ||
| && let Some(static_value) = member_expr.as_static_value() | ||
| && let Some(name_text) = static_value.as_string_constant() | ||
| { | ||
| return EVAL_LIKE_FUNCTIONS.contains(&name_text); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Sequence expression: (0, setTimeout)(...) | ||
| AnyJsExpression::JsSequenceExpression(sequence) => { | ||
| // Get the last expression in the sequence | ||
| if let Ok(right) = sequence.right() { | ||
| return is_eval_like_function(&right, model); | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Parenthesized expression: may contain a sequence expression | ||
| AnyJsExpression::JsParenthesizedExpression(paren) => { | ||
| // Unwrap the parenthesized expression and check recursively | ||
| if let Ok(inner) = paren.expression() { | ||
| return is_eval_like_function(&inner, model); | ||
| } | ||
| false | ||
| } |
| if let Some((reference, name)) = global_identifier(expr) { | ||
| let name_text = name.text(); | ||
|
|
||
| // Check if it's one of the global objects and is actually global | ||
| if GLOBAL_OBJECTS.contains(&name_text) && model.binding(&reference).is_none() { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't understand the need GLOBAL_OBJECTS. global_identifier already wraps those. I believe the AI got it wrong. Please debug
| // Handle chained access: window.window, globalThis.globalThis | ||
| // Only recurse if the member name is also a global object | ||
| if let AnyJsExpression::JsStaticMemberExpression(member) = expr |
There was a problem hiding this comment.
global_identifier already does that.
| // Binary expression: "a" + "b" | ||
| JsBinaryExpression(bin) => { | ||
| if let Ok(operator) = bin.operator() { | ||
| use biome_js_syntax::JsBinaryOperator::Plus; |
| fn is_eval_like_function( | ||
| callee: &AnyJsExpression, | ||
| model: &biome_js_semantic::SemanticModel, | ||
| ) -> bool { |
There was a problem hiding this comment.
Change this to Option<bool> and convert everything to use ok()? or ? where applicable
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs`:
- Around line 193-222: The is_string_argument function misses handling
parenthesized expressions (so calls like setTimeout(("code"), 100) are not
detected); update is_string_argument to first strip parentheses using
omit_parentheses() on the incoming &AnyJsExpression (similar to
global_identifier), then match on the unwrapped expression (keep the existing
branches: AnyJsLiteralExpression, JsTemplateExpression, JsBinaryExpression) so
parenthesised string literals, templates, or concatenations are correctly
recognized; reference the is_string_argument function and the omit_parentheses
helper when making the change.
|
@ematipico Thanks for the reviews — all feedback has been addressed. The implementation was simplified, issue metadata added, and tests updated to explicitly cover optional chaining. All tests are passing. I’d appreciate another look when you have a chance. |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Thank you @baeseokjae
Please address one last comment
| fn unwrap_callee(expr: &AnyJsExpression) -> AnyJsExpression { | ||
| match expr { | ||
| AnyJsExpression::JsSequenceExpression(sequence) => { | ||
| if let Ok(right) = sequence.right() { | ||
| return unwrap_callee(&right); | ||
| } | ||
| expr.clone() | ||
| } | ||
|
|
||
| AnyJsExpression::JsParenthesizedExpression(paren) => { | ||
| if let Ok(inner) = paren.expression() { | ||
| return unwrap_callee(&inner); | ||
| } | ||
| expr.clone() | ||
| } | ||
|
|
||
| _ => expr.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Just use AnyJsExpression::omit_parentheses. I don't think we need to check the sequence expression (there are no tests)
There was a problem hiding this comment.
Thanks for the suggestion!
I’ve updated the code to use AnyJsExpression::omit_parentheses() and removed unwrap_callee().
I also intentionally removed the existing sequence expression cases:
they’re already covered by noCommaOperator, and handling them here would overlap responsibilities.
Since omit_parentheses() only strips parentheses, this keeps the rule focused.
All tests are passing!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_implied_eval.rs`:
- Around line 147-155: The current detection misses Node's "global" because
global_identifier only checks for "globalThis" and "window"; update the
global_identifier logic (and any related constant list of global roots) to also
accept the identifier "global" and ensure it recognizes both member access
patterns used in tests (dot access like global.setInterval and computed string
keys like global["setTimeout"]); keep is_eval_like_function and
EVAL_LIKE_FUNCTIONS unchanged but ensure global_identifier returns the same
(reference, name) pair when the root is "global" so is_eval_like_function will
flag those calls.
|
@baeseokjae as you can see from here #8838 (comment) there's been a performance regression, which we should address |
|
@ematipico Thanks for raising this. My understanding is that because this rule runs on Also, it looks like the CodSpeed run didn’t have a clean Would you prefer that I re-run with a proper baseline first, or should I acknowledge the regression on CodSpeed? |
|
We have many rules that use the semantic model, and they don't usually impact the performance, unless there's a bottleneck in the logic of the lint rule. I've been here for a while :) Why don't you rebase your branch, so that we can re run the job? |
baeseokjae
force-pushed
the
feat/no-implied-eval
branch
2 times, most recently
from
630eddb to
b92180c
Compare
baeseokjae
force-pushed
the
feat/no-implied-eval
branch
from
b92180c to
a4ccd4c
Compare
|
@ematipico Makes sense. I rebased onto main and re-ran the perf tests — everything looks fine now, no regression. Thanks for taking a look — feel free to check again when you have a moment. :) |
2 participants
Important
This PR was created with AI assistance (Claude Opus 4.5).
I reviewed and adjusted the implementation, diagnostics, and snapshots.
Summary
Implements JS/JSX part of #8735
This PR introduces the
noImpliedEvallint rule in the nursery group. The rule detects impliedeval()usage through functions such assetTimeout,setInterval, andsetImmediatewhen they are called with string arguments.Test Plan
invalid.js, 3 ininvalid.jsx)cargo test -p biome_js_analyze -- no_implied_evalDocs
Documentation is provided via inline rustdoc, including: