From 224a35c5c69750f7deea19744d4bc14cb069789e Mon Sep 17 00:00:00 2001 From: Rin Tepis <13428807+SCLeoX@users.noreply.github.com> Date: Tue, 27 Jul 2021 01:32:29 +0800 Subject: [PATCH] Don't insert `__self: this` within constructors of derived classes (#13552) * Don't insert `__self: this` prior to `super()` calls (#13550) `__self: this` is inserted for debugging purposes. However, it will cause a runtime error if it is inserted prior to a `super()` call in a constructor. This commit will prevent `__self: this` from inserted when there is a following `super()` call. * Prevent adding `__self` within a constructor that has `super()` altogether. * Fix 2 typos in the comments. * Add an additional test case for constructors that do not have a `super()` call. * Detect `super()` call by testing whether the class has a superclass. * Update method name and corresponding comments * Add an additional test for the case where the derived class do not have a `super()` call. * Apply the same changes to babel-plugin-transform-react-jsx --- .../input.js | 51 ++++++++ .../output.js | 118 ++++++++++++++++++ .../input.js | 51 ++++++++ .../output.js | 118 ++++++++++++++++++ .../src/index.js | 60 ++++++++- .../react-source/disable-with-super/input.js | 51 ++++++++ .../react-source/disable-with-super/output.js | 65 ++++++++++ .../src/create-plugin.ts | 69 ++++++++-- 8 files changed, 570 insertions(+), 13 deletions(-) create mode 100644 packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/input.js create mode 100644 packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/output.js create mode 100644 packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/input.js create mode 100644 packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/output.js create mode 100644 packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/input.js create mode 100644 packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/output.js diff --git a/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/input.js b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/input.js new file mode 100644 index 0000000000..46c970f6f2 --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/input.js @@ -0,0 +1,51 @@ +class A { } + +class B extends A { + constructor() { + ; + super(); + ; + } +} + +class C { + constructor() { + ; + class D extends A { + constructor() { + super(); + } + } + const E = class extends A { + constructor() { + super(); + } + }; + } +} + +class E extends A { + constructor() { + this.x = () => ; + this.y = function () { + return ; + }; + function z() { + return ; + } + { } + super(); + } +} + +class F { + constructor() { + + } +} + +class G extends A { + constructor() { + return ; + } +} diff --git a/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/output.js b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/output.js new file mode 100644 index 0000000000..0dba5ae4fd --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/output.js @@ -0,0 +1,118 @@ +var _reactJsxDevRuntime = require("react/jsx-dev-runtime"); + +var _jsxFileName = "/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/within-derived-classes-constructor/input.js"; + +class A {} + +class B extends A { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag1", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 5, + columnNumber: 5 + }, void 0); + + super( /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag2", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 6, + columnNumber: 11 + }, void 0)); + + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag3", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 7, + columnNumber: 5 + }, void 0); + } + +} + +class C { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag4", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 13, + columnNumber: 5 + }, this); + + class D extends A { + constructor() { + super(); + } + + } + + const E = class extends A { + constructor() { + super(); + } + + }; + } + +} + +class E extends A { + constructor() { + this.x = function () { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag5", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 29, + columnNumber: 20 + }, void 0); + }; + + this.y = function () { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag6", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 31, + columnNumber: 14 + }, this); + }; + + function z() { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag7", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 34, + columnNumber: 14 + }, this); + } + + { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag8", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 36, + columnNumber: 7 + }, void 0); + } + super(); + } + +} + +class F { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag9", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 43, + columnNumber: 5 + }, this); + } + +} + +class G extends A { + constructor() { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag10", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 49, + columnNumber: 12 + }, void 0); + } + +} diff --git a/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/input.js b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/input.js new file mode 100644 index 0000000000..46c970f6f2 --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/input.js @@ -0,0 +1,51 @@ +class A { } + +class B extends A { + constructor() { + ; + super(); + ; + } +} + +class C { + constructor() { + ; + class D extends A { + constructor() { + super(); + } + } + const E = class extends A { + constructor() { + super(); + } + }; + } +} + +class E extends A { + constructor() { + this.x = () => ; + this.y = function () { + return ; + }; + function z() { + return ; + } + { } + super(); + } +} + +class F { + constructor() { + + } +} + +class G extends A { + constructor() { + return ; + } +} diff --git a/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/output.js b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/output.js new file mode 100644 index 0000000000..ec47ac7b13 --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-development/test/fixtures/windows/within-derived-classes-constructor-windows/output.js @@ -0,0 +1,118 @@ +var _reactJsxDevRuntime = require("react/jsx-dev-runtime"); + +var _jsxFileName = "\\packages\\babel-plugin-transform-react-jsx-development\\test\\fixtures\\windows\\within-derived-classes-constructor-windows\\input.js"; + +class A {} + +class B extends A { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag1", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 5, + columnNumber: 5 + }, void 0); + + super( /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag2", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 6, + columnNumber: 11 + }, void 0)); + + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag3", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 7, + columnNumber: 5 + }, void 0); + } + +} + +class C { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag4", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 13, + columnNumber: 5 + }, this); + + class D extends A { + constructor() { + super(); + } + + } + + const E = class extends A { + constructor() { + super(); + } + + }; + } + +} + +class E extends A { + constructor() { + this.x = function () { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag5", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 29, + columnNumber: 20 + }, void 0); + }; + + this.y = function () { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag6", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 31, + columnNumber: 14 + }, this); + }; + + function z() { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag7", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 34, + columnNumber: 14 + }, this); + } + + { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag8", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 36, + columnNumber: 7 + }, void 0); + } + super(); + } + +} + +class F { + constructor() { + /*#__PURE__*/ + _reactJsxDevRuntime.jsxDEV("sometag9", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 43, + columnNumber: 5 + }, this); + } + +} + +class G extends A { + constructor() { + return /*#__PURE__*/_reactJsxDevRuntime.jsxDEV("sometag10", {}, void 0, false, { + fileName: _jsxFileName, + lineNumber: 49, + columnNumber: 12 + }, void 0); + } + +} diff --git a/packages/babel-plugin-transform-react-jsx-self/src/index.js b/packages/babel-plugin-transform-react-jsx-self/src/index.js index 6d5403ef9c..70c6baabb0 100644 --- a/packages/babel-plugin-transform-react-jsx-self/src/index.js +++ b/packages/babel-plugin-transform-react-jsx-self/src/index.js @@ -1,7 +1,8 @@ /** * This adds a __self={this} JSX attribute to all JSX elements, which React will use - * to generate some runtime warnings. - * + * to generate some runtime warnings. However, if the JSX element appears within a + * constructor of a derived class, `__self={this}` will not be inserted in order to + * prevent runtime errors. * * == JSX Literals == * @@ -16,11 +17,64 @@ import { types as t } from "@babel/core"; const TRACE_ID = "__self"; +/** + * Finds the closest parent function that provides `this`. Specifically, this looks for + * the first parent function that isn't an arrow function. + * + * Derived from `Scope#getFunctionParent` + */ +function getThisFunctionParent(path) { + let scope = path.scope; + do { + if ( + scope.path.isFunctionParent() && + !scope.path.isArrowFunctionExpression() + ) { + return scope.path; + } + } while ((scope = scope.parent)); + return null; +} + +/** + * Returns whether the class has specified a superclass. + */ +function isDerivedClass(classPath) { + return classPath.node.superClass !== null; +} + +/** + * Returns whether `this` is allowed at given path. + */ +function isThisAllowed(path) { + // This specifically skips arrow functions as they do not rewrite `this`. + const parentMethodOrFunction = getThisFunctionParent(path); + if (parentMethodOrFunction === null) { + // We are not in a method or function. It is fine to use `this`. + return true; + } + if (!parentMethodOrFunction.isMethod()) { + // If the closest parent is a regular function, `this` will be rebound, therefore it is fine to use `this`. + return true; + } + // Current node is within a method, so we need to check if the method is a constructor. + if (parentMethodOrFunction.node.kind !== "constructor") { + // We are not in a constructor, therefore it is always fine to use `this`. + return true; + } + // Now we are in a constructor. If it is a derived class, we do not reference `this`. + return !isDerivedClass(parentMethodOrFunction.parentPath.parentPath); +} + export default declare(api => { api.assertVersion(7); const visitor = { - JSXOpeningElement({ node }) { + JSXOpeningElement(path) { + if (!isThisAllowed(path)) { + return; + } + const node = path.node; const id = t.jsxIdentifier(TRACE_ID); const trace = t.thisExpression(); diff --git a/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/input.js b/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/input.js new file mode 100644 index 0000000000..46c970f6f2 --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/input.js @@ -0,0 +1,51 @@ +class A { } + +class B extends A { + constructor() { + ; + super(); + ; + } +} + +class C { + constructor() { + ; + class D extends A { + constructor() { + super(); + } + } + const E = class extends A { + constructor() { + super(); + } + }; + } +} + +class E extends A { + constructor() { + this.x = () => ; + this.y = function () { + return ; + }; + function z() { + return ; + } + { } + super(); + } +} + +class F { + constructor() { + + } +} + +class G extends A { + constructor() { + return ; + } +} diff --git a/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/output.js b/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/output.js new file mode 100644 index 0000000000..efa9d8c9b7 --- /dev/null +++ b/packages/babel-plugin-transform-react-jsx-self/test/fixtures/react-source/disable-with-super/output.js @@ -0,0 +1,65 @@ +class A {} + +class B extends A { + constructor() { + ; + super(); + ; + } + +} + +class C { + constructor() { + ; + + class D extends A { + constructor() { + super(); + } + + } + + const E = class extends A { + constructor() { + super(); + } + + }; + } + +} + +class E extends A { + constructor() { + this.x = () => ; + + this.y = function () { + return ; + }; + + function z() { + return ; + } + + { + ; + } + super(); + } + +} + +class F { + constructor() { + ; + } + +} + +class G extends A { + constructor() { + return ; + } + +} diff --git a/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts b/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts index 430a867c36..40974d740b 100644 --- a/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts +++ b/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts @@ -103,16 +103,22 @@ export default function createPlugin({ name, development }) { } } - const self = t.jsxAttribute( - t.jsxIdentifier("__self"), - t.jsxExpressionContainer(t.thisExpression()), + const attributes = []; + if (isThisAllowed(path)) { + attributes.push( + t.jsxAttribute( + t.jsxIdentifier("__self"), + t.jsxExpressionContainer(t.thisExpression()), + ), + ); + } + attributes.push( + t.jsxAttribute( + t.jsxIdentifier("__source"), + t.jsxExpressionContainer(makeSource(path, state)), + ), ); - const source = t.jsxAttribute( - t.jsxIdentifier("__source"), - t.jsxExpressionContainer(makeSource(path, state)), - ); - - path.pushContainer("attributes", [self, source]); + path.pushContainer("attributes", attributes); }, }; @@ -277,6 +283,49 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`, }, }; + // Finds the closest parent function that provides `this`. Specifically, this looks for + // the first parent function that isn't an arrow function. + // + // Derived from `Scope#getFunctionParent` + function getThisFunctionParent(path) { + let scope = path.scope; + do { + if ( + scope.path.isFunctionParent() && + !scope.path.isArrowFunctionExpression() + ) { + return scope.path; + } + } while ((scope = scope.parent)); + return null; + } + + // Returns whether the class has specified a superclass. + function isDerivedClass(classPath) { + return classPath.node.superClass !== null; + } + + // Returns whether `this` is allowed at given path. + function isThisAllowed(path) { + // This specifically skips arrow functions as they do not rewrite `this`. + const parentMethodOrFunction = getThisFunctionParent(path); + if (parentMethodOrFunction === null) { + // We are not in a method or function. It is fine to use `this`. + return true; + } + if (!parentMethodOrFunction.isMethod()) { + // If the closest parent is a regular function, `this` will be rebound, therefore it is fine to use `this`. + return true; + } + // Current node is within a method, so we need to check if the method is a constructor. + if (parentMethodOrFunction.node.kind !== "constructor") { + // We are not in a constructor, therefore it is always fine to use `this`. + return true; + } + // Now we are in a constructor. If it is a derived class, we do not reference `this`. + return !isDerivedClass(parentMethodOrFunction.parentPath.parentPath); + } + function call(pass, name, args) { const node = t.callExpression(get(pass, `id/${name}`)(), args); if (PURE_ANNOTATION ?? get(pass, "defaultPure")) annotateAsPure(node); @@ -476,7 +525,7 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`, extracted.key ?? path.scope.buildUndefinedNode(), t.booleanLiteral(children.length > 1), extracted.__source ?? path.scope.buildUndefinedNode(), - extracted.__self ?? t.thisExpression(), + extracted.__self ?? path.scope.buildUndefinedNode(), ); } else if (extracted.key !== undefined) { args.push(extracted.key);