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

Update detection of pure nodes (Scope#isPure) #14424

Merged
merged 8 commits into from Apr 9, 2022
Merged

Update detection of pure nodes (Scope#isPure) #14424

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
df1bb80
extract purity test
JLHwung Apr 5, 2022
ac872bb
update scope.isPure
JLHwung Apr 5, 2022
a954708
update test fixtures
JLHwung Apr 5, 2022
723318f
mark decorated class(elements) as impure
JLHwung Apr 6, 2022
5da9f64
add constantsOnly test
JLHwung Apr 6, 2022
d9b69e0
update isPure type definitions
JLHwung Apr 6, 2022
18c5ad7
remove unreachable codes
JLHwung Apr 6, 2022
02a010c
private-in is pure
JLHwung Apr 6, 2022
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
45 changes: 39 additions & 6 deletions packages/babel-traverse/src/scope/index.ts
View file
Open in desktop
Expand Up @@ -42,6 +42,13 @@ import {
unaryExpression,
variableDeclaration,
variableDeclarator,
isRecordExpression,
isTupleExpression,
isClassAccessorProperty,
isObjectProperty,
isDecorator,
isTopicReference,
isMetaProperty,
} from "@babel/types";
import type * as t from "@babel/types";
import { scope as scopeCache } from "../cache";
Expand Down Expand Up @@ -535,7 +542,7 @@ export default class Scope {
*/

isStatic(node: t.Node): boolean {
if (isThisExpression(node) || isSuper(node)) {
if (isThisExpression(node) || isSuper(node) || isTopicReference(node)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A topic reference should be treated as a constant IdentifierReference, therefore it is static and pure.

return true;
}

Expand Down Expand Up @@ -823,10 +830,21 @@ export default class Scope {
if (!binding) return false;
if (constantsOnly) return binding.constant;
return true;
} else if (
isThisExpression(node) ||
isMetaProperty(node) ||
isTopicReference(node)
) {
return true;
} else if (isClass(node)) {
if (node.superClass && !this.isPure(node.superClass, constantsOnly)) {
return false;
}
if (node.decorators) {
for (const decorator of node.decorators) {
if (!this.isPure(decorator, constantsOnly)) return false;
Copy link
Member

Choose a reason for hiding this comment

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

If there is a decorator, it's never pure.

i.e.

const dec = () => console.log(1);

@dec
class A {}

here @dec is pure because dec is a constant, but then it's evaluated.

}
}
return this.isPure(node.body, constantsOnly);
} else if (isClassBody(node)) {
for (const method of node.body) {
Expand All @@ -838,24 +856,39 @@ export default class Scope {
this.isPure(node.left, constantsOnly) &&
this.isPure(node.right, constantsOnly)
);
} else if (isArrayExpression(node)) {
} else if (isArrayExpression(node) || isTupleExpression(node)) {
for (const elem of node.elements) {
if (!this.isPure(elem, constantsOnly)) return false;
if (elem !== null && !this.isPure(elem, constantsOnly)) return false;
}
return true;
} else if (isObjectExpression(node)) {
} else if (isObjectExpression(node) || isRecordExpression(node)) {
for (const prop of node.properties) {
if (!this.isPure(prop, constantsOnly)) return false;
}
return true;
} else if (isDecorator(node)) {
return this.isPure(node.expression, constantsOnly);
} else if (isMethod(node)) {
if (node.computed && !this.isPure(node.key, constantsOnly)) return false;
if (node.kind === "get" || node.kind === "set") return false;
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
if (node.decorators) {
for (const decorator of node.decorators) {
if (!this.isPure(decorator, constantsOnly)) return false;
}
}
return true;
} else if (isProperty(node)) {
} else if (isProperty(node) || isClassAccessorProperty(node)) {
// @ts-expect-error todo(flow->ts): computed in not present on private properties
if (node.computed && !this.isPure(node.key, constantsOnly)) return false;
return this.isPure(node.value, constantsOnly);
if (node.decorators) {
for (const decorator of node.decorators) {
if (!this.isPure(decorator, constantsOnly)) return false;
}
}
if (isObjectProperty(node) || node.static) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Evaluating a non-static field definition should be pure as long as it has no computed keys / fancy decorators, since the field initializer is not executed yet.

return this.isPure(node.value, constantsOnly);
}
return true;
} else if (isUnaryExpression(node)) {
return this.isPure(node.argument, constantsOnly);
} else if (isTaggedTemplateExpression(node)) {
Expand Down
97 changes: 97 additions & 0 deletions packages/babel-traverse/test/path/isPure.js
View file
Open in desktop
@@ -0,0 +1,97 @@
import { parse } from "@babel/parser";

import _traverse from "../../lib/index.js";
const traverse = _traverse.default;

function getPath(code) {
const ast = parse(code, {
plugins: [
["decorators", { version: "2021-12", decoratorsBeforeExport: true }],
["recordAndTuple", { syntaxType: "hash" }],
"decoratorAutoAccessors",
["pipelineOperator", { proposal: "hack", topicToken: "%" }],
],
});
let path;
traverse(ast, {
Program: function (_path) {
path = _path;
_path.stop();
},
});
return path;
}

describe("isPure() returns true", () => {
it.each([
"class C { [0]() {} }",
"class C extends class {} {}",
"class C { static accessor x = 1 }",
"class C { accessor x = f() }",
"class C { #x = f() }",
"@dec class C {}; function dec () {}",
"class C { static target = new.target }",
])(`NodePath(%p).get("body.0").isPure() should be true`, input => {
const path = getPath(input).get("body.0");
expect(path.node).toBeTruthy();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assertion ensures that the pathRef passed in .get() is correct. In fact our previous tests on "class X { get foo() { return 1 } }" always succeed because path.get(body.0.expression) is an empty NodePath.

expect(path.isPure()).toBe(true);
});

it.each([
"({ x: 1, foo() { return 1 } })",
"String.raw`foo`",
`"a" + "b"`,
`[function () {}]`,
`#{ 0: 0, 1n: 1, two: "two"}`,
`#[0, 1n, "2", \`3\`]`,
`[,]`,
`-1 || void 0`,
`null ?? (true && false)`,
`this`,
])(`NodePath(%p).get("body.0.expression").isPure() should be true`, input => {
const path = getPath(input).get("body.0.expression");
expect(path.node).toBeTruthy();
expect(path.isPure()).toBe(true);
});

it.each(["let a = 1; `${a}`", `let a = 1; a |> % + %`])(
Copy link
Member

Choose a reason for hiding this comment

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

Uh, the first one is definitely not pure:

let a = { toString() { console.log("Hi :)") } };
`${a}`;

however, it was already behaving like this before this PR so if we want to change it I'd prefer to do it in another one.

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 point, we can introduce the pureToString compiler assumption, just like we have pureGetter.

`NodePath(%p).get("body.1.expression").isPure() should be true`,
input => {
const path = getPath(input).get("body.1.expression");
expect(path.node).toBeTruthy();
expect(path.isPure()).toBe(true);
},
);
});

describe("isPure() returns false", () => {
it.each([
"class X { get foo() { return 1 } }",
"@dec() class X {}",
"class C { @dec foo() {} }",
"class C { static {} }",
Copy link
Member

Choose a reason for hiding this comment

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

This is technically pure, however the only reason to use a static block is to have a side effect so we can err on the side of "it's never pure" and avoid recursing in its contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, static {} is likely only used in the Babel testcase.

"class C extends class { [f()] } {}",
])(`NodePath(%p).get("body.0").isPure() should be false`, input => {
const path = getPath(input).get("body.0");
expect(path.node).toBeTruthy();
expect(path.isPure()).toBe(false);
});

it.each(["`${a}`", "tagged`foo`"])(
`NodePath(%p).get("body.0.expression").isPure() should be false`,
input => {
const path = getPath(input).get("body.0.expression");
expect(path.node).toBeTruthy();
expect(path.isPure()).toBe(false);
},
);

it.each(["let a = 1; `${a++}`"])(
`NodePath(%p).get("body.1.expression").isPure() should be false`,
input => {
const path = getPath(input).get("body.1.expression");
expect(path.node).toBeTruthy();
expect(path.isPure()).toBe(false);
},
);
});
35 changes: 1 addition & 34 deletions packages/babel-traverse/test/scope.js
View file
Open in desktop
Expand Up @@ -365,39 +365,6 @@ describe("scope", () => {
).toBe(false);
});

it("purity", function () {
expect(
getPath("({ x: 1, foo() { return 1 } })")
.get("body")[0]
.get("expression")
.isPure(),
).toBeTruthy();
expect(
getPath("class X { get foo() { return 1 } }")
.get("body")[0]
.get("expression")
.isPure(),
).toBeFalsy();
expect(
getPath("`${a}`").get("body")[0].get("expression").isPure(),
).toBeFalsy();
expect(
getPath("let a = 1; `${a}`").get("body")[1].get("expression").isPure(),
).toBeTruthy();
expect(
getPath("let a = 1; `${a++}`")
.get("body")[1]
.get("expression")
.isPure(),
).toBeFalsy();
expect(
getPath("tagged`foo`").get("body")[0].get("expression").isPure(),
).toBeFalsy();
expect(
getPath("String.raw`foo`").get("body")[0].get("expression").isPure(),
).toBeTruthy();
});

test("label", function () {
expect(getPath("foo: { }").scope.getBinding("foo")).toBeUndefined();
expect(getPath("foo: { }").scope.getLabel("foo").type).toBe(
Expand Down Expand Up @@ -542,7 +509,7 @@ describe("scope", () => {
path.scope.crawl();
path.scope.crawl();

expect(path.scope.references._jsx).toBeTruthy();
expect(path.scope.references._jsx).toBe(true);
});

test("generateUid collision check after re-crawling", function () {
Expand Down