Rustで見つけたバグを自分で修正してプルリクしてみた
はじめに
先日、Rustコンパイラのバグを見つけたので、日ごろお世話になっている感謝を込めながらissueに登録したのですが、内容的に自分でも直せそうな予感がしたので、自分で直してプルリクし、無事にマージされたので、その軌跡を記しておこうと思います。
なお、Github上でのやりとりは全て英語となりますが、google翻訳に頼りっぱなしでやってます。
この記事は、
・英語よく分からない
・gitよくわからない
・というか、オープンソースにコントリビュートするとか不安でしかない
・ミスったら恥ずかしいし、ディスられたり罵られたりしたら怖い
などと考えている人の背中を押せるような記事に出来れば良いな、などと思いながら書き進めていこうと思います。
issueの内容
こちらから閲覧できます。
問題の内容としては、
fn main() {
struct StructA<A, B = A> {
_marker: std::marker::PhantomData<fn() -> (A, B)>,
}
struct StructB {
a: StructA<isize, [u8]>,
}
}
というコードをコンパイルした時に、
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> src\main.rs:7:12
|
7 | a: StructA<isize, [u8]>,
| ^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `[u8]`
note: required by a bound in `StructA`
--> src\main.rs:2:23
|
2 | struct StructA<A, B = A> {
| ^^^^^ required by this bound in `StructA`
help: consider relaxing the implicit `Sized` restriction
|
2 | struct StructA<A, B = A: ?Sized> {
| ++++++++
というエラーメッセージが出るのですが、
?Sizedの位置がおかしく、本来は
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> src\main.rs:7:12
|
7 | a: StructA<isize, [u8]>,
| ^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `[u8]`
note: required by a bound in `StructA`
--> src\main.rs:2:23
|
2 | struct StructA<A, B = A> {
| ^^^^^ required by this bound in `StructA`
help: consider relaxing the implicit `Sized` restriction
|
2 | struct StructA<A, B: ?Sized = A> {
| ++++++++
と出力されるのが正しいはず、という事を主張しています。
いっちょ自分で修正してみるか
issueを出した後は放置しておけば、きっと誰かが修正してくれると思うのですが、これくらいのバグなら自分でなおせるんじゃね？
と思い、取り組んでみる事にしました。
取り組みに際しては、こちらの記事が大変参考になりました。
実際にやってみると、意外と簡単でもあったのですが、プログラムの修正以外のアレコレが結構大変でした。
ですので、実際に同じことにチャレンジする方の参考のために、体験談とともに、気を付けた方がよいポイントなどを綴っていこうと思います。
issueにアサインしてもらう
まず初めにやるべき事としては、rustのソースの入手はもちろんの事ながら、他の誰かが同時に修正に取り組み始めてはマズいので、issueに「自分がやる」と宣言するのが良いと思います。
具体的には
@rustbot claim
とコメントを打つことで、自分がアサインされるらしいとの事で、おそるおそるやってみました。
そうすると、わりとすぐに、
rustbot assigned OdenShirataki
とコメントが来て、あっさりと自分がアサインされました。
なお、この時点では自分で修正できるという確信は無かったというか、できんかったらどうしよう、という気持ちももちろんあったのですが、途中で降りる事も出きるようですし、どうせ今まで放置されてきたバグで、しかも緊急性の高いものとも思えないので、割と気楽な気持ちでやっていました。
ソースコードの入手
こちらからgitで取得できます。が、いきなりcloneするのではなく、自分のアカウントにフォークしてから進めた方が良いです。
なぜかというと、プルリクエストする際に、フォークしたリポジトリからプルリクエストすることになるからです。
私はその事をしらなかったので、git cloneからやりはじめ、改めてフォークをするという二度手間が発生しました。
これを読んでいる皆様は、必ず自分のアカウントにフォークしてから始めてるようにしてください。
まずは原因箇所を特定しよう
この記事を読んでいる方は、rustのソースを初めて読む方が多いと思います。
私もこの度初めて読みました。
今回のissueはエラーレポートに関するものなので、特定は簡単な部類だと思います。
?Sizedの出力位置がおかしいので、同時に出力される「consider relaxing the implicit Sized restriction」で検索してみると、いくつかファイルが見つかりました。
この時点で候補となるファイルは2つほどに絞られました。（結果的には両方のファイルを修正することになります。後述。）
同じメッセージを出力する箇所がいくつかあるので、次は実際にどこを通るのかを突き止める必要がありました。
rustをコンパイルしてみよう
この辺りの段階にくると、実際にコードを修正し、動作させる必要があります。
具体的なやり方はこちらに書かれています。
どうやら、
x というpythonで書かれたコマンドでコンパイラをコンパイルできるようです。
ただし、rustのnightlyが必要らしく、
nightlyのインストールと設定が必要です。
こちらを参考にし、nightlyをインストールします。
git cloneしたディレクトリで、下記のコマンドを実行すると、そのディレクトリでの
buildがnightlyになります。
rustup override set nightly
この状態で、手っ取り早く問題個所を特定するために、通る可能性がありそうな個所いくつかに目星を付けてprintln!を仕込んでいきました。
問題のコンパイルなのですが、
./x build --stage 0 library
とか
./x build --stage 1 library
というコマンドでコンパイルできるようです。
stageという単位でバージョンを分けてコンパイラをコンパイルできます。
さらに、
に書かれている通り、ツールチェーンをリンクする事で、cargoを使って自分でコンパイルしたコンパイラでコンパイルができるようになります。
具体的なコマンドはこうです。
cargo +stage1 build
このように、+xxxxでツールチェーンを指定してコンパイルができます。
対象ファイルが特定できました。
println!を仕込んだコンパイラでコンパイルを実行する事で、対象となるファイルを特定する事が出来ました。
今回ターゲットとなるのは
compiler\rustc_trait_selection\src\traits\error_reporting\type_err_ctxt_ext.rs
というファイルでした。
これの、
fn maybe_suggest_unsized_generics
という関数が問題個所です。
いざ修正
コードの内容としては想像していたより難解なものではなかったのですが、それでも勝手がわからない中での手探りだったので、いろいろと弄っての修正となりました。最終的には一行のみの修正で落ち着いたのですが。
ここまでの所要時間は3時間程度だったと思います。
具体的な修正箇所はこちら
いざプルリクエスト
修正が終わったら、自分のフォークにpushして、githubの画面から、プルリクエストします。
画面のUIに従ってやれば良いだけなので、思ったよりも簡単でした。
ここからが大変なところ
修正そのものは難なく終え、プルリクエストも無事に終わったものの、本当の闘いはこれからでした。
大変だったことその1 - レビューからの修正 -
プルリクエストを投げると、当然のことながら、それをマージすべきかどうかの審査が入ります。
マズいところがあればコメントで指摘がもらえます。
案の定、一発では上手くいかず指摘をもらってしまいました。
その後、どうやって修正するのかわからなかったので、一旦プルリクを取りさげて改めてプルリクを投稿しなおしました。
大変だったことその2 - commitのsquash -
これが一番大変だったかもしれません。
どうやら、1プルリク1コミットというのが推奨される作法のようで、
複数のコミットをしたら、
Also, please squash this into one commit. This doesn't need to reference the issue number in the title of the commit, either -- it just needs a title that explains what it does.
というコメントをもらいました。
普段そういう事をしないので、やり方が全くわかりませんでしたが、
ここの記事が具体的でわかりやすかったです。
しかし、その後のpushでgitに怒られたりします。
squashする事でリモートとローカルのcommitに差異が出るためだと思いますので、
git push --force origin master
でやればOKなようです。
このやり方を覚えるまでがまぁまぁ大変でした。
rustにコントリビューションする場合は、常にsquashしてからpushするのが無難なようです。
大変だったことその3 - 自動テスト -
プルリクエストを送ったり、pushしたりすると、テストが自動的に実行されます。
普通の修正だとあまり問題はなかったかもしれません。
ところが、今回、エラーメッセージの修正というところに落とし穴がありました。
rust-lang/rustのディレクトリの中に
tests/ui
というディレクトリがあります。
自動テストはこの中のテストを全て実行するようです。
そして、それを全部パスしなければならないのですが、ファイルが大量にあります。めちゃくちゃ時間がかかります。
それだけならまだ良いのですが、エラーメッセージのテストの場合、
・エラーを発生させる.rsのプログラム
・期待されるエラー出力
の二つが必要になるようです。
今回、これにめちゃくちゃはまりました。
まず、期待されるエラー出力が必要というのは、いろいろ試行錯誤してやっとわかった事で、当初は
・エラーを発生させる.rsのプログラム
がエラーになるのでテストが通らない、（かのように読めてしまう）エラーメッセージに翻弄されてました。
何回かテストをしたり、test/ui内のファイル構成を眺めているうちに
・期待されるエラー出力
が必要、という事に気づいたわけです。
という事は、
・今回の修正によって、エラー出力が変わる
という事が発生するので、過去のテストコードが、それに合っていないものがある
という事が発生するため、
・過去に誰かが書いたテストコードを私が修正しなければならない
という状況が発生していたのです（ガビーン）
とはいえ、対象となるファイルは自分のもの以外は一つだけだったので、頑張って修正しました。
それで、出力の差異は無くなったはずなのですが、まだエラーがなくなりません。
なぜかと思ってエラーメッセージをよく読むと、やはり
・エラーを発生させる.rsのプログラム
がエラーになるのでテストが通らない、
という状況で間違いなさそうなのです。
いやいやいや、エラーメッセージのテストなのでエラー出るにきまってるやん、どないせいっちゅうねん、、、
、、、(´・ω・｀)
と途方に暮れつつ、他のテストコードを眺めていると、エラーメッセージ系のテストコードに特徴的なコメントが書かれていることに気づきました。
そのコメントを追加したコードがこちらです
fn main() {
struct StructA<A, B = A> {
_marker: std::marker::PhantomData<fn() -> (A, B)>,
}
struct StructB {
a: StructA<isize, [u8]>,
//~^ ERROR: the size for values of type `[u8]` cannot be known at compilation time [E0277]
}
}
ここまでやると、やっと自動テストが通りました。
似たようなバグもついでになおしちゃくれないかい？
テストも通ったし後は待つだけかな、と思っていたら、担当していたレビュアーから「他にも似たような事になるコードがある事に気付いちゃったので、時間と興味があるならついでに直してよ」みたいな事を言われました。
中身を見てみると、今回の修正したファイルとは別のファイルでの処理だったので、元のissueとは直接関係ないけどいいんかいな、などと思いつつ、冷静に考えれば、そもそもプルリクエスト一般がissueのfixだとは限らず、もはや元のissueとは切り離して進めるべきと考えて承諾する事にしました。
なにより（ここまで漕ぎつけるのに結構手間取ったコントリビューション初心者の私に）態々ご指名いただいては、中々に断りにくいというのもあります。
修正内容的には明らかに別の原因でしたので、コミットを分けた方が良い気もしたのですが、そうするとまたsquashしてと言われる可能性がある事と、エラーコードおよびエラーメッセージは同一のものでしたので、両方の修正に適用できるコメントに修正してコミットしてみました。
ただ、この修正については
trait Trait { type P<X>; }
impl Trait for () { type P<X> = [u8]; }
の場合に
trait Trait { type P: ?Sized<X>; }
++++++++
とサジェストされるのでおかしい、という内容だったのですが、
そもそも、
typeにジェネリクスな型を指定するってどういう事
(´・ω・｀)？
Xを使わない型を指定してもコンパイル通る理屈がわかんねー、Xどこ行った？？
などと謎を抱えながらの修正となりました。
その後
無事に修正が完了し、自動テストも通って、ヤキモキしながら待つこと約一日。レビュアーからapproveされました。
その後、botからキューに入った、というコメントが登録されます。
どうやらキューに入ったものから順次マージがされていく仕組みのようです。
そこから半日ほどかかってやっとマージされました。
なお、rustbotからのコメントによれば、バージョン1.78.0でリリースされるらしいです。
今は1.76.0なのでまだ先ですが、リリースを楽しみに待ちたいと思います。
感想とあとがき
内容としては軽微な修正
（実質的には合計で3行程度変えただけ）
でしたが、自分以外が書いたコードというのは中々読むのが大変なものです。
が、今回のRustのソースコードを読んでみて、分かりやすい変数名がつけてあったりと、かなり読みやすい部類のものに感じました。
また、Rustコンパイラ自体がRustで書かれており、C/C++と違って、nullが入っているかもしれない、という事を気にしなくてよいので、思い切った修正を入れる事にためらう必要が無く、やはりRustは良いね、となった次第です。
オープンソースへのコミットは、コミュニケーションが英語という事で躊躇されたり、そもそも参加するのにハードルが高いと感じる事も多いと思います。
実際、私も自分でバグを発見したりしなければ、自分がコントリビュートする事など考えなかったと思うのですが、実際にやってみると新鮮で有意義な体験ができたと思います。
オープンソースへのコントリビュートに興味があるけど躊躇しているような方に、この記事が少しでも参考になればと思います。
Discussion