Skip to content

Commit

Permalink
fix parent pointer assignment during mutations
Browse files Browse the repository at this point in the history 
Summary:
A bug in the logic - it was not correctly handling parent pointers for shallowly cloned nodes.
Some mutations rely on traversing the tree for you to figure out the best place to perform the mutation - for example inserting before a mutation involves looking for the statement parent.
This would mean that if if you attempted to mutate a shallowly cloned node a broken mutation could easily occur due to incorrect parent pointers.

To fix this the mutations now return the root node of the mutation, and the tooling will traverse the tree and ensure the pointers are correct automatically.

Reviewed By: pieterv

Differential Revision: D32271466

fbshipit-source-id: deb18abce536b52373ed8652d6768959c15c2e94
  • Loading branch information
@bradzacher @facebook-github-bot
bradzacher authored and facebook-github-bot committed Nov 12, 2021
1 parent 8b8e095 commit feaaf81
Show file tree
Hide file tree
Showing 18 changed files with 411 additions and 186 deletions.
0 tools/hermes-parser/js/.prettierrc → tools/hermes-parser/js/.prettierrc.json
View file
File renamed without changes.
35 changes: 31 additions & 4 deletions tools/hermes-parser/js/hermes-estree/src/HermesESTreeSelectorTypes.js.flow
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,38 @@

'use strict';

import type {ESNode} from './types';
import type {
AnyTypeAnnotation,
CallExpression,
ESNode,
ExpressionStatement,
FunctionDeclaration,
Literal,
ReturnStatement,
VariableDeclaration,
VariableDeclarator,
} from './types';

// This file is overridden in the output by codegen wherein
// we add an individual property sig for each and every node.
/*
* !!! THIS FILE WILL BE OVERWRITTEN BY CODEGEN !!!
*
* Statically it should only contain the minimal set of
* definitions required to typecheck the local code.
*/

interface FunctionDeclaration_With_id extends FunctionDeclaration {
+id: $NonMaybeType<FunctionDeclaration['id']>;
}

export type ESQueryNodeSelectors = $ReadOnly<{
[selector: string]: (node: ESNode) => void,
+AnyTypeAnnotation?: (node: AnyTypeAnnotation) => void,
+CallExpression?: (node: CallExpression) => void,
+ExpressionStatement?: (node: ExpressionStatement) => void,
+FunctionDeclaration?: (node: FunctionDeclaration) => void,
+'FunctionDeclaration[id]'?: (node: FunctionDeclaration_With_id) => void,
+Literal?: (node: Literal) => void,
+ReturnStatement?: (node: ReturnStatement) => void,
+VariableDeclaration?: (node: VariableDeclaration) => void,
+VariableDeclarator?: (node: VariableDeclarator) => void,
+[selector: string]: (node: ESNode) => void,
}>;
181 changes: 135 additions & 46 deletions tools/hermes-parser/js/hermes-transform/__tests__/transform/transform-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,14 @@
* @format
*/

import {transform} from '../../src/transform/transform';
import {transform as transformOriginal} from '../../src/transform/transform';
import * as t from '../../src/generated/node-types';
// $FlowExpectedError[cannot-resolve-module]
import prettierConfig from '../../../.prettierrc.json';

function transform(code, visitors) {
return transformOriginal(code, visitors, prettierConfig);
}

describe('transform', () => {
it('should do nothing (including no formatting) if no mutations are applied', () => {
Expand All @@ -23,10 +29,6 @@ describe('transform', () => {
const code = 'const x = 1';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.replaceStatementWithMany(node, [
t.VariableDeclaration({
kind: 'const',
Expand All @@ -52,10 +54,6 @@ const y = null;
const code = 'const x = 1;';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.insertBeforeStatement(
node,
t.VariableDeclaration({
Expand Down Expand Up @@ -86,10 +84,6 @@ const x = 1;
const code = 'const x = 1;';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.insertAfterStatement(
node,
t.VariableDeclaration({
Expand Down Expand Up @@ -120,10 +114,6 @@ const y = 1;
const code = 'if (condition) return true;';
const result = transform(code, context => ({
ReturnStatement(node) {
if (node.type !== 'ReturnStatement') {
return;
}

context.insertBeforeStatement(
node,
t.VariableDeclaration({
Expand Down Expand Up @@ -157,27 +147,19 @@ if (condition) {
const code = 'const x = 1; console.log("I will survive");';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.removeStatement(node);
},
}));

expect(result).toBe(`\
console.log("I will survive");
console.log('I will survive');
`);
});

it('wraps statements in a BlockStatement if they were in a bodyless parent', () => {
const code = 'if (condition) return true;';
const result = transform(code, context => ({
ReturnStatement(node) {
if (node.type !== 'ReturnStatement') {
return;
}

context.removeStatement(node);
},
}));
Expand All @@ -195,10 +177,6 @@ if (condition) {
const code = 'const x = 1;';
const result = transform(code, context => ({
Literal(node) {
if (node.type !== 'Literal') {
return;
}

context.replaceNode(node, t.BooleanLiteral({value: true}));
},
}));
Expand All @@ -212,10 +190,6 @@ const x = true;
const code = 'const x = 1;';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.replaceNode(
node,
t.VariableDeclaration({
Expand All @@ -240,10 +214,6 @@ let y = null;
const code = 'const x: any = 1;';
const result = transform(code, context => ({
AnyTypeAnnotation(node) {
if (node.type !== 'AnyTypeAnnotation') {
return;
}

context.replaceNode(node, t.NumberTypeAnnotation());
},
}));
Expand All @@ -259,10 +229,6 @@ const x: number = 1;
const code = 'const x = 1;';
const result = transform(code, context => ({
VariableDeclaration(node) {
if (node.type !== 'VariableDeclaration') {
return;
}

context.replaceStatementWithMany(node, [
t.VariableDeclaration({
kind: 'const',
Expand Down Expand Up @@ -296,10 +262,6 @@ const z = true;
const code = 'if (condition) return true;';
const result = transform(code, context => ({
ReturnStatement(node) {
if (node.type !== 'ReturnStatement') {
return;
}

context.replaceStatementWithMany(node, [
t.VariableDeclaration({
kind: 'const',
Expand Down Expand Up @@ -340,4 +302,131 @@ if (condition) {
});
});
});

describe('complex transforms', () => {
it('should support transforms on the same subtree', () => {
const code = `\
class Foo {
method(): () => () => Foo {
function bar(): () => Foo {
function baz(): Foo {
return this;
}
return baz;
}
return bar;
}
}
`;

// transform which replaces all function declarations with arrow functions
const result = transform(code, context => ({
'FunctionDeclaration[id]'(node) {
context.replaceNode(
node,
t.VariableDeclaration({
kind: 'const',
declarations: [
t.VariableDeclarator({
id: context.shallowCloneNode(node.id),
init: t.ArrowFunctionExpression({
async: node.async,
body: context.shallowCloneNode(node.body),
expression: false,
params: context.shallowCloneArray(node.params),
predicate: context.shallowCloneNode(node.predicate),
returnType: context.shallowCloneNode(node.returnType),
typeParameters: context.shallowCloneNode(
node.typeParameters,
),
}),
}),
],
}),
);
},
}));

expect(result).toBe(`\
class Foo {
method(): () => () => Foo {
const bar = (): (() => Foo) => {
const baz = (): Foo => {
return this;
};
return baz;
};
return bar;
}
}
`);
});

it('should fail if you attempt to insert before a removed node', () => {
const code = `\
if (true) call();
`;

expect(() =>
transform(code, context => ({
ExpressionStatement(node) {
context.replaceNode(
node,
t.ExpressionStatement({
expression: t.StringLiteral({
value: 'removed',
}),
}),
);

context.insertBeforeStatement(
node,
t.ExpressionStatement({
expression: t.StringLiteral({
value: 'inserted',
}),
}),
);
},
})),
).toThrowErrorMatchingInlineSnapshot(
`"Expected to find the target \\"ExpressionStatement\\" on the \\"IfStatement.alternate\\", but found a different node. This likely means that you attempted to mutate around the target after it was deleted/replaced."`,
);
});

it('should allow insertion before removal', () => {
const code = `\
if (true) call();
`;

const result = transform(code, context => ({
ExpressionStatement(node) {
context.insertBeforeStatement(
node,
t.ExpressionStatement({
expression: t.StringLiteral({
value: 'inserted',
}),
}),
);

context.replaceNode(
node,
t.ExpressionStatement({
expression: t.StringLiteral({
value: 'removed',
}),
}),
);
},
}));

expect(result).toBe(`\
if (true) {
('inserted');
('removed');
}
`);
});
});
});
1 change: 0 additions & 1 deletion tools/hermes-parser/js/hermes-transform/__tests__/traverse/traverse-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ describe('traverse', () => {
},
'BinaryExpression > Identifier.left'(node) {
if (node.type !== 'Identifier') {
// TODO - improve the types
return;
}

Expand Down
27 changes: 13 additions & 14 deletions tools/hermes-parser/js/hermes-transform/src/detachedNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function detachedProps<T: BaseNode>(
*/
export function shallowCloneNode<T: ESNode>(
node: T,
newProps: $Shape<T>,
newProps: $Shape<T> = {},
): DetachedNode<T> {
return detachedProps(null, (Object.assign({}, node, newProps): $FlowFixMe));
}
Expand All @@ -71,14 +71,7 @@ export function deepCloneNode<T: ESNode>(
newProps,
);

// set the parent pointers
SimpleTraverser.traverse(clone, {
enter(node, parent) {
// $FlowExpectedError[cannot-write]
node.parent = parent;
},
leave() {},
});
updateAllParentPointers(clone);

// $FlowExpectedError[class-object-subtyping]
return detachedProps(null, clone);
Expand All @@ -87,8 +80,8 @@ export function deepCloneNode<T: ESNode>(
/**
* Corrects the parent pointers in direct children of the given node
*/
export function setParentPointersInDirectChildren<T: ESNode>(
node: DetachedNode<T>,
export function setParentPointersInDirectChildren(
node: DetachedNode<ESNode>,
): void {
for (const key of getVisitorKeys(node)) {
if (
Expand All @@ -107,8 +100,14 @@ export function setParentPointersInDirectChildren<T: ESNode>(
}

/**
* Convert from the opaque type
* Traverses the entire subtree to ensure the parent pointers are set correctly
*/
export function asESNode<T: ESNode>(node: DetachedNode<T>): T {
return node;
export function updateAllParentPointers(node: ESNode | DetachedNode<ESNode>) {
SimpleTraverser.traverse(node, {
enter(node, parent) {
// $FlowExpectedError[cannot-write]
node.parent = parent;
},
leave() {},
});
}
Loading

0 comments on commit feaaf81

Please sign in to comment.