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

fix(rome_js_analyze): Verify method name of React API calls #3619

Merged
merged 4 commits into from
Nov 9, 2022
Merged

fix(rome_js_analyze): Verify method name of React API calls #3619

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
556ccff
fix(rome_js_analyze): Assert `React` symbols resolve to `react` module
MichaReiser Nov 9, 2022
9927363
Support `React` global
MichaReiser Nov 9, 2022
806a4d9
Restore lib.rs
MichaReiser Nov 9, 2022
d37b991
Fix `noArrayIndexKey` test
MichaReiser Nov 9, 2022
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
119 changes: 56 additions & 63 deletions crates/rome_js_analyze/src/react.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import React from "react";
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved

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
Open in desktop
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
Open in desktop
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