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.

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

id in import attributes should not be referenced #13733

Merged
merged 4 commits into from Sep 7, 2021

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 3, 2021

Q                       A
Fixed Issues? t.isReferenced returns true for id in import attributes, e.g. the type in import json from "./foo.json" assert { type: "json" }
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR starts from (todo) flow->ts cleanups. Found this bug when reading source.

Also simplified the class elements handling. All the fall through annotations are removed.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: types labels Sep 3, 2021
@@ -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.

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 3, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48560/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 3, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a9335f3:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

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.

@JLHwung JLHwung merged commit d87a3d9 into babel:main Sep 7, 2021
@JLHwung JLHwung deleted the fix-is-referenced branch September 7, 2021 12:29
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants