Did JavaScript anaylsis just get updated. I have 200 new code smells

Did an update get applied to the rules for JavaScript in SonarCloud.

My last branch and merge to master resulted in 200 new code smells in a React application. All the smells are down to this issue:

JSX props should not use arrow functions

These have been there for months and now have only started appearing as an issue.

Did something get updated as I am sure we have not made any changes to our Sonar config. We just use defaults withing the admin panel.

1 Like

Hey there.

javascript:S6480 was recently deployed on SonarCloud.

Are these issues being raised as new (instead of on Overall code)? Ideally, they should be backdated so that there isn’t noise in your New Code Period.

Looks like that is it. It affected both the latest commit - 14 warnings, last 30 days - 30 warnings and full code base, nearly 200.

So, looking at the description page here: RSPEC, it looks like this is also non compliant:

const handleDelete = () => {
    // delete code
};

<Button onClick={handleDelete}>

Basically the advice is use a regular function and not an arrow function like this:

function handleDelete() {
    // delete code
};

<Button onClick={handleDelete}>

Colin This rule seems overreaching to me. It seems ok when we consider .bind functions but not arrow functions.

As React docs states “Using an arrow function in render creates a new function each time the component renders, which may break optimizations based on strict identity comparison.

But for the majority of time React components don’t fall in that category, since memorizing components all around comes with a cost and its benefits for the most part are slim, or none.

Given that, now we face a rule that is covering exception cases. Even worse, now we have to wrap useCallback on these functions unnecessary, and the usage of useCallback itself comes with a cost.

4 Likes

Thanks for the feedback. I’ve flagged this for attention.

3 Likes

Since this change our builds seem to have really slowed down too. We are using Bitbucket. The last build we stopped it after 15 minutes. The current build has been running for an hour.

Here is the final part of the build log where Sonar is running:

INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/python
INFO: Read 0 type definitions
INFO: No UCFGs have been included for analysis.
INFO: Sensor PythonSecuritySensor [security] (done) | time=0ms
INFO: Sensor JsSecuritySensor [security]
INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: Read 0 type definitions
INFO: Reading UCFGs from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: 06:55:01.944254 Building Runtime Type propagation graph
INFO: 06:55:02.251601 Running Tarjan on 10106 nodes
INFO: 06:55:02.291766 Tarjan found 10106 components
INFO: 06:55:02.333819 Variable type analysis: done
INFO: 06:55:02.336878 Building Runtime Type propagation graph
INFO: 06:55:02.548116 Running Tarjan on 10106 nodes
INFO: 06:55:02.574083 Tarjan found 10106 components
INFO: 06:55:02.63977 Variable type analysis: done
INFO: Analyzing 1205 ucfgs to detect vulnerabilities.
INFO: Taint analysis starting. Entrypoints: 280
INFO: Running symbolic analysis for 'JS'

Before Sonar Scan step would take a 1-2 minutes to complete.

I just stopped the build that has been running for an hour, this is the last part of the output:

INFO: Sensor PythonSecuritySensor [security]
INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/python
INFO: Read 0 type definitions
INFO: No UCFGs have been included for analysis.
INFO: Sensor PythonSecuritySensor [security] (done) | time=0ms
INFO: Sensor JsSecuritySensor [security]
INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: Read 0 type definitions
INFO: Reading UCFGs from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: 07:13:16.888297 Building Runtime Type propagation graph
INFO: 07:13:17.258845 Running Tarjan on 10106 nodes
INFO: 07:13:17.330553 Tarjan found 10106 components
INFO: 07:13:17.418892 Variable type analysis: done
INFO: 07:13:17.421627 Building Runtime Type propagation graph
INFO: 07:13:17.561611 Running Tarjan on 10106 nodes
INFO: 07:13:17.606656 Tarjan found 10106 components
INFO: 07:13:17.669776 Variable type analysis: done
INFO: Analyzing 1205 ucfgs to detect vulnerabilities.
INFO: Time spent writing ucfgs 979ms

I can send it all if it helps. This is a React/JS app without TypeScript if that makes any different.

I’ve also ran into this new rule which seems a little overreaching. For the better part of the previous year, this is the first time i’m seeing this pop up after upgrading to 9.8 of the community edition.

From the SonarCloud spec:

Each time the parent is rendered, the function will be re-created and trigger a render of the component causing excessive renders and more memory use.

To add to rbuzatto’s point, the precondition for this statement is only relevant for class-based components extended from PureComponent, and function components wrapped in React.memo. For the vast majority of circumstances, every component is always rerendered below a rerendering parent, to negligible impact.

See Mark Ericson’s excellent blog post on rendering.

I don’t know if SonarCloud can go this deep when spidering code, but this rule might make sense if it can confirm that the component being sent the arrow function is memoized with extends PureComponent or React.memo(). Otherwise it’s an inaccurate warning.

1 Like

Re: the build failures. I needed to increase the memory allocation in Bitbucket. I was doing it wrong. It might be that the latest update scan is also consuming more memory.

Hi @jeffski,

Thank you for reporting back what worked for you :+1:

Do you mind sharing the memory config before/after and an approximation of your project’s amount of LOC ?

Best,
Gabriel

Colin, do you think the rule can be disabled? I join the comment of Rafael.

Hello @RomainJoly,

We have decided to disable the S6480 rule by default for now, until we can get more information and improve the rule. It can still be enabled manually.

See the Github issue:

Also another thread on the same topic:

For clarification, the memory issue faced by @jeffski has no relation to this particular rule.

Cheers,
Gabriel

3 Likes

There are a couple of incorrect assumptions in that rule:

  • The impact of using the arrow function in built-in components (like div) is negligible, and adding useCallback to every function -as the rule description sugggest- is worse (it uses more memory as it adds an internal state for the cache and a dependency check).
  • A component may use a callback proxy to avoid re-renders in a callback (see this discussion in the React repo: https://github.com/facebook/react/issues/14099#issuecomment-440013892)
  • The are multiple strategies to avoid re-renders. Depending on how complex the component is, memoizing callbacks may not help. Another strategy is to memoize the whole sub-tree (for example One simple trick to optimize React re-renders).

Probably it’s better to remove the rule or at least re-word it (the rule description is technically wrong).