qryxip commented 3 months ago

内容

crates/voicevox_coreに対するhttps://github.com/VOICEVOX/voicevox_core/pull/175を取り消します。理由は次の通りです。

今はhttps://github.com/VOICEVOX/voicevox_core/pull/353がある
そもそも#![forbid(unsafe_code)]と異なり、#![deny(unsafe_code)]は実質的に何の効力も無い

その他

qryxip commented 3 months ago

恩恵で言えば、個人的には率直に言って今となってはゼロに近いと思っています。unsafeなコードは厳重に扱われるべきであるし数も減らすべきですが、それに対して#[deny(unsafe_code)]は有効ではないと思っています。

例として、あるクレート内に次のコードがあるとします。

unsafe {
    external_c_function();
}

このクレート全体に#[deny(unsafe_code)]を掛けてもunsafeを封じることにはなりません。denyレベルであれば下層でallowし直せるので、このようにすればunsafeが使えます。今のcrates/voicevox_coreはこうなっています。

+#[allow(unsafe_code)]
 unsafe {
     external_c_function();
 }

この#[allow(unsafe_code)]ですが、文字数を増やすことにしかなっていないと思います。何故ならばunsafeなアイテムはそもそもunsafe {}で囲う必要があるからです。

unsafeコードを記述する敷居を高くしたいのであれば、このようなコメントを書く文化がRustにはあります。こういうコメントを書く方に倒した方がよいと思います。

 unsafe {
+    // SAFETY: This is safe because:
+    // 1. …
+    // 2. …
     external_c_function();
 }

175 の当時は今で言うcrates/voicevox_core_c_apiの部分がcrates/voicevox_core内に含まれており、`extern "C"`関数宣言部分とそれ以外の部分が違う世界であるということを表明する意義があったかもしれません。ただ今となってはクレートとして分離されているため、理由としては無くなっていると思います。

ちなみにですがdenyではなくforbidであれば、部分的にallowし直すことはできないためunsafeコードを封じることができます。#[forbid(unsafe_code)]であればある一つのクレート、つまり一塊のパブリックAPIがSafe Rustで構成されていると表明できるという意義があります。参考: Rust Safety Dance list_windows_video_cards()だけ別クレート(もしくは別リポジトリ)に確保して、unsafeなコードが無くなったcrates/voicevox_coreをforbidにするというのはありだとは思います。

プルリク来たときに変更お願いしやすいかも or そもそもunsafeじゃないコードをもらえやすくなるかも

個人的には明確かつ正しい内容のsafetyコメントが本当に書けて、かつUnsafe Rustに手を出す妥当な理由が存在するのであれば、unsafeなコードを含むプルリクを受け付けてもよいと思っています。その判断はむしろレビューの段階の方がよいのではないかと。最終的にunsafeなコードを拒否することになるにせよ、建設的な議論の末にそうなると信じています。

以上は短時間でざっと書いたものなので、必要であれば追加の補足をしたいと思います。

Hiroshiba commented 3 months ago

なるほどです、詳しくありがとうございます！！

説明かコメントを求める方向が良いのかなと思いました！少なくとも自分はunsafeに対する感覚を持てたので、レビュー時にunsafeに目を光らせられると思います。

クレート分離もなるほどです。数が増えてきたら分離しても良いかもですね！たぶんOS依存のutilityっぽいコードになるでしょうし、まとまってた方がなんか便利そうなので。

unsafeな理由と、それでも大丈夫な理由はコメントに書く文化、取り入れていきたいなと感じました。詳しくありです！！！

qryxip commented 2 months ago

クレート分離もなるほどです。数が増えてきたら分離しても良いかもですね！たぶんOS依存のutilityっぽいコードになるでしょうし、まとまってた方がなんか便利そうなので。

OS毎というよりは、Linux版の実装と一緒にVOICEVOX/gpu-list-rsみたいな感じで分離という形がちょうどいいかなと思っています。

unsafeなコードはなるべく避けてください、みたいなドキュメントってあった方がコーディングしやすくなる（というか説得しやすくなる）とかありますかね？あれば貢献者ガイドラインに書き足してもよさそうかも。まあ書いてなくても普通にunsafeは避けれるなら避けるべきと案内できそうな気はしてます。

問題になることがあるなら、極端なケースだと「次のC++コードをRustに翻訳してください」みたいな指示をもとにChatGPTかCopilotが生成したコードをそのままPRとしてお出しされたときですかね？そういう場合にも確かにガイドラインは効きそうではありますね。 (そこまで行くとどの道別ベクトルでしんどそうですが)

書くとしたら、「unsafeをそこまで忌避している訳では無い。ただし一般的なRustのプロジェクトと同様、本当に必要な時に限る。とりあえずRustonomiconとTwo Kinds of Invariantsは読め。話はそれからだ」みたいな感じがいいかなと思っています。

Hiroshiba commented 2 months ago

単純に初学者の方で、unsafeは避けるべきと知らなかったとかはありそうかもです。まあ課題になってからドキュメントに書くか検討でも良さそう！

VOICEVOX / voicevox_core

Rust APIの`unsafe_code`を`allow`にする #773

内容

関連 Issue

その他