feat(rome_js_analyze): noAccumulatingSpread

[Vivalldi]

Summary

From a performance standpoint, spread on an accumulator is terrible,O(n^2) (see discussion for details). The approach I took in this rule is to query on JsSpread nodes. Initially we're only covering cases where the node is spreading a known accumulator (those found in .reduce & .reduceRight).

Discussion #4320

Future Ideas

At some point I'd like to cover cases where spreading is being done on any accumulator, such as in the case of for loops

// invalid
let acc = [];
for (bar in foo) {
  acc = [...acc, bar];
}

// valid
const acc = [];
for (bar in foo) {
  acc.push(bar);
}

Test Plan

Since this rule queries on spread operators I've added tests that cover known invalid cases while also ensuring that valid uses of spread are maintained.

• Known .reduce cases
• Known .reduceRight cases

Changelog

• The PR requires a changelog line

Documentation

• The PR requires documentation
• I will create a new PR to update the documentation

[netlify]

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	9a10db3
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/644fbdfc7860f60008aee819
😎 Deploy Preview	https://deploy-preview-4426--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[ematipico]

@Vivalldi if you rebase your PR, the tests CI should correctly run! There was some issue with the CI

[Vivalldi]

@Vivalldi if you rebase your PR, the tests CI should correctly run! There was some issue with the CI

Thanks! Rebased and hopefully it works out

[ematipico]

There are some cases that we should check, to avoid possible false positives.

@Vivalldi if you agree and if you'd like to follow-up, we can merge this PR and you could make another PR to supplement these new cases. Or, you could do it now in this PR.

I would choose the first option, but let the decision to you!

[ematipico]

Can we add an example where .reduce is not a built-in function? For example:

var a = {
	reduce() {}
};
a.reduce();

[ematipico]

I believe, among the use cases, we should have somewhere we check that .reduce and .reduceRight are actually called on arrays.

For example, e case like this should be valid:

var a = { reduce() {} }
a.reduce();

Or

class A { reduce() {} }
var a = new A();
a.reduce()

[Vivalldi]

@ematipico, I agree with the additional cases. Happy to get this PR in and I'll update test cases in a follow up