1405943 - Implement Pipeline Operator |>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Koki Takahashi]

The pipeline operator proposed to TC39, which is stage-1 now, is very handy and intuitive extension of function calling. It is good to have experimental implementation in SpiderMonkey.

Proposal: https://github.com/tc39/proposal-pipeline-operator

Comment 1

[Tooru Fujisawa [:arai]]

thank you for filing :)

I mentor this bug.

Comment 2

[Koki Takahashi]

Attachment: WIP implementation

This is patch to implement pipeline operator based on the current draft spec https://github.com/tc39/proposal-pipeline-operator/pull/51/files#diff-3540caefa502006d8a33cb1385720803

Comment 3

[Koki Takahashi]

Comment on attachment 8917718 [details] [diff] [review]
WIP implementation

Note that this patch is also heavily credited to [e-mail] and mentored by [e-mail]. I'm thankful to those.

Comment 4

[Till Schneidereit [:till]]

Note that there is significant skepticism about this proposal in tc39. Stage 1 just means that further exploration was deemed useful and is not really a signal for whether something will ever end up in the spec.

For that reason, I think this should not only be Nightly-only, but also behind some additional flag. Ideally we'd have a new flag for each similar thing, but in practice it's unfortunately quite annoying to add them.

Jason, what's your opinion on this?

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8917718 [details] [diff] [review]
WIP implementation

Review of attachment 8917718 [details] [diff] [review]:
-----------------------------------------------------------------

The code change itself looks good :)

as till said, this needs to be behind configure flag or something,
that means, if Firefox or SpiderMonkey are built without that flag, they shouldn't accept pipeline operator syntax, but just throw the same error as unpatched code.

here's the example for adding configure flag, that defines macro
https://hg.mozilla.org/mozilla-central/diff/862ba8683d19/js/moz.configure

::: .gitignore
@@ +1,1 @@
> +js/src/build_DBG.OBJ

please revert this change

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3308,4 @@
>        case PNK_DIV:
>        case PNK_MOD:
>        case PNK_POW:
> +      case PNK_PIPELINE:

it would be nice to put this into its own block, with `pn->pn_count == 2` assertion,
next to the block for PNK_CALL, since this is actually function call and the comment about binary operator above doesn't match.

@@ +9443,4 @@
>  }
>  
>  bool
> +BytecodeEmitter::emitCallee(ParseNode* callee, ParseNode* call, bool spread, bool* callop) {

"{" for function definition should be in its own line

@@ +9515,5 @@
> +}
> +
> +bool BytecodeEmitter::emitPipeline(ParseNode* pn)
> +{
> +  uint32_t argc = pn->pn_count - 1;

argc should always be 1, so, instead of this, please add `MOZ_ASSERT(pn->pn_count == 2)`,
and then replace `argc` below with 1.

@@ +9517,5 @@
> +bool BytecodeEmitter::emitPipeline(ParseNode* pn)
> +{
> +  uint32_t argc = pn->pn_count - 1;
> +
> +  if (argc >= 2) {

so, this block is unnecessary.

@@ +9526,5 @@
> +  if (!emitTree(pn->pn_head))
> +      return false;
> +
> +  bool callop = true;
> +  bool spread = false;

this can be removed.

@@ +9527,5 @@
> +      return false;
> +
> +  bool callop = true;
> +  bool spread = false;
> +  ParseNode* pn2 = pn->pn_head->pn_next;

can you rename `pn2` to `callee` ?

@@ +9529,5 @@
> +  bool callop = true;
> +  bool spread = false;
> +  ParseNode* pn2 = pn->pn_head->pn_next;
> +
> +  if (emitterMode == BytecodeEmitter::SelfHosting && !spread && pn2->isKind(PNK_NAME)) {

this is for handling special-syntax in self-hosted JS files (*.js files under js/src/builtin), that we can specify `this` value, and we don't have to allow it for pipeline operator,
please remove this block.

https://dxr.mozilla.org/mozilla-central/rev/3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf/js/src/builtin/Array.js#51
>     return callFunction(ArrayIndexOf, list, searchElement, fromIndex);

@@ +9552,5 @@
> +          return emitSelfHostedHasOwn(pn);
> +      // Fall through
> +  }
> +
> +  if (!emitCallee(pn2, pn, spread, &callop))

spread here should be replaced with:
  /* spread = */ false

@@ +9555,5 @@
> +
> +  if (!emitCallee(pn2, pn, spread, &callop))
> +      return false;
> +
> +  if (!callop) {

please add `// Emit room for |this|.` comment before this line (same as emitCallOrNew),
or maybe move this into yet another method that's called from both emitPipeline and emitCallOrNew, with descriptive name what it does.

@@ +9603,5 @@
> +
> +    if (emitterMode == BytecodeEmitter::SelfHosting && !spread && pn2->isKind(PNK_NAME)) {
> +        // Calls to "forceInterpreter", "callFunction",
> +        // "callContentFunction", or "resumeGenerator" in self-hosted
> +        // code generate inline bytecode.

can you re-wrap this comment into 80 chars?
(we're using 80 chars for comment, 99 chars for code)

::: js/src/frontend/BytecodeEmitter.h
@@ +835,5 @@
>      MOZ_MUST_USE bool emitSuperElemOperands(ParseNode* pn,
>                                              EmitElemOption opts = EmitElemOption::Get);
>      MOZ_MUST_USE bool emitSuperElemOp(ParseNode* pn, JSOp op, bool isCall = false);
> +
> +    // Emit pipeline operator.

this comment should be placed before emitPipeline

::: js/src/frontend/FoldConstants.cpp
@@ +405,5 @@
>          MOZ_CRASH("ContainsHoistedDeclaration should have indicated false on "
>                    "some parent node without recurring to test this node");
>  
> +      case PNK_PIPELINE:
> +        MOZ_ASSERT(node->isArity(PN_NULLARY));

this should be PN_LIST

::: js/src/frontend/ParseNode.h
@@ +21,4 @@
>  class FunctionBox;
>  class ObjectBox;
>  
> +  #define FOR_EACH_PARSE_NODE_KIND(F) \

please revert the change to this line

::: js/src/frontend/Parser.cpp
@@ +7994,4 @@
>  }
>  
>  static const JSOp ParseNodeKindToJSOp[] = {
> +    JSOP_NOP,

please add some comment that explains this is for pipeline

::: js/src/frontend/TokenStream.cpp
@@ +1730,4 @@
>        case '|':
>          if (matchChar('|'))
>              tp->type = TOK_OR;
> +        else if(matchChar('>'))

please put a space after `if`

::: js/src/tests/js1_8_5/extensions/pipeline-operator.js
@@ +1,1 @@
> +var BUGNUMBER = 1405943;

this directory is for language extensions in JavaScript 1.8.5.

this file should be placed in js/src/tests/ecma_{YEAR_OR_SOMETHING}/Pipeline/{FILENAME_FOR_EACH_SECTION}.js
also separate file for each section (like, precedence, evaluation order, etc)

@@ +2,5 @@
> +var summary = "Implement pipeline operator";
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +// Operator precedence

so, this section (and all other sections) should be in its own file,
so that it's easier to see what's broken if any issue happens.

@@ +10,5 @@
> +const toString = (v) => v.toString();
> +let a = 8;
> +assertEq(10 |> double |> increment |> double, 42);
> +assertEq(++ a |> double, 18);
> +assertEq(typeof 42 |> getType, "string");

this isn't testing the precedence.
the result becomes "string" in any case
  (typeof 42) |> getType
  typeof (42 |> getType)

@@ +21,5 @@
> +assertEq(0 < 1 |> toString, "true");
> +assertEq("a" in {} |> toString, "false");
> +assertEq(10 instanceof String |> toString, "false");
> +assertEq(10 == 10 |> toString, "true");
> +assertEq(0b0101 & 0b1100 |> increment, 0b0101);

this is also not testing the precedence.
the result is 0b0101 in both case.

@@ +22,5 @@
> +assertEq("a" in {} |> toString, "false");
> +assertEq(10 instanceof String |> toString, "false");
> +assertEq(10 == 10 |> toString, "true");
> +assertEq(0b0101 & 0b1100 |> increment, 0b0101);
> +assertEq(false && true |> toString, "false");

it would be nice to test the case that other operator is placed on the right side of pipeline operator, for each case.

@@ +23,5 @@
> +assertEq(10 instanceof String |> toString, "false");
> +assertEq(10 == 10 |> toString, "true");
> +assertEq(0b0101 & 0b1100 |> increment, 0b0101);
> +assertEq(false && true |> toString, "false");
> +assertEq(true ? 10 : 20 |> toString, 10);

can you add other cases that pipeline operator is in other place of conditional operator? (I mean, A and B for `A ? B : C`)

@@ +44,5 @@
> +    assertEq("testvalue" |> eval, 10);
> +})();
> +
> +// Parse error
> +assertThrows(() => Reflect.parse("2 | > parseInt"), SyntaxError);

Reflect.parse is defined by default only in shell, but this test is executed also on browser.

please enclose this and the following Reflect.parse block with something like the following:
https://dxr.mozilla.org/mozilla-central/rev/3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf/js/src/tests/ecma_2017/AsyncFunctions/BoundNames.js#15
> if (typeof Reflect !== "undefined" && Reflect.parse) {

@@ +61,5 @@
> +    }
> +};
> +
> +10 |> receiver.foo;
> +assertThrowsInstanceOf(() => 10 |> receiver.bar, Error);

can you explain what this is testing?

Comment 6

[Koki Takahashi]

Attachment: implementation

This is fixed implementation.

Comment 7

[Koki Takahashi]

Attachment: Implementation

Fixed patch format.

Comment 8

[Jason Orendorff [:jorendorff]]

Yes, this can land behind the configure flag, with the understanding that if the proposal does not advance in TC39's next two monthly meetings, we will back it out.

This patch is very good work.

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8918829 [details] [diff] [review]
Implementation

Review of attachment 8918829 [details] [diff] [review]:
-----------------------------------------------------------------

thanks again :)
almost there.

next time, try exporting the diff as a patch file, that contains commit message and author information.

::: js/moz.configure
@@ +256,5 @@
>      set_define('LIBFUZZER', enable_libfuzzer)
> +
> +# Enable pipeline operator
> +# ===================================================
> +js_option('--enable-pipeline-operator', default=True, help='Enable pipeline operator')

this feature needs to be disabled by default.
please change this line to `default=False`

::: js/src/frontend/BytecodeEmitter.cpp
@@ +9519,5 @@
>  
> +    return true;
> +}
> +
> +bool BytecodeEmitter::emitPipeline(ParseNode* pn)

please put "bool" in its own line.
(newline after it)

@@ +9521,5 @@
> +}
> +
> +bool BytecodeEmitter::emitPipeline(ParseNode* pn)
> +{
> +  MOZ_ASSERT(pn->pn_count == 2);

please use 4 spaces indentation, instead of 2 spaces.

@@ +9578,5 @@
> +
> +    ParseNode* pn2 = pn->pn_head;
> +    bool spread = JOF_OPTYPE(pn->getOp()) == JOF_BYTE;
> +
> +    if (emitterMode == BytecodeEmitter::SelfHosting && !spread && pn2->isKind(PNK_NAME)) {

it would be nice to put `pn2->isKind(PNK_NAME)` condition first.

::: js/src/frontend/Parser.cpp
@@ +7997,5 @@
> +    /*
> +      This is for pipeline operator which does not emit its own JSOp
> +      but has highest precedence in binary operators
> +    */
> +    JSOP_NOP,

the definition of ParseNodeKindToJSOp is moved to BytecodeEmitter.cpp in bug 1405760.
please rebase your patch onto current mozilla-central.

also, please use the following style there:
    // JSOP_NOP for pipeline operator which does not emit its own JSOp
    // but has highest precedence in binary operators.
("JSOP_NOP" instead of "this" to be clearer. also the trailing period)

@@ +8064,5 @@
> +    10, /* PNK_SUB */
> +    11, /* PNK_STAR */
> +    11, /* PNK_DIV */
> +    11, /* PNK_MOD */
> +    12  /* PNK_POW */

I guess, we could make this macro+enum, like token kind etc,
so that we don't have to change all lines when adding an entry.
but it's for other bug, not here.

::: js/src/tests/ecma_2018/Pipeline/eval.js
@@ +5,5 @@
> +
> +const testvalue = 10;
> +
> +if (hasPipeline) {
> +	// Pipelined eval should be indirect eval call

please use 4 SPACEs (0x20) instead of a HTAB (0x09), everywhere.

@@ +8,5 @@
> +if (hasPipeline) {
> +	// Pipelined eval should be indirect eval call
> +	(() => {
> +	    const testvalue = 20;
> +	    assertEq(eval('"testvalue" |> eval'), 10);

instead of enclosing each occurrence of pipeline operator, you can just enclose whole code with eval, like:

if (hasPipeline) {
    eval(`
    // Pipelined eval should be indirect eval call
    (() => {
        const testvalue = 20;
        assertEq("testvalue" |> eval, 10);
    })();
    `);
}

so you can just remove enclosing eval when it gets enabled, that's easy and generates less diff.

::: js/src/tests/ecma_2018/Pipeline/evaluation-order.js
@@ +3,5 @@
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +if (hasPipeline) {
> +	// Evaluation order

if you place a comment here, it would be nice to describe more about what it's testing, or the detail of the expected behavior.

::: js/src/tests/ecma_2018/Pipeline/parse-error.js
@@ +5,5 @@
> +
> +if (hasPipeline) {
> +	// Parse error
> +	if (typeof Reflect !== "undefined" && Reflect.parse) {
> +		assertThrows(() => Reflect.parse("2 | > parseInt"), SyntaxError);

you can use Function constructor to test this, without relying on the existence of Reflect.parse

@@ +8,5 @@
> +	if (typeof Reflect !== "undefined" && Reflect.parse) {
> +		assertThrows(() => Reflect.parse("2 | > parseInt"), SyntaxError);
> +		assertThrows(() => Reflect.parse("2 ||> parseInt"), SyntaxError);
> +		assertThrows(() => Reflect.parse("2 |>> parseInt"), SyntaxError);
> +		assertThrows(() => Reflect.parse("2 <| parseInt"), SyntaxError);

can you add "2 |> |> parseInt" as well?
it's a syntax error with valid token.

also, to test parser itself, it would be nice to test the following as well.
"2 |>"
"|> parseInt"

::: js/src/tests/ecma_2018/Pipeline/parse.js
@@ +1,2 @@
> +const BUGNUMBER = 1405943;
> +const summary = "Implement pipeline operator (parse)";

this is specific to Reflect.parse, so it would be nice to say "Reflect.parse", instead of just "parse"
and also rename this file to "reflect-parse.js"

::: js/src/tests/ecma_2018/Pipeline/precedence.js
@@ +9,5 @@
> +	const increment = (n) => n + 1;
> +	const getType = (v) => typeof v;
> +	const toString = (v) => v.toString();
> +	let a = 8;
> +	assertEq(eval('10 |> double |> increment |> double'), 42);

sorry I overlooked this.

this hits assertion failure in BytecodeEmitter::emitPipeline
MOZ_ASSERT(pn->pn_count == 2);

you can see the problematic parse tree with the following code:
  parse("1|>2|>3|>4")
that shows:
(STATEMENTLIST [(SEMI (PIPELINE [1
                                 2
                                 3
                                 4]))])


because of the following code that creates list for same operator.
  https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/js/src/frontend/Parser.cpp#8088-8101
  https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/js/src/frontend/ParseNode.cpp#574
(that's the reason why binary operators are PNK_LIST instead of PNK_BINARY, as you noticed before)

so, we need to either:
  1. avoid creating list with more than 3 elements, while parsing (so current emitter works)
  2. support list with 3 or more elements while emitting (so we don't have to change parse tree)
IMO 2 is better, since that's how other binary operators work.

that said, it would be better creating a testcase for Reflect.parse with the above code.

::: js/src/tests/ecma_2018/Pipeline/receiver.js
@@ +10,5 @@
> +	        assertEq(value, 10);
> +	        assertEq(extra, undefined);
> +	        assertEq(arguments.length, 1);
> +	        assertEq(arguments[0], 10)
> +	        assertEq(this, receiver);

great test :)

::: js/src/tests/ecma_2018/Pipeline/shell.js
@@ +1,1 @@
> +const hasPipeline = (() => {

please make `hasPipeline` a function that checks the existence of pipeline operator when it gets called.

::: js/src/tests/ecma_2018/Pipeline/type-error.js
@@ +7,5 @@
> +    // Type error
> +    assertThrowsInstanceOf(() => eval("10 |> 10"), TypeError);
> +    assertThrowsInstanceOf(() => eval('10 |> "foo"'), TypeError);
> +    assertThrowsInstanceOf(() => eval("10 |> null"), TypeError);
> +    assertThrowsInstanceOf(() => eval("10 |> undefined"), TypeError);

it would be nice to test boolean, object, array, regexp, and symbol as well.

Comment 10

[Koki Takahashi]

Attachment: 0001-Implement-pipeline-operator.patch

Thanks for the feedback :) This is fixed implementation (Part 1).

Comment 11

[Koki Takahashi]

Attachment: 0001-Bug-1405943-Part-1-Implement-Pipeline-Operator.patch

Fixed patch format.

Comment 12

[Koki Takahashi]

Attachment: 0002-Bug-1405943-Part-2-Implement-Pipeline-Operator.patch

And this is part 2.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8919152 [details] [diff] [review]
0001-Bug-1405943-Part-1-Implement-Pipeline-Operator.patch

Review of attachment 8919152 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there :D

::: js/src/frontend/BytecodeEmitter.cpp
@@ +9547,5 @@
> +
> +bool
> +BytecodeEmitter::emitPipeline(ParseNode* pn)
> +{
> +    MOZ_ASSERT(pn->isArity(PN_LIST));

please add `MOZ_ASSERT(pn->pn_count >= 2)`

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8919153 [details] [diff] [review]
0002-Bug-1405943-Part-2-Implement-Pipeline-Operator.patch

Review of attachment 8919153 [details] [diff] [review]:
-----------------------------------------------------------------

please add browser.js (empty file) in the same directory as shell.js.

::: js/src/tests/ecma_2018/Pipeline/evaluation-order.js
@@ +3,5 @@
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +if (hasPipeline()) {
> +	eval(`

please use 4 SPACEs.
here and some other files.

::: js/src/tests/ecma_2018/Pipeline/parse-error.js
@@ +3,5 @@
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +if (hasPipeline()) {
> +    // Parse error

it would be nice to change this comment to "Invalid Token", and add "Invalid Syntax" above "2 |>" case.

::: js/src/tests/ecma_2018/Pipeline/shell.js
@@ +1,1 @@
> +const hasPipeline = () => {

please use normal function declaration.

function hasPipeline() {
...
}

Comment 15

[Koki Takahashi]

Attachment: 0001-Bug-1405943-Part-1-Implement-Pipeline-Operator.patch

Fixed implementation (Part 1).

Comment 16

[Koki Takahashi]

Attachment: 0002-Bug-1405943-Part-2-Add-tests-for-pipeline-operator.patch

Fixed implementation (Part 2).

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8919164 [details] [diff] [review]
0001-Bug-1405943-Part-1-Implement-Pipeline-Operator.patch

Review of attachment 8919164 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent :D

here's try run for your patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2668c099c84b1f3cad05191908615041667c1c07

when the try run finishes without any failure, these patches can be landed

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7371550c7fc5
Part 1: Implement Pipeline Operator |>. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5d50e17fef
Part 2: Add tests for pipeline operator. r=arai

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7371550c7fc5
https://hg.mozilla.org/mozilla-central/rev/0e5d50e17fef

Comment 20

[Alexandre Folle de Menezes]

Shouldn't the latest nightly be passing the Kangax test then?
http://kangax.github.io/compat-table/esnext/

Comment 21

[Tooru Fujisawa [:arai]]

(In reply to Alexandre Folle de Menezes from comment #20)
> Shouldn't the latest nightly be passing the Kangax test then?
> http://kangax.github.io/compat-table/esnext/

Currently this is behind configure flag, and disabled by default.

Comment 22

[Florian Scholz (Open Web Docs)]

Added https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Pipeline_operator and marked it as experimental. It won't appear on the release notes as it's disabled by default.

Feel free to add more to the MDN wiki page.