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

Commit

Permalink
fix(rome_js_analyze): Assert React symbols resolve to react module
Browse files Browse the repository at this point in the history 
## Summary

The `is_react_call_api` in some situations didn't test if the method name is the tested for method name.

For example, `is_react_call_api(..., "cloneElement")` returned true for `React.clone` or just `clone`.

I fixed the same issue in `jsx_member_name_is_react_fragment`

## Test Plan

I added some new tests veryfing that the logic correctly verifies the member name.
  • Loading branch information
@MichaReiser
MichaReiser committed Nov 9, 2022
1 parent 99f59bf commit 55f93e4
Show file tree
Hide file tree
Showing 22 changed files with 382 additions and 408 deletions.
119 changes: 56 additions & 63 deletions crates/rome_js_analyze/src/react.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

pub mod hooks;

use rome_js_semantic::SemanticModel;
use rome_js_semantic::{Binding, SemanticModel};
use rome_js_syntax::{
JsAnyCallArgument, JsAnyExpression, JsAnyNamedImportSpecifier, JsCallExpression,
JsIdentifierBinding, JsImport, JsImportNamedClause, JsNamedImportSpecifierList,
Expand Down Expand Up @@ -247,46 +247,42 @@ pub(crate) fn is_react_call_api(
) -> Option<bool> {
// we bail straight away if the API doesn't exists in React
debug_assert!(VALID_REACT_API.contains(&api_name));

Some(match expression {
JsAnyExpression::JsStaticMemberExpression(node) => {
let object = node.object().ok()?;
let member = node.member().ok()?;
let member = member.as_js_name()?;

if member.value_token().ok()?.text_trimmed() != api_name {
return Some(false);
}

let object = node.object().ok()?;
let identifier = object.as_js_identifier_expression()?.name().ok()?;

let mut maybe_from_react = identifier.syntax().text_trimmed() == "React"
&& member.syntax().text_trimmed() == api_name;
return model.declaration(&identifier).and_then(|binding| {
let binding_identifier = JsIdentifierBinding::cast_ref(binding.syntax())?;

if let Some(binding_identifier) = model.declaration(&identifier) {
let binding_identifier =
JsIdentifierBinding::cast_ref(binding_identifier.syntax())?;
if let Some(js_import) = binding_identifier
.syntax()
.ancestors()
.find_map(|ancestor| JsImport::cast_ref(&ancestor))
{
maybe_from_react = js_import.source_is("react").ok()?;
js_import.source_is("react").ok()
} else {
Some(false)
}
}
maybe_from_react
});
}
JsAnyExpression::JsIdentifierExpression(identifier) => {
let name = identifier.name().ok()?;
let mut maybe_react = identifier.syntax().text_trimmed() == api_name;
if let Some(identifier_binding) = model.declaration(&name) {
let binding_identifier =
JsIdentifierBinding::cast_ref(identifier_binding.syntax())?;
if let Some(js_import) = binding_identifier
.syntax()
.ancestors()
.find_map(|ancestor| JsImport::cast_ref(&ancestor))
{
maybe_react = js_import.source_is("react").ok()?;
}
}
maybe_react

model
.declaration(&name)
.and_then(|binding| is_react_export(binding, api_name))
.unwrap_or(false)
}
_ => return None,
_ => false,
})
}

Expand All @@ -303,24 +299,22 @@ pub(crate) fn jsx_member_name_is_react_fragment(
let object = member_name.object().ok()?;
let member = member_name.member().ok()?;
let object = object.as_jsx_reference_identifier()?;
let mut maybe_react_fragment = object.value_token().ok()?.text_trimmed() == "React"
&& member.value_token().ok()?.text_trimmed() == "Fragment";
if let Some(reference) = model.declaration(object) {
if let Some(js_import) = reference

if member.value_token().ok()?.text_trimmed() != "Fragment" {
return Some(false);
}

model.declaration(object).and_then(|declaration| {
if let Some(js_import) = declaration
.syntax()
.ancestors()
.find_map(|ancestor| JsImport::cast_ref(&ancestor))
{
let source_is_react = js_import.source_is("react").ok()?;
maybe_react_fragment =
source_is_react && member.value_token().ok()?.text_trimmed() == "Fragment";
js_import.source_is("react").ok()
} else {
// `React.Fragment` is a binding but it doesn't come from the "react" package
maybe_react_fragment = false;
Some(false)
}
}

Some(maybe_react_fragment)
})
}

/// Checks if the node `JsxReferenceIdentifier` is a react fragment.
Expand All @@ -334,37 +328,36 @@ pub(crate) fn jsx_reference_identifier_is_fragment(
model: &SemanticModel,
) -> Option<bool> {
match model.declaration(name) {
Some(reference) => {
let ident = JsIdentifierBinding::cast_ref(reference.syntax())?;

let import_specifier = ident.parent::<JsAnyNamedImportSpecifier>()?;
let name_token = match &import_specifier {
JsAnyNamedImportSpecifier::JsNamedImportSpecifier(named_import) => {
named_import.name().ok()?.value().ok()?
}
JsAnyNamedImportSpecifier::JsShorthandNamedImportSpecifier(_) => {
ident.name_token().ok()?
}
JsAnyNamedImportSpecifier::JsUnknownNamedImportSpecifier(_) => {
return None;
}
};

if name_token.text_trimmed() != "Fragment" {
return Some(false);
}

let import_specifier_list = import_specifier.parent::<JsNamedImportSpecifierList>()?;
let import_specifiers = import_specifier_list.parent::<JsNamedImportSpecifiers>()?;
let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
let import = import_clause.parent::<JsImport>()?;
import.source_is("react").ok()
}

Some(reference) => is_react_export(reference, "Fragment"),
None => {
let value_token = name.value_token().ok()?;
let is_fragment = value_token.text_trimmed() == "Fragment";
Some(is_fragment)
}
}
}

fn is_react_export(binding: Binding, name: &str) -> Option<bool> {
let ident = JsIdentifierBinding::cast_ref(binding.syntax())?;
let import_specifier = ident.parent::<JsAnyNamedImportSpecifier>()?;
let name_token = match &import_specifier {
JsAnyNamedImportSpecifier::JsNamedImportSpecifier(named_import) => {
named_import.name().ok()?.value().ok()?
}
JsAnyNamedImportSpecifier::JsShorthandNamedImportSpecifier(_) => ident.name_token().ok()?,
JsAnyNamedImportSpecifier::JsUnknownNamedImportSpecifier(_) => {
return Some(false);
}
};

if name_token.text_trimmed() != name {
return Some(false);
}

let import_specifier_list = import_specifier.parent::<JsNamedImportSpecifierList>()?;
let import_specifiers = import_specifier_list.parent::<JsNamedImportSpecifiers>()?;
let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
let import = import_clause.parent::<JsImport>()?;

import.source_is("react").ok()
}
32 changes: 14 additions & 18 deletions crates/rome_js_analyze/src/semantic_analyzers/correctness/no_array_index_key.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use rome_analyze::{declare_rule, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_js_semantic::SemanticModel;
use rome_js_syntax::{
JsArrowFunctionExpression, JsCallExpression, JsExpressionStatement, JsFunctionDeclaration,
JsFunctionExpression, JsIdentifierBinding, JsIdentifierExpression, JsMethodClassMember,
JsAnyCallArgument, JsArrowFunctionExpression, JsCallExpression, JsExpressionStatement,
JsFunctionDeclaration, JsFunctionExpression, JsIdentifierBinding, JsMethodClassMember,
JsMethodObjectMember, JsParameterList, JsPropertyObjectMember, JsReferenceIdentifier,
JsxAttribute, JsxOpeningElement, JsxSelfClosingElement,
};
Expand Down Expand Up @@ -326,28 +326,24 @@ fn find_react_children_function_argument(
"forEach" | "map"
);

let object = member_expression.object().ok()?;

let mut is_react_children = false;
// case we have `Children`
if let Some(identifier) = JsIdentifierExpression::cast_ref(object.syntax()) {
if identifier.name().ok()?.value_token().ok()?.text_trimmed() == "Children" {
is_react_children = array_call;
}
} else {
// case we have `React.Children`
is_react_children = is_react_call_api(&object, model, "Children")? && array_call;
if !array_call {
return None;
}

if is_react_children {
let object = member_expression.object().ok()?;

// React.Children.forEach/map or Children.forEach/map
if is_react_call_api(&object, model, "Children")? {
let arguments = call_expression.arguments().ok()?;
let arguments = arguments.args();
let mut arguments = arguments.into_iter();
let _ = arguments.next()?.ok()?;
let second_argument = arguments.next()?.ok()?;
let second_argument = second_argument.as_js_any_expression()?;

FunctionLike::cast(second_argument.clone().into_syntax())
match (arguments.next(), arguments.next(), arguments.next()) {
(Some(_), Some(Ok(JsAnyCallArgument::JsAnyExpression(second_argument))), None) => {
FunctionLike::cast(second_argument.into_syntax())
}
_ => None,
}
} else {
None
}
Expand Down
2 changes: 2 additions & 0 deletions crates/rome_js_analyze/tests/specs/a11y/noPositiveTabindex/reactCreateElementInvalid.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import React from "react";

React.createElement("div", { tabIndex: '1' })
React.createElement("div", { tabIndex: 1 })
React.createElement("div", { tabIndex: +1 })
Expand Down
44 changes: 24 additions & 20 deletions crates/rome_js_analyze/tests/specs/a11y/noPositiveTabindex/reactCreateElementInvalid.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ expression: reactCreateElementInvalid.js
---
# Input
```js
import React from "react";

React.createElement("div", { tabIndex: '1' })
React.createElement("div", { tabIndex: 1 })
React.createElement("div", { tabIndex: +1 })
Expand All @@ -13,63 +15,65 @@ React.createElement("div", { tabIndex: +01 })

# Diagnostics
```
reactCreateElementInvalid.js:1:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
reactCreateElementInvalid.js:3:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Avoid positive values for the tabIndex prop.
> 1 │ React.createElement("div", { tabIndex: '1' })
1 │ import React from "react";
2 │
> 3 │ React.createElement("div", { tabIndex: '1' })
│ ^^^
2 │ React.createElement("div", { tabIndex: 1 })
3 │ React.createElement("div", { tabIndex: +1 })
4 │ React.createElement("div", { tabIndex: 1 })
5 │ React.createElement("div", { tabIndex: +1 })
i Elements with a positive tabIndex override natural page content order. This causes elements without a positive tab index to come last when navigating using a keyboard.
```

```
reactCreateElementInvalid.js:2:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
reactCreateElementInvalid.js:4:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Avoid positive values for the tabIndex prop.
1 │ React.createElement("div", { tabIndex: '1' })
> 2 │ React.createElement("div", { tabIndex: 1 })
3 │ React.createElement("div", { tabIndex: '1' })
> 4 │ React.createElement("div", { tabIndex: 1 })
│ ^
3 │ React.createElement("div", { tabIndex: +1 })
4 │ React.createElement("div", { tabIndex: +01 })
5 │ React.createElement("div", { tabIndex: +1 })
6 │ React.createElement("div", { tabIndex: +01 })
i Elements with a positive tabIndex override natural page content order. This causes elements without a positive tab index to come last when navigating using a keyboard.
```

```
reactCreateElementInvalid.js:3:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
reactCreateElementInvalid.js:5:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Avoid positive values for the tabIndex prop.
1 │ React.createElement("div", { tabIndex: '1' })
2 │ React.createElement("div", { tabIndex: 1 })
> 3 │ React.createElement("div", { tabIndex: +1 })
3 │ React.createElement("div", { tabIndex: '1' })
4 │ React.createElement("div", { tabIndex: 1 })
> 5 │ React.createElement("div", { tabIndex: +1 })
│ ^^
4 │ React.createElement("div", { tabIndex: +01 })
5 │
6 │ React.createElement("div", { tabIndex: +01 })
7 │
i Elements with a positive tabIndex override natural page content order. This causes elements without a positive tab index to come last when navigating using a keyboard.
```

```
reactCreateElementInvalid.js:4:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
reactCreateElementInvalid.js:6:40 lint/a11y/noPositiveTabindex ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Avoid positive values for the tabIndex prop.
2 │ React.createElement("div", { tabIndex: 1 })
3 │ React.createElement("div", { tabIndex: +1 })
> 4 │ React.createElement("div", { tabIndex: +01 })
4 │ React.createElement("div", { tabIndex: 1 })
5 │ React.createElement("div", { tabIndex: +1 })
> 6 │ React.createElement("div", { tabIndex: +01 })
│ ^^^
5 │
7 │
i Elements with a positive tabIndex override natural page content order. This causes elements without a positive tab index to come last when navigating using a keyboard.
Expand Down
2 changes: 2 additions & 0 deletions crates/rome_js_analyze/tests/specs/a11y/useButtonType/inObject.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import React from "react";

// invalid
React.createElement('button');
React.createElement('button', {
Expand Down
Loading

0 comments on commit 55f93e4

Please sign in to comment.