Review: Async.Promise #567

rhysd · 2017-12-24T16:25:18Z

This PR takes over #526 because its timeline is too long.

I rebased the branch into 7 commits. Please see #526 for the detail of design of this library.

As described in #526, this PR only implements APIs of ES6 promises. So discussion for adding other (maybe useful) APIs is out of scope. Let's start from small library. After the smallest set of Promise is added to master branch, we can try it in real Vim plugin. And then, let's discuss further rich APIs (like bluebird).

In this PR, I want to

brash up implementation/documentation
fix bugs
add missing test cases

Thank you for your review in advance.

As of #526, either English or Japanese is ok.

vim-jp-bot · 2017-12-24T16:32:17Z

autoload/vital/__vital__/Async/Promise.vim

+  elseif has_callback && success
+    call s:_resolve(a:promise, Result)
+  elseif !success
+    call s:_reject(a:promise, Err)


[vimlint] _{reported by reviewdog 🐶}
EVL104: variable may not be initialized on some execution path: l:Err

この警告は問題ないことを確認済みですが，出ないようにコード直したほうが良いでしょうか？

提案
lint なので強制力は無いですが、出ないほうが望ましいと思います。
特に Result と合わせる必要もないので Err の代わりに v:exception を直接使えばいいかなという気がします

ですね，そうしておきます．ありがとうございます．

v:exception は catch した節の中でしか使えないのを忘れていました…

mkj... 間違った情報すみません。

rhysd · 2017-12-25T02:20:48Z

#526 からレビューしてくれそうな気配がある人を Reviewers にアサインしてみました．年末で忙しいところすみませんが，レビューいただけると助かります．

lambdalisue · 2017-12-25T03:28:56Z

🎉 来週までには見ます！

lambdalisue

とりあえず Promise.vim だけ見ました。
いくつか「提案」と「確認」を書いたのでフィードバックお願いします。

lambdalisue · 2017-12-25T18:10:12Z

autoload/vital/__vital__/Async/Promise.vim

+
+" @vimlint(EVL103, 1, a:resolve)
+" @vimlint(EVL103, 1, a:reject)
+function! s:noop(resolve, reject) abort


メモ
すでに covimerage 側で修正されているかもしれませんが gina.vim に covimerage を導入した際に空っぽの関数があるとパースエラーが出ました。gina.vim では関数内部にコメントを一行書くことで対処しました。念のため

coverage ちゃんと取れてるっぽいです💪

lambdalisue · 2017-12-25T18:14:13Z

autoload/vital/__vital__/Async/Promise.vim

+  elseif has_callback && success
+    call s:_resolve(a:promise, Result)
+  elseif !success
+    call s:_reject(a:promise, Err)


提案
lint なので強制力は無いですが、出ないほうが望ましいと思います。
特に Result と合わせる必要もないので Err の代わりに v:exception を直接使えばいいかなという気がします

lambdalisue · 2017-12-25T18:48:52Z

autoload/vital/__vital__/Async/Promise.vim

+  " expression by **reference**.
+  call map(
+       \   copy(a:promises),
+       \   {i, p -> p.then({v -> wait_group.notify_done(i, v)}, a:reject)},


確認
map に lambda 式を使うと 1.5 倍ほど遅くなりますがよろしいでしょうか？

https://gist.github.com/lambdalisue/7cbaa0ece9496ec8052f5a86d33d8f17#file-test-vim

ただ最速の expr タイプはわかりにくくなるので all で想定する promises 数がそんなに多くないのであれば許容範囲だと思います。

知りませんでした… 文字列版も検討してみます．ここは all() の引数なので，そこまで要素数が多くなることを想定していませんが，それでも速いに越したことは無いですし．

lambdalisue · 2017-12-25T18:49:34Z

autoload/vital/__vital__/Async/Promise.vim

+function! s:_race(promises, resolve, reject) abort
+  for p in a:promises
+    call p.then(a:resolve, a:reject)
+  endfor


確認
ここも同様に map(..., 'expr') タイプの方が 1.5 倍ほど早いですがよろしいでしょうか？

https://gist.github.com/lambdalisue/7cbaa0ece9496ec8052f5a86d33d8f17#file-test-vim

ベンチマークを測った上で検討してみます．正直，.then のオーバーヘッドのほうが大きくてほぼ影響ないんじゃないかという気もしています．

上記のベンチマークは 100000 回のループなので無視しても良いと思っています。
そのため「確認」としました

lambdalisue · 2017-12-25T18:52:09Z

autoload/vital/__vital__/Async/Promise.vim

+  endif
+  return child
+endfunction
+let s:PROMISE.then = function('s:_promise_then')


質問
辞書に対して直接関数定義を行わないで、一度スクリプト関数として定義してから function で funcref 化して辞書に代入しているのには何か理由があるのでしょうか？

ここは以前ツイッターで @haya14busa さんに指摘されたのですが，辞書関数だとエラーが出た時のコールバックの関数名が数字になってしまってデバッグが困難になるためです．

なるほど納得。でもデバッグの事情の為にこういうハックするのは少しモニョりますね。今回のケースだと非同期なのでデバッグの簡潔さを重要視した方が良いですが‥‥

コードが若干複雑になってしまうのは確かにあると思います．

実は vital core でもやってます

vital.vim/autoload/vital/vital.vim

Line 76 in e553b10

let s:Vital.import = s:_function('s:import')

書くの面倒だという以外はいいことしかないと思ってる

lambdalisue · 2017-12-25T19:01:06Z

autoload/vital/__vital__/Async/Promise.vim

+" States of promise
+let s:PENDING = 0
+let s:FULFILLED = 1
+let s:REJECTED = 2


提案
ECMA script では Promise.race()で Promise の状態取得が出来ますが Vim script は関数呼び出しのオーバーヘッドが大きいので上記の定数を s:_vital_created(module) を使ってモジュール定数として定義し s:PROMISE._state を公開アトリビュートとする（必要であれば lockvar も？）のはどうでしょうか？

" 使用感イメージ let p1 = Promise.resolve() let p2 = Promise.reject('foobar') let p3 = Promise.new({ resolve -> execute('echo "Do nothing"') }) echo p1.state == Promise.FULFILLED echo p2.state == Promise.REJECTED echo p3.state == Promise.PENDING

すみません，#526 に1文だけ書いてこの PR には書いていなかったのですが，この PR は ES6 の Promise の API の実装にとどめる形にさせてください．理由は，

bluebird のような強力な API を実装することも考えられるため，今入れたい API を言い始めるとキリが無さそう

まずは最小の状態で master にマージして実戦投入して，real world で本当に必要な API だけを足したい

ためです．PR の本文にも追記しておきます💪

なのでここからは一応オフトピですが，せっかく提案いただいたので僕の考えを書いておきます．

一般的に，非同期処理で実行の状態を見て処理を分岐するのはバッドプラクティスだと思います．例えば JavaScript には VM 上で他の非同期に走っているコンテキストの状態を取る手段はありませんし，Go ではあえてゴルーチン（coroutine）の状態や ID，今自分がどのゴルーチン（コンテキスト）で実行されているかを取る API を提供していません．
また，飽くまで僕自身の経験としてですが，Promise の状態を取りたいと思ったことが無いというのもあります．

ただ，bluebird のように Promise の 3rd party 実装では Promise の状態を同期的に introspection できるものもあるので，real world で本当に必要なユースケースが発生した時に入れることを考えて議論するのが良いかなと思います．

ご説明ありがとうございます。納得しかない

lambdalisue · 2017-12-25T19:15:23Z

autoload/vital/__vital__/Async/Promise.vim

+  call timer_start(0, function('s:_publish', [a:promise]))
+endfunction
+
+function! s:_resolve_one(index, value) dict abort


提案
辞書関数ではなく wait_group インスタンスを第一引数に持つ通常関数にしませんか？以下理由

wait_group の定義位置と離れているので self が何を指しているのかパッと理解できない

Vim script だと . によるアトリビュート検索も結構遅かった気がするので partial で変数化して Notify(i, v) みたいに呼び出したほうが繰り返し時に早そう（パフォーマンス測定はしていません）

なるほど．特に 1. はその通りですし，ここで辞書関数にする必要は実際無さそうなので普通の関数にします．ありがとうございます．

tyru · 2017-12-26T13:57:30Z

autoload/vital/__vital__/Async/Promise.vim

+endfunction
+
+function! s:_race(promises, resolve, reject) abort
+  for p in a:promises


p は Funcref になり得るので l:P がいいと思います

あれ p は s:PROMISE オブジェクトのコピー（インスタンス）な気がします。

@lambdalisue さんのおっしゃる通り，p は dict なので小文字で大丈夫です．

あ、確かに。了解です。

tyru · 2017-12-26T13:58:25Z

autoload/vital/__vital__/Async/Promise.vim

+endfunction
+
+function! s:_resolve(promise, ...) abort
+  let Result = a:0 > 0 ? a:1 : v:null


同名のグローバル関数が定義されていた場合にエラーにならないように l:Result がいいと思います

tyru · 2017-12-26T13:59:30Z

autoload/vital/__vital__/Async/Promise.vim

+  let success = 1
+  if has_callback
+    try
+      let Result = a:callback(a:result)


同名のグローバル関数が定義されていた場合にエラーにならないように l:Result がいいと思います

同名のグローバル関数があってもローカル変数が優先されてエラーにならないようにみえます．7.4 だとそうなるのでしょうか？

https://wandbox.org/permlink/CGv0HxAPsALNt0yk

7.4.729 でも試してみましたが結果は上のものと変わりませんでした

tyru · 2017-12-26T13:59:42Z

autoload/vital/__vital__/Async/Promise.vim

+    try
+      let Result = a:callback(a:result)
+    catch
+      let Err = v:exception


同名のグローバル関数が定義されていた場合にエラーにならないように l:Err がいいと思います

（コメント書いた diff が outdated で閉じちゃったのでこちらにも）

同名のグローバル関数があってもローカル変数が優先されてエラーにならないようにみえます．7.4.729 までは同じ挙動のようなのですが，もっと古いものだとそうなったりするのでしょうか？

https://wandbox.org/permlink/CGv0HxAPsALNt0yk

これだとアウトです（参照ではなく let での定義時に l: が付いてないとエラー）
https://wandbox.org/permlink/vubuQFFNBkoSp47g

v:exeption は関数ではないので関係ないのでは?　Result は関数になりうりそうなので l: つけておいてもよさそう。

あと関連して Err が funcref にならないなら大文字にする必要はないとおもいます. 逆に funcref なら大文字かつ l: はつけるべき。

@tyru なるほど…ありがとうございます．そういうことでしたか．

@haya14busa 確かに例外として投げられるのは文字列なので Err のほうは err で大丈夫そうですね．誤解生みそうなので修正します

tyru · 2017-12-26T14:02:01Z

autoload/vital/__vital__/Async/Promise.vim

+  let parent = self
+  let state = parent._state
+  let child = s:new(s:NOOP)
+  let Res = get(a:000, 0, v:null)


同名のグローバル関数が定義されていた場合にエラーにならないように l:Res がいいと思います

tyru · 2017-12-26T14:21:33Z

doc/vital/Async/Promise.txt

+	  \  Promise.new({resolve -> timer_start(100, {-> resolve('second')})}),
+	  \])
+	  \.then({v -> execute('echo ' . v)})
+	  \.catch({e -> execute('echo ' . e)})


execute('echo e') か execute('echo ' . string(e)) ?

tyru · 2017-12-26T14:21:42Z

doc/vital/Async/Promise.txt

+	  \  Promise.new({resolve -> timer_start(50, {-> execute('throw "ERROR!"')})}),
+	  \  Promise.new({resolve -> timer_start(100, {-> resolve('second')})}),
+	  \])
+	  \.then({v -> execute('echo ' . v)})


execute('echo v') か execute('echo ' . string(v)) ?

tyru · 2017-12-26T14:22:00Z

doc/vital/Async/Promise.txt

+	  \  Promise.new({resolve -> timer_start(50, {-> resolve('first')})}),
+	  \  Promise.new({resolve -> timer_start(100, {-> resolve('second')})}),
+	  \])
+	  \.then({v -> execute('echo ' . v)})


execute('echo v') か execute('echo ' . string(v)) ?

tyru · 2017-12-26T14:22:46Z

doc/vital/Async/Promise.txt

+	  " Outputs '42'
+
+	  Promise.resolve(Promise.reject('ERROR!'))
+	  \.catch({reason -> execute('echo ' . reason)})


execute('echo reason') か execute('echo ' . string(reason)) ?

tyru · 2017-12-26T14:25:30Z

doc/vital/Async/Promise.txt

+  \]
+  \)
+    \.then({-> exeute('echom "All repositories were successfully cloned!"', '')})
+    \.catch({err -> execute('echom "Failed to clone: " . ' string(err), '')})


. が足らないです。
execute('echom "Failed to clone: " . ' . string(err), '')

これでもいいかも？
execute('echom "Failed to clone:" ' . string(err), '')

lambdalisue · 2017-12-26T15:00:47Z

僕はフィードバックの内容と今後の変更内容（予定）に特に意見はないので Approve にしておきます。

tyru · 2017-12-26T15:14:44Z

doc/vital/Async/Promise.txt

+                \     code ? reject(buf.err) : resolve(buf.out)
+                \   },
+                \ })})
+  endfunction


自分も詳しく job の仕様を把握してませんが、試した限りだと exit_cb が close_cb よりも後に実行されるとは限らないようです。
https://gist.github.com/tyru/0ffee5ba2b8cab7390f2652c9a97bc24
上記の結果 (messages) だと exit_cb のが先に実行されてしまっているため、 buf が空になってしまっています。
close_cb のみ指定して s:read_to_buf() に resolve してもらうとかですかね？ (それでいいという確証はないです…)

私の環境だとコールバックの発火順は exit_cb のほうが後なので，どうやらプラットフォーム依存ですね…ちなみに close_cb だけだと exit status が分からないのでコマンドが失敗したかどうか分からないです．

ちゃんとやるならどちらの順序で発火しても動くようにすべきですが，それをやると example としてイマイチになってしまう気がします（本質的でないコードが増えすぎる）．なにか良い修正案とかないでしょうか… @lambdalisue @thinca @mattn

プロセスの終了とチャネルのクローズはそもそも別の物なので例として不適かと思います。
プロセスが終了した後に全ての出力が欲しいのであれば、オプションに 'drop': 'never' を渡してチャネルから ch_read で持ってくるのが良いかと思います

あと、プロセスの終了はどうでもよくて出力が全部欲しい場合は Vim のヘルプにある通り close_cb でやるべきですかね。
http://vim-jp.org/vimdoc-ja/channel.html#read-in-close-cb

個人的には実用面で言えば「プロセスの出力結果がほしい」が要求であって「プロセスが終了してから出力結果がほしい」ではないと思います。なので例としては、こちらのほうが適切かと思います。

すでに喋るべき内容は語られていた。

@lambdalisue @tyru

お手数ですが，これで動いているか確認いただけませんでしょうか？
自分の環境では元のコードも動いてしまっていたので，これで問題が解決したのかどうかが確認できず…

let s:Promise = vital#vital#import('Async.Promise') function! s:read(chan, part) abort let out = '' while ch_status(a:chan, {'part' : a:part}) ==# 'buffered' let out .= ch_read(a:chan, {'part' : a:part}) endwhile return out endfunction function! s:sh(...) abort let cmd = join(a:000, ' ') let buf = {} return s:Promise.new({resolve, reject -> job_start(cmd, { \ 'drop' : 'never', \ 'close_cb' : {ch -> 'do nothing' }, \ 'exit_cb' : {ch, code -> \ code ? \ reject(s:read(ch, 'err')) : \ resolve(s:read(ch, 'out')) \ }, \ })}) endfunction call s:sh('ls', '-l') \.then({out -> execute('echom ' . string('Output: ' . out), '')}) \.catch({err -> execute('echom ' . string('Error: ' . err), '')})

ちなみに，close_cb は指定しないと job の開始に失敗してしまいました

@rhysd 動きました。ただいくつか修正した方が良さそうな点がありました。

open の場合も考慮する

ch_status(a:chan, {'part' : a:part}) =~# 'open\|buffered'

末尾に改行入れる

let out .= ch_read(a:chan, {'part' : a:part}) . "\n"

:echom だと改行を含む文字列の場合つらい感じになるので :echo のがいいかもです (最後の .then() と .catch() の中)

ちなみに，close_cb は指定しないと job の開始に失敗してしまいました

これ自分の環境 (WSL) でも再現しました。詳しく追ってないですが Error: みたいに err が空文字で渡ってきちゃいますね。

Twitter でも言いましたが、このコードだとエラーにならなかったです。
もしかして job 以外の所でコケて .catch() に来てるかも？
https://gist.github.com/tyru/4707726873f3e4955f331109f9dc0734

私の手元で確認した限りでは channel status が fail になっていたので job の開始に失敗しているんじゃないかと思います．他の箇所で例外が起きているなら err が空になるのは（Promise の実装がバグってない限り）おかしいですね

動きました。修正点としては @tyru さんと同意見で、以下がいいかなと思います。

let s:Promise = vital#vital#import('Async.Promise') function! s:read(ch, part) abort let out = [] while ch_status(a:ch, {'part' : a:part}) =~# '^\%(open\|buffered\)$' call add(out, ch_read(a:ch, {'part': a:part})) endwhile return join(out, "\n") endfunction function! s:sh(...) abort let cmd = join(a:000, ' ') return s:Promise.new({resolve, reject -> job_start(cmd, { \ 'drop' : 'never', \ 'close_cb' : {ch -> 'do nothing' }, \ 'exit_cb' : {ch, code -> \ code ? \ reject(s:read(ch, 'err')) : \ resolve(s:read(ch, 'out')) \ }, \ })}) endfunction call s:sh('ls', '-l') \.then({out -> execute('echo ' . string('Output: ' . out), '')}) \.catch({err -> execute('echo ' . string('Error: ' . err), '')})

あと s:sh('cat', '/foo/bar') でちゃんと Error: cat: /foo/bar: No such file or directory が出ました。

環境: macOS High Sierra

rhysd · 2017-12-26T15:39:45Z

race() と all() の内部実装を map()（述語部を eval 文字列にする）にして，下記のようなスクリプトで1000回実行時間の平均を取ってみました．

let P = vital#vital#import('Async.Promise')

let ps = map(range(1000), {i -> g:P.new({res -> timer_start(i, res)})})

" race()
let amount = 0.0
let total = 10
let i = 0
while i < 10
    let stamp = reltime()
    call P.race(ps)
    let amount += str2float(reltimestr(reltime(stamp)))
    let i += 1
endwhile

echom 'amount: ' . string(amount)
echom 'avg: ' . string(amount / 10.0)

" all()
let amount = 0.0
let total = 10
let i = 0
while i < 10
    let stamp = reltime()
    call P.all(ps)
    let amount += str2float(reltimestr(reltime(stamp)))
    let i += 1
endwhile

echom 'amount: ' . string(amount)
echom 'avg: ' . string(amount / 10.0)

`race()`

従来実装（for ループ）

amount: 0.487715
avg: 0.048772

新実装（map(copy(...), '...')）

amount: 0.469726
avg: 0.046973

Result: 1.04x faster

`all()`

従来実装（map とラムダ式）

amount: 0.543475
avg: 0.054348

新実装（map と eval 文字列述語）

amount: 0.503912
avg: 0.050391

Result: 1.08x faster

1000要素でこれなので，正直誤差範囲かなと思いますがどうでしょう

lambdalisue · 2017-12-26T15:52:33Z

1000要素でこれなので，正直誤差範囲かなと思いますがどうでしょう

僕のベンチでも 100000 回とかでやっと差が出てる感じなので把握した上で lambda はありだと思います 👍

rhysd · 2017-12-26T16:02:05Z

@lambdalisue 了解です．もし今後実際に使ってみてそこで問題があることが分かったらその時に対処しましょう:+1:

rhysd · 2017-12-26T16:13:10Z

execute() の第2引数に時間を吸われたので @tyru さんにいただいている残りの指摘箇所は明日修正します．

haya14busa · 2017-12-26T16:46:05Z

autoload/vital/__vital__/Async/Promise.vim

+  endif
+  return child
+endfunction
+let s:PROMISE.then = function('s:_promise_then')


実は vital core でもやってます

vital.vim/autoload/vital/vital.vim

Line 76 in e553b10

let s:Vital.import = s:_function('s:import')

書くの面倒だという以外はいいことしかないと思ってる

haya14busa · 2017-12-27T07:41:16Z

autoload/vital/__vital__/Async/Promise.vim

+    try
+      let Result = a:callback(a:result)
+    catch
+      let Err = v:exception


v:exeption は関数ではないので関係ないのでは?　Result は関数になりうりそうなので l: つけておいてもよさそう。

あと関連して Err が funcref にならないなら大文字にする必要はないとおもいます. 逆に funcref なら大文字かつ l: はつけるべき。

haya14busa · 2017-12-27T07:42:47Z

autoload/vital/__vital__/Async/Promise.vim

+    try
+      let Result = a:callback(a:result)
+    catch
+      let Err = v:exception


これだとスタックトレースの情報 (v:throwpoint) が消えてしまうのでエラーオブジェクト作って v:exeption と v:throwpoint 両方持たせておいたほうがよさそう?

[追記補足] これ"よさそう?"って微妙なカンジで最初は書いてましたが、help の github_issues の例とか実は僕も動かなくてデバックしてて、スタックトレースだしてもムズい(lambdaをおえない...)のにエラーだけだとほんとによくわからないエラーしかでないことがあるので、真剣に検討してもいいかなぁと思います。

うーん，それはエラーオブジェクトを定義する必要が出てしまいますね…例外が起きたら v:exception だけでなくて，v:throwpoint も結合させた文字列で reject するとかどうでしょう？

僕は @haya14busa さんのいうエラーオブジェクトのほうが良いかと思います。
分離されたデータをライブラリに勝手に結合されてイラッとした経験結構多いので……（例: stdout と stderr を勝手に結合するやつとか）

{ 'message': v:exception [, 'throwpoint': v:throwpoint]} くらいでどうでしょうか. 文字列結合はできるなら避けたい

haya14busa · 2017-12-27T07:54:56Z

autoload/vital/__vital__/Async/Promise.vim

+endfunction
+
+function! s:is_available() abort
+  return has('nvim') || v:version >= 800


nvim よく追えてないですが lambda は割りと最近入ってるはずなのでチェックとしてどれくらい機能してるか微妙かも?
has('lambda') とかのほうがいいかなと思います. あとは v:t_ や timer_startも使ってはいるので 8 以上っていうのは残してもよいかな?

うーん，なるほど．確かにそのとおりですね．僕も具体的にどのバージョンかまでは把握していないです… 機能ベースで言うと，has('lambda') && has('timers') のほうが良さそうですね

+1 to has('lambda') && has('timers') .

一般的にも機能ベースでチェックして、それでもできないならバージョンでチェックするというのがいいと思います

haya14busa · 2017-12-27T08:34:18Z

doc/vital/Async/Promise.txt

+	be |Funcref| and they are guaranteed to be called __asynchronously__.
+>
+	  echo 'hi'
+	  Promise.new({resolve -> execute('echo "halo"') || resolve(42)})


上下に echoあって実行できそうにみえるし call Promise or call s:Promise にしておいてもよさそう。
この help 全体的に call なしで Promise.new.... としているけどわざとですかね?
せっかくなら call つけてほぼそのまま実行できるくらいにしておいてもよいかなと思いますがどっちでもよさそう

あと execute('...', '') にしないとこの結果を確認できないので直しておいてあげると読者に優しそうですかね。

はい，第2引数は各所で忘れていたので修正します…ありがとうございます． :call も追加します

[補足] 面倒そうなので一度 s:echom() とか定義してもいいかなともちょっと感じましたがおまかせします

haya14busa · 2017-12-27T08:49:05Z

doc/vital/Async/Promise.txt

+  \  s:sh('git', 'clone', 'https://github.com/rhysd/clever-f.vim.git'),
+  \]
+  \)
+    \.then({-> exeute('echom "All repositories were successfully cloned!"', '')})


[nit] s/exeute/execute/

nice catch です．misspell さん頼む…

haya14busa · 2017-12-27T09:06:17Z

doc/vital/Async/Promise.txt

+  let s:Promise = vital#vital#import('Async.Promise')
+
+  function! s:wait(ms)
+    return s:Promise.new({resolve -> timer_start(a:ms)})


return s:Promise.new({resolve -> timer_start(a:ms, resolve)}) ですかね。 resolve できてない

その通りです．ありがとうございます．

haya14busa · 2017-12-27T09:35:01Z

autoload/vital/__vital__/Async/Promise.vim

+function! s:_notify_done(wg, index, value) abort
+  let a:wg.done[a:index] = a:value
+  let a:wg.resolved += 1
+  if a:wg.resolved == a:wg.total


[nit] total じゃなくて remaining にしてデクリメントしていって0ならresolveってすると一つ変数を減らせる説 (total, resolved -> remaining)

(https://github.com/stefanpenner/es6-promise/blob/97478eb6fcd628e0d679438c88bd64b5079a9122/lib/es6-promise/enumerator.js#L95 らへんとかをみた)

あー，確かに．キーの数は減らしたいのでそうしてみます．

haya14busa · 2017-12-27T09:41:05Z

autoload/vital/__vital__/Async/Promise.vim

+  endif
+
+  let wait_group = {
+      \   'done': repeat([v:null], total),


[nit] results とかのほうがわかりやすい気がしますがどうでしょう? done って done フラグ感がある(Go脳かもしれない) これもes6-promise では results だった

haya14busa · 2017-12-27T09:46:58Z

autoload/vital/__vital__/Async/Promise.vim

+
+function! s:resolve(...) abort
+  let promise = s:new(s:NOOP)
+  call s:_resolve(promise, a:0 > 0 ? a:1 : v:null)


[nit] get(a:000, 0, v:null) とか他のところで使ってるし書き方統一してもよさそう。

(個人的には get(a:, 1, v:null) が好みだけど統一されてたらなんでもok)

個人的な好みにより a:0 になりました

[感想] 自分がいちばん好みじゃないの来た...! (勿論直してほしいとかではなくて単なる感想で、言い分としてはa:0は考えることが増えるのと(a:0が数でぇ...+1を見に行く...)、ほかの get or default イディオムと合わせたいので自分は get(a:, n, default) を使ってる (g: とかと一緒のイディオムにする)

haya14busa · 2017-12-27T09:53:05Z

autoload/vital/__vital__/Async/Promise.vim

+  let a:parent._fulfillments += [ a:on_fulfilled ]
+  let a:parent._rejections += [ a:on_rejected ]
+  if is_empty && a:parent._state > s:PENDING
+    call timer_start(0, function('s:_publish', [a:parent]))


[optional] call timer_start(0, ...) っていうイディオム関数化しておいてもよさそう? asap とか next_tick とか?
読者が暗黙に timer_start(0,...) がどういう意味かわかってるという仮定をおいてるけど関数化されててコメントもあったりすると可読性よさそう

とても気持ちはわかるのですが

ユーザー定義関数のオーバーヘッドが大きい

無駄にコールスタックを積んでしまう

の二点から今のままが良いと思います。

#567 (comment)

と同じで Vim script の事情から本来不要なハックや最適化が必要になるの少しモニョりますが、仕方ない……

その最適化が本当に必要な箇所かどうかが結構あやしいとは思いますが(2はなんかchainしすぎるとダメとか見た気がするのでわからんでもない), まぁoptionalですね。

はい。ここがユーザーが触る場所ならば僕も関数でラップする事には同意なのですが、ライブラリ内部のコードで（個人の主観としては）ほぼイディオム的な使い方なのでラップする必要が薄いと思います。

提案ありがとうございます．個人的な感覚では timer_start(0, ...) のままで良いかなと思います．0秒のタイマーでも意味は通じると思いますし，ラップして関数1つ増やす保守コストに見合うほどではないなと思いました．

JavaScript では確かに asap とか next_tick みたいにラップすることが多いのですが，それは環境によって下が setImmediate() だったり setTimeout() だったりするからなのですよね（使えるなら setImmediate のほうが良いため）

haya14busa · 2017-12-27T10:05:44Z

autoload/vital/__vital__/Async/Promise.vim

+    elseif settled == s:REJECTED
+      let l:CB = a:promise._rejections[i]
+    else
+      throw 'vital: Async.Promise: Cannot publish a pending promise'


このチェックはfor loop の外で settled 変数に入れたあと or 前くらいがよさそう

publish の呼ばれ方的にこのパスが呼ばれることがない（防衛的に入れてた）ので気づきませんでしたが，確かにそうですね．ループ前にチェックするようにします．

haya14busa · 2017-12-27T11:14:32Z

test/Async/Promise.vimspec

+
+    It should create with default value(=v:null) when no argument is given to resolve()/reject()
+      let l = l:
+      call P.new({resolve -> resolve()}).then({x -> extend(l, {'done' : x})})


[nit][optional] 実装のほうにも似たコメント書きましたが done より result 感がある?

確かに．フラグをセットするんじゃなくて値をセットしてるとこは result にしといたほうが値の意図が明確で良さそうです

haya14busa · 2017-12-27T11:19:47Z

test/Async/Promise.vimspec

+      Assert Equals(p._state, PENDING)
+      let p = p.then({-> extend(l, {'done' : 1})})
+      Assert Equals(p._state, PENDING)
+      Assert Equals(done, 0)


あってるので別によさそうだけど, call s:wait_has_key(l, 'done') してあげないとフェアじゃないかも?
...と思ったけど無駄にunit test が長くなるのもアレだしムズカシイ

(見直すとそもそも wait_has_key は done すでに定義してるので使えなかった.... ので sleep 1ms とかするとフェア感?(本当か?)

ここは待たなくても timer_start でキューイングされた処理は後のどこかで無害な感じで実行されて終わるだけなので，待たなくて良いかなと

あーすみません，このコメントの意図を読み違えてました．.then がタイマーを挟むようになった今，確かにある程度待たないと .then が発火していないことを確認するのにはこれでは不十分でした．ちなみに .then は永遠に発火しないので，done が定義済みかによらず wait_has_key は使えないです．

haya14busa · 2017-12-27T11:22:55Z

test/Async/Promise.vimspec

+        let p = P.new({resolve -> resolve(Val)})
+        Assert Equals(p._state, FULFILLED)
+        let p2 = p.then({x -> x}).then({r -> extend(l, {'Done' : r})})
+        call s:wait_has_key(l, 'Done')


wait_has_key の前でまだ Done には値入ってないことを確認すると "asynchronously" なことがわかりやすくてよさそう

haya14busa · 2017-12-27T11:29:15Z

test/Async/Promise.vimspec

+      for l:Val in [42, 'foo', {'foo': 42}, {}, [1, 2, 3], [], function('empty')]
+        let p = P.new({_, reject -> reject(Val)})
+        let p2 = p.then({-> extend(l, {'Done' : 'Error: resolved to .then'})}).catch({v -> extend(l, {'Done' : v})})
+        call s:wait_has_key(l, 'Done')


ここも wait_has_key のまえにチェック入れたほうが asynchronouslyのテストであることがわかりやすくてよさそう?

haya14busa · 2017-12-27T11:31:29Z

test/Async/Promise.vimspec

+      Assert Equals(done, 42)
+    End
+
+    It can take rejection handler at 2nd parameter


このテスト catch 使ってないし Describe .then のとこにあったほうがよさそう

これは .then のとこに置くべきなのを間違えてますね…ありがとうございます！

lambdalisue · 2017-12-27T16:54:21Z

今気が付きましたが 8f6b866 は Neovim を捨てることになります。

Neovim v0.2.3-240-g6ff13d (ほぼ現在の HEAD) で v:t_none はサポートされていません。いかがサポートされている値です。

v:t_bool
v:t_dict
v:t_float
v:t_list
v:t_number
v:t_string

この辺のサポートは結構あとだった気がするので type() を使っていただけないと Neovim では使えなくなってしまいます...

rhysd · 2017-12-27T17:19:16Z

@lambdalisue うお，マジですか．8f6b866 に強い動機があるわけではないため，削除しようと思います．ご指摘ありがとうございます．

rhysd · 2017-12-27T17:26:15Z

8f6b866 を削除するために push -f しました．

たくさんのレビューありがとうございます．2時を回ったので，今日はこのあたり（ #567 (comment) ）にしてまた明日の夜に修正予定です．

目下のところ，議論すべきは .catch で v:throwpoint が取れない点ですね．

vim-jp-bot · 2017-12-28T15:59:06Z

autoload/vital/__vital__/Async/Promise.vim

+endfunction
+
+function! s:_subscribe(parent, child, on_fulfilled, on_rejected) abort
+  let is_empty = empty(a:parent._children)


[vimlint] _{reported by reviewdog 🐶}
EVL102: unused variable l:is_empty

rhysd · 2017-12-28T16:03:14Z

Discussion point: Error Handling

Problem:

call s:P.new({-> execute('throw "oops"')})
  \.catch({err -> execute('echo err', '')})

We can refer v:exception which was thrown in preceding executions, but v:throwpoint was lost. I want to discuss how to solve this problem.

1. Concat `v:exception` and `v:throwpoint`

Use v:exception . "\n\n" . v:throwpoint instead of v:exception

Output:

oops

function <SNR>140_new[5]..<lambda>1, line 1

Pros: We don't need to define our own exception object
Cons: Parsing is required to retrieve error message

2. Use exception `{'exception' : v:exception, 'throwpoint' : v:throwpoint}` object only when exception is thrown

Wrapping v:exception and v:throwpoint with dict to preserve all information

Output:

{'throwpoint': 'function <SNR>183_new[5]..<lambda>3, line 1', 'exception': 'oops'}

Pros: Easy to access to retrieve error message (and throwpoint)
Cons: We need to define our own dict. Learning cost may be a bit up

3. Always use reason `{'message' : xxx, 'stack' : ...}` object for rejected Promises' values

When any value is rejected, wrap the value with dict (type is {message: any, stack?: string}). When it is an exception thrown in preceding execution, add v:throwpoint as stack field.

call s:P.new({-> execute('throw "oops"')})
  \.catch({err -> execute('echo err', '')})

call s:P.new({_, reject -> reject('hello')})
  \.catch({err -> execute('echo err', '')})

Output:

{'stack': 'function <SNR>183_new[5]..<lambda>3, line 1', 'message': 'oops'}
{'message': 'hello'}

Pros: We can handle rejected Promises' values in persistent way
Cons: We always need to dereference the value by accessing to `.message` (it may not be string) field

僕的には reject に手軽に好きな値が使えて例外時にも情報量が減らない 2. が良いかなと思うのですが，どうでしょう？

tyru

LGTM 💪

thinca

テストまで見ました。

thinca · 2017-12-30T15:40:32Z

doc/vital/Async/Promise.txt

+	be |Funcref| and they are guaranteed to be called __asynchronously__.
+>
+	  echo 'hi'
+	  call Promise.new({resolve -> execute('echo "halo"', '') || resolve(42)})


この書き方たと、例えば echo "halo" の部分が echon "1halo" だと || の左側が TRUE になるので resolve(42) が実行されません。つまりたまたまうまく動くだけのように見えます…。
そうと知らずにこの例をコピーして使う人が出てくることを危惧しています。

thinca · 2017-12-30T15:59:58Z

doc/vital/Async/Promise.txt

+- Operation has completed successfully
+- Operation has failed with an error
+
+{promise}.then([{onResolved} [, {onRejected}]])


なるほど。この help 内でも Fulfilled Promise のようなワードがあったので、resolve の方が表記が揺れなくてわかりやすそうという意図なら、これらも Resolved Promise にするのがいいかも?
とは言え私もそこまで強い意志はないです。意図してないうっかりだったらアレなので念のため確認くらいの気持ちでした。

thinca · 2017-12-31T12:06:02Z

test/Async/Promise.vimspec

+function! s:wait_has_key(obj, name) abort
+  " if has_key(a:obj, a:name)
+  "   throw printf('s:wait_has_key(): Key "%s" is already in object %s', a:name, a:obj)
+  " endif


このコメントは必要なやつでしょうか?

必要ないやつでした…

thinca · 2017-12-31T12:53:07Z

test/Async/Promise.vimspec

+      let l = l:
+      call P.new({resolve -> resolve()}).then({x -> extend(l, {'result' : x})})
+      call s:wait_has_key(l, 'result')
+      " :Assert Equal() does not support v:null by themis.vim


一応 master に入ってる最新版 themis.vim ならいけるはずです(が、別に無理にしなくて OK)。

おっと，そうですね．rebase して最新の themis を使うようにします．

thinca · 2017-12-31T12:53:55Z

test/Async/Promise.vimspec

+    End
+
+    It should make settled Promise with v:null when no argument is given to resolve()/reject()
+      let l = l:


私が言い出しておいてアレですが、この使い方なら無理に l: 使わなくても新規の辞書 {} でも良さそう…。(その場合は Assert の部分が l.result になる)

分かる… うーん，ただ l. 付けなくて良い利点はあるんですよね．特に理由が無ければこのままでも良いかなぁと

ハック感が抜ける(…までは行かなくても薄れる)メリットはありそうですが、全部直すの面倒なのも同意なのでこのままでも OK です 🙆

thinca · 2017-12-31T12:58:55Z

test/Async/Promise.vimspec

+    End
+  End
+
+  Describe .then()


.then() と .catch() は Promise オブジェクトのメソッド(モジュール直下の関数ではない)ので、別途 Describe Promise object 的なブロックに入れるのが良さそう。

thinca · 2017-12-31T14:06:18Z

test/Async/Promise.vimspec

+    End
+  End
+
+  Describe .reject()


.reject() に resolved Promise を渡した場合のテストがほしいなー。

thinca

間に合わなかった！
些末なコメントをしましたが特筆すべきことは特にありませんでした。
改めて実装ありがとうございます！素晴らしいです 👏

thinca · 2017-12-31T14:37:49Z

autoload/vital/__vital__/Async/Promise.vim

+let s:REJECTED = 2
+
+let s:DICT_T = type({})
+let s:NULL_T = type(v:null)


これわざわざ type() 使ってますが、仕様上 v:null を使うことになっているので foo is v:null なチェックでも良さそうな気がします。そうすれば左辺値に type() を適用する必要がなくなるのでほんの少し速くなるかも。

確かにわざわざ型の ID を見ているのは冗長ですね．ありがとうございます，直します．

thinca · 2017-12-31T15:02:00Z

autoload/vital/__vital__/Async/Promise.vim

+    call s:_fulfill(a:promise, Result)
+  elseif a:settled == s:REJECTED
+    call s:_reject(a:promise, Result)
+  endif


💭 [IMO]
ちょっと読んでて混乱しました。
最初の pending の判定を除くと、上2つが has_callback が TRUE の場合、下 2 つが has_callback が FALSE の場合なので、L43 の if の中にそれぞれ書いてしまっても良いかも。
その場合 pending の判定が重複しちゃうのでどちらが良いかむずいですが… 中に入れてしまえば L54 の不要な代入も自然に減らせそうな気がします。

確かにここはちょっと冗長かつ微妙な実装になっているのですが，意図としてはコールバックの発火処理と次の（子供の）Promise へチェーンする処理を分離する実装になっています．

a:promise._state != s:PENDING をダブらせて上の if の中で判定するというのも考えたのですが，コールバック実行部分とチェーンの続きを発火する部分が混ざると，それはそれで結構読みづらい印象でした．

haya14busa

こちらからは激LGTM 👍

Discussion for ES6 Promise: promises-aplus/promises-spec#108 Standard ES6 Promise does not check self fulfillment. And in Vim script, there is no way to refer itself in thenable object because Promise object is copy of s:PROMISE.

… called

because it is usually shorter and faster. (As long as I measured, partial is 10% faster than lambda when some variable is captured.

Although previous code has no problem, but linter complains about it.

@lambdalisue

Thank you @lambdalisue and @tyru!

@haya14busa

Thank you @haya14busa and @tyru!

@tyru

Thank you @tyru and @haya14busa!

…ll()

@haya14busa

nit festival! Thank you @haya14busa

@thinca

thank you @thinca

rhysd · 2017-12-31T17:57:47Z

themis.vim のアップデートの取り込みと fixup になっている箇所の適用のために push -f しました．

@thinca 明らかに直したほうが良さそうなところを修正しました．再度修正箇所とレビューコメントの確認をお願いできますでしょうか

thinca

thinca · 2018-01-01T04:25:42Z

test/Async/Promise.vimspec

+    End
+
+    It should make settled Promise with v:null when no argument is given to resolve()/reject()
+      let l = l:


ハック感が抜ける(…までは行かなくても薄れる)メリットはありそうですが、全部直すの面倒なのも同意なのでこのままでも OK です 🙆

rhysd · 2018-01-01T10:26:57Z

では，これでマージしようと思います．細かいところまでチェックいただいた皆さんありがとうございました🙏

rhysd added the Review me label Dec 24, 2017

rhysd mentioned this pull request Dec 24, 2017

[WIP] Promise #526

Closed

rhysd changed the title ~~Promise review~~ Review: Async.Promise Dec 24, 2017

vim-jp-bot reviewed Dec 24, 2017

View reviewed changes

rhysd requested review from thinca, tyru, lambdalisue, haya14busa and mattn December 25, 2017 02:19

lambdalisue requested changes Dec 25, 2017

View reviewed changes

tyru requested changes Dec 26, 2017

View reviewed changes

lambdalisue approved these changes Dec 26, 2017

View reviewed changes

tyru requested changes Dec 26, 2017

View reviewed changes

haya14busa reviewed Dec 27, 2017

View reviewed changes

haya14busa requested changes Dec 27, 2017

View reviewed changes

rhysd force-pushed the promise-review branch from 45cea5d to 337e8c4 Compare December 27, 2017 17:23

vim-jp-bot reviewed Dec 28, 2017

View reviewed changes

tyru approved these changes Dec 31, 2017

View reviewed changes

thinca reviewed Dec 31, 2017

View reviewed changes

haya14busa approved these changes Dec 31, 2017

View reviewed changes

rhysd added 20 commits January 1, 2018 00:48

Async.Promise: First implementation based on es6-promise

dd78349

Async.Promise: Add tests for all APIs

088e312

Async.Promise: Do not check self fulfillment

ad71bbe

Discussion for ES6 Promise: promises-aplus/promises-spec#108 Standard ES6 Promise does not check self fulfillment. And in Vim script, there is no way to refer itself in thenable object because Promise object is copy of s:PROMISE.

Async.Promise: Ensure fulfilled/rejected callbacks are asynchronously…

78a7476

… called

Async.Promise: Fix tests for asynchronous fulfillment/rejection

ba7d405

Async.Promise: Prefer partial to lambda

1e82791

because it is usually shorter and faster. (As long as I measured, partial is 10% faster than lambda when some variable is captured.

Async.Promise: Add a help document

b0e9d8f

Async.Promise: Fix maybe-uninitialized variable

fea1b27

Although previous code has no problem, but linter complains about it.

Async.Promise: Prefer function to method

264c07f

Async.Promise: Fix job example in document

f392520

Thank you @lambdalisue and @tyru!

Async.Promise: Explicit l: at :let to avoid functioin name conflicts

58041c4

Thank you @haya14busa and @tyru!

Async.Promise: Give tags to examples and fix example codes

e956ca0

Thank you @tyru and @haya14busa!

Async.Promise: Refactor state check in _publish() and wait_group in a…

057d36e

…ll()

Async.Promise: Check features rather than versions for availability

cf1bd23

Async.Promise: Fix test descriptions and variable names, more checks

427a0cd

nit festival! Thank you @haya14busa

Async.Promise: Promise value is never settled in _subscribe()

82b5c31

Async.Promise: Reject with dict when catching an exception

e9770e2

Async.Promise: Fix/brush up the help document

1095e9b

thank you @thinca

Async.Promise: Use themis.vim v1.5.4

b8770b7

Async.Promise: Prefer is v:null to == type(v:null)

fbed16a

rhysd force-pushed the promise-review branch from 23e695d to fbed16a Compare December 31, 2017 17:56

thinca approved these changes Jan 1, 2018

View reviewed changes

rhysd merged commit bee84ae into vim-jp:master Jan 1, 2018

rhysd deleted the promise-review branch January 1, 2018 10:27

Review: Async.Promise #567

Review: Async.Promise #567

Conversation

rhysd commented Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysd commented Dec 25, 2017

lambdalisue commented Dec 25, 2017

lambdalisue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented Dec 26, 2017

Choose a reason for hiding this comment

rhysd Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

lambdalisue Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

lambdalisue Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysd Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

tyru Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

rhysd commented Dec 26, 2017 • edited Loading

race()

all()

lambdalisue commented Dec 26, 2017

rhysd commented Dec 26, 2017

rhysd commented Dec 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysd commented Dec 24, 2017 •

edited

Loading

rhysd Dec 26, 2017 •

edited

Loading

lambdalisue Dec 26, 2017 •

edited

Loading

lambdalisue Dec 26, 2017 •

edited

Loading

rhysd Dec 27, 2017 •

edited

Loading

tyru Dec 27, 2017 •

edited

Loading

lambdalisue Dec 27, 2017 •

edited

Loading

rhysd commented Dec 26, 2017 •

edited

Loading

`race()`

`all()`

rhysd Dec 27, 2017 •

edited

Loading