2016年6月24日金曜日

DispatchProxyのPRを送ってみた

前回の続きです。

というわけで、.NET Core の、System.Reflection.DispatchProxy が返すオブジェクトからなぜかプロパティとイベントが取れない問題があって、さすがにこれはどうかなと思ったので、PRを送ってみたので、その顛末でも書きます。ほとんどポエムです。

問題の解説

そもそも、

IFoo foo = DispatchProxy.Create<IFoo, SomeProxy>();
foo.Bar = "ABC";

みたいなコードがコンパイルされ、動作するのに、

foo.GetType().GetRuntimeProperty("Bar");

nullを返すのはなぜでしょうか?
それは、fooオブジェクトの実態である動的に生成された型が、プロパティ(やイベント)のアクセッサーメソッドのみを実装してプロパティやイベントを宣言していないからです。
前回の記事をよく見ると、

動的に生成したクラスに、インターフェイスが宣言しているすべてのメソッドを宣言します。

と書いてありますね。 そう、DispatchProxy.Create<T, TProxy>()は動的に生成した型にアクセッサーメソッドを実装するだけで、それらが属するプロパティやイベントを型で宣言させないのです(もう少し内部的な話をすると、動的に生成されたプロキシ型のメタデータテーブルにプロパティとイベントのエントリを書き出さないということです)。
しかもなんと、インターフェイスを実装した型では、そのインターフェイスのプロパティやイベントのアクセッサーメソッドさえ宣言されていれば、プロパティやイベントは宣言しなくても動作するのです(これが動作するのが正しいかどうかという話はありそうですが)。
C# が出力するのはアクセッサーメソッド呼び出しのILですからちゃんとコンパイルでき、かつ動作するのですが、プロパティやイベントが宣言されていないので、リフレクションでプロパティやイベントそのもののメタデータは取れないというわけです。

ポエム

これだとちょっと困るので、DispatchProxyGenerator の中で、プロキシ型にインターフェイスのプロパティとイベントを宣言させるようにしたPRを送ったよという話です。
ちなみに、diffを見ながらじゃないとついていくのがつらいと思います。

第0の関門:どう実装するか

TypeBuilder.DefineProperty()DefineEvent()ですね。あと、アクセッサーメソッドをPropertyBuilderEventBuilderに設定する必要があります。
なぜかこの辺のAPIは頭に入っているので、実装はあっさり終わりました。

第1関門:設計意図

意気揚々と直しましたが、そういえば、そもそもこれって意図的にプロパティやイベントを実装していないのかもしれません。
なので、とりあえずissueをあげて様子を見ました。
きっとRTM前で忙しいでしょうし、ということで、10日くらい待ちました。
で、音沙汰もないうちにリリース1週間前になり、よく見ると依存先のバージョンがrc3からrc4になってるし、これはもう1.1向けの開発モードになってそうだなということで、「何か問題あるかな? なさそうなら実装持ってるからPRできるよ」って聞いてみたところ、「急いで入れる必要ないよね? 実際に直し始めたら色々聞きたくなると思うけど、他にやることあるからちょっと待ってー」って言われて、「ていうか直したんだったらもうPR投げていいよー」と言われたので、PRすることにしました。

第2の関門:ビルドと単体テスト

PRを出すには、当然単体テストを実行しなければなりません(追加のテストは既に書いたものとします)。
ところが、corefxのプロジェクトは少々特殊なproject.jsonになっているので、素直にビルドできません。
corefxのドキュメントを読めばいいのですが、1.0 RC2 時点で、Windows環境でビルドするには以下のようにします。Visual Studio(ライセンス的に問題がない限り、無料のCommunity Editionでも大丈夫のはずです)が必要です。

  1. Visual Studio 2013の MSBuild へのパスが通っているコマンドプロンプトを開きます(「開発者コマンドプロンプト」とかそういうやつです。ちなみに、私はVS 2015用でやりましたが、問題はありませんでした)。
  2. corefxのルートにある、init-tools.cmdを実行します。
  3. 修正したコードのテストプロジェクトのディレクトリに移動し、msbuild /t:BuildAndTestを実行します。

ちゃんとドキュメントを読んでいればなんてことない話でした。

あと、実は初めてのPRだったのでちょっと戸惑ったり、CLAへの署名とかもありましたが、特に躓きはありませんでした。

第3の関門:レビュー

主にvarの使い方でダメ出しをくらいました。「型が明確な場合を除きvarを使わない」とありますが、これは「コンストラクター呼び出しの結果を割り当てる場合を除き」くらいのニュアンスだったようです。
他には、

  • 変数の宣言位置(実はリファクタリングしたときの修正漏れ)
  • ifの本体の改行位置(普段は中括弧必須派なので、うっかり)

などのポカミスが多かったです。正直手間を取らせて申し訳ない。
そして、次の関門。

第4の関門:MethodInfo.Equalsの罠

MethodInfo.Equalsは.NET Core だと参照比較として実装されているから、MethodInfoDictionaryのキーにしちゃダメよー」というありがたい突っ込みをいただきました。
(.NET メタプログラミング界隈の人たちが青ざめる姿が見えた気がしました)
MetadataToken比較するようなEqualityComparer作ればいいと思うYo」とのアドバイスをもらったので、それはそもそも.NET Coreに入っているべきではと思いつつ、それを英語で記述するよりは C# でコードを書く方が早いので書きました。確かに、メタデータテーブルのインデックスであるMetadataTokenを比較するのは確実で、かつ高速な手段ですしね。

第5の関門:netstandard1.3

はい。MemberInfo.MetadataTokenはnetstandard1.5のAPIなのです(.NET Framework側には昔からありますが)。System.Reflection.DispatchProxyは1.3用のライブラリなので、MetadataTokenは使えません。
なので、愚直に名前とパラメーターの型を比較する実装にしました。

第6の関門:ジェネリクス

意気揚々とプッシュしたEqualityComparer<MethodInfo>実装について、「この比較だとジェネリック型引数見てないからダメじゃね?」という真っ当な指摘を受けました。その通りですね。
ついでに「ECMAがどうかわからないけど、静的メソッドかインスタンスメソッドかも比較しないとまずいんじゃないかなー」と言われたので、ECMA-335を見てみました。
そのものずばりのところはないのですが、「I.8.6.1.6 Signature Matching」を見る限り、

  • 静的メソッドかインスタンスメソッドは比較すべき
  • 戻り値の型も比較すべき
  • 呼び出し規約(CallingConvention)も比較すべき

と判断し、これの真偽(実はいらないんじゃないか)を確かめるよりこの通り実装した方が早いので、実装しました(DispatchProxy.Create()をループの中で呼び出して遅いとかいう人はいないでしょうし、そもそも動的コード生成やJITコンパイルに比べたら大したことないでしょうし)。

第7の関門:raise

ここで、「そういえば、イベントのaddメソッドとremoveメソッドは割り当ててるけど、raiseメソッドも割り当てないとダメじゃないかな」という発言が出ました。こいつ、気付きやがった……。同時に「じゃあotherはどうする?」って聞き返そうかと思いましたが、.NET Coreにotherメソッドを割り当てるためのAPIないし、0x20歳を過ぎた大人なので藪蛇になることは言わないことにしました。

とはいえ、C#コンパイラにraiseメソッドを出力させる方法はありません。つまり、テストコードで動的型生成をしないと行けないということです。それは正直ちょっとやりすぎかなーと思った(この後保守するのが面倒になるんじゃないかなと思った)ので、「ほんとにやる?」って返信して、寝ました。もう丑三つ時回ってましたし。

翌日、仕事から帰ってきたところ、ノーレスだったので、四の五の言わずにやれや、ということだと判断し、あきらめて実装しました。実装はECMA-335を見ながらやったのですが、raiseメソッドのシグネチャはvoid raise_xxx(Event e)でなければならないとか書いてありつつ、Eventが何なのかは一切説明がありませんでした。ただ、普通に考えればイベントハンドラーに渡すデータでよかろうということで、適当にEventArgsを渡すようなメソッドを生成させました(これを書いていて思ったのですが、イベントの型は任意のデリゲートになり得るで、EventArgsではないような気はしますね。じゃあ何なのかというと謎ですが、テストでコード生成例外は起きてないし、所詮テストコードの動作の話なので良しとしましょう)。

あと、このためにテストプロジェクトからSystem.Reflection.Emitパッケージへの参照を増やす必要がありました。

第8の関門:dotnet-bot

10日間も間を開けたのは失敗でした。そう、1.1開発モードにシフトしているので、依存先パッケージのバージョンがガンガンアップデートされていきます(いつの間にかrc4betaになってました)。
そう、raise_メソッドのテストのためにテストプロジェクトのproject.jsonをいじっているので、botによって依存先パッケージのバージョンが書き換わると、修正がコンフリクトするのです。具体的には、変更をプッシュして「コンフリクトがないからマージできるよ」ってGitHubに言われても、数時間のうちにコンフリクト状態になってしまう状態でした。
ところが、レビューしてくれた人たち以外は忙しいのか何なのか、LGTMが増えず、マージされません。
コンフリクトのまま放置していもレビューしてくれなさそうなので、仕事から帰ったらマージして再プッシュとかしてみたものの、毎日再プッシュされてもレビューする側は嫌だと思うので、「毎日project.jsonマージしなおして上げなおしたらうざいと思うんで、レビューできる状態になったら教えて。そのときまとめて再プッシュするから」と言ったら、「いや、もう何も言わないってことはいいってことでしょ」と言われ、マージされたわけです。

結論

  • corefxの「varは控えめ」は本当に控えめ。
  • さすがにレビューはしっかりやってる
  • project.jsonいじると頻繁にマージコンフリクトするから気を付けよう

感想としては結構楽しかったです(レビューコメントの英語も勉強になりましたし)。

余談

レビューしてくれた人、日本人っぽい名前だったのですが、日本時間の23:00(シアトルは7:00)台にレスくれるので、早起きなのか、日本にいる人が夜中に対応してくれてたのかがよくわからず。
いずれにせよ、深夜早朝に丁寧な対応ありがとうございました。

0 件のコメント:

コメントを投稿