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

id in import attributes should not be referenced #13733

Merged
merged 4 commits into from Sep 7, 2021
Merged

id in import attributes should not be referenced #13733

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cfdbdf
fix ts-expect-error
JLHwung Sep 3, 2021
d8bf4f5
chore: reformat tests
JLHwung Sep 3, 2021
ee7a9e8
fix: id in import attributes should not be referenced
JLHwung Sep 3, 2021
a9335f3
review comments
JLHwung Sep 6, 2021
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
33 changes: 20 additions & 13 deletions packages/babel-types/src/validators/isReferenced.ts
View file
Open in desktop
Expand Up @@ -13,14 +13,14 @@ export default function isReferenced(
// yes: NODE.child
// no: parent.NODE
case "MemberExpression":
case "JSXMemberExpression":
case "OptionalMemberExpression":
if (parent.property === node) {
// @ts-expect-error todo(flow->ts): computed is missing on JSXMemberExpression
return !!parent.computed;
}
return parent.object === node;

case "JSXMemberExpression":
return parent.object === node;
// no: let NODE = init;
// yes: let id = NODE;
case "VariableDeclarator":
Expand All @@ -44,31 +44,34 @@ export default function isReferenced(
case "ClassMethod":
case "ClassPrivateMethod":
case "ObjectMethod":
// @ts-expect-error todo(flow->ts) params have more specific type comparing to node
if (parent.params.includes(node)) {
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR we checked key and params and return true if node is neither in params nor in key. But the return true branch is unreachable, unless the input node does not have parental relationships with parent.

if (parent.key === node) {
return !!parent.computed;
}
// fall through
return false;

// yes: { [NODE]: "" }
// no: { NODE: "" }
// depends: { NODE }
// depends: { key: NODE }
// fall through
case "ObjectProperty":
if (parent.key === node) {
return !!parent.computed;
}
if (parent.value === node) {
return !grandparent || grandparent.type !== "ObjectPattern";
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

When can the node be neither the key nor the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! node could be neither key nor the value when parent is not the actual parent of node. This is actually ill-defined behaviour of t.isReferenced(node, parent): It returns true if the parent.type is not handled by current logic and return false if handled since the conditions may never holds. Unless we change the interface to isReferenced(NodePath), I don't think we can mitigate such issues.

I have updated ObjectProperty handling to align with other types such as ClassProperty.

// no: class { NODE = value; }
// yes: class { [NODE] = value; }
// yes: class { key = NODE; }
// fall through
case "ClassProperty":
case "ClassPrivateProperty":
if (parent.key === node) {
// @ts-expect-error todo(flow->ts): computed might not exist
return !!parent.computed;
}
// @ts-expect-error todo(flow->ts): ObjectMethod does not have value property
if (parent.value === node) {
return !grandparent || grandparent.type !== "ObjectPattern";
return true;
case "ClassPrivateProperty":
if (parent.key === node) {
return false;
}
return true;
JLHwung marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -136,6 +139,10 @@ export default function isReferenced(
case "ImportSpecifier":
return false;

// no: import "foo" assert { NODE: "json" }
case "ImportAttribute":
return false;

// no: <div NODE="foo" />
case "JSXAttribute":
return false;
Expand Down
49 changes: 38 additions & 11 deletions packages/babel-types/test/validators.js
View file
Open in desktop
Expand Up @@ -122,20 +122,38 @@ describe("validators", function () {
expect(t.isReferenced(node, parent)).toBe(true);
});

it("returns true if node is a value of ObjectProperty of an expression", function () {
const node = t.identifier("a");
const parent = t.objectProperty(t.identifier("key"), node);
const grandparent = t.objectExpression([parent]);
describe("ObjectProperty", () => {
it("returns true if node is a value of ObjectProperty of an expression", function () {
const node = t.identifier("a");
const parent = t.objectProperty(t.identifier("key"), node);
const grandparent = t.objectExpression([parent]);

expect(t.isReferenced(node, parent, grandparent)).toBe(true);
});
expect(t.isReferenced(node, parent, grandparent)).toBe(true);
});

it("returns false if node is a value of ObjectProperty of a pattern", function () {
const node = t.identifier("a");
const parent = t.objectProperty(t.identifier("key"), node);
const grandparent = t.objectPattern([parent]);
it("returns false if node is a value of ObjectProperty of a pattern", function () {
const node = t.identifier("a");
const parent = t.objectProperty(t.identifier("key"), node);
const grandparent = t.objectPattern([parent]);

expect(t.isReferenced(node, parent, grandparent)).toBe(false);
});

it("returns true if node is computed property key of an expression", function () {
const node = t.identifier("a");
const parent = t.objectProperty(node, t.identifier("value"), true);
const grandparent = t.objectExpression([parent]);

expect(t.isReferenced(node, parent, grandparent)).toBe(false);
expect(t.isReferenced(node, parent, grandparent)).toBe(true);
});

it("returns true if node is computed property key of a pattern", function () {
const node = t.identifier("a");
const parent = t.objectProperty(node, t.identifier("value"), true);
const grandparent = t.objectPattern([parent]);

expect(t.isReferenced(node, parent, grandparent)).toBe(true);
});
});

describe("TSPropertySignature", function () {
Expand Down Expand Up @@ -269,6 +287,15 @@ describe("validators", function () {
expect(t.isReferenced(node, parent, grandparent)).toBe(true);
});
});

describe("import attributes", function () {
it("returns false for import attributes", function () {
const node = t.identifier("foo");
const parent = t.importAttribute(node, t.stringLiteral("bar"));

expect(t.isReferenced(node, parent)).toBe(false);
});
});
});

describe("isBinding", function () {
Expand Down