-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(lint): added more test cases for variable declarations in noLeakedRender
#8537
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
Changes from 8 commits
366aa74
07c7b5c
fa57558
030d68a
aa5fa26
10b6a36
6341386
b54dcbb
67c76f5
87d2a58
4678d89
1c1b80a
ee5f1a4
8910f8f
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,31 @@ | ||
| --- | ||
| '@biomejs/biome': patch | ||
| --- | ||
|
|
||
| Related to [#8491](https://github.com/biomejs/biome/issues/8491): Fixed the issue with false positive errors for safe boolean expressions. | ||
|
|
||
| This new change will check for safe boolean expressions in variable declarations. | ||
|
|
||
| For example, | ||
|
|
||
| Valid: | ||
|
|
||
| ```jsx | ||
| let isOne = 1; | ||
| let isPositiveNumber = number > 0; | ||
|
|
||
| return <div> {isOne && "One } { isPositiveNumber && "Is positive" }</div> | ||
|
dibashthapa marked this conversation as resolved.
Outdated
ematipico marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
dibashthapa marked this conversation as resolved.
|
||
|
|
||
| Invalid: | ||
|
|
||
| ```jsx | ||
| let emptyStr = ''; | ||
| let isZero = 0; | ||
|
|
||
| return ( | ||
| <div> | ||
| {emptyStr && 'Empty String'} {isZero && 'Number is zero'}{' '} | ||
| </div> | ||
| ); | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,11 @@ use biome_analyze::{ | |
| Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, | ||
| }; | ||
| use biome_console::markup; | ||
| use biome_js_semantic::SemanticModel; | ||
| use biome_js_syntax::{ | ||
| AnyJsExpression, JsConditionalExpression, JsLogicalExpression, JsLogicalOperator, JsSyntaxNode, | ||
| JsxExpressionChild, JsxTagExpression, binding_ext::AnyJsBindingDeclaration, | ||
| jsx_ext::AnyJsxElement, | ||
| AnyJsExpression, AnyJsLiteralExpression, JsConditionalExpression, JsLogicalExpression, | ||
| JsLogicalOperator, JsReferenceIdentifier, JsSyntaxNode, JsxExpressionChild, JsxTagExpression, | ||
| binding_ext::AnyJsBindingDeclaration, jsx_ext::AnyJsxElement, | ||
| }; | ||
| use biome_rowan::{AstNode, declare_node_union}; | ||
| use biome_rule_options::no_leaked_render::NoLeakedRenderOptions; | ||
|
|
@@ -98,7 +99,7 @@ declare_lint_rule! { | |
|
|
||
| impl Rule for NoLeakedRender { | ||
| type Query = Semantic<NoLeakedRenderQuery>; | ||
| type State = bool; | ||
| type State = (); | ||
| type Signals = Option<Self::State>; | ||
| type Options = NoLeakedRenderOptions; | ||
|
|
||
|
|
@@ -165,38 +166,46 @@ impl Rule for NoLeakedRender { | |
|
|
||
| if let AnyJsExpression::JsIdentifierExpression(ident) = &left { | ||
| let name = ident.name().ok()?; | ||
|
|
||
| // Use the semantic model to resolve the variable binding and check | ||
| // if it's initialized with a boolean literal. This allows us to | ||
| // if it's initialized with safe expressions. This allows us to | ||
| // handle cases like: | ||
| // let isOpen = false; // This is safe | ||
| // let isError = errorCount > 0; // This is safe | ||
| // let isEqual = a === b; // This is safe | ||
| // let emptyStr = ''; // This is unsafe | ||
| // return <div>{isOpen && <Content />}</div>; // This should pass | ||
| if let Some(binding) = model.binding(&name) | ||
| && binding | ||
| .tree() | ||
| .declaration() | ||
| .and_then(|declaration| { | ||
| if let AnyJsBindingDeclaration::JsVariableDeclarator(declarator) = | ||
| declaration | ||
| { | ||
| Some(declarator) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .and_then(|declarator| declarator.initializer()) | ||
| .and_then(|initializer| initializer.expression().ok()) | ||
| .and_then(|expr| { | ||
| if let AnyJsExpression::AnyJsLiteralExpression(literal) = expr { | ||
| Some(literal) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .and_then(|literal| literal.value_token().ok()) | ||
| .is_some_and(|token| matches!(token.text_trimmed(), "true" | "false")) | ||
| { | ||
| return None; | ||
| // return <div> {isError && <Error />}</div>; // This should pass | ||
| // return <div> {isEqual && <Equal />}</div>; // This should pass | ||
| // return <div> {emptyStr && <Empty />}</div>; // This should fail | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To remove?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not actually, this was supposed to be comment for future contributors, so that it makes sense what the code is doing. Maybe i added more noise by doing that @ematipico should i remove ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, it wasn't clear. I would move the phrasing first, the example afterwards |
||
|
|
||
| if let Some(variable) = find_variable(model, &name) { | ||
| match variable { | ||
| AnyJsExpression::AnyJsLiteralExpression(expr) => { | ||
| match expr { | ||
| AnyJsLiteralExpression::JsStringLiteralExpression(str) => { | ||
| if str.inner_string_text().ok()?.text().is_empty() { | ||
| return Some(()); | ||
| } | ||
| return None; | ||
| } | ||
| AnyJsLiteralExpression::JsNullLiteralExpression(_) => { | ||
| return Some(()); | ||
| } | ||
| AnyJsLiteralExpression::JsNumberLiteralExpression(num) => { | ||
| let value = num.value_token().ok()?; | ||
| if value.text_trimmed() == "0" { | ||
| return Some(()); | ||
| } | ||
| return None; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
| _ => return None, | ||
| }; | ||
| } | ||
| AnyJsExpression::JsUnaryExpression(_) | ||
| | AnyJsExpression::JsBinaryExpression(_) | ||
| | AnyJsExpression::JsCallExpression(_) => return None, | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
159
to
179
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if the file exists and view the relevant lines
echo "=== Checking file and code at lines 167-210 ==="
if [ -f "crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs" ]; then
sed -n '167,210p' crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
else
echo "File not found at expected location"
find . -name "*no_leaked_render*" -type f 2>/dev/null | head -5
fi
echo ""
echo "=== Checking BigInt test cases ==="
find crates/biome_js_analyze/tests -name "*noLeakedRender*" -o -name "*no_leaked_render*" 2>/dev/null | head -5
echo ""
echo "=== Searching for BigInt 0n in test specs ==="
rg "0n" crates/biome_js_analyze/tests/specs/nursery/ 2>/dev/null | grep -i leaked | head -10
echo ""
echo "=== Checking declare_lint_rule! for fix_kind ==="
rg -A 5 "declare_lint_rule!" crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs 2>/dev/null | head -20Repository: biomejs/biome Length of output: 3281 🏁 Script executed: #!/bin/bash
# Check the complete declare_lint_rule! block for fix_kind
echo "=== Full declare_lint_rule! block ==="
rg -A 50 "declare_lint_rule!" crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs 2>/dev/null | grep -E "(declare_lint_rule|fix_kind|name =|version =)" | head -10
# Also check if JsBigintLiteralExpression is handled anywhere
echo ""
echo "=== Checking for BigInt literal handling ==="
rg "JsBigintLiteralExpression" crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
# Check the test file exists and what's in it
echo ""
echo "=== Test spec file contents (first 50 lines) ==="
if [ -f "crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.js" ]; then
head -50 crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.js
elif [ -f "crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.ts" ]; then
head -50 crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.ts
else
find crates/biome_js_analyze/tests/specs/nursery/noLeakedRender -type f | head -3
fiRepository: biomejs/biome Length of output: 457 🏁 Script executed: #!/bin/bash
# Get the full declare_lint_rule! block up to the closing brace
echo "=== Full declare_lint_rule! macro invocation ==="
awk '/^declare_lint_rule!\s*\{/,/^}/' crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs | head -80
echo ""
echo "=== Checking for fix_kind in the rule ==="
rg "fix_kind" crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rsRepository: biomejs/biome Length of output: 2409 Specify The code correctly catches falsy literals (empty strings, null, numeric 0), but two issues remain:
The semantic model integration for variable resolution is solid and follows best practices. 🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai The rule doesn't implement the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ematipico, you're absolutely right! Thank you for the clarification. Since this rule only emits diagnostics and doesn't implement the The BigInt AnyJsLiteralExpression::JsBigIntLiteralExpression(bigint) => {
let value = bigint.value_token().ok()?;
if value.text_trimmed() == "0n" {
return Some(());
}
return None;
}But that's optional depending on how common BigInt usage is in your target codebases. ✏️ Learnings added
🧠 Learnings used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @dibashthapa |
||
|
|
||
|
|
@@ -205,7 +214,7 @@ impl Rule for NoLeakedRender { | |
| return None; | ||
| } | ||
|
|
||
| Some(true) | ||
| Some(()) | ||
| } | ||
| NoLeakedRenderQuery::JsConditionalExpression(expr) => { | ||
| let alternate = expr.alternate().ok()?; | ||
|
|
@@ -216,7 +225,7 @@ impl Rule for NoLeakedRender { | |
| return None; | ||
| } | ||
|
|
||
| Some(true) | ||
| Some(()) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -276,3 +285,17 @@ fn is_inside_jsx_expression(node: &JsSyntaxNode) -> Option<bool> { | |
| || AnyJsxElement::can_cast(parent.kind()), | ||
| ) | ||
| } | ||
|
|
||
| fn find_variable(model: &SemanticModel, name: &JsReferenceIdentifier) -> Option<AnyJsExpression> { | ||
| let Some(binding) = model.binding(name) else { | ||
| return None; | ||
| }; | ||
|
|
||
| let declaration = binding.tree().declaration()?; | ||
| let AnyJsBindingDeclaration::JsVariableDeclarator(declarator) = declaration else { | ||
| return None; | ||
| }; | ||
|
|
||
| let expr = declarator.initializer()?.expression().ok()?; | ||
| return Some(expr); | ||
| } | ||
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.
If this PR doesn't close the "related" bug, there's no need to add it to the changeset
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.
This PR do fix the issue
However, it still doesn't addresses the fixes for #8491 (comment)
I couldn't find the perfect way to describe the fixes, hence "Related"
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.
I think it's fine to just mention what this patch fixes. If you want to highlight the issue, which is good, I suggest finding a different wording e.g. "There are still pending fixes. Head to #xxx for more details". "Related #xxxx` doesn't seem useful for our users
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.
Fixed