Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: references of Object.assign in prefer-object-spread #18148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: references of Object.assign in prefer-object-spread #18148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/src/rules/prefer-object-spread.md
View file
Open in desktop
Expand Up @@ -55,6 +55,9 @@ Object.assign(foo, bar);
Object.assign(foo, { bar, baz });

Object.assign(foo, { ...baz });

var foo = Object.assign;
var bar = foo({}, baz);
Comment on lines +59 to +60
Copy link
Contributor

@snitin315 snitin315 Feb 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should be reported because foo has never been reassigned.

Incorrect

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

let doSomething;

doSomething = Object.assign;

var bar = doSomething({}, a, b); // Error

Correct

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

let doSomething;

if (foo) {

doSomething = Object.assign;
} else {
doSomething = somethingElse
}
var bar = doSomething({}, a, b); // OK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like i have understood the issue wrong but what about autofixes is it also correct

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

let doSomething;

doSomething = Object.assign;

var bar = { ...a, ...b};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe the auto fix is also correct, doSomething will be reported by the no-unused-vars rule.

@mdjermanovic Can you once confirm the expected behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that ReferenceTracker returns both certain references (e.g., const x = Object.assign; x();) and potential references (those that are references only in some code paths, e.g., const x = foo ? Object.assign : bar; x();), which is okay for some rules (e.g., for no-obj-calls, because it's a bug if JSON() is called in any code paths) but not for this one.

We currently don't have a utility that would help find only certain references. The solution could be:

  1. Add an option to ReferenceTracker to not return potential references.
  2. Make another utility.
  3. Limit this rule to check only explicit Object.assign calls. Maybe also <global object>.Object.assign (like no-eval).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, changes in this PR falls in 3rd point so what should we do now, move forward or look for another approaches (point 1st and 2nd)?
the reason i chose 3rd one is i noticed in this rule's previous version is that references was used so that it allows the Object.assign if Object is importing from a module, and i didn't find any tests which doesn't call Object.assign directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still doubtful that this fix is correct and doesn't lead to bugs.

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

let doSomething;

doSomething = Object.assign;

var bar = { ...a, ...b};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could first ask at https://github.com/eslint-community/eslint-utils if option 1 would be possible. If not, then I think option 3 is fine.

```

:::
Expand Down
15 changes: 15 additions & 0 deletions lib/rules/prefer-object-spread.js
View file
Open in desktop
Expand Up @@ -239,6 +239,20 @@ function defineFixer(node, sourceCode) {
};
}

/**
* Limit the reference to the Object.assign.
* @param {ASTNode|null} node The node that the rule warns on, i.e. the Object.assign call
* @returns {boolean} true if the reference has Object.assign
*/
function isObjectAssignReference(node) {
return (
(
(node.callee.object && (node.callee.object.name === "Object" || node.callee.object.property.name === "Object")) &&
(node.callee.property && node.callee.property.name === "assign")
)
);
}

/** @type {import('../shared/types').Rule} */
module.exports = {
meta: {
Expand Down Expand Up @@ -276,6 +290,7 @@ module.exports = {
// Iterate all calls of `Object.assign` (only of the global variable `Object`).
for (const { node: refNode } of tracker.iterateGlobalReferences(trackMap)) {
if (
isObjectAssignReference(refNode) &&
refNode.arguments.length >= 1 &&
refNode.arguments[0].type === "ObjectExpression" &&
!hasArraySpread(refNode) &&
Expand Down
4 changes: 4 additions & 0 deletions tests/lib/rules/prefer-object-spread.js
View file
Open in desktop
Expand Up @@ -86,6 +86,10 @@ ruleTester.run("prefer-object-spread", rule, {
code: "class C { #assign; foo() { Object.#assign({}, foo); } }",
languageOptions: { ecmaVersion: 2022 }
},
{
code: "var foo = Object.assign; \n var bar = foo({}, baz);",
languageOptions: { ecmaVersion: 2022 }
},

// ignore Object.assign() with > 1 arguments if any of the arguments is an object expression with a getter/setter
"Object.assign({ get a() {} }, {})",
Expand Down