fix(rome_js_analyze): false negatives of noArrayIndexKey

[lucasweng]

Summary

• Resolves 🐛 noArrayIndexKey ignores valid cases #3670

• Improves the detection of the following cases

  // Return statement
  return array.map((_, index) => <Component key={index} />);
  
  // Assignment expression
  let element = null
  if (condition) {
      elements = array.map((_, index) => <Component key={index} />);
  }
  
  // Parenthesized expression
  function App({things}) {
      const elements = useMemo(() => (
          things.map((_, index) => <Component key={index} />)
      ), [things]);
      return elements;
   }
  function App() {
      return (
          <HoC>
              {({things}) => (
                  things.map((_, index) => <Component key={index} />)
              )}
          </HoC>
      )
  }
  
  // Call expression
  const Component = () => array.map((_, index) => <Component key={index} />);
  
  // Variable declaration
  let elements = array.map((_, index) => <Component key={index} />);
  
  // `flatMap` or `Array.from` method call
  array.flatMap((_, index) => {
      return <Component key={index} />;
  })
  Array.from(sets, (_, index) => {
      return <Component key={index} />;
  })

Test Plan

Added new test cases

[netlify]

❌ Deploy Preview for docs-rometools failed.

Name	Link
🔨 Latest commit
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/637781eff15ce21882c2c3d7

[MichaReiser]

Thanks for your PR. I think there are some more issues with the rule (unrelated to your changes) and it may be possible to simplify the finding of the closest call expression.

[MichaReiser]

I'm sorry for commenting here but GitHub doesn't allow me to comment unchanged lines.

We should use the semantic model to resolve where the expression assigned to key is declared. This replaces the find_function_parent call on line 135

        let Some(parameter) = model
            .declaration(&reference)
            .and_then(|declaration| declaration.syntax().parent())
            .and_then(JsFormalParameter::cast);

        let Some(function) = parameter
            .parent::<JsParameterList>()
            .and_then(|list| list.parent::<JsParameters>())
            .and_then(|parameters| parameters.parent::<JsAnyFunction>()) else { return None; };

        let Some(call_expression) = parameter
            .parent::<JsCallArgumentList>()
            .and_then(|arguments| arguments.parent::<JsCallArguments>())
            .and_then(|arguments| arguments.parent::<JsCallExpression>()) else { return None };

[lucasweng]

No worries and thanks @MichaReiser!

I was trying to refactor the rule by matching on JsxAttribute and JsPropertyObjectMember following suggestions in #3681 (comment) and #3681 (comment).

1. Use the semantic model to find the declaration of index that is inside a JsArrowFunctionExpression or JsFunctionExpression
2. From the function expression above, find the closest call expression that belongs to an array method
3. Then traverse down to check if the declaration is the second argument of the function call to avoid false positive like the following

things.map((id) => {
    return <Jsx key={id} />
})

4. For JsPropertyObjectMember we need to navigate down further to check if the JsObjectExpression is the second argument of a React.cloneElement call

React.Children.forEach(this.props.children, (_, index) => {
  const obj = {key: index}
  // we don't know if the key above is used as `JsxAttribute`
  return notCloneElement(obj)
})

But I'm not sure if there is a better way to address the false positives without navigating up and then down like the above steps 🤔
Or should we address the more common use cases first without handling edge cases that are less likely to appear in userland?

[MichaReiser]

Then traverse down to check if the declaration is the second argument of the function call to avoid false positive like the following

You can probably use parameter.syntax().index(). It gives you the index in the enclosing parameter list (you have to divide it by two because the list elements are parameter, comma, parameter2`.

React.Children.forEach(this.props.children, (_, index) => {
const obj = {key: index}
// we don't know if the key above is used as JsxAttribute
return notCloneElement(obj)
})

I'm not sure if I understand the question/problem here. I believe it should be sufficient to only detect calls to cloneElement where the object is the argument cloneElement { key: index }.

The way I would expect cloneElement to work if you match on object member is that:

• You test if the object member name is key
• You navigate upwards from the object member until you find the enclosing call expression
• You test if it is a react API call
• If that's the case, do the same as for JSX elements by resolving the binding for index
  • then finding the enclosing call expression
  • test if the call expression is an array method.

[lucasweng]

bc7a03c now the rule is matching on JsxAttribute and JsPropertyObjectMember, and it will

1. Find if it's a key property
2. Retrieve the reference identifier from the key property,
3. Use the semantic model to find the declaration of the reference is a parameter in a function expression
4. Find the enclosing call expression
5. Check if the call expression is an array method and the parameter is the array index of that method
6. If it's matching on JsPropertyObjectMember, detect calls to cloneElement

Also, added tests for flatMap and Array.from

How do we test if the current implementation is faster or slower than the previous one?

[lucasweng]

Not sure if I can remove ReactCloneElementCall::from_call_expression, it's no longer used because of this refactor

[MichaReiser]

ReactCloneElementCall

It's safe to remove the method as it is no longer used.

[MichaReiser]

!bench_analyzer

[MichaReiser]

Awesome work! Nice to see how you managed to shorten the implementation by 100 lines of code!

[github-actions]

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.03      2.8±0.06ms     4.1 MB/sec    1.00      2.7±0.13ms     4.3 MB/sec
analyzer/index.js         1.06      8.1±0.33ms     4.0 MB/sec    1.00      7.7±0.33ms     4.3 MB/sec
analyzer/lint.ts          1.00      3.8±0.09ms    10.9 MB/sec    1.01      3.8±0.07ms    10.8 MB/sec
analyzer/parser.ts        1.05      9.4±0.25ms     5.2 MB/sec    1.00      8.9±0.22ms     5.5 MB/sec
analyzer/router.ts        1.01      6.3±0.22ms    10.0 MB/sec    1.00      6.2±0.18ms    10.1 MB/sec
analyzer/statement.ts     1.04      9.1±0.28ms     3.9 MB/sec    1.00      8.8±0.21ms     4.0 MB/sec
analyzer/typescript.ts    1.03     13.8±0.41ms     3.9 MB/sec    1.00     13.5±0.49ms     4.0 MB/sec

[MichaReiser]

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.03      2.8±0.06ms     4.1 MB/sec    1.00      2.7±0.13ms     4.3 MB/sec
analyzer/index.js         1.06      8.1±0.33ms     4.0 MB/sec    1.00      7.7±0.33ms     4.3 MB/sec
analyzer/lint.ts          1.00      3.8±0.09ms    10.9 MB/sec    1.01      3.8±0.07ms    10.8 MB/sec
analyzer/parser.ts        1.05      9.4±0.25ms     5.2 MB/sec    1.00      8.9±0.22ms     5.5 MB/sec
analyzer/router.ts        1.01      6.3±0.22ms    10.0 MB/sec    1.00      6.2±0.18ms    10.1 MB/sec
analyzer/statement.ts     1.04      9.1±0.28ms     3.9 MB/sec    1.00      8.8±0.21ms     4.0 MB/sec
analyzer/typescript.ts    1.03     13.8±0.41ms     3.9 MB/sec    1.00     13.5±0.49ms     4.0 MB/sec

Nice!

[MichaReiser]

And there are now errors that Rome detects in the playground because of your fixes :D Awesome job