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

Conversation

@oliverdunk
Copy link
Contributor

oliverdunk commented Apr 10, 2020

Q                       A
Fixed Issues? Fixes #11400
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In babel-plugin-transform-spread, when transforming spreaded arguments in a CallExpression, we weren't handling the case where the callee was a TSAsExpression. This meant we incorrectly determined the this of our apply call, transforming:

(dog.bark as any)(...args)

... into ...

dog.bark.apply(void 0, args)

I've fixed that by exploring deeper, into the expression of the TSAsExpression.

oliverdunk added 2 commits Apr 10, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented Apr 10, 2020

Not sure if there's a better solution here. It feels like the ideal would be passing the entire path to a "get me this without the TypeScript" function.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Apr 11, 2020

Other tests:

class A {
  #x;
  
  fn() {
    (this.#x as any)();
  }
}
(a.b as any)?.()

And also with Flow type casts ((a.b: any)(...args)) and old-style TS casts ((<any> a.b)(...args)).

Also, it would be nice to make it work when the createParenthesizedExpressions parser option (which creates explicit nodes for (foo)) is enabled with (a.b)(...args).

It feels like the ideal would be passing the entire path to a "get me this without the TypeScript" function.

I agree! Missing the correct this with type casts is a common pitfall and a shared function would be useful. What about a new @babel/helper-get-call-context package?

@oliverdunk oliverdunk force-pushed the oliverdunk:od/11400-typescript-spread-as branch from 44027e1 to e090927 May 3, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented May 3, 2020

Thanks for the feedback @nicolo-ribaudo! I've created the package, and added a bunch of extra tests - let me know what you think.

I wasn't able to test (a.b as any)?.(), because I don't think babel-plugin-transform-spread supports optional call expressions. Do you agree? If so, I can open an issue to fix that.

I have two quick tweaks which I will make tomorrow morning:

  • My new package should probably support optional call expressions (it nearly does, but I just realised that I made a mistake by asserting I was passed a call expression).
  • It looks like the tests are failing. I'll try merging with master, unless you can see that something else is going wrong.
@oliverdunk oliverdunk requested a review from nicolo-ribaudo May 3, 2020
oliverdunk added 2 commits May 4, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented May 4, 2020

Thanks for the feedback @nicolo-ribaudo! I've created the package, and added a bunch of extra tests - let me know what you think.

I wasn't able to test (a.b as any)?.(), because I don't think babel-plugin-transform-spread supports optional call expressions. Do you agree? If so, I can open an issue to fix that.

I have two quick tweaks which I will make tomorrow morning:

  • My new package should probably support optional call expressions (it nearly does, but I just realised that I made a mistake by asserting I was passed a call expression).
  • It looks like the tests are failing. I'll try merging with master, unless you can see that something else is going wrong.

Made changes as promised.

@codesandbox
Copy link

codesandbox bot commented May 4, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e6c3a4c:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration
@babel-bot
Copy link
Collaborator

babel-bot commented May 4, 2020

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

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 8, 2020

@oliverdunk The tests I proposed in #11404 (comment) where without spread 😛

The caller is also needed by the optional chaining plugin.

oliverdunk added 2 commits May 16, 2020
}

fn() {
_classPrivateFieldGet(this, _x)();

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo May 17, 2020 Member

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

This comment has been minimized.

@oliverdunk

oliverdunk May 17, 2020 Author Contributor

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?

This comment has been minimized.

@oliverdunk

oliverdunk May 18, 2020 Author Contributor

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.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo May 22, 2020 Member

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

This comment has been minimized.

@oliverdunk

oliverdunk May 23, 2020 Author Contributor

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

@oliverdunk
Copy link
Contributor Author

oliverdunk commented May 17, 2020

I'm not sure if I missed something when adding my new package as a dependency. I thought Lerna would find it, but the tests are failing and make bootstrap errors too.

oliverdunk added 3 commits May 23, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented May 23, 2020

We should probably strip types from optional member expressions, too! 😅

Does replacing @babel/helper-get-call-context with @babel/helper-skip-types sound ok?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 23, 2020

We should probably strip types from optional member expressions, too! 😅

What do you mean?

Btw, for some other PRs (e.g. #11552) it might be useful to add a function to this package that, give the callee, returns the call (the opposite of the one introduced by this PR).

@oliverdunk
Copy link
Contributor Author

oliverdunk commented May 23, 2020

What do you mean?

I'm thinking of this case: (a?.b as ExampleType)?.c

@oliverdunk oliverdunk changed the base branch from master to main Jun 29, 2020
@oliverdunk oliverdunk requested a review from nicolo-ribaudo Jun 29, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jun 29, 2020

Thanks for the feedback! Found some time to take a look at it.

@nicolo-ribaudo, I've hopefully addressed all of your comments. Would love for you to take another look.

@JLHwung, I need to fix the helper call where I wrongly pass an ASTNode. I'll ping you when that's done. In the meantime, I did leave a response to another bit of feedback you left.

Also changed the base branch to main, good call on the switch!

oliverdunk added 2 commits Jun 30, 2020
@oliverdunk oliverdunk requested a review from JLHwung Jun 30, 2020
Copy link
Member

existentialism left a comment

Just a small nit, but otherwise LGTM, nice work @oliverdunk!

@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jul 3, 2020

Thanks for the reviews @existentialism and @kaicataldo! I went ahead and pushed the rename.

@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jul 3, 2020

The changelog comes from the PR title, right? If so, we might want something broader:

Introduce @babel/helper-skip-transparent-expression-wrappers package, to skip irrelevant types and parentheses in plugins

Copy link
Contributor

JLHwung left a comment

LGTM with some comments.

import * as t from "@babel/types";
import type { NodePath } from "@babel/traverse";

export function getCallContext(callPath: NodePath): NodePath {

This comment has been minimized.

@JLHwung

JLHwung Jul 3, 2020 Contributor

I think getCallContext is a consumer of helper-skip-transparent-expression-wrappers and it should not be in this package.

Do we have a test case on Expected type "CallExpression" or "OptionalCallExpression" ? I think we can go with skipTransparentExprWrappers(path.get("callee")) and not worry about path.type, they should be validated by transformers or implied by how the path is filtered. i.e. "CallExpression|OptionalCallExpression"(path).

This comment has been minimized.

@oliverdunk

oliverdunk Jul 3, 2020 Author Contributor

I'm alright with that. I think it made more sense when the scope of the package was smaller.

@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jul 3, 2020

@JLHwung All sorted 😄

@oliverdunk oliverdunk requested a review from JLHwung Jul 3, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jul 22, 2020

@JLHwung Removed the inline helper functions! This should be good for another look.

I'm happy to keep tweaking this, but at the same time, I won't be offended if you want to jump in with a few commits. If you still have some tidy up in mind it might be quicker :)

@oliverdunk oliverdunk requested a review from JLHwung Jul 22, 2020
Copy link
Contributor

JLHwung left a comment

Awesome!

@JLHwung JLHwung merged commit db56261 into babel:main Jul 30, 2020
7 of 8 checks passed
7 of 8 checks passed
build build
Details
test262-pr Workflow: test262-pr
Details
Gitpod Open an online workspace in Gitpod
Details
Travis CI - Pull Request Build Passed
Details
babel/repl REPL preview is available
Details
build-standalone Workflow: build-standalone
Details
ci/codesandbox Building packages succeeded.
Details
e2e Workflow: e2e
Details
@oliverdunk oliverdunk deleted the oliverdunk:od/11400-typescript-spread-as branch Jul 30, 2020
@oliverdunk
Copy link
Contributor Author

oliverdunk commented Jul 30, 2020

@JLHwung Thanks for the merge!

@JLHwung
Copy link
Contributor

JLHwung commented Jul 30, 2020

@oliverdunk Thank you for all the efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.