prefer-object-spread invalid autofix with accessors

[mdjermanovic]

Tell us about your environment

• ESLint Version: 6.1.0
• Node Version: 10.16.0
• npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration

module.exports = {
  parserOptions: {
    ecmaVersion: 2018,
  },
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Example #1

/*eslint prefer-object-spread: "error"*/

const foo = { a:1 };

const bar = Object.assign(
  { 
    get a() { return this._a }, 
    set a(v) { this._a = v + 1 } 
  }, 
  foo
)

console.log(bar.a) // 2

Example #2

/*eslint prefer-object-spread: "error"*/

const foo = Object.assign(
  {}, 
  { 
    get a() { return this._a }, 
    set a(v) { this._a = v + 1 } 
  }
);

foo.a = 1;
console.log(foo.a) // 1

eslint index.js

What did you expect to happen?

Not to change behavior.

What actually happened? Please include the actual, raw output from ESLint.

Example #1

/*eslint prefer-object-spread: "error"*/

const foo = { a:1 };

const bar = {
  get a() { return this._a }, 
  set a(v) { this._a = v + 1 }, 
  ...foo
}

console.log(bar.a) // 1

Example #2

/*eslint prefer-object-spread: "error"*/

const foo = {
  get a() { return this._a }, 
  set a(v) { this._a = v + 1 }
};

foo.a = 1;
console.log(foo.a) // 2

Are you willing to submit a pull request to fix this bug?

Yes, just to define what should be done (no warning, warning without the fix or something else).

[ljharb]

The autofix did this, or you manually did?

If you want to preserve the behavior of reifying getters and setters, you’d need to do:

const foo = {
  ...{
    get a() { return this._a; },
    set a(v) { this._a = v + 1; }
  }
}

[mdjermanovic]

The autofix did this.

I guess it would be best to simply not fix the code at all if there are any getters/setters in source or target object literals?

Perhaps not even warn in Example 1. On the other hand, Example 2 is a possible error in user's code.

[ljharb]

Probably best to not autofix there at all, unless it can use the output i suggested.

[ljharb]

In general tho, I’d say if you’re relying on setters being triggered during an Object.assign call, that’s not something I’d let pass code review :-)

[mdjermanovic]

In general tho, I’d say if you’re relying on setters being triggered during an Object.assign call, that’s not something I’d let pass code review :-)

I agree, not my code just noted that the fix shouldn't change behavior, whatever it is.

An additional question might be should the rule warn at all in the first example?

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[mdjermanovic]

Reopening this since it's an invalid autofix (at least in the second example).

[platinumazure]

Marking as accepted since we definitely don't want to change behavior here if possible. Also marking as "needs bikeshedding" since we still need to figure out the desired behavior here.

[mdjermanovic]

After thinking about this again, I believe that prefer-object-spread shouldn't even report in any of these cases.

Even without the autofix, the rule would suggest using object literal/object spread instead, which is wrong as it isn't equivalent. Maybe there could be another rule to warn about using getters/setters with Object.assign (to disallow that style if it's intentional or find errors if it isn't).

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[ljharb]

I think this should be reopened?

[kaicataldo]

Assigning to myself so it doesn’t get autoclosed again, but please feel free to assign yourself @mdjermanovic.

[kaicataldo]

🤦‍♂ Reopened now.

[kaicataldo]

@mdjermanovic Are you still advocating for this change?

[ljharb]

This seems like it's a bugfix, so i'd hope it can still happen without additional advocacy :-)

[mdjermanovic]

Yes, fixing x = Object.assign({}, { get a(){} }) to x = { get a(){} } is certainly wrong.

I'll make a PR to ignore Object.assign if there are any getters/setters inside (as described in this comment), that seems like the most appropriate solution for this edge case.

Semi-related, __proto__ or super in source objects would cause a similar bug, but it's better to open a separate issue for these.