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.
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
fix: references of Object.assign in prefer-object-spread
#18148
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
var foo = Object.assign; | ||
var bar = foo({}, baz); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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};
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Add an option to
ReferenceTracker
to not return potential references. - Make another utility.
- Limit this rule to check only explicit
Object.assign
calls. Maybe also<global object>.Object.assign
(likeno-eval
).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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};
There was a problem hiding this comment.
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.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What rule do you want to change?
prefer-object-spread
What change do you want to make (place an "X" next to just one item)?
[ ] Generate more warnings
[x] Generate fewer warnings
[ ] Implement autofix
[ ] Implement suggestions
How will the change be implemented (place an "X" next to just one item)?
[ ] A new option
[ ] A new default behavior
[x] Other
Please provide some example code that this change will affect:
What does the rule currently do for this code?
Rule reports and fixes the
doSomething({}, a, b)
as it's a reference of Object.assignWhat will the rule do after it's changed?
Rule will not report
doSomething({}, a, b)
and only report theObject.assign({}, a)
andGlobalThis.Object.assign({}, a)
Is there anything you'd like reviewers to focus on?
Fixes #12826