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

tc39-proposal-pattern-matching #6761

Closed
wants to merge 41 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@vincentdchan
Contributor

vincentdchan commented Nov 7, 2017

Q                       A
Fixed Issues? No
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? parsing pattern matching expression
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

Example:

var test = {
    a: [1,2,3],
    b: {
      bb: "ok"
    },
    c: [[1,2], [3,4]],
}

// nested
assert.equal(match(test) {
  {c:[[1,2], [d, 4]]}: d, // match 3, and bind d to 3
  else: "foo"
}, 3);
var test = {
    a: 1,
    b: 2,
    c: {
      cc: "hello",
      ccc: " world",
    },
};

// rest matching
assert.equal(match(test) {
  {c: {cc, ...rest}}: cc + rest.ccc,  // match object, and bind rest property to `rest`
  else: "foo"
}, "hello world");

Progress:

  • a parser for match expression
  • a transform plugin
  • rest identifier feature

vincentdchan added some commits Nov 6, 2017

do the basic parsing of pattern matching
define AST in types.js
add a parsing plugin
add basic test and fix a parsing bug
use new match pattern
new AST of clause
fix "else" parsing bug
add a test case
add tests and fix bug
fix a bug about string literal
add test of match function call
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Nov 7, 2017

Collaborator

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

Collaborator

babel-bot commented Nov 7, 2017

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

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 7, 2017

Member

Thanks for the PR! Issue for babel is at babel/proposals#6

Just FYI @krzkaczor also had started a PR at babel/babylon#635 before we moved babylon back into this repo

Member

hzoo commented Nov 7, 2017

Thanks for the PR! Issue for babel is at babel/proposals#6

Just FYI @krzkaczor also had started a PR at babel/babylon#635 before we moved babylon back into this repo

@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Nov 7, 2017

Contributor

@hzoo Oh, I'll take a look at it, I'm sure some code and tests can be reused.

Contributor

vincentdchan commented Nov 7, 2017

@hzoo Oh, I'll take a look at it, I'm sure some code and tests can be reused.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 9, 2017

Member

Btw can ast test changes in https://deploy-preview-1421--babel.netlify.com/repl/build/5694/, from babel/website#1421

ah nvm need to make a syntax plugin and add it to stage-0 to test

Member

hzoo commented Nov 9, 2017

Btw can ast test changes in https://deploy-preview-1421--babel.netlify.com/repl/build/5694/, from babel/website#1421

ah nvm need to make a syntax plugin and add it to stage-0 to test

Show outdated Hide outdated .vscode/settings.json Outdated
add syntax plugin for pattern matching
remove vscode setting
@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Nov 11, 2017

Contributor

@hzoo got it.

Contributor

vincentdchan commented Nov 11, 2017

@hzoo got it.

vincentdchan and others added some commits Nov 13, 2017

@vincentdchan vincentdchan changed the title from Babylon: parsing part of pattern matching to tc39-proposal-pattern-matching Nov 13, 2017

vincentdchan added some commits Nov 13, 2017

basic transform part
including literal and array pattern matching

literal pattern matching transform

array pattern matching transform

simplify generations of expressions

fix array pattern matching bug

identifier pattern matching transform

fix numericliteral bug
Show outdated Hide outdated packages/babylon/.flowconfig Outdated
@@ -0,0 +1,3 @@
let a = match(b) {
"3": "4"
};

This comment has been minimized.

@xtuc

xtuc Nov 22, 2017

Member

I think there is an issue here, the expected output wasn't transpiled.

And I noticed that the transpiled version is incorrect:

var a = function () {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}();

It seems that the b binding is missing.

@xtuc

xtuc Nov 22, 2017

Member

I think there is an issue here, the expected output wasn't transpiled.

And I noticed that the transpiled version is incorrect:

var a = function () {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}();

It seems that the b binding is missing.

This comment has been minimized.

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@xtuc I don't understand. I suppose babel-generator just generate code from ASTs, it should not transform the ASTs.

And I think the transpiled version you pinned is right, what should b bind to?

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@xtuc I don't understand. I suppose babel-generator just generate code from ASTs, it should not transform the ASTs.

And I think the transpiled version you pinned is right, what should b bind to?

This comment has been minimized.

@titouancreach

titouancreach Nov 22, 2017

@vincentdchan It would rather be:

var a = function (b) {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}();

? Otherwise, how do we expect the (b === 3) to work ?

@titouancreach

titouancreach Nov 22, 2017

@vincentdchan It would rather be:

var a = function (b) {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}();

? Otherwise, how do we expect the (b === 3) to work ?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Nov 22, 2017

Member

b is defined somewhere else. e.g.

let b = "3";
let a = match(b) {
  "3": "4"
}
@nicolo-ribaudo

nicolo-ribaudo Nov 22, 2017

Member

b is defined somewhere else. e.g.

let b = "3";
let a = match(b) {
  "3": "4"
}

This comment has been minimized.

@xtuc

xtuc Nov 22, 2017

Member

@vincentdchan yes sorry this is indeed the generator.

Nicolo is right, the fact that the variable isn't defined here confused me.
What is the best: pass it as arg or as closure? Probably not the same from the memory point of view.

@xtuc

xtuc Nov 22, 2017

Member

@vincentdchan yes sorry this is indeed the generator.

Nicolo is right, the fact that the variable isn't defined here confused me.
What is the best: pass it as arg or as closure? Probably not the same from the memory point of view.

This comment has been minimized.

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@titouancreach I suppose you want to write:

var a = function (b) {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}(b); // <-- parameter

@xtuc I think the differences between arg and closure can be ignored. The function is disposable. After it's called, the closure will be released by GC. Actually, the script engine decides those things, I focus on the correctness of syntax.

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@titouancreach I suppose you want to write:

var a = function (b) {
  {
    if (b === "3") return "4";else throw new Error("No patterns are matched");
  }
}(b); // <-- parameter

@xtuc I think the differences between arg and closure can be ignored. The function is disposable. After it's called, the closure will be released by GC. Actually, the script engine decides those things, I focus on the correctness of syntax.

This comment has been minimized.

@titouancreach
@titouancreach

titouancreach Nov 22, 2017

@vincentdchan Yes exactly

This comment has been minimized.

@xtuc

xtuc Nov 22, 2017

Member

Ok sounds good

@xtuc

xtuc Nov 22, 2017

Member

Ok sounds good

else: "else",
4: "abandoned"
}
```

This comment has been minimized.

@xtuc

xtuc Nov 22, 2017

Member

This is spec compliant but it makes no sens to me to allow unreachable clauses after the else.

@xtuc

xtuc Nov 22, 2017

Member

This is spec compliant but it makes no sens to me to allow unreachable clauses after the else.

This comment has been minimized.

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@xtuc I agree with you. See the latest commit.

@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@xtuc I agree with you. See the latest commit.

This comment has been minimized.

@xtuc

xtuc Nov 22, 2017

Member

Actually since it was spec compliant we should probably keep it. I opened an issue for that let's see.

@xtuc

xtuc Nov 22, 2017

Member

Actually since it was spec compliant we should probably keep it. I opened an issue for that let's see.

This comment has been minimized.

@vincentdchan
@vincentdchan

vincentdchan Nov 22, 2017

Contributor

@xtuc OK

@titouancreach

This comment has been minimized.

Show comment
Hide comment
@titouancreach

titouancreach Dec 7, 2017

Any plan to merge this ?

titouancreach commented Dec 7, 2017

Any plan to merge this ?

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Dec 11, 2017

Member

@titouancreach yes we will, we are currently busy with the Babel 7 release sorry

Member

xtuc commented Dec 11, 2017

@titouancreach yes we will, we are currently busy with the Babel 7 release sorry

Merge branch 'master' into pattern-matching
# Conflicts:
#	packages/babel-preset-stage-0/package.json
#	packages/babel-preset-stage-0/src/index.js
@babakness

This comment has been minimized.

Show comment
Hide comment
@babakness

babakness Feb 2, 2018

Please merge this!! Excited to try it out :-)

babakness commented Feb 2, 2018

Please merge this!! Excited to try it out :-)

@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Feb 28, 2018

Contributor

@babakness It's coming soon! I am working on it.

Contributor

vincentdchan commented Feb 28, 2018

@babakness It's coming soon! I am working on it.

@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Feb 28, 2018

Contributor

@hzoo @xtuc It's complete!

Contributor

vincentdchan commented Feb 28, 2018

@hzoo @xtuc It's complete!

switch (pattern.type) {
case "NumericLiteral":
case "BigIntLiteral":

This comment has been minimized.

@xtuc

xtuc Mar 2, 2018

Member

Since BigInt is not implemented in Babel, I think we should already include it here. It sounds like extra maintenance if it change.

Also it's not testable?

We'll need to create a issue to track that tho.

@xtuc

xtuc Mar 2, 2018

Member

Since BigInt is not implemented in Babel, I think we should already include it here. It sounds like extra maintenance if it change.

Also it's not testable?

We'll need to create a issue to track that tho.

This comment has been minimized.

@vincentdchan

vincentdchan Mar 2, 2018

Contributor

So should we keep it?

@vincentdchan

vincentdchan Mar 2, 2018

Contributor

So should we keep it?

This comment has been minimized.

@xtuc

xtuc Mar 2, 2018

Member

I would suggest to remove it, but let's wait for more thoughts on this.

@xtuc

xtuc Mar 2, 2018

Member

I would suggest to remove it, but let's wait for more thoughts on this.

Show outdated Hide outdated packages/babel-plugin-proposal-pattern-matching/src/index.js Outdated
Show outdated Hide outdated ...al-pattern-matching/test/fixtures/stupid-fixture-nesting-folder/array.js Outdated
Show outdated Hide outdated ...l-pattern-matching/test/fixtures/stupid-fixture-nesting-folder/errors.js Outdated
@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Mar 4, 2018

Contributor

There are still two issues here:

This PR lasted four months. 😂😂😂

Contributor

vincentdchan commented Mar 4, 2018

There are still two issues here:

This PR lasted four months. 😂😂😂

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Mar 5, 2018

Member

This PR lasted four months.

I didn't finish my last review, sorry. And sorry if that takes so long, thanks!

Member

xtuc commented Mar 5, 2018

This PR lasted four months.

I didn't finish my last review, sorry. And sorry if that takes so long, thanks!

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Mar 25, 2018

nudge: this spec looks pretty different now. See https://github.com/tc39/proposal-pattern-matching

zkat commented Mar 25, 2018

nudge: this spec looks pretty different now. See https://github.com/tc39/proposal-pattern-matching

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Mar 26, 2018

Member

Thanks for the update @zkat, we can see the changes in the PR here tc39/proposal-pattern-matching#65.

Sorry for the delai @vincentdchan, TC39 was missing a champion on this proposal and @zkat took over championing now (thanks 😄 )

Member

xtuc commented Mar 26, 2018

Thanks for the update @zkat, we can see the changes in the PR here tc39/proposal-pattern-matching#65.

Sorry for the delai @vincentdchan, TC39 was missing a champion on this proposal and @zkat took over championing now (thanks 😄 )

@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Mar 26, 2018

Contributor

@xtuc OK. I have communicated with @zkat privately, and I will still work on this PR. But it takes time to implement the new proposal.

Contributor

vincentdchan commented Mar 26, 2018

@xtuc OK. I have communicated with @zkat privately, and I will still work on this PR. But it takes time to implement the new proposal.

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Mar 26, 2018

Member

@vincentdchan sounds good to me, thanks for your work on this!

Member

xtuc commented Mar 26, 2018

@vincentdchan sounds good to me, thanks for your work on this!

@zloirock

This comment has been minimized.

Show comment
Hide comment
@zloirock

zloirock Mar 26, 2018

Member

I added Symbol.{patternMatch, patternValue} for this proposal to core-js@3.

Member

zloirock commented Mar 26, 2018

I added Symbol.{patternMatch, patternValue} for this proposal to core-js@3.

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Mar 27, 2018

Member

Btw @vincentdchan we could close this PR and open a new one if you want to avoid the old comments here, might be easier to follow?

Member

xtuc commented Mar 27, 2018

Btw @vincentdchan we could close this PR and open a new one if you want to avoid the old comments here, might be easier to follow?

@vincentdchan vincentdchan referenced this pull request Mar 27, 2018

Open

[WIP] tc39/pattern-matching #7633

3 of 5 tasks complete
@vincentdchan

This comment has been minimized.

Show comment
Hide comment
@vincentdchan

vincentdchan Mar 27, 2018

Contributor

Closed! Move to #7633

Contributor

vincentdchan commented Mar 27, 2018

Closed! Move to #7633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment