Closed qryxip closed 1 year ago
qryxip
commented
1 year ago 半年前こんなことを言ったのですが、caminoのUtf8Path/Utf8PathBufの導入を再考しませんか?
https://github.com/VOICEVOX/voicevox_core/pull/217#discussion_r950660084
このPRを出すときもPR自体出すべきか、どういう修正にしようか等ちょっと悩んだので、やっぱり.unwrap()やfrom_utf8_lossy()の数は少なければ少ないほど読む/書く人の認知負荷が減るんじゃないかなーと。
(https://github.com/VOICEVOX/voicevox_project/issues/24でRust APIを提供できるかもしれない可能性を除いたとしても)
PickledChair
commented
1 year ago そういえばそんな議論をしていたことがありましたね……。UTF8Path に関しては、個人的にはヒホさんと同意見です。
他の理由としては、私の感覚として、むしろサードパーティのライブラリを導入する方が学習コストや認知負荷が上がる気がしたというのもあります。この辺りは個々人で感覚が異なると思うので難しいところな気がしますが、それであれば、より広く知られている API としての標準ライブラリを使う、というのも一つの選択かなと思いました。
qryxip
commented
1 year ago 考えていることを今ここで書いておきます。UTF-8の表明は別に重大な問題ではないのですが、この際なので
もうちょっと一般的な話として広げられたらなと思っています。
いやいいですね。備忘録としてだけ。
「UTF-8である」ということを型で表明する必要性はあるか?
結果的な.unwrap()の数よりもどちらかと言うと、「UTF-8でなければならない」という不変条件が括り付けられたPathがC/Python API → Rust API → open_jtalk-rsと回されている状態が気になっています。
あとunwrap自体についてですが、パニックは大体の場合回復不可能な状態を示しておりバグであることを示します。そのため別に無理して撲滅する必要はありませんが、次のような温度感で接するべきかと思います。
## TL;DR
不正な値の存在の存在を許してはいけない。 不正な値が存在できてしまう時点で、未定義動作を覚悟するくらいのつもりでいるべきである。
満たされるべき条件を満たさない時点で、プログラムの内部的な整合性は既に破綻しており、未定義動作も同然の状態である。 これ以上余計なことをする前にさっさとクラッシュせよ。
整合性破壊バグから「うまく復帰」できると思うのは甘え (極論)。
Panic を恐れるべからず - 何とは言わない天然水飲みたさ
これに従えば、型付けを強める選択肢を取らないのであれば代わりにこういう感じのを入れてもいいのではないかなと思っています。
@@ -38,6 +38,9 @@ impl Mecab {
self.0.as_ref().unwrap() as *const open_jtalk_sys::Mecab as *mut open_jtalk_sys::Mecab
}
+ /// # Panics
+ ///
+ /// `dic_dir`がUTF-8ではないときパニックする。
pub fn load(&mut self, dic_dir: impl AsRef<Path>) -> bool {
let dic_dir = CString::new(dic_dir.as_ref().to_str().unwrap()).unwrap();
unsafe {@@ -111,7 +111,12 @@ impl OpenJtalk {
}
pub fn load(&mut self, mecab_dict_dir: impl AsRef<Path>) -> Result<()> {
- let result = self.mecab.load(mecab_dict_dir.as_ref());
+ let result = self.mecab.load(mecab_dict_dir.as_ref().to_str().expect(
+ "`mecab_dict_dir`が有効なUTF-8ではない
+
+現段階において、このパラメータはUTF-8であることをチェック済であるはずである。
+参考: https://github.com/VOICEVOX/voicevox_core/pull/217#discussion_r950660084",
+ ));
if result {
self.dict_loaded = true;
Ok(())特に今のVOICEVOX COREはhttps://github.com/VOICEVOX/voicevox_core/pull/218により、パニックが発生したときの挙動が「メッセージを表示後即座にプロセスの強制終了」なのでそこも考慮する必要があるかなと思っています。
そのために外部ライブラリを入れる必要性はあるか?
Rustの標準ライブラリの立ち位置について、2017年に公式ブログに書かれた文章があります。私はこの年にRustを書き始めたのですが、2023年の今も私の知る限りは立ち位置は変わっていないと思います。 標準ライブラリのみのRustは"batteries include"ではなく(敢えて言うなら"battery excluded")、Cargoのエコシステムがあって始めてそう言えます。
The Rust Libz Blitz - Rust Blog
Rustの標準ライブラリは、「破壊的変更を入れる羽目になる可能性が低い」ことを大前提として「無いと流石に困るもの」か「あっても特に困らない」ものしか基本はない、という理解を私はしています。あと互換性を守るためならこういうこともしたりします。
ちなみに明確に#[deprecated]になってなくてもそれなりの問題を抱えたAPIは、私の知る限りでもいくつもあります。std::fs::canonizalizeとかstd::fs::remove_dir_allとか。
(std::sync::mpscもサードパーティの方が推奨される事態になっていて、最近になってそのサードパーティの実装にそのまま入れ替えられることで復活を果たすということをやってました)
std::pathがそうだとは言いませんが、その機能に満足できないような状況になったときに外部ライブラリを使うのは、Rust的には慣習的かと思います。
あと今私が提案しているcaminoについては、デファクトになりつつある上にAPIをstd::pathに極限まで似せているため、「caminoのことはわからないけどstd::pathの方なら私は完全に知り尽している」という事は起きないんじゃないかなと思っています。
ただ今回の場合2.はやめてstdのみで1.をやる手があります。camino登場前はstrで「UTF-8のファイルパス」を表すことがよくあった(例: path-slash)ので、そうすればよいです。
qryxip
commented
1 year ago 反応に困るであろうことを長々と書いてしましましたが、「外部ライブラリを入れる」という行為についてこの先も議論することがあるかもしれないので今表明しておくと、Rustがbatteries excludedな言語であることを考慮していいんじゃないかなという思いです。
あと無制限に依存を増やしていくことも抑えるべきだとも思っています。
caminoが有名じゃなかったり、メンテが止まっていたり、取り扱いに注意が必要だったり、変なAPIをしてたりしたら私も流石に提案してなかったと思います。また例えば「我々が欲しいものはCargoの内部API(依存ライブラリ226個 & 6週間ごとに破壊的変更)にあるからそれ使おう」とか「Rust Analyzerの内部API(週一で破壊的変更)を使おう」といったことを言われたらリターンが見合わない限り反対するでしょう。
Hiroshiba
commented
1 year ago なるほどです。 依存ライブラリに関して結構考えたりちょっと調べたりしてるんですが、「ライブラリを採用すべきか」の良い指標や良い指針が見当たらなくてどうすべきか迷ってたりします。
とりあえず、標準非標準は置いといて、そもそも依存ライブラリを増やすメリデメを書いてみました。
a. 依存ライブラリを増やすメリット
b. デメリット
ややこしいライブラリだとb-1の工数が増えたり逆にa-2のメリットが消えたり、 標準ライブラリとかみんな知ってるライブラリだとb-1やb-2のデメリットが減ったり、 破壊的変更が多かったらb-3のデメリットが増えたり、 って感じですかね。。
で、今回の場合、a-2やb-1のようなライブラリやその周辺の知識が必要なものは、それを学ぶだけで工数がかかりそうです。 なので、知ってれば即マージできるけど、知らないと調べないといけないから時間かかるので、コミッターとレビュワーで気持ちが乖離しがちかもです。
レビュワー側的には、破壊的変更の頻度と、APIの安定性、あと使われ具合なんかを調べて判断って感じが良いのかなと思いました。 それやるのはそこそこ大変ですが、まあ知識増えるので面倒ばかりじゃないかなと。 僕は次からそういうとこ意識して、無意識的に「依存ライブラリ増やすのやだな」と思っていたフシが有るのを改めようかなと思います！
qryxip
commented
1 year ago たった今思い付いたのですが、ライブラリを入れるときはこういうテンプレートを入れることを推奨するというのはどうでしょうか...?
Utf8Path{,Buf}を提供します。
UTF-8を強制する代わりに、可能な限りstd::pathのように振る舞うようになっています。
cargo_metadataに採用されたのが大きく、そこから間接的にcargo pluginなどの開発ツールに広く使われています。Rust Analyzer (VSCode拡張とかの裏で動いているやつ)にも入っています。
camino単体でもそこそこ有名なツール/ライブラリに採用されているのが観測できます。
はい。メンテされています。
ない。default featuresでは依存0。コードも2k行程度。
❯ tokei
===============================================================================
Language Files Lines Code Comments Blanks
===============================================================================
Markdown 4 415 0 290 125
TOML 5 64 56 1 7
-------------------------------------------------------------------------------
Rust 9 2008 1558 143 307
|- Markdown 6 1528 9 1098 421
(Total) 3536 1567 1241 728
===============================================================================
Total 18 2487 1614 434 439
===============================================================================要らないと思います。「中身がOsStrじゃなくてstr(相当)なstd::path」とさえ理解していればいいはずです。
Hiroshiba
commented
1 year ago 全部に対して書くのお願いすると今度はプルリクが大変になっちゃうので、適宜聞く感じがいいかなとか思いました！ 判断が必要そうな時に、書いてくださった内容を伺う形になるかなと。 （判断基準をドキュメント化しとくのを考えてます）
まあこれからは、caminoくらいの知名度と薄さだと、「なぜ必要か」がわかればささっと依存OKの判断ができると思います。
qryxip
commented
1 year ago Hiroshiba
commented
1 year ago なるほどです。同感です。 例えばminiserdeにしないと解決できない問題とかあるなら話は別かもですが、って感じですねぇ。
VOICEVOX COREはおそらくコードレベルの提案はほとんど来なくて、ユーザーの興味的にVOICEVOXとしての機能の提案が多いと思います。 なのでたぶんserdeを他のに変えよう、という提案はたぶん１年で１つ来るか１つも来ないかもだろうなと思います。
まず前提として、Rustの
Path/PathBufはUTF-8である保証はありません。UTF-8として扱いたくなったときは、次のどちらかを明示的に行う必要があります。'�'に変換してUTF-8とする本PRは2.の変換をしている部分を1.の形に置き換えます。
個人的な意見としては2.はメッセージ出力のみに限定されるべきで、実際にファイル操作に使うパスの文字列加工の過程で使われるべきではないと思います。
またvoicevox_coreは一貫して1.だったと思います。このリポジトリだと混在している状態ではありますが、 #9 以前には2.の形は1箇所しか無く、残りはすべて1.の形であるように見えました。