Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_analyze): false positives for noUselessFragments #3668 #3858

Merged
merged 1 commit into from
Nov 28, 2022
Merged

fix(rome_js_analyze): false positives for noUselessFragments #3668 #3858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions crates/rome_js_analyze/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ mod tests {
use rome_diagnostics::{Diagnostic, DiagnosticExt, PrintDiagnostic, Severity};
use rome_js_parser::parse;
use rome_js_syntax::{SourceType, TextRange, TextSize};
use std::slice;

use crate::{analyze, AnalysisFilter, ControlFlow};

Expand All @@ -149,25 +150,21 @@ mod tests {
String::from_utf8(buffer).unwrap()
}

const SOURCE: &str = r#"function f() {
return (
<div
><img /></div>
)
};
f();
const SOURCE: &str = r#"const obj = {
element: <>test</>
};
"#;

let parsed = parse(SOURCE, FileId::zero(), SourceType::jsx());

let mut error_ranges: Vec<TextRange> = Vec::new();
let options = AnalyzerOptions::default();
let _rule_filter = RuleFilter::Rule("correctness", "flipBinExp");
let rule_filter = RuleFilter::Rule("correctness", "noUselessFragments");
analyze(
FileId::zero(),
&parsed.tree(),
AnalysisFilter {
// enabled_rules: Some(slice::from_ref(&rule_filter)),
enabled_rules: Some(slice::from_ref(&rule_filter)),
..AnalysisFilter::default()
},
&options,
Expand Down
37 changes: 23 additions & 14 deletions crates/rome_js_analyze/src/semantic_analyzers/correctness/no_useless_fragments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,10 @@ use rome_js_factory::make::{
ident, js_expression_statement, js_string_literal_expression, jsx_tag_expression,
};
use rome_js_syntax::{
JsLanguage, JsSyntaxKind, JsxAnyChild, JsxAnyElementName, JsxAnyTag, JsxChildList, JsxElement,
JsxFragment, JsxTagExpression,
};
use rome_rowan::{
declare_node_union, AstNode, AstNodeList, BatchMutation, BatchMutationExt, SyntaxNodeOptionExt,
JsLanguage, JsParenthesizedExpression, JsSyntaxKind, JsxAnyChild, JsxAnyElementName, JsxAnyTag,
JsxChildList, JsxElement, JsxFragment, JsxTagExpression,
};
use rome_rowan::{declare_node_union, AstNode, AstNodeList, BatchMutation, BatchMutationExt};

declare_rule! {
/// Disallow unnecessary fragments
Expand Down Expand Up @@ -108,31 +106,42 @@ impl Rule for NoUselessFragments {
let model = ctx.model();
match node {
NoUselessFragmentsQuery::JsxFragment(fragment) => {
let matches_allowed_parents = node
let parents_where_fragments_must_be_preserved = node
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be safer to instead allow list the parents where the fragment must not be preserved? That way adding new AST node types won't break the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it wouldn't. I personally see it way harder to maintain

.syntax()
.parent()
.map(|parent| match JsxTagExpression::try_cast(parent) {
Ok(parent) => {
let parent_kind = parent.syntax().parent().kind();
matches!(
parent_kind,
Some(
Ok(parent) => parent
.syntax()
.parent()
.and_then(|parent| {
if let Some(parenthesized_expression) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to use an equivalent of omit_parentheses iterating through the ancestor nodes, or it's only going to catch the first parenthesized expression it encounters and fail on a case like this: { key: (((<>content</>))) }

Copy link
Contributor Author

@ematipico ematipico Nov 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I am aware of it and I intentionally left it out. I consider it an edge case that we can tackle if we really know if could cause issue.

There's to consider that the formatter will remove all the parentheses

JsParenthesizedExpression::cast_ref(&parent)
{
parenthesized_expression.syntax().parent()
} else {
Some(parent)
}
})
.map(|parent| {
matches!(
parent.kind(),
JsSyntaxKind::JS_RETURN_STATEMENT
| JsSyntaxKind::JS_INITIALIZER_CLAUSE
| JsSyntaxKind::JS_CONDITIONAL_EXPRESSION
| JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION
| JsSyntaxKind::JS_FUNCTION_EXPRESSION
| JsSyntaxKind::JS_FUNCTION_DECLARATION
| JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER
)
)
}
})
.unwrap_or(false),
Err(_) => false,
})
.unwrap_or(false);

let child_list = fragment.children();

if !matches_allowed_parents {
if !parents_where_fragments_must_be_preserved {
match child_list.first() {
Some(first) if child_list.len() == 1 => {
Some(NoUselessFragmentsState::Child(first))
Expand Down
9 changes: 9 additions & 0 deletions crates/rome_js_analyze/tests/specs/correctness/noUselessFragments/issue_3668.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// should not trigger
function Component2() {
const str = 'str';
return (<>{str}</>);
}

const obj = {
element: <>test</>
};
19 changes: 19 additions & 0 deletions crates/rome_js_analyze/tests/specs/correctness/noUselessFragments/issue_3668.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: issue_3668.jsx
---
# Input
```js
// should not trigger
function Component2() {
const str = 'str';
return (<>{str}</>);
}

const obj = {
element: <>test</>
};

```