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

add support for logical assignments with private properties #11702

Merged
merged 2 commits into from Jul 30, 2020

Conversation

@ryzokuken
Copy link
Contributor

ryzokuken commented Jun 10, 2020

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

Patches the logic for handling assignment operators and adds support for
handling the logical assignment operators appropriately.

@codesandbox
Copy link

codesandbox bot commented Jun 10, 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 25bfbf4:

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

babel-bot commented Jun 10, 2020

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

@hzoo
hzoo approved these changes Jun 10, 2020
Copy link
Member

hzoo left a comment

awesome work, separating out logical/binary expression and a test case. I guess the others must be tested already? (||= &&=)

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 10, 2020

@hzoo right, I'll add more tests for those.

@ryzokuken ryzokuken force-pushed the ryzokuken:gh-11646 branch from 95fec86 to 15150b3 Jun 10, 2020
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 10, 2020

@hzoo PTAL now, I suppose this should suffice. Let me know if it needs any more work.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 10, 2020

We already support private properties and logical assignment, just not together. Would it be ok for you to mark this as a "Bug fix" (since it's just an integration problem) and merge it in a patch release?

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 11, 2020

We already support private properties and logical assignment, just not together. Would it be ok for you to mark this as a "Bug fix" (since it's just an integration problem) and merge it in a patch release?

Right, I was unsure, thanks for letting me know.

@ryzokuken ryzokuken force-pushed the ryzokuken:gh-11646 branch from 15150b3 to f781b05 Jun 11, 2020
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 11, 2020

Thanks @nicolo-ribaudo @JLHwung and others, I fixed the code and the PR body. Hope this is better.

ryzokuken added a commit to ryzokuken/babel that referenced this pull request Jun 11, 2020
Replace a hardcoded check for logical assignment operators with the
LOGICAL_OPERATORS constant in
plugin-proposal-logical-assignment-operators.

Refs: babel#11702 (comment)
Copy link
Contributor

JLHwung left a comment

Thanks!

@JLHwung JLHwung dismissed their stale review Jun 11, 2020

Justin's concern is valid.

@@ -0,0 +1,11 @@
class Foo {

This comment has been minimized.

@JLHwung

JLHwung Jun 11, 2020 Contributor

Please also duplicate the test to test/fixtures/private-loose.

@JLHwung JLHwung changed the base branch from master to main Jun 19, 2020
@JLHwung
Copy link
Contributor

JLHwung commented Jun 25, 2020

Friendly ping @ryzokuken :)

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 25, 2020

Sorry @JLHwung, I'll finish it up this weekend.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jun 28, 2020

@jridgewell I pushed the test you asked for, but I still don't quite understand how to fix the memoization issue. Could you please elaborate what needs to be done in that regard?

@@ -0,0 +1,6 @@
{
"plugins": [
"proposal-logical-assignment-operators",

This comment has been minimized.

@JLHwung

JLHwung Jun 29, 2020 Contributor

Can you add loose: true to both plugins?

{
  "plugins": [
    ["proposal-logical-assignment-operators", { "loose": true }],
    ["proposal-class-properties", { "loose": true }]
  ]
}

The private-loose folder is test cases about loose options so we are sure that both loose and non-loose (default) are working.

@JLHwung
Copy link
Contributor

JLHwung commented Jun 29, 2020

@ryzokuken The memoization is done in

// Give the state handler a chance to memoise the member, since we'll
// reference it twice. The second access (the set) should do the memo
// assignment.
this.memoise(member, 2);

The second argument 2 means when we should memoize the member call. i.e. obj.self().#a += 1 is roughly transformed to

// obj.self().#a = obj.self().#a + 1
classFieldSet($objSelf = obj.self(), a, classFieldGet($objSelf, a) + 1)

via

this.set(member, t.binaryExpression("+", this.get(member), value))

We should memoize obj.self() in classFieldSet because it is the first argument of classFieldSet and thus evaluated before classFieldGet. However since in Babel we have called this.get and this.set sequentially, 2 is a counter to indicate that we should memoize the member in the second call of [this.get, this.set] sequence. (Think about TTL in ICMP protocol)

However obj.self().#a ??= 1 is different here, it should be transformed as

// obj.self().#a ?? (obj.self().#a = 1)
classFieldGet($objSelf = obj.self(), a) ?? classFieldSet($objSelf, a, 1)

// current PR
classFieldGet($objSelf, a) ?? classFieldSet($objSelf = obj.self(), a, 1)

via

t.LogicalExpression("??", this.get(member), this.set(member))

Note that the call sequence is unchanged -- this.get, this.set. But we should memoize in classFieldGet because it is now evaluated before classFieldSet. To achieve that we can decrement the memoize counter for logical expressions.

// add some comment about why 1 is chosen for logical expressions
this.memoise(member, 1);
@jridgewell
Copy link
Member

jridgewell commented Jun 30, 2020

Thank you for explaining @JLHwung, that's extremely thorough!

@JLHwung
Copy link
Contributor

JLHwung commented Jul 15, 2020

@ryzokuken 🏓️ Logical Assignment is seeking advancement in July meeting, will be great if we can ship this bug fix after that.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jul 15, 2020

@JLHwung my bad, I'll get that done.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jul 20, 2020

Update: I've made the change, I'll push this in a bit, sorry for the delay. I got super busy with all sorts of weird things.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jul 20, 2020

@JLHwung does this look good enough? Let me know if it does and I'll autosquash and force push.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 20, 2020

@ryzokuken Can you update the test case (config/fixture) w.r.t.
#11702 (review)
and
#11702 (comment) ?

_classPrivateFieldGet(this, _nullish) ?? _classPrivateFieldSet(this, _nullish, 42);
_classPrivateFieldGet(this, _and) && _classPrivateFieldSet(this, _and, 0);
_classPrivateFieldGet(this, _or) || _classPrivateFieldSet(this, _or, 0);
_classPrivateFieldGet(_this$self, _nullish) ?? _classPrivateFieldSet(_this$self = this.self(), _nullish, 42);

This comment has been minimized.

@jridgewell

jridgewell Jul 20, 2020 Member

This test still needs to be updated. I believe the transform code is correct, the test is just out of date now.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jul 22, 2020

@jridgewell @JLHwung the tests should be updated now. Sorry for the delay again, I noticed the stage advancement yesterday.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 22, 2020

The e2e was fixed on upstream main. Can you rebase?

@ryzokuken ryzokuken force-pushed the ryzokuken:gh-11646 branch from 3d35b78 to 86cfb91 Jul 22, 2020
ryzokuken added a commit to ryzokuken/babel that referenced this pull request Jul 22, 2020
Replace a hardcoded check for logical assignment operators with the
LOGICAL_OPERATORS constant in
plugin-proposal-logical-assignment-operators.

Refs: babel#11702 (comment)
ryzokuken added 2 commits Jun 10, 2020
Patches the logic for handling assignment operators and adds support for
handling the logical assignment operators appropriately.

Fixes: #11646
Replace a hardcoded check for logical assignment operators with the
LOGICAL_OPERATORS constant in
plugin-proposal-logical-assignment-operators.

Refs: #11702 (comment)
@ryzokuken ryzokuken force-pushed the ryzokuken:gh-11646 branch from 86cfb91 to 25bfbf4 Jul 22, 2020
Copy link
Contributor

JLHwung left a comment

Justin's concern has been addressed. @ryzokuken Thanks for all the efforts!

@JLHwung JLHwung merged commit 2ac49ba into babel:main Jul 30, 2020
8 of 9 checks passed
8 of 9 checks passed
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
codecov/project 91.85% (target 90.00%)
Details
e2e Workflow: e2e
Details
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Jul 30, 2020

Thanks for all the help on this, everyone!

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.

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