Rust APIが公開するエラーの種類を`ErrorKind`として表現する #589

qryxip · 2023-08-27T06:18:58Z

内容

voicevox_core::ErrorKindを追加し、voicevox_core::Errorはopaqueにします。それに伴い、voicevox_core::Error以外のエラー型の可視性をすべてpub(crate)にします。

Python/Java/Rust(今後) APIのためにRust APIを整理することが目的です。

その他

qryxip

補足:

qryxip · 2023-08-27T06:22:16Z

crates/voicevox_core/src/error.rs

@@ -26,6 +72,7 @@ pub enum Error {
    #[error("無効なspeaker_idです: {style_id:?}")]
    InvalidStyleId { style_id: StyleId },

+    #[allow(dead_code)] // FIXME


UnloadedModelと統合される形になると思ってます。

qryxip · 2023-08-27T06:23:36Z

crates/voicevox_core/src/error.rs

+    /// OpenJTalkのユーザー辞書の設定に失敗した。
+    UseUserDict,
+    /// ユーザー辞書の単語のバリデーションに失敗した。
+    InvalidWord,


ここのdocは現状のResultCodeからそのまま持って来ました。

crates/voicevox_core/src/user_dict/word.rs

qryxip · 2023-08-27T06:28:15Z

crates/voicevox_core_c_api/src/lib.rs

-
-#[cfg(test)]
-mod tests {
-    use super::*;
-    use anyhow::anyhow;
-    use pretty_assertions::assert_eq;
-    use voicevox_core::Error;
-    use voicevox_core::Result;
-
-    #[rstest]
-    #[case(Ok(()), VoicevoxResultCode::VOICEVOX_RESULT_OK)]
-    #[case(
-        Err(Error::NotLoadedOpenjtalkDict),
-        VoicevoxResultCode::VOICEVOX_RESULT_NOT_LOADED_OPENJTALK_DICT_ERROR
-    )]
-    #[case(
-        Err(Error::GetSupportedDevices(anyhow!("some get supported devices error"))),
-        VoicevoxResultCode::VOICEVOX_RESULT_GET_SUPPORTED_DEVICES_ERROR
-    )]
-    fn into_result_code_with_error_works(
-        #[case] result: Result<()>,
-        #[case] expected: VoicevoxResultCode,
-    ) {
-        let actual = into_result_code_with_error(result.map_err(Into::into));
-        assert_eq!(expected, actual);
-    }
-}


今はe2eがあるし、ここが果たす役割もそう多くないと思ったので削除。

qryxip · 2023-08-27T06:31:28Z

crates/voicevox_core/src/__internal.rs

+pub fn validate_pronunciation(pronunciation: &str) -> std::result::Result<(), impl Display> {
+    crate::user_dict::validate_pronunciation(pronunciation)
+}


InvalidWordErrorという型を隠蔽したままPython APIに輸出するため。

ここ実装が正直ちょっといびつだなと感じました！

UserDictWordのpronunciationのpydantic.validationをなくすと、pronunciationは誰からもvalidationされない感じなんでしたっけ。。だったらとりあえずここに書くのは仕方なそう。
もしUserDictWordを使用する関数（add_word）内でvalidationエラーになる場合は･･･どうすべきか難しいですね･･･。

うーーーーん、validate_user_dict_word関数を作成するとか･･･？
まあでもどちらにせよこのプルリクエストで解決することではなさそうなので、とりあえずFIXME書くとかでもいいかも。。

#595 を作りました。

関数のシグネチャと、__internalという名の#[doc(hidden)]なモジュールに包まれていることから、意図にコメントは要らないかなと思うので、FIXMEが無くても上記のissueがあるだけでよいかなと思いました。

なるほどです。
確かにソースコードがやっていることの意図はわかると思います。
ただ、この部分は今仕方なくこうなっていて、どちらかというと改修したい対象だということが実装からはわからなそう･･･？

そのことをコメントか何かに書いとくのどうでしょう。
例えば、この辺りの実装を発展させたプルリクエストを抑止できるかなぁと。

FIXMEを入れました。
e8045e4 (#589)

qryxip · 2023-08-27T06:42:05Z

crates/voicevox_core/src/engine/full_context_label.rs

@@ -49,7 +49,7 @@ fn string_feature_by_regex(re: &Regex, label: &str) -> Result<String> {
 }

 impl Phoneme {
-    pub fn from_label(label: impl Into<String>) -> Result<Self> {
+    pub(crate) fn from_label(label: impl Into<String>) -> Result<Self> {


各エラー型の可視性の降格により、シグネチャにそれらのエラー型が表われる関数がコンパイルエラーになるので同様に可視性を調整。

これによりコード中に特に理由無くpubとpub(crate)が混在することになりますが、Rust API内のアイテムの可視性については元々深く考えられていないということで今のところはよいかなと思いました。

混じってしまうのはまあありなのかなと思いました。
ただ混じってしまうぐらいなんだったら、今は全部統一的にpubにしておいて、pub(crate)にする機会を別途作成してそこで一気に変更とかもありかもとか思いました。

まあでもせっかく書いちゃったんだし、こちらのプルリクエストの形で一旦マージするのもありかもです。
あまり強い意見ではないです。

#594 を作りました。
(初心者歓迎タスクにしました)

Hiroshiba

全部読みました！

タイトル周りの変更はLGTMなのですが、可視性の変更周りでいくつか気になったのでコメントしてみました。
APIの実装的にはおそらく戻って全部一気に変更した方がいいのですが、まあ実装しちゃったし戻すのそこまでモチベーションないということであれば、とりあえずマージでもそんなに問題じゃないかなと思います。

Hiroshiba · 2023-08-28T19:59:16Z

あ、もう1点質問です！
そもそも何の目的のためにこの整理を行う感じでしょうか？ 👀

Hiroshiba · 2023-08-28T20:10:51Z

crates/voicevox_core/src/error.rs

+#[error(transparent)]
+pub struct Error(#[from] ErrorRepr);


多分できることがError==ErrorReprだと思うのですが、であれば最初から内部で使う構造体もErrorで良いのではとちょっと思いました。
メリットがあれば知りたいです。
あるいは一般的にクレート内のエラー構造体と外に出すエラー構造体はレイヤーが異なるため型を分けるべきである、みたいな思想があるのであれば一旦従うのもありかなと思いました。

こちらのコメントで納得したのでResolveです！

qryxip · 2023-08-29T18:17:53Z

#589 (comment)への返答でもあるのですが、Rustのpublic APIとしての情報に制限をかけたかったからです。public APIとしてはこのリポジトリの場合、

std::error::Errorとしての機能
エラーの種別

のみを露出するのが適切で、それ以外を隠蔽するようにしたらこの形になりました。

またエラー型に限らずenumの形をnewtypeパターンで隠蔽するのは、少なくともRustにおいては一般的です。今回はthiserrorのドキュメントに載っていた以下の形に従いました。クラス間の「継承」があったり、そもそも型付けが弱い言語だとそもそも選択肢に挙がらないパターンだとは思います。

// PublicError is public, but opaque and easy to keep compatible.
#[derive(Error, Debug)]
#[error(transparent)]
pub struct PublicError(#[from] ErrorRepr);

impl PublicError {
    // Accessors for anything we do want to expose publicly.
}

// Private and free to change across minor version of the crate.
#[derive(Error, Debug)]
enum ErrorRepr {
    ...
}

thiserror - Rust

qryxip · 2023-08-29T18:47:41Z

crates/voicevox_core/src/error.rs

@@ -54,6 +101,49 @@ pub enum Error {
    InvalidWord(InvalidWordError),
 }

+/// エラーの種類。
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+pub enum ErrorKind {


ErrorKindの定義および変換をしてくれるライブラリがあった。
kinded

作者はnutypeやwhatlangを作った人。強制的にDisplay/FromStrが付く点には異議を唱えたいところではあるが、それ以外は筋がよいと感じるし、実装のコードも簡潔。

が、initial commitが28日前、最終更新日が21日前で、総ダウンロード数658…

もうちょっと待ってみて、ライブラリ側やこちら側も安定して来てそうだったら導入を検討しても良さそうなのかなと思いました！

Hiroshiba · 2023-08-30T15:46:27Z

あとこの2点かなと！
#589 (comment)
#589 (comment)

qryxip · 2023-09-03T07:40:12Z

レビュー対応 (Rust APIが公開するエラーの種類をErrorKindとして表現する #589 (comment))
Merge branch 'main' into HEAD
Add: Java APIを作成 #558 に対応 (d6489cc (#589))

Hiroshiba

LGTM！！！

メンテナンス性向上のためのコメントの提案と、素人考えのコメントを書いてみました。

crates/voicevox_core/src/__internal.rs

Co-authored-by: Hiroshiba <[email protected]>

Hiroshiba · 2023-09-03T16:34:41Z

問題ないと思うのでマージします！！

qryxip added 2 commits August 27, 2023 15:08

Rust APIが公開するエラーの種類をErrorKindとして表現する

e5ac602

addし忘れた__internal.rsを追加

de3c38b

qryxip commented Aug 27, 2023

View reviewed changes

qryxip mentioned this pull request Aug 27, 2023

[2月末リリース予定!] v0.16 #545

Open

69 tasks

qryxip commented Aug 27, 2023

View reviewed changes

Hiroshiba reviewed Aug 28, 2023

View reviewed changes

qryxip commented Aug 29, 2023

View reviewed changes

qryxip mentioned this pull request Aug 31, 2023

Rust API内の各アイテムの可視性を引き締める #594

Closed

3 tasks

qryxip added 3 commits September 3, 2023 16:24

FIXMEを追加

e8045e4

Merge branch 'main' into introduce-voicevox-core-errorkind

d4b8b0b

VOICEVOX#558 に対応

d6489cc

qryxip requested a review from Hiroshiba September 3, 2023 07:41

Hiroshiba approved these changes Sep 3, 2023

View reviewed changes

crates/voicevox_core/src/__internal.rs Show resolved Hide resolved

crates/voicevox_core/src/__internal.rs Show resolved Hide resolved

qryxip and others added 2 commits September 3, 2023 23:29

[skip ci] __initernalにコメント追加

58a3558

Co-authored-by: Hiroshiba <[email protected]>

__internalのコメントをdocに

1020465

Hiroshiba merged commit 203a75b into VOICEVOX:main Sep 3, 2023
48 checks passed

qryxip mentioned this pull request Sep 5, 2023

C APIのエラー表示をtracing::error!にする #600

Merged

qryxip added a commit to qryxip/voicevox_core that referenced this pull request Sep 6, 2023

VOICEVOX#589 に追従

1ccd3ad

This was referenced Sep 24, 2023

InvalidStyleId, InvalidModelId, UnknownWordを…NotFoundにする #622

Merged

UnloadedModel → ModelNotFound #623

Merged

エラーメッセージにおけるcontextとsourceを明確に区分する #624

Merged

qryxip mentioned this pull request Oct 12, 2023

Python APIとJava APIのエラーを詳細にする #640

Merged

2 tasks

qryxip mentioned this pull request Oct 30, 2023

Error::InferenceFailedにsourceとしてOrtErrorを持たせる #668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust APIが公開するエラーの種類を`ErrorKind`として表現する #589

Rust APIが公開するエラーの種類を`ErrorKind`として表現する #589

qryxip commented Aug 27, 2023 •

edited

Loading

qryxip left a comment

qryxip Aug 27, 2023

qryxip Aug 27, 2023

qryxip Aug 27, 2023

qryxip Aug 27, 2023 •

edited

Loading

Hiroshiba Aug 28, 2023 •

edited

Loading

qryxip Aug 31, 2023

Hiroshiba Sep 1, 2023 •

edited

Loading

qryxip Sep 3, 2023

qryxip Aug 27, 2023 •

edited

Loading

Hiroshiba Aug 28, 2023

qryxip Aug 31, 2023 •

edited

Loading

Hiroshiba left a comment •

edited

Loading

Hiroshiba commented Aug 28, 2023 •

edited

Loading

Hiroshiba Aug 28, 2023

Hiroshiba Aug 30, 2023

qryxip commented Aug 29, 2023 •

edited

Loading

qryxip Aug 29, 2023 •

edited

Loading

Hiroshiba Aug 30, 2023

Hiroshiba commented Aug 30, 2023

qryxip commented Sep 3, 2023

Hiroshiba left a comment

Hiroshiba commented Sep 3, 2023

Rust APIが公開するエラーの種類をErrorKindとして表現する #589

Rust APIが公開するエラーの種類をErrorKindとして表現する #589

Conversation

qryxip commented Aug 27, 2023 • edited Loading

内容

関連 Issue

その他

qryxip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qryxip Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qryxip Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qryxip Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba left a comment • edited Loading

Choose a reason for hiding this comment

Hiroshiba commented Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qryxip commented Aug 29, 2023 • edited Loading

qryxip Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba commented Aug 30, 2023

qryxip commented Sep 3, 2023

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba commented Sep 3, 2023

Rust APIが公開するエラーの種類を`ErrorKind`として表現する #589

Rust APIが公開するエラーの種類を`ErrorKind`として表現する #589

qryxip commented Aug 27, 2023 •

edited

Loading

qryxip Aug 27, 2023 •

edited

Loading

Hiroshiba Aug 28, 2023 •

edited

Loading

Hiroshiba Sep 1, 2023 •

edited

Loading

qryxip Aug 27, 2023 •

edited

Loading

qryxip Aug 31, 2023 •

edited

Loading

Hiroshiba left a comment •

edited

Loading

Hiroshiba commented Aug 28, 2023 •

edited

Loading

qryxip commented Aug 29, 2023 •

edited

Loading

qryxip Aug 29, 2023 •

edited

Loading