Should we forbid multiple assignments in one statement?

[samreid]

@zepumph and @samreid were tricked by this multi-assignment in chipper (it was cleverly hidden amongst thousands of single-assignment lines):

const fileMap = stringFilesContents[ repository.name ][ locale ] = {};

and it crossed our minds that maybe we should prevent multi-assignment using eslint. We tried adding the lint rule 'no-multi-assign':2, and found 324 lint errors, in repos such as scenery, hookes-law, density-buoyancy-common, trig-tour and many others. Should we turn on this lint rule, or should everyone get used to the idea of multiple assignment?

[zepumph]

The lint rule looks really cool! https://eslint.org/docs/rules/no-multi-assign

[pixelzoom]

+1 for preventing multiple assignment. It's a useful feature. But there are other gotchas (like scope) that aren't mentioned here. See for example https://www.undefinednull.com/2014/02/03/multiple-left-hand-assignment-in-javascript-is-really-bad-think-once-before-you-do-it/.

[jessegreenberg]

+1 for the lint rule

[pixelzoom]

After looking as usages (and eliminating a few, see above commits), I've changed my mind. There are places where multiple assignment is useful, clearer, and easier to maintain.

For example, in GRAPHING_LINES/PointSlopeEquationNode, parts of a point-slope equation need to be made visible and styled based on certain conditions, and that's done using multiple assignment like this:

188 yNode.visible = equalsNode.visible = xNode.visible = true;
189 yNode.fill = equalsNode.fill = xNode.fill = lineColor;
...
196 yNode.visible = equalsNode.visible = slopeMinusSignNode.visible = xNode.visible = true;
197 yNode.fill = equalsNode.fill = slopeMinusSignNode.fill = xNode.fill = lineColor;

Compared to the alternative:

yNode.visible = true;
equalsNode.visible = true;
xNode.visible = true;
yNode.fill = lineColor;
equalsNode.fill = lineColor;
xNode.fill = lineColor;
...
yNode.visible = true;
equalsNode.visible = true;
slopeMinusSignNode.visible = true;
xNode.visible = true;
yNode.fill = lineColor;
equalsNode.fill = lineColor;
slopeMinusSignNode.fill = lineColor;
xNode.fill = lineColor;

So -1 for prohibiting multiline assignment. +1 for learning when to use it, and what the pitfalls are.

[samreid]

Here's another alternative for discussion:

[ yNode, equalsNode, slopeMinusSignNode, xNode ].forEach( node => node.setVisible( true ) );

[pixelzoom]

Let's compare these side-by-side:

yNode.visible = equalsNode.visible = slopeMinusSignNode.visible = xNode.visible = true;

[ yNode, equalsNode, slopeMinusSignNode, xNode ].forEach( node => node.setVisible( true ) );

-1 for replacing multiple assignment with forEach -- it's more difficult to write, more difficult to read, and undoubtedly more expensive.

forEach also does not work for situations like:

line.stroke = rectangle.fill = newColor;

[samreid]

@pixelzoom how about enabling the lint rule, and applying a // disable-eslint-line directive for special cases?

[pixelzoom]

I've looked through most of the usages flagged by the lint rule. The case you provided in #794 (comment) was by far the most inappropriate. phetsims/sun@7778ef1 was a distance second, and only because it resulted in 2 missing visibility annotations. So imo, this is a non-problem.

[jessegreenberg]

I never use multiple assignments on one statement because I thought it was disallowed by our style guidlines. This isn't listed in https://github.com/phetsims/phet-info/blob/master/checklists/code_review_checklist.md#coding-conventions anymore, but I found that it used to be in our old "Coding Style Guidelines"document. I didn't know (or forgot) it was removed.

EDIT: I guess I can't link to old versions of teh google doc, but it used to say

Variables should be declared on one line with a separate ‘var’ in front of each. This makes moving and duplicating lines much easier

[pixelzoom]

Variables should be declared on one line with a separate ‘var’ in front of each. This makes moving and duplicating lines much easier

Multiple declaration is not equivalent to multiple assignment. Multiple declaration should be avoided (or used very carefully) because of the "scope" pitfall described in https://www.undefinednull.com/2014/02/03/multiple-left-hand-assignment-in-javascript-is-really-bad-think-once-before-you-do-it/.

I haven't seen any multiple declaration in the cases identified by the lint rule, but it's possible I missed it.

[samreid]

We could use the eslint-rule one-var for detecting multiple declaration.

[zepumph]

We could use the eslint-rule one-var for detecting multiple declaration.

we already do, see chipper/eslint/.eslintrc.js

[jessegreenberg]

we already do, see chipper/eslint/.eslintrc.js

That explains why it was removed from the style guidelines.

So is the example in #794 (comment) a multiple declaration or multiple assignment? Or both? Is that why that one is confusing while examples in #794 (comment) are OK?

[pixelzoom]

The example in #794 (comment) is one declaration and one assignment mixed together. Declaration of fileMap, assignment to stringFilesContents. Not recommended.

The examples in #794 (comment) are all multiple assignment. And again, those should be used judiciously, when it's clear.

[chrisklus]

From 10/31/19:

We are fine with allowing multiple assignments and fine with preventing multiple assignment within a declaration.

@zepumph volunteered to 1. See if a lint rule for this already exists, and if not 2. Investigate creating one, and if that doesn't work out, then 3. Add this to the CRC.

[zepumph]

I added a custom lint rule above. It forbids assignments within a variable declaration. It caught about 15 usages, and I think this is an improvement. Closing

[zepumph]

Actually, I just remembered a comment from @samreid over in #739 (comment). His recommendation was to try to add this back into the eslint project. That one fell off my radar, and now I've forgotten how the rule was manipulated, but this one would probably be a good candidate for making an issue over in eslint.

[zepumph]

I submitted an issue over in eslint, eslint/eslint#12545. I will wait to see how to proceed.

[zepumph]

I'm going to unassign this for now, since nothing has happened from eslint just yet. I have no action steps here.

[zepumph]

The eslint issue ,https://github.com/eslint/eslint/issues/12545, has been adopted and closed, as soon as a new version is put out, we can update to that version and use that instead of our own hacked lint rule.

[zepumph]

eslint/eslint#12545 is complete, and https://eslint.org/docs/latest/rules/no-multi-assign#options has been pushed to production. I am able to remove our custom lint rule. Lint Everything is passing! Yay open source! Closing.