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

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

Merged
merged 8 commits into from Apr 9, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 5, 2022

Q                       A
Fixed Issues? Support more language features in scope#isPure
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Also extracted the tests to a separate file.

@JLHwung JLHwung added PR: Polish 💅 A type of pull request used for our changelog categories pkg: traverse labels Apr 5, 2022
if (!this.isPure(decorator, constantsOnly)) return false;
}
}
if (isObjectProperty(node) || node.static) {
Copy link
Contributor Author

@JLHwung JLHwung Apr 5, 2022

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.

@@ -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

@JLHwung JLHwung Apr 5, 2022

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.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 5, 2022

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

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 5, 2022

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

"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

@JLHwung JLHwung Apr 5, 2022

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.


return _ref = (n + _ref2) * 2, `${_ref} apples`.toUpperCase();
}).join());
const result = (_ref = Math.pow(5, 2), [1, 2, 3].map(n => `${(n + _ref) * 2} apples`.toUpperCase()).join());
Copy link
Contributor Author

@JLHwung JLHwung Apr 5, 2022

Choose a reason for hiding this comment

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

Nice. Babel is generating less.

} 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

@nicolo-ribaudo nicolo-ribaudo Apr 6, 2022

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.

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

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

@nicolo-ribaudo nicolo-ribaudo Apr 8, 2022

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

@JLHwung JLHwung Apr 8, 2022

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.

"class C { @dec foo() {} }",
"class C { @dec foo }",
"class C { @dec accessor foo = 1 }",
"class C { static {} }",
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 8, 2022

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

@JLHwung JLHwung Apr 8, 2022

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.

@nicolo-ribaudo nicolo-ribaudo changed the title Scope#isPure updates Update detection of pure nodes (Scope#isPure) Apr 9, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 3f1bd8e into babel:main Apr 9, 2022
35 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the scope-isPure-updates branch Apr 9, 2022
@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 Jul 10, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2022
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: traverse PR: Polish 💅 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