Rule idea: no-multi-assign

[silverwind]

Proposing a new rule which forbids multiple assignments in a single expression.

Code that should trigger a warning:

a = b = c;

Code that should not trigger a warning:

b = c;
a = c;

They're hard to read and it's not immediately clear what is assigned to what. In above example, one could think that the order of evaluation is left-to-right, while it actually is right-to-left.

[ilyavolodin]

Hmm... It's a valid rule, but I'm not 100% sure it's important enough to be included in the core. We are trying to limit new rules for core to "must-haves", I'm not sure it meets the bar. @eslint/eslint-team thoughts?

[mikesherov]

I really like this as a core rule. 👍

[michaelficarra]

@silverwind Evaluation order is very much left-to-right in assignment.

z = x = {};
y = {};
x.a = x = y;
x === y; // true
z.a === y; // true

[silverwind]

@michaelficarra maybe I was unclear, but it definitely assigns the right side first:

o = {};
Object.defineProperty(o, "a", {set: function() { console.log("set a"); } });
Object.defineProperty(o, "b", {set: function() { console.log("set b"); } });
o.a = o.b = 1
// prints:
// set b
// set a

[mysticatea]

Maybe x.a = x = y; evaluates as the following flow.

// x.a = x = y;

// save references of destinations by left-to-right.
d1 = &x.a
d2 = &x

// assign the value to the references by right-to-left.
*d2 = y;
*d1 = y;

I guess that @michaelficarra's concern is that a = b = c and b = c; a = c; are not always same.

[mysticatea]

let x = {}
let obj = {
    get x() {
        console.log("get obj.x")
        return this._x
    },
    set x(value) {
        console.log("set obj.x", value)
        this._x = value
    },

    get y() {
        console.log("get obj.y")
        return this._y
    },
    set y(value) {
        console.log("set obj.y", value)
        this._y = value
    },

    _x: x,
    _y: x
}

obj.x.x = obj.y.y = obj.x = obj.y = 1

console.log(x)

get obj.x
get obj.y
set obj.y 1
set obj.x 1
{ y: 1, x: 1 }

[SpadeAceman]

Wow @mysticatea – that example hurts my brain!

I've been using my own custom-written version of this rule since I saw it suggested here. It's helped me identify and clean up some confusing code I'd written a while back, so to me it seems like a good core rule to have. (Using multiple assignments can seem like a good implementation of DRY, but IMO what it gains in being concise it loses in being less than clear down the road.)

[platinumazure]

This seems simple enough to implement and could be useful (as a stylistic rule).

One thing that would probably help rustle up some 👍s from the team is if you can find a prevalent style guide or a JSCS rule which disallows multiple assignments.

[nzakas]

Unless someone steps forward to champion this rule, it's time to close this issue.

[silverwind]

There's another unmentioned risk of multiple assignment, which is the creation of unintended globals, like b here:

let a = b = 2;

But I've just tried it and no-undef correctly identifies the issue, so that somewhat devalues this possible rule in my eyes.

@nzakas Let's keep this issue open a few more days, and close it off as rejected if no strong case can be presented.

[mysticatea]

Actually, I don't oppose this rule. Though I like a rule which has always/never options since I think this is a stylistic rule.

If someone champions this rule, I'll support.

[SpadeAceman]

An example of an issue only this rule could catch:

a += b += 2 // the second += is most likely a mistake

a += b + 2 // this is likely what was intended

As for precedent, aside from Python, a number of recently-created languages also treat assignments as statements, or expressions which return no value:

• Go (section 12, Semantics: "assignment is not an expression")
• Rust ("assignments always evaluate to ()" – an empty tuple)
• Swift ("the assignment operator in Swift does not itself return a value", "Swift already deviates from C in that the =, += and other assignment-like operations return Void (for a number of reasons).")

While I wasn't able to find more info on why these these language design decisions were made, it seems significant to me that they all reject the concept of treating assignments as expressions.

[michaelficarra]

@mysticatea What would an always option for this rule even mean?

@SpadeAceman Thanks for the perspective. Redefining this rule as "allow assignments only in statement position" makes it much more appealing. 👍

[platinumazure]

@michaelficarra Does "statement position" also include loop initialization/afterthought and conditional tests, in your description? We would want to be very careful not to ruin for loops, for example.

[mysticatea]

@michaelficarra

never

// ✔ GOOD
a = 1
b = 1

// ✘ BAD
a = b = 1

always

// ✔ GOOD
a = b = 1

// ✘ BAD
a = 1
b = 1

---

Ah, I had not considered about +=.
The rule to prevent a += b = c sounds good to me.

[michaelficarra]

@platinumazure Anywhere the value is thrown away. So even void (a = b) is technically fine by me, and for(a = b;;) should be allowed, though I don't see why anyone would write that.

@mysticatea Ah, so always means adjacent assignments to the same primitive value or reference? I don't think that's needed.

[mysticatea]

@michaelficarra OK.

So, I guess this is no-multi-assign which forbids (node.type === "AssignmentExpression" && node.parent.type === "AssignmentExpression").

[platinumazure]

@mysticatea What about AssignmentExpression with parent of VariableDeclarator? That would look like a "multiple assignment" too, IMO.

[ljharb]

VariableDeclarators are asked about in #6932

[mysticatea]

@platinumazure Sounds good. (node.type === "AssignmentExpression" && /^(?:Assignment(?:Expression|Pattern)|VariableDeclarator)$/.test(node.parent.type))

[alberto]

@eslint/eslint-team last call for a champion or else we should close it

[platinumazure]

I'll champion.

[nzakas]

@platinumazure you need to drum up two more 👍s.

[platinumazure]

Maybe just one? I'm counting @mysticatea and @michaelficarra.

@mysticatea, @michaelficarra Are you endorsing this?

[nzakas]

I can't count (I think we may need to standardize where we 👍 to make this easier). It also looks like there was a 👍 from @mikesherov and @IanVS, so marking as accepted.

[kaicataldo]

@platinumazure Are you still championing this?

[platinumazure]

Well, I want to, but with the caveat that I can't actively drive anything here for a few weeks. I had forgotten this, probably because I forgot to assign this to me. We've gotten a recent request for this as well, so can we keep it open?

…

On Jan 7, 2017 11:36 PM, "Kai Cataldo" ***@***.***> wrote: @platinumazure <https://github.com/platinumazure> Are you still championing this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6424 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWerBtGU-zMdJWxxMVd06aEji-KjQGks5rQHXVgaJpZM4I2xeU> .

[kaicataldo]

I'm up for keeping it open! Just wanted to make sure it still had a champion :) Thanks!

[stewx]

I will be submitting a PR for this shortly.

[stewx]

PR here: #7904