[RFC] Pipe Operator - Githubissues

clsource commented 3 years ago

The pipe operator |> seems something that could ease the function calls.

The pipe operator would invoke the call method and pass the previous result to the next function

var myfunc = Fn.new { "hello" }
var otherfunc = Fn.new {|salute| "%(salute) you!" }
var thirdfunc = Fn.new {|result| result * 2 }

var result = myfunc |> otherfunc |> thirdfunc

// the same as 
var result = thirdfunc.call(otherfunc.call(myfunc.call()))

// hello you!hello you!
System.print(result)

// also can pass non function values
// so you can use the function result if its needs params
"hello" |> otherfunc |> thirdfunc

// if more params are needed besides the first
// can be passed using a List as a intermediate step
// the previous value would be prepended to the list
myfunc |> ["name", 12] |> otherfunc |> thirdfunc

Resources:

clsource commented 3 years ago

Wren is an OOP language. Maybe looking for functions without call() is a fool's errand xd

clsource commented 3 years ago

I implemented a simple proof of concept

imagen


var myFunc = Fn.new {| message, second, third |
    System.print(message)
    System.print("Second: %(second), Third: %(third)")
    return true
}

var isTrue = Fn.new {|result|
    if (result) {
        return System.print("Is True")
    }
    return System.print("Is False")
}

"Hello" |> [1, 2] |> myFunc |> isTrue

By using the Lists as a special value used for curryng it can be used without problems.

It was implemented as a simple new operator following the steps defined by @ChayimFriedman2 https://discord.com/channels/484796661751873554/484796759437213716/825821272263884801

Creating a token for it in the lexer: 1) Add a TokenType in https://github.com/wren-lang/wren/blob/4d1d0d972efffd072f6a7baa5c37f312258c3955/src/vm/wren_compiler.c#L53-L138 2) Lex it in https://github.com/wren-lang/wren/blob/4d1d0d972efffd072f6a7baa5c37f312258c3955/src/vm/wren_compiler.c#L985 3) Define a precedence for it in https://github.com/wren-lang/wren/blob/4d1d0d972efffd072f6a7baa5c37f312258c3955/src/vm/wren_compiler.c#L1581-L1602 4) Create an operator callback for it in https://github.com/wren-lang/wren/blob/4d1d0d972efffd072f6a7baa5c37f312258c3955/src/vm/wren_compiler.c#L2602-L2678 that emits bytecode for . To define new bytecode tweak https://github.com/wren-lang/wren/blob/main/src/vm/wren_opcodes.h and https://github.com/wren-lang/wren/blob/main/src/vm/wren_vm.c.

wren_core.wren

class Bool {
  |> (other) {
    if (other is Fn) {
      return other.call(this)
    }
    if (other is List) {
      return [this] + other
    }
    return this
  }
}
class Fiber { }
class Fn {
  |> (other) {
    if (other is Fn) {
      return other.call(this.call())
    }

    if (other is List) {
      return [this.call()] + other
    }

    return this
  }
}

class Null {
  |> (other) {
    if (other is Fn) {
      return other.call(this)
    }
    if (other is List) {
      return [this] + other
    }
    return this
  }
}
class Num {
  |> (other) {
    if (other is Fn) {
      return other.call(this)
    }
    if (other is List) {
      return [this] + other
    }
    return this
  }
}

class Sequence {
  ...
  |> (other) {
    if (other is Fn) {
      return other.call(this)
    }
    if (other is List) {
      return [this] + other
    }
    return this
  }
}

...

class List is Sequence {
  ...

  |> (other) {
    if (other is Fn) {
      var arity = this.count
      if (arity == 1) {
        return other.call(this)
      }

      if (arity == 2) {
        return other.call(this[0], this[1])
      }

      if (arity == 3) {
        return other.call(this[0], this[1], this[2])
      }

      // TODO: Implement more Params
      return this
    }

    if (other is List) {
      return this + other
    }

    return this
  }
}

wren_compiler.c

Token Type

  // line 137
  // Pipe operator |>
  TOKEN_PIPEOP

Grammar Rule

// line 2658
  /* TOKEN_PIPEOP        */ INFIX_OPERATOR(PREC_TERM, "|>"),

Lexer

// line 979
case '|':
        if (matchChar(parser, '>')) {
          makeToken(parser, TOKEN_PIPEOP);
          return;
        }
        twoCharToken(parser, '|', TOKEN_PIPEPIPE, TOKEN_PIPE);
        return;

ChayimFriedman2 commented 3 years ago

I'd define a lower precedence for this operator.

mhermier commented 3 years ago

I have mixed feelings about the addition of that feature. The only interesting feature I see is to be able to transform a list to function arguments, but we can do the same with something like a 'Fn::callAll(argumentsList)'. I like the fact that there are more operators it opens door for more expressiveness, but I have some reserve about the implementation...

clsource commented 3 years ago

The implementation was just a proof of concept. It needs more polish :) It was quickly done just to know how Wren can use this pattern.

mhermier commented 3 years ago

I think I found what annoyed me, the implementation tries to be too much smart at dealing with list.

As this, you have to pass through a function call if you want to repack them so the fallowing call take it as a single argument parameter. And this will easily boom if there are more arguments than what the stack allow.

I think it should be the other way around. List should never expanded unless passing through a special expansion transform or operator. Likewise "Hello" |> [1, 2] was very confusing, found it odd that it resulted in a sort of concatenation...

clsource commented 3 years ago

Yes it was odd. Just was a quick way of testing :)

(Elixir does not allow this either)

imagen

As you mention it, maybe Fn can be extended with a new function.

class Fn {
  callWith(args) {

    if (!(args is List)) {
      return this.call(args)
    }

    var actions = {
      1: Fn.new { this.call(args[0]) },
      2: Fn.new { this.call(args[0], args[1]) },
      3: Fn.new { this.call(args[0]), args[1], args[2]},
      4: Fn.new { this.call(args[0]), args[1], args[2], args[3]},
      5: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4]},
      6: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5]},
      7: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6]},
      8: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7]},
      9: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8]},
      10: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9]},
      11: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10]},
      12: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11]},
      13: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12]},
      14: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13]},
      15: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14]},
      16: Fn.new { this.call(args[0]), args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14], args[15]},
    }

    var action = actions[args.count]
    if (action) {
      return action.call()
    }

    return this.call(args)
  }
}

mhermier commented 3 years ago

That implementation is too much smart:

The function is intended to works with arrayed sequence, don't check for List, simply call directly and crash/error if there is nothing to call.
The function create 16 functions in memory to trash them instantly, either do them static passing action.call(this, args) or the dumb way aka if chain because we don't have switch/case

clsource commented 3 years ago

callWith(args) {

    var arity = args.count

    if(arity == 1) { return this.call(args[0]) }
    if(arity == 2) { return this.call(args[0], args[1]) }
    if(arity == 3) { return this.call(args[0], args[1], args[2])}
    if(arity == 4) { return this.call(args[0], args[1], args[2], args[3])}
    if(arity == 5) { return this.call(args[0], args[1], args[2], args[3], args[4])}
    if(arity == 6) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5])}
    if(arity == 7) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6])}
    if(arity == 8) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7])}
    if(arity == 9) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8])}
    if(arity == 10) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9])}
    if(arity == 11) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10])}
    if(arity == 12) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11])}
    if(arity == 13) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12])}
    if(arity == 14) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13])}
    if(arity == 15) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14])}
    if(arity == 16) { return this.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14], args[15])}

    return this.call(args)
  }

mhermier commented 3 years ago

Based on yours, I was able to make the following to work:

class Fn {
  callWith(args) {
    if (!__calls) {
      __calls = {
         0: Fn.new {|self, args| self.call() },
         1: Fn.new {|self, args| self.call(args[0]) },
         2: Fn.new {|self, args| self.call(args[0], args[1]) },
         3: Fn.new {|self, args| self.call(args[0], args[1], args[2]) },
         4: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3]) },
         5: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4]) },
         6: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5]) },
         7: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6]) },
         8: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7]) },
         9: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8]) },
        10: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9]) },
        11: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10]) },
        12: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11]) },
        13: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12]) },
        14: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13]) },
        15: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14]) },
        16: Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14], args[15]) },
      }
    }
    return __calls[args.count].call(this, args)
  }
}

I added 0 case since it is possible to also pass an empty sequence. I have to check that 16 argument that I found odd. And then we fill have to benchmark that somehow...

Edit: Thinking at it again, I can use a list for more faster access... Hunting some merge issues in my commits and will properly edit it to array.

mhermier commented 3 years ago

class Fn {
  callWith(args) {
    if (!__calls) {
      __calls = [
        /*  0 */ Fn.new {|self, args| self.call() },
        /*  1 */ Fn.new {|self, args| self.call(args[0]) },
        /*  2 */ Fn.new {|self, args| self.call(args[0], args[1]) },
        /*  3 */ Fn.new {|self, args| self.call(args[0], args[1], args[2]) },
        /*  4 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3]) },
        /*  5 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4]) },
        /*  6 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5]) },
        /*  7 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6]) },
        /*  8 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7]) },
        /*  9 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8]) },
        /* 10 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9]) },
        /* 11 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10]) },
        /* 12 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11]) },
        /* 13 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12]) },
        /* 14 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13]) },
        /* 15 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14]) },
        /* 16 */ Fn.new {|self, args| self.call(args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7], args[8], args[9], args[10], args[11], args[12], args[13], args[14], args[15]) },
      ]
    }
    return __calls[args.count].call(this, args)
  }
}

Done and checked, we need some kind of benchmark now...

ChayimFriedman2 commented 3 years ago

I prefer this as a C method that can just memcpy() the list into the stack:

DEF_PRIMITIVE(fn_callWith)
{
  if (!validateList(vm, args[1], "Arguments")) return false;
  int argsCount = AS_LIST(args[1])->elements.count;
  argsCount = argsCount <= MAX_PARAMETERS ? argsCount : MAX_PARAMETERS;

  int stackSize = (int)(vm->fiber->stackTop - vm->fiber->stack);
  wrenEnsureStack(vm, vm->fiber, stackSize + argsCount - 1); // `-1` because we pop the list

  memcpy(vm->fiber->stackTop - 1, AS_LIST(args[1])->elements.data, sizeof(Value) * argsCount);
  vm->fiber->stackTop += argsCount - 1;

  wrenCallFunction(vm, vm->fiber, AS_CLOSURE(args[0]), argsCount);
}

FUNCTION_CALL(vm->fnClass, "callWith", fn_callWith);

bool validateList(WrenVM* vm, Value arg, const char* argName)
{
  if (IS_LIST(arg)) return true;
  RETURN_ERROR_FMT("$ must be a list.", argName);
}

Also, I think callWith() is a bad name. I don't have a better one, though.

clsource commented 3 years ago

another name could be invoke or run or exec

ChayimFriedman2 commented 3 years ago

All of these names do not emphasis the distinction between regular calls, so I don't like them.

clsource commented 3 years ago

If we go to JS we can see apply and spread as possible candidates.

Note: While the syntax of this function is almost identical to that of call(), the fundamental difference is that call() accepts an argument list, while apply() accepts a single array of arguments.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply

clsource commented 3 years ago

I came to realize that some functions would be needed for the pipe to work flawlessly

// A print property in System that returns a function for printing a message

class System {
    static put { 
        Fn.new {|message| System.print(message)} }
    }

// A prepend/append function in Lists (for arguments)

class List {
   prepend {
     Fn.new {|value| [value] + this}
   }

   append {
     Fn.new {|value| this + [value]}
   }
}

// Test Case 1
// Simple Pipe from Primitive to Function
"Hello"  |> System.put

// Test Case 2
// Prepend a value to the List (would need a prepend function in list that accepts one argument
"Hello" |> [1, 2].prepend |> System.put

mhermier commented 3 years ago

The C implementation is not doable as this, the VM cannot be re-entered using wrenCallFunction.

About the naming, it is a complicated matter, I thought about callAll to follow the addAll, printAll, writeAll, ... naming convention that already exist. Using another name would fell surprising, I think.

mhermier commented 3 years ago

Ran some benchmark I made and it seems the linear table trick is not worth it:

fib_callAll - wren             .......... 1.86s 0.0642 122.62% relative to baseline
fib_callAll16 - wren           .......... 5.80s 0.0611  87.90% relative to baseline

This is a difference between calling fibonacci with array dispatch vs if dispatch, the first one has 2 parameters (required for fib) and the second one forced to 16 to see the extremes.

Considering most function should have a low number of parameters (< 4 I would say) and considering the 22% advantage with 2 parameters which is a good median for 0 to 4 parameters, the if choice seems the correct one.

clsource commented 3 years ago

Ok upon making some test I felt unconfortable if the list is automatically parsed as params when piping to a function.

So for solving that I implemented a new property in List to be able to call the pipe as callAll

// class List
   static args {
    if (!__args) {
      class ListArgs {
        static new {
          return Fn.new {|args| this.new(args)}
        }

        construct new (args) {
          _args = args
        }

        |> (other) { other.callAll(_args) }
      }

      __args = ListArgs
    }

    return __args
  }

  |> (other) { other.call(this) }

Also added some utility functions


prepend {
    return Fn.new {|other|
      if (other is List) {
        return other + this
      }
      return [other] + this
    }
  }

  append {
    return Fn.new {| other |
      if (other is List) {
        return this + other
      }
      return this + [other]
    }
  }

Final result


var myFunc = Fn.new {| message, second, third |
    System.print(message)
    System.print("Second: %(second), Third: %(third)")
    return true
}

var isTrue = Fn.new {|result|
    if (result) {
        return System.print("Is True")
    }
    return System.print("Is False")
}

"Hello" |> [1, 2].prepend |> List.args.new |> myFunc |> isTrue

/*
Hello
Second: 1, Third: 2
Is True
*/

clsource commented 3 years ago

After the experiments I had some insights:

1 Implementing Pipe is Doable and simplifies using Functions in Wren
2 It would require adding properties to core classes that returns functions (so pipe can use call) as an alternative to the normal methods
3 Adding such functions is not complex (just a wrapper) but it would need a whole new test suit to be sure they work as expected
4 Adding the ability to accept a list instead of arguments to a function is a nice addition.
5 Considering the amount of new code added to the Core. Its unlikely this feature would be integrated at all or at least in a reasonable amount of time. At least the list as arguments is feasible to be merged.

mhermier commented 3 years ago

Despite the low probability of the adoption of the original request, maybe you should submit the operator '|>' part. If it has the lowest priority possible (which should be just above ASSIGNMENT), it can be an handy operator to allow composition in user libraries, while not interfering with other operators. On my side, I'll probably submit the Fn::callAll(_) change, it is a nice tool to have on some occasions.

ChayimFriedman2 commented 3 years ago

The C implementation is not doable as this, the VM cannot be re-entered using wrenCallFunction.

It is, just like Fn.call(...) are. This is why they are defined using FUNCTION_CALL() and not PRIMITIVE().

If we go to JS we can see apply and spread as possible candidates.

I know js, and apply() seems a bad name to me too, just like it is in js 😉.

About the naming, it is a complicated matter, I thought about callAll to follow the addAll, printAll, writeAll, ... naming convention that already exist. Using another name would fell surprising, I think.

That's better.

I came to realize that some functions would be needed for the pipe to work flawlessly

No, this is just a hacky workaround. What we really need is being able to pipe methods + currying. And design the core library for that from the start. Or just give up, because Wren is not a functional language.

clsource commented 3 years ago

maybe you should submit the operator '|>' part. If it has the lowest priority possible (which should be just above ASSIGNMENT), it can be an handy operator to allow composition in user libraries, while not interfering with other operators.

I sent a PR but I realized that using the symbol |> for composition would confuse and make Wren look similar to a functional language, which is not.

I think I will close this issue and maybe in the future a generic operator for composition can be defined and is not confused with functional language like operations :)

wren-lang / wren

[RFC] Pipe Operator #941

wren_core.wren

wren_compiler.c

Token Type

Grammar Rule

Lexer