feat(rome_js_analyze): noAssignInExpressions

[Conaclos]

Summary

Closes #4013

This is an exclusive rule for Rome!
This disallows any assignment in an expression.

This covers two ESLint's rules: no-cond-assign and no-return-assign.

Implemenattion alternatives

The rule could be relaxed to allow assignment of assignment such as:

let a, b;
a = b = 5;

and void-assign:

let a;
void (a = 5);

I could keep the rule as is and open an issue to gather interest.

Test Plan

Unit tests and doc tests included.

[netlify]

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	5637bcb
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/6392262c22028b000970af19
😎 Deploy Preview	https://deploy-preview-3928--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.

[xunilrj]

Nice! Thanks for the contribution.
I have thought about some cases. What do you think?

for (let a = 0; a < 10; a+= 5) {
}

const f = (a) => a+=1;

[Conaclos]

for (let a = 0; a < 10; a+= 5) {}

Thanks for the suggestion! I will add this case.

const f = (a) => a+=1;

I am more doubtful about this one. Indeed, a reader could think that the previous code is equivalent to the following:

const f = (a) => { a+=1; };

While it is equivalent to:

const f = (a) => { a+=1; return a; };

Moreover, it could be confusing regarding the rule name. Although this could be renamed to noConfusingAssign.

[leops]

How does this new rule interact with the noConditionalAssignment rule ? We wouldn't want to emit two diagnostics for if(a = b) {}, maybe it would be possible to merge them ?

[Conaclos]

@leops Thanks for the info, I did not notice this rule.

I could suggest of removing noConditionalAssignment from the recommended rule set. And so waiting for potential user feedback of noAssignInExpressions. Then we could remove noConditionalAssignment. What do you think?

[Conaclos]

By the way, I could add the code action of noConditionalAssignment.

EDIT: done.

[leops]

I could suggest of removing noConditionalAssignment from the recommended rule set. And so waiting for potential user feedback of noAssignInExpressions. Then we could remove noConditionalAssignment. What do you think?

noConditionalAssignment is still in the nursery group as it was recently introduced as part of the v11 release cycle, so it wouldn't be a breaking change to merge it functionality into noAssignInExpressions and remove it as part of this PR.

[Conaclos]

@leops done :)

[ematipico]

Please, let's not delete a rule from one release to another; I think it sends an incorrect signal to the users.

We should at least deprecate the rule first and delete it in the next release cycle.

We have an API RuleDiagnostic::new().deprecated(), and we can add a footer note telling users to use another rule. Ultimately, we should remove it from the recommended rules and update the documentation, telling users that the rule is deprecated and asking them to use the new one

[leops]

Please, let's not delete a rule from one release to another; I think it sends an incorrect signal to the users.

We should at least deprecate the rule first and delete it in the next release cycle.

Is this really necessary if the rule is still under the nursery group ? As I understand it, having rules clearly identified as unstable would allow us to make important changes in the way those rules work without having to worry too much about the whole braking change process

[ematipico]

Is this really necessary if the rule is still under the nursery group ?

That's on me, sorry. I didn't see that this rule was under nursery. I suppose we can nuke it without problems!

[Conaclos]

As I understand it, having rules clearly identified as unstable would allow us to make important changes in the way those rules work without having to worry too much about the whole braking change process

The only drawback I see could be breaking a config file.

[ematipico]

As I understand it, having rules clearly identified as unstable would allow us to make important changes in the way those rules work without having to worry too much about the whole braking change process

The only drawback I see could be breaking a config file.

We'll soon deliver a way to auto-magically make migration paths automatic, so this won't be a problem anymore

[Conaclos]

I updated the branch.
I relaxed the rule for one more case: multiple initialization. The following code is now allowed:

let a, b;
a = b = 1;

[Conaclos]

Ready for merging

[xunilrj]

I updated the branch. I relaxed the rule for one more case: multiple initialization. The following code is now allowed:

let a, b;
a = b = 1;

The problem is that we now support pretty complex assignments. For example:

a = (b = 1, b = 2)

Even worse. Given that we support the assignment to live inside sequences, we can have

a = (<ANYTHING>, b = 2, <ANYTHING>)

crazy example

a = (class {}, b = 2, function() {});

[Conaclos]

My reasoning was that a recommended rule could disallow sequences (except in for updates and init).

This seems good enough to my taste.
However, I can update the code to explicitly rule out these cases.

[ematipico]

My reasoning was that a recommended rule could disallow sequences (except in for updates and init).

This seems good enough to my taste. However, I can update the code to explicitly rule out these cases.

One problem I could see is that this rule will be recommended, while the suggested rule no-sequence is not part of the recommended rules from ESLint.

[Conaclos]

One problem I could see is that this rule will be recommended, while the suggested rule no-sequence is not part of the recommended rules from ESLint.

Makes sense. I updated the code to rule out the cases you pointed out.