MLtonコンパイラにPR取り込まれたら、お肉が食べられる話

本稿はML Advent Calendar 2020の8日目の記事です。

手記。
PRの仕方など、何らかの情報になるかもと残しておく。

MLtonコンパイラにバグを見つけた

今年の頭くらいにAtCoderの問題を解くのにハマっていた。
使っていた言語はSMLで、AtCoderではMLtonを使っていた。
そこで、String.scanの動作が手元のSML#とAtCoder上のMLtonで違うことを発見した。¹
具体的なコード例は以下である。
型を合わせるためにgetcやfullが登場しているが、重要なのはString.scanである。

(*
 * expected: NONE
 * result:   SOME ""
 *)
val result = String.scan Substring.getc (Substring.full "\n")

このString.scanというのはSMLのBasisライブラリに属している。
そしてBasisライブラリには仕様がある。
そこで、The Standard ML Basis Libraryを確認してみると、以下のようになっている。

If no conversion is possible, e.g., if the first character is non-printable or begins an illegal escape sequence, NONE is returned.
（自分の翻訳）
変換ができないとき、例えば最初の文字がnon-printableや誤ったエスケープシーケンスの場合、NONEが返される。²

つまり、non-printableな文字である\nスタートの文字列を受け取ると、String.scanはNONEを返すべきである。
MLtonコンパイラの動作がBasisと違っていることを把握した。

GitHubでIssueとPRを確認

はじめに既知のバグかを確認した。
MLton公式を見るとGitHubへのリンクがある。
そこからGitHubのMLtonコンパイラのページへ飛んだ。
公式を使うのは大事で、Google検索やGitHubでの検索は公式じゃないものもヒットする。

この公式GitHubで、IssueとPRをcloseされたもの込みで検索した。
確か、StringとかString.scanとかで検索したと思う。
何件かヒットするが、自分が見つけたバグに該当するものは見つからなかった。

GitHubから最新版を取得して再現を確認

AtCoderのコンパイラは古いことで有名である。
そこで、GitHubからMLtonコンパイラソースをcloneし、手元で最新版をビルドして試した。
aptなどのパッケージをupdateしつつ使っていても同じである。
リリース版と開発途中では差分があるため、本当に最新版でもバグが残っているか確認する必要がある。

ビルド環境を用意したり、ビルドが重たかったりとなかなかに手間がかかる。
やはり最新版でも同様の結果となり、これは公式では未知のバグだと確認できた。

MLtonのソースから該当の実装箇所を探す

MLtonのソースを見るのは初めてだった。
とりあえずscanやStringでレポジトリ内をざっくり検索したと思う。³
そこで、該当のString.scanの実装がbasis-library/text/string.smlあたりにあるのを見つけた。

val scan =
   fn reader =>
   let
      fun loop (state, cs) =
         case Char.scan reader state of
            NONE => SOME (implode (rev cs),
                          Char.formatSequences reader state)
          | SOME (c, state) => loop (state, c :: cs)
   in
      fn state => loop (state, [])
   end
val fromString = StringCvt.scanString scan

よくある話だが、コンパイラを作るときはプリミティブなモジュールを作って、それを元にモジュール内容を追加していき、最終的なモジュールを作りがちである。
そのため、MLtonでもString、Char、PreChar、PreCharXのような似た名前のモジュールが頻出して理解に苦労した記憶がある。
ときには、同じ名前だがシャドウイングで違うモジュールを指していたりもする。⁴

小さな変更をしてPRを立てた

該当関数に小さな変更だけをした。
たった2行の変更でいけそうと思った。
このとき、インデントや括弧の位置は改行の位置など、コーディング規約は周りに合わせた。
自分の手元のsml-modeとはインデント幅が違うくて苦労した。⁵

diff --git a/basis-library/text/string.sml b/basis-library/text/string.sml
index 6013ff065..f1adb9b19 100644
--- a/basis-library/text/string.sml
+++ b/basis-library/text/string.sml
@@ -67,8 +67,9 @@ functor StringFn(Arg : STRING_ARG)
          let
             fun loop (state, cs) =
                case Char.scan reader state of
-                  NONE => SOME (implode (rev cs),
-                                Char.formatSequences reader state)
+                  NONE => if null cs then NONE
+                          else SOME (implode (rev cs),
+                                     Char.formatSequences reader state)
                 | SOME (c, state) => loop (state, c :: cs)
          in
             fn state => loop (state, [])

その後、レポジトリのContributingの項目を見ると、「Issue、PR歓迎」的なことが書いてあった。 ⁶
のでこのままGitHubでPRだけを立てた。
「先にIssueを立ててからPRに取り掛かる」、「IssueとPRを同時に立てる」、「小さいのでPRだけでよい」などが考えられるが、どんなときどれがいいのかよく理解していない。
誰かIssueとPRの良い運用を紹介してほしい、参考にしたい。

ちなみにこのPRは、当時MLtonが使っていたTravis CIのチェックで落ちた。
作者からコメントがつき、「確かにBasisと実装が違ってる。ただ、String.scanやString.fromStringのテストが落ちているので、テスト修正もこのPRに含めてくれ。」と言われた。
テストの存在に気づいていなかったので、テストを書くことにした。

指摘を受け、テストを書き直しPRも書き直した

テストはregression/string.fromString.smlあたりにあった。
このテスト例をBasisマニュアルにあった例に書き直した。
すると、自分の変更したコードがテストを通らないことがわかった。
辛い。

最初は簡単な変更になると思っていたが、思惑が外れていたことを理解した。
ここで、Basisを読み直し、仕様を正しく理解するために時間を費やした。
SML#の実装やSML/NJの実装も見に行った。
特にSML/NJの実装はネストが深く、コードを見て絶望した記憶がある。

結局、PR内容はやや大きめに膨らんだ。
メインの変更は、新たな補助関数を導入し3重のcase文となった。
とはいえ、内容を理解してかなり短く書けたと思う。
変更が大きくなったことで、コードのインデントを周辺に揃えるのにやはり苦労した。

PRが受け入れられマージされた

テストも無事に通るようになった。
Travis CIのテストは落ちてドキッとしたが、それは別な変更のせいでMacでmasterを一時的にビルドできないせいだった。
たった一言だが、メイン開発者からのお礼の一言がとても嬉しかった。

LAPRAS春のOSS祭りでアマゾンギフト券が当たった

ちょうどLAPRASにて、OSS祭りをやっていた。
せっかくPRが取り込まれたし、1等の和牛が欲しくて応募してみた。
すると1等ではなかったが、見事にアマギフが当たった。
実質、関数型プログラミングで和牛券をもらった。
これはイケてる！

なんと、MLtonコンパイラにPR取り込まれた話で応募したら、ギフト券があたった！牛肉当たらなかったので、いい肉を買う資金にしよう🤤みんなもコンパイラにパッチ送ってお肉を食べていこうな！ https://t.co/tyYv9KDUg1

— よんた (@keita44_f4) April 2, 2020

まとめ

• バグを見つけたらPRチャンス
• バグは最新版での再現性のチェックをする
  • 手元のパッケージ化されたリリース版は古いことがある
• PR先に合わせた書き方をする
  • なるべく最小限のdiffとする
  • インデントや命名などコーディング規約を周りに合わせる
  • コミットログを他のコミットメッセージに合わせる
• 情報は公式や一次情報を使う
  • GitHubへのリンクは公式ページからたどる
  • ライブラリなどの仕様は一次情報元で調べる
• 関数型プログラミングはお肉が食べられる

1. AtCoderに参加して普段使わないような文字列入出力の操作をたくさんした ↩

2. 下のほうにあるImplementation noteに「特殊な仕様のせいで、Char.scanを流用してもString.scanは作れないよ、HAHAHA（意訳）」と書いてあって、どうして、となった。 ↩

3. 検索といえば、みんなもgrepを使うよりも高速なripgrepに乗り換えていこうな ↩

4. シャドウイングが入った型エラーは、コンパイラによってはとても見づらいことも。「Char.charを指定しているけど、ここはChar.charを指定しないといけないよ」とエラーが出て何もわからなくなった。 ↩

5. このコードのインデント幅が3でびっくりした。2か4あたりが主流だと思っている。 ↩

6. Contributingを確認するのも大事だし、OSSを作ったらContributeルールを書いておくのも大事 ↩