Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Correct
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
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 theno-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., forno-obj-calls
, because it's a bug ifJSON()
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:
ReferenceTracker
to not return potential references.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
ifObject
is importing from a module, and i didn't find any tests which doesn't callObject.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.
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.