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

Skip TSAsExpression when transforming spread in CallExpression #11404

Merged
merged 30 commits into from Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
087405e
Skip TSAsExpression when transforming spread in CallExpression
oliverdunk Apr 10, 2020
dc5316e
Merge branch 'master' into od/11400-typescript-spread-as
oliverdunk Apr 10, 2020
e090927
Create @babel/helper-get-call-context package
oliverdunk May 3, 2020
b41cc10
Support OptionalCallExpressions
oliverdunk May 4, 2020
f406e41
Merge branch 'master' into od/11400-typescript-spread-as
oliverdunk May 4, 2020
a0bb643
Merge branch 'master' into od/11400-typescript-spread-as
oliverdunk May 16, 2020
2bdb62d
Use helper in optional chaining plugin, and move tests
oliverdunk May 17, 2020
06d092f
Update package.json files
oliverdunk May 23, 2020
ed10970
Use dot notation to access property
oliverdunk May 23, 2020
1d957b3
Remove private method tests until future MR
oliverdunk May 23, 2020
5d7c306
Update packages/babel-plugin-transform-spread/package.json
nicolo-ribaudo May 23, 2020
defdad1
Rename @babel/helper-get-call-context to @babel/helper-skip-transpare…
oliverdunk May 23, 2020
8c3b44b
Handle typed OptionalMemberExpressions
oliverdunk May 24, 2020
db8f8c3
Make @babel/helper-skip-transparent-expr-wrappers a dependency
oliverdunk May 24, 2020
b4948cf
Merge branch 'master' into od/11400-typescript-spread-as
oliverdunk May 31, 2020
c69c921
Support TSNonNullExpressions
oliverdunk May 31, 2020
db8f57a
Use named import instead of default
oliverdunk May 31, 2020
117bb67
Merge branch 'master' into od/11400-typescript-spread-as
oliverdunk Jun 1, 2020
4e49c03
Add test for call context when parenthesized call expression has type
oliverdunk Jun 1, 2020
c0ecd48
Merge branch 'main' into od/11400-typescript-spread-as
oliverdunk Jun 29, 2020
e40676b
Improve handling of member expressions inside transparent expression …
oliverdunk Jun 29, 2020
dd1b57d
Add comment explaining what a transparent expression wrapper is
oliverdunk Jun 29, 2020
4250471
Add newlines to test fixtures
oliverdunk Jun 29, 2020
24d550a
Merge branch 'main' into od/11400-typescript-spread-as
oliverdunk Jun 30, 2020
7069abe
Pass correct parameter type to skipTransparentExprWrappers
oliverdunk Jun 30, 2020
0704742
Rename to babel-helper-skip-transparent-expression-wrappers
oliverdunk Jul 3, 2020
f6cc6e2
Remove getCallContext helper
oliverdunk Jul 3, 2020
cea347f
Fixed exports key
oliverdunk Jul 3, 2020
d654efc
Preserve types in babel-plugin-transform-spread tests
oliverdunk Jul 3, 2020
e6c3a4c
Use external-helpers to avoid inlining helper functions in tests
oliverdunk Jul 22, 2020
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
3 changes: 3 additions & 0 deletions packages/babel-helper-get-call-context/.npmignore
@@ -0,0 +1,3 @@
src
test
*.log
17 changes: 17 additions & 0 deletions packages/babel-helper-get-call-context/README.md
@@ -0,0 +1,17 @@
# @babel/helper-get-call-context

> Helper function to get the context of a CallExpression

## Install

Using npm:

```sh
npm install --save-dev @babel/helper-get-call-context
```

or using yarn:

```sh
yarn add @babel/helper-get-call-context --dev
```
14 changes: 14 additions & 0 deletions packages/babel-helper-get-call-context/package.json
@@ -0,0 +1,14 @@
{
"name": "@babel/helper-get-call-context",
"version": "7.9.6",
"description": "Helper function to get call context",
"repository": "https://github.com/babel/babel/tree/master/packages/babel-helper-get-call-context",
"license": "MIT",
"publishConfig": {
"access": "public"
},
"main": "lib/index.js",
"dependencies": {
"@babel/traverse": "^7.9.6"
oliverdunk marked this conversation as resolved.
Show resolved Hide resolved
}
}
23 changes: 23 additions & 0 deletions packages/babel-helper-get-call-context/src/index.js
@@ -0,0 +1,23 @@
import type { NodePath } from "@babel/traverse";

export default function(callPath: NodePath): NodePath {
if (!callPath.isCallExpression() && !callPath.isOptionalCallExpression()) {
throw new Error(
`Expected type "CallExpression" or "OptionalCallExpression" ` +
`but instead got "${callPath.type}".`,
);
}

let calleePath = callPath.get("callee");

while (
calleePath.isTSAsExpression() ||
calleePath.isTypeCastExpression() ||
calleePath.isTSTypeAssertion() ||
calleePath.isParenthesizedExpression()
) {
calleePath = calleePath.get("expression");
}

return calleePath;
}
Expand Up @@ -21,6 +21,7 @@
"devDependencies": {
"@babel/core": "^7.9.0",
"@babel/helper-plugin-test-runner": "^7.8.3",
"@babel/plugin-transform-block-scoping": "^7.8.3"
"@babel/plugin-transform-block-scoping": "^7.8.3",
"@babel/helper-get-call-context": "7.9.6"
oliverdunk marked this conversation as resolved.
Show resolved Hide resolved
}
}
@@ -1,4 +1,5 @@
import { declare } from "@babel/helper-plugin-utils";
import getCallContext from "@babel/helper-get-call-context";
import syntaxOptionalChaining from "@babel/plugin-syntax-optional-chaining";
import { types as t } from "@babel/core";

Expand Down Expand Up @@ -26,6 +27,7 @@ export default declare((api, options) => {
const { parentPath, scope } = path;
let isDeleteOperation = false;
const optionals = [];
const chains = [];

let optionalPath = path;
while (
Expand All @@ -35,6 +37,11 @@ export default declare((api, options) => {
const { node } = optionalPath;
if (node.optional) {
optionals.push(node);
chains.push(
optionalPath.isOptionalCallExpression()
? getCallContext(optionalPath)["node"]
: node["object"],
oliverdunk marked this conversation as resolved.
Show resolved Hide resolved
);
}

if (optionalPath.isOptionalMemberExpression()) {
Expand All @@ -56,7 +63,7 @@ export default declare((api, options) => {

const isCall = t.isCallExpression(node);
const replaceKey = isCall ? "callee" : "object";
const chain = node[replaceKey];
const chain = chains[i];

let ref;
let check;
Expand Down
@@ -0,0 +1,3 @@
{
"plugins": ["proposal-optional-chaining"]
}
@@ -0,0 +1 @@
(a.b as any)?.()
@@ -0,0 +1,7 @@
{
"presets": [
[
"typescript"
]
]
}
@@ -0,0 +1,3 @@
var _a$b, _a;

(_a$b = (_a = a).b) === null || _a$b === void 0 ? void 0 : _a$b.call(_a);
@@ -0,0 +1,6 @@
{
"plugins": [
"proposal-private-methods",
"proposal-class-properties"
]
}
@@ -0,0 +1,7 @@
class A {
#x;

fn() {
(this.#x as any)();
}
}
@@ -0,0 +1,12 @@
{
"presets": [
[
"typescript"
]
],
"plugins": [
[
"proposal-class-properties"
]
]
}
@@ -0,0 +1,17 @@
function _classPrivateFieldGet(receiver, privateMap) { var descriptor = privateMap.get(receiver); if (!descriptor) { throw new TypeError("attempted to get private field on non-instance"); } if (descriptor.get) { return descriptor.get.call(receiver); } return descriptor.value; }

class A {
constructor() {
_x.set(this, {
writable: true,
value: void 0
});
}

fn() {
_classPrivateFieldGet(this, _x)();
Copy link
Member

Choose a reason for hiding this comment

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

This still call still doesn't have the correct this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, admittedly I didn't look close enough and thought it was right! That's a tough one, because we're going in the opposite direction. The transformation is done by babel-helper-member-expressions-to-functions, which takes a member, and then checks to see if its parent is a call expression.

I was able to fix it with the following diff: https://hastebin.com/onotaqusiw.diff

It seems like this would be an issue for the entire helper. Maybe the helper should be a type stripper, instead of just something for calls? It would export a function for each direction.

The final tricky bit would be things like this:

if (parentPath.isAssignmentExpression({ left: node })) {

We could probably change that to this?

if (parentPath.isAssignmentExpression({ left: previousParent })) {

With previousParent set to node initially, and then the last parent each time we go a level up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It crossed my mind that if we move the logic to a plugin, we can't keep track of the previous parent. Maybe this logic has to be duplicated in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could keep this for a follow-up PR then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I have a first draft, so I'll open a WIP MR with that once this is merged.

}

}

var _x = new WeakMap();
3 changes: 2 additions & 1 deletion packages/babel-plugin-transform-spread/package.json
Expand Up @@ -19,6 +19,7 @@
},
"devDependencies": {
"@babel/core": "^7.8.3",
"@babel/helper-plugin-test-runner": "^7.8.3"
"@babel/helper-plugin-test-runner": "^7.8.3",
"@babel/helper-get-call-context": "7.9.6"
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
}
}
8 changes: 5 additions & 3 deletions packages/babel-plugin-transform-spread/src/index.js
@@ -1,4 +1,5 @@
import { declare } from "@babel/helper-plugin-utils";
import getCallContext from "@babel/helper-get-call-context";
import { types as t } from "@babel/core";

export default declare((api, options) => {
Expand Down Expand Up @@ -94,7 +95,8 @@ export default declare((api, options) => {
const args = node.arguments;
if (!hasSpread(args)) return;

const calleePath = path.get("callee");
const calleePath = getCallContext(path);

if (calleePath.isSuper()) return;

let contextLiteral = scope.buildUndefinedNode();
Expand All @@ -120,7 +122,7 @@ export default declare((api, options) => {
node.arguments.push(first);
}

const callee = node.callee;
const callee = calleePath.node;

if (calleePath.isMemberExpression()) {
const temp = scope.maybeGenerateMemoised(callee.object);
Expand All @@ -132,7 +134,7 @@ export default declare((api, options) => {
}
t.appendToMemberExpression(callee, t.identifier("apply"));
} else {
node.callee = t.memberExpression(node.callee, t.identifier("apply"));
node.callee = t.memberExpression(callee, t.identifier("apply"));
}

if (t.isSuper(contextLiteral)) {
Expand Down
@@ -0,0 +1 @@
(a.b: any)(...args)
@@ -0,0 +1,7 @@
{
"presets": [
oliverdunk marked this conversation as resolved.
Show resolved Hide resolved
[
"flow"
]
]
}
@@ -0,0 +1,3 @@
var _a;

(_a = a).b.apply(_a, babelHelpers.toConsumableArray(args));
@@ -0,0 +1,3 @@
{
"plugins": ["external-helpers", "transform-spread", "transform-parameters"]
}
@@ -0,0 +1 @@
(a.b)(...args)
@@ -0,0 +1,5 @@
{
"parserOpts": {
"createParenthesizedExpressions": true
}
}
@@ -0,0 +1,3 @@
var _a;

((_a = a).b.apply)(_a, babelHelpers.toConsumableArray(args));
oliverdunk marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1 @@
(<any> a.b)(...args)
@@ -0,0 +1,7 @@
{
"presets": [
[
"typescript"
]
]
}
@@ -0,0 +1,3 @@
var _a;

(_a = a).b.apply(_a, babelHelpers.toConsumableArray(args));
@@ -0,0 +1 @@
(dog.bark as any)(...args)
@@ -0,0 +1,7 @@
{
"presets": [
[
"typescript"
]
]
}
@@ -0,0 +1,3 @@
var _dog;

(_dog = dog).bark.apply(_dog, babelHelpers.toConsumableArray(args));