Fix: camelcase duplicate warning bug (fixes #10801) #10802
Merged
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.
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
See #10801 for description of behavior
What changes did you make? (Give an overview)
Fixed the logic in
lib/rules/camelcase.js
, and added a test that would've been broken before the fixBasically, there appears to have been faulty logic introduced by #10373:
This guard (pre-#10373) was preventing reporting the
key
of aProperty
:Then #10373 replaced that guard with this:
The hole introduced in the logic is if
assignmentKeyEqualsValue
butnode.parent.key !== node.parent.value
, thevalue
gets reported twice (once while visiting thekey
and once while visiting thevalue
)I believe that the only reason existing
invalid
tests likevar { category_id } = query;
weren't failing on duplicate warnings is that the default parser (espree
?) must reuse the sameIdentifier
node for thekey
andvalue
of a shorthandProperty
(as opposed to the Coffeescript custom parser I'm working on, which doesn't reuse the same node, thus how I found this bug)But then by adding an
invalid
test forvar { category_id: category_id } = query;
, you get a duplicate warning (it exposes the hole in the logic, sinceassignmentKeyEqualsValue
but theProperty
'skey
andvalue
are two differentIdentifier
objects)So the fix is basically to restore the pre-#10373 guard logic so that you effectively never visit the
Property
key
Is there anything you'd like reviewers to focus on?