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

本稿は[ML Advent Calendar 2020](https://qiita.com/advent-calendar/2020/ml)の8日目の記事です。

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

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

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

[^3]: AtCoderに参加して普段使わないような文字列入出力の操作をたくさんした

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

この`String.scan`というのはSMLのBasisライブラリに属している。
そしてBasisライブラリには仕様がある。
そこで、[The Standard ML Basis Library](https://smlfamily.github.io/Basis/string.html#SIG:STRING.scan:VAL)を確認してみると、以下のようになっている。

> 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が返される。[^1]


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

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

# GitHubでIssueとPRを確認

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

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

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

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

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

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

MLtonのソースを見るのは初めてだった。
とりあえず`scan`や`String`でレポジトリ内をざっくり検索したと思う。[^4]
そこで、該当の`String.scan`の実装が[`basis-library/text/string.sml`](https://github.com/MLton/mlton/blob/master/basis-library/text/string.sml#L65)あたりにあるのを見つけた。

[^4]: 検索といえば、みんなもgrepを使うよりも高速な[ripgrep](https://github.com/BurntSushi/ripgrep)に乗り換えていこうな

```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`のような似た名前のモジュールが頻出して理解に苦労した記憶がある。
ときには、同じ名前だがシャドウイングで違うモジュールを指していたりもする。[^2]

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

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

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

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

```diff
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歓迎」的なことが書いてあった。 [^6]
のでこのままGitHubで[PR](https://github.com/MLton/mlton/pull/371)だけを立てた。
「先にIssueを立ててからPRに取り掛かる」、「IssueとPRを同時に立てる」、「小さいのでPRだけでよい」などが考えられるが、どんなときどれがいいのかよく理解していない。
誰かIssueとPRの良い運用を紹介してほしい、参考にしたい。

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

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

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

テストは[`regression/string.fromString.sml`](https://github.com/MLton/mlton/blob/master/regression/string.fromString.sml)あたりにあった。
このテスト例をBasisマニュアルにあった例に書き直した。
すると、自分の変更したコードがテストを通らないことがわかった。
辛い。

最初は簡単な変更になると思っていたが、思惑が外れていたことを理解した。
ここで、Basisを読み直し、仕様を正しく理解するために時間を費やした。
[SML#の実装](https://github.com/smlsharp/smlsharp/blob/404c01a179543ca90182892a1a27c5faa77bea72/src/basis/main/String.sml#L180)や[SML/NJの実装](https://github.com/sml-nj/smlnj/blob/aeb2661e99a7fb23e56df33e956f23baf12afadb/sml/system/Basis/Implementation/string.sml#L202)も見に行った。
特にSML/NJの実装はネストが深く、コードを見て絶望した記憶がある。

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

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

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

![image.png](https://qiita-image-store.s3.ap-northeast-1.amazonaws.com/0/42721/c8e0ec51-054c-5879-5d7f-555084760d3c.png)


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

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

<blockquote class="twitter-tweet"><p lang="ja" dir="ltr">なんと、MLtonコンパイラにPR取り込まれた話で応募したら、ギフト券があたった！牛肉当たらなかったので、いい肉を買う資金にしよう🤤みんなもコンパイラにパッチ送ってお肉を食べていこうな！ <a href="https://t.co/tyYv9KDUg1">https://t.co/tyYv9KDUg1</a></p>&mdash; よんた (@keita44_f4) <a href="https://twitter.com/keita44_f4/status/1245650986439290881?ref_src=twsrc%5Etfw">April 2, 2020</a></blockquote> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script> 

# まとめ

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