`Path`の適切ではないlossyなUTF-8変換をやめる #12

qryxip · 2023-04-03T09:00:57Z

まず前提として、RustのPath/PathBufはUTF-8である保証はありません。UTF-8として扱いたくなったときは、次のどちらかを明示的に行う必要があります。

UTF-8として壊れていたらエラーもしくはクラッシュ
UTF-8として壊れている部分を'�'に変換してUTF-8とする

本PRは2.の変換をしている部分を1.の形に置き換えます。

個人的な意見としては2.はメッセージ出力のみに限定されるべきで、実際にファイル操作に使うパスの文字列加工の過程で使われるべきではないと思います。

またvoicevox_coreは一貫して1.だったと思います。このリポジトリだと混在している状態ではありますが、 #9 以前には2.の形は1箇所しか無く、残りはすべて1.の形であるように見えました。

qryxip · 2023-04-03T09:20:15Z

半年前こんなことを言ったのですが、caminoのUtf8Path/Utf8PathBufの導入を再考しませんか?

VOICEVOX/voicevox_core#217 (comment)

このPRを出すときもPR自体出すべきか、どういう修正にしようか等ちょっと悩んだので、やっぱり.unwrap()やfrom_utf8_lossy()の数は少なければ少ないほど読む/書く人の認知負荷が減るんじゃないかなーと。
(VOICEVOX/voicevox_project#24でRust APIを提供できるかもしれない可能性を除いたとしても)

PickledChair

勉強になります。LGTM！

実用上はあまり問題ない（macOS 以外では xcrun を実行する時点で panic する。かつ、macOS の文字コードは基本的に UTF-8）ものの、不用意に lossy のメソッドを使うのは確かに良くないですね、気づきませんでした……。また、考えてみれば display というメソッド名自体も明らかに表示用ですね。良い学びになりました。

voicevox_core の方でも「この方針で実装する」という明確な意志で実装されてきたわけではなく、問題が生じそうだと気づいた時点で場当たり的に対処してきたという認識です（なのでこのリポジトリには不統一な部分が存在した）。今後は今回の PR で指摘された点についてもっと意識的に考えていきたいと思います。

Hiroshiba

LGTM！！

少なくともlossyはやめた方が良いなと思いました。
エラーが出る箇所が変わってしまってわかりづらいので。

UTF8Pathは、そのコメントの議論を追い直した感じ「どっちでも良い」なのかなと思いました。
unwrapが増えるのと依存が１つ増えるのどちらが良いかと言われると、ぶっちゃけunwrap１つのがメンテが単純だろうな〜〜〜というのが正直な感想です･･･！

PickledChair · 2023-04-03T09:49:21Z

そういえばそんな議論をしていたことがありましたね……。UTF8Path に関しては、個人的にはヒホさんと同意見です。

他の理由としては、私の感覚として、むしろサードパーティのライブラリを導入する方が学習コストや認知負荷が上がる気がしたというのもあります。この辺りは個々人で感覚が異なると思うので難しいところな気がしますが、それであれば、より広く知られている API としての標準ライブラリを使う、というのも一つの選択かなと思いました。

qryxip · 2023-04-03T15:37:51Z

考えていることを今ここで書いておきます。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はrelease buildのサイズが小さくなるようにした voicevox_core#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 · 2023-04-05T14:02:09Z

反応に困るであろうことを長々と書いてしましましたが、「外部ライブラリを入れる」という行為についてこの先も議論することがあるかもしれないので今表明しておくと、Rustがbatteries excludedな言語であることを考慮していいんじゃないかなという思いです。

あと無制限に依存を増やしていくことも抑えるべきだとも思っています。

caminoが有名じゃなかったり、メンテが止まっていたり、取り扱いに注意が必要だったり、変なAPIをしてたりしたら私も流石に提案してなかったと思います。また例えば「我々が欲しいものはCargoの内部API(依存ライブラリ226個 & 6週間ごとに破壊的変更)にあるからそれ使おう」とか「Rust Analyzerの内部API(週一で破壊的変更)を使おう」といったことを言われたらリターンが見合わない限り反対するでしょう。

Hiroshiba · 2023-04-05T14:24:34Z

なるほどです。
依存ライブラリに関して結構考えたりちょっと調べたりしてるんですが、「ライブラリを採用すべきか」の良い指標や良い指針が見当たらなくてどうすべきか迷ってたりします。

とりあえず、標準非標準は置いといて、そもそも依存ライブラリを増やすメリデメを書いてみました。

a. 依存ライブラリを増やすメリット

開発速度がその瞬間早くなる
そのライブラリが担当する周辺知識を得る必要がある程度なくなる
アプデを勝手にやってくれる

b. デメリット

そのライブラリ独特の知識をある程度得る必要がある
メンテされなくなったとき依存外し工数がかかる
破壊的変更時に対応が必要

ややこしいライブラリだとb-1の工数が増えたり逆にa-2のメリットが消えたり、
標準ライブラリとかみんな知ってるライブラリだとb-1やb-2のデメリットが減ったり、
破壊的変更が多かったらb-3のデメリットが増えたり、
って感じですかね。。

で、今回の場合、a-2やb-1のようなライブラリやその周辺の知識が必要なものは、それを学ぶだけで工数がかかりそうです。
なので、知ってれば即マージできるけど、知らないと調べないといけないから時間かかるので、コミッターとレビュワーで気持ちが乖離しがちかもです。

レビュワー側的には、破壊的変更の頻度と、APIの安定性、あと使われ具合なんかを調べて判断って感じが良いのかなと思いました。
それやるのはそこそこ大変ですが、まあ知識増えるので面倒ばかりじゃないかなと。
僕は次からそういうとこ意識して、無意識的に「依存ライブラリ増やすのやだな」と思っていたフシが有るのを改めようかなと思います！

qryxip · 2023-04-05T15:01:19Z

たった今思い付いたのですが、ライブラリを入れるときはこういうテンプレートを入れることを推奨するというのはどうでしょうか...?

camino

このライブラリは何

Utf8Path{,Buf}を提供します。

UTF-8を強制する代わりに、可能な限りstd::pathのように振る舞うようになっています。

どれくらい有名か

cargo_metadataに採用されたのが大きく、そこから間接的にcargo pluginなどの開発ツールに広く使われています。Rust Analyzer (VSCode拡張とかの裏で動いているやつ)にも入っています。

camino単体でもそこそこ有名なツール/ライブラリに採用されているのが観測できます。

メンテされているのか

はい。メンテされています。

リポジトリ

bloatの心配は

ない。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 · 2023-04-06T06:10:30Z

全部に対して書くのお願いすると今度はプルリクが大変になっちゃうので、適宜聞く感じがいいかなとか思いました！
判断が必要そうな時に、書いてくださった内容を伺う形になるかなと。
（判断基準をドキュメント化しとくのを考えてます）

まあこれからは、caminoくらいの知名度と薄さだと、「なぜ必要か」がわかればささっと依存OKの判断ができると思います。

qryxip · 2023-04-06T18:59:46Z

私の基準をお伝えしておくと、例えばserde::Serializeが常にfallibleであるせいでこういうunwrap/expectが必要になるからといって、JSONへの変換はserdeじゃなくてminiserdeやnanoserdeでやろうと言われたら渋い顔になると思います。流石にserdeの歴史と安定感を考えると、std::path → caminoのようには…

https://github.com/VOICEVOX/voicevox_core/blob/0049bc51775c153fe513fa8ee2ba2cbe00e2c415/crates/voicevox_core/src/status.rs#L197-L199

(ただ実際に来た場合、ここ(VOICEVOX全体)のメンバーからの提案だったら懸念を伝えるのみ、新規のfirst-time contributorからだったりしたらそのままOK出してしまう、という風にしてしまうかも)

Hiroshiba · 2023-04-08T00:57:51Z

なるほどです。同感です。
例えばminiserdeにしないと解決できない問題とかあるなら話は別かもですが、って感じですねぇ。

VOICEVOX COREはおそらくコードレベルの提案はほとんど来なくて、ユーザーの興味的にVOICEVOXとしての機能の提案が多いと思います。
なのでたぶんserdeを他のに変えよう、という提案はたぶん１年で１つ来るか１つも来ないかもだろうなと思います。

Pathの適切ではないlossyなUTF-8変換をやめる

e64fcb3

PickledChair approved these changes Apr 3, 2023

View reviewed changes

qryxip mentioned this pull request Apr 3, 2023

impl Iterator → impl IntoIterator #11

Merged

Hiroshiba approved these changes Apr 3, 2023

View reviewed changes

Hiroshiba merged commit d1501da into VOICEVOX:main Apr 3, 2023

qryxip deleted the stop-lossy-utf8-conversion branch April 3, 2023 10:14

qryxip mentioned this pull request Apr 5, 2023

derive-gettersをamplify_deriveに入れ替える VOICEVOX/voicevox_core#460

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Path`の適切ではないlossyなUTF-8変換をやめる #12

`Path`の適切ではないlossyなUTF-8変換をやめる #12

qryxip commented Apr 3, 2023

qryxip commented Apr 3, 2023 •

edited

Loading

PickledChair left a comment

Hiroshiba left a comment •

edited

Loading

PickledChair commented Apr 3, 2023

qryxip commented Apr 3, 2023 •

edited

Loading

## TL;DR

qryxip commented Apr 5, 2023

Hiroshiba commented Apr 5, 2023 •

edited

Loading

qryxip commented Apr 5, 2023 •

edited

Loading

Hiroshiba commented Apr 6, 2023 •

edited

Loading

qryxip commented Apr 6, 2023

Hiroshiba commented Apr 8, 2023 •

edited

Loading

Pathの適切ではないlossyなUTF-8変換をやめる #12

Pathの適切ではないlossyなUTF-8変換をやめる #12

Conversation

qryxip commented Apr 3, 2023

qryxip commented Apr 3, 2023 • edited Loading

PickledChair left a comment

Choose a reason for hiding this comment

Hiroshiba left a comment • edited Loading

Choose a reason for hiding this comment

PickledChair commented Apr 3, 2023

qryxip commented Apr 3, 2023 • edited Loading

## TL;DR

qryxip commented Apr 5, 2023

Hiroshiba commented Apr 5, 2023 • edited Loading

qryxip commented Apr 5, 2023 • edited Loading

camino

このライブラリは何

どれくらい有名か

メンテされているのか

bloatの心配は

取り扱いに注意は要るか

Hiroshiba commented Apr 6, 2023 • edited Loading

qryxip commented Apr 6, 2023

Hiroshiba commented Apr 8, 2023 • edited Loading

`Path`の適切ではないlossyなUTF-8変換をやめる #12

`Path`の適切ではないlossyなUTF-8変換をやめる #12

qryxip commented Apr 3, 2023 •

edited

Loading

Hiroshiba left a comment •

edited

Loading

qryxip commented Apr 3, 2023 •

edited

Loading

Hiroshiba commented Apr 5, 2023 •

edited

Loading

qryxip commented Apr 5, 2023 •

edited

Loading

Hiroshiba commented Apr 6, 2023 •

edited

Loading

Hiroshiba commented Apr 8, 2023 •

edited

Loading