Make isReferenced return false for method parameter name
#11089
Conversation
nicolo-ribaudo
added
pkg: types
PR: Bug Fix 🐛
| if (param === node) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
What about using parent.params.indexOf(node) !== -1?
There was a problem hiding this comment.
Ah, yeah, that's much better. Updated.
| @@ -66,6 +67,9 @@ export default function isReferenced( | |||
| if (parent.value === node) { | |||
| return !grandparent || grandparent.type !== "ObjectPattern"; | |||
| } | |||
| if (parent.params && parent.params.indexOf(node) !== -1) { | |||
There was a problem hiding this comment.
| if (parent.params && parent.params.indexOf(node) !== -1) { | |
| if (parent.params.includes(node)) { |
There was a problem hiding this comment.
@nicolo-ribaudo you asked me to remove the use of .includes for the sake of Node 6 support, but now that I check, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes indicates it's supported there. Is using .includes ok?
There was a problem hiding this comment.
Oh I thought that it wasn't 😅
If it works, it's ok to use it.
There was a problem hiding this comment.
Cool, I checked a node 6 repl and .includes seems to work.
@jridgewell removing the check for whether parent.params is present before calling .includes, does mean an error would be thrown if we reach this line when parent is an ObjectProperty or ClassProperty. The only way that could happen is if node is neither the key nor the value of the parent Object/ClassProperty, which I think currently could only happen if it's a decorator or type annotation. I don't know whether it's important for isReferenced to be able to handle being called with non-identifier-like nodes such as Decorator.
There was a problem hiding this comment.
What about refactoring the switch like this then?
case "ClassMethod":
case "ClassPrivateMethod":
case "ObjectMethod":
check parent.params.includes(node);
/* falls through */
case "ObjectProperty":
case "ClassProperty":
case "ClassPrivateProperty":
// all the existing checksThere was a problem hiding this comment.
That works, updated.
I'm now getting some failing tests in babel-preset-env related to "edge" being 18 instead of the fixture's expectation of 17, but I get them on master too - I'm guessing it was caused by a new version of caniuse-lite being released today.
…exists Co-Authored-By: Justin Ridgewell <justin@ridgewell.name>
| case "ClassMethod": | ||
| case "ClassPrivateMethod": | ||
| case "ObjectMethod": | ||
| if (parent.params.includes(node)) { |
There was a problem hiding this comment.
May want to add that a comment that fall-through is intentional
nicolo-ribaudo
changed the title
isReferenced return false for method parameter name
* Change isReferenced to return false for object/class method parameter names. * use indexOf instead of for-of loop * replace `.indexOf` check with `.includes` and assume `parent.params` exists Co-Authored-By: Justin Ridgewell <justin@ridgewell.name> * check .params within case block for ClassMethod/ClassPrivateMethod/ObjectMethod only * add comment clarifying that case clause fall-through is intentional Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
github-actions
bot
added
the
outdated
The
isReferencedmethod (if I understand correctly) tries to determine if a given node is a reference to the value stored in some variable, as opposed to being e.g. a declaration of a variable or the left-hand-side of an assignment. As described in issue #11087, it currently misidentifies method parameter names as references; this fixes that.The logic in
isReferencedis based on the type of the parent node, and currently Object/ClassProperty and Object/Class(Private)Method parent nodes are handled by the same case block. That block checks for when the child node is thekeyof the property/method node, and for when it's part of an ObjectPattern, but otherwise returns true. This change adds a check for when the child node is part of theparamslist of the parent node.