Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

group_imports=StdExternalCrateでRustfmtをかける #466

Closed
3 tasks done
qryxip opened this issue Apr 19, 2023 · 26 comments
Closed
3 tasks done

group_imports=StdExternalCrateでRustfmtをかける #466

qryxip opened this issue Apr 19, 2023 · 26 comments

Comments

@qryxip
Copy link
Member

qryxip commented Apr 19, 2023

内容

--config group_imports=StdExternalCrateでRustfmtをかけます。

#237と類似したものです。現在このリポジトリのuseのスタイルは、qwertyさんが書いたのとSuitCaseさんが書いたのと私が書いたので混沌とした状態になっています。これを一つに統一します。

StdExternalCrateである理由は、Rust Analyzerが(現在のところ)アイテムの自動インポートを行うときにそのようなスタイルで追加してくるからです。

ra_import

Pros 良くなる点

  • このリポジトリにPRを出す人が混乱しないでよくなる
    現在このリポジトリには統一されたルールなんてものは無いわけですが、初めて来た人にとってはそのことは自明ではなく、ほんのちょっとだけ混乱する要素になるかなと思っています。

Cons 悪くなる点

実現方法

  1. cargo +nightly fmt -- --config group_imports=StdExternalCrate
  2. (未定) ActionsでNightly Rustを管理するようにし、Actions上でgroup_imports=StdExternalCrateをチェックするようにする

VOICEVOXのバージョン

N/A

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@Hiroshiba
Copy link
Member

importの順番が一意になる感じですよね。
コードの統一感は見通しの良さに繋がっていろんな恩恵があるので割と導入したいなと思いました!

基幹部分にnightly使うのはだいぶ危うそうですが、formatterだけならありだと思います。
cargo +nightlyはnightlyなRustのインストールが必要な感じでしょうか…?

あるいは同等のことをするサードパーティライブラリがあるなら、そちらに依存するのもありかもです。
実際エディタやエンジンはESLintやisortなので。

@qryxip
Copy link
Member Author

qryxip commented Apr 19, 2023

cargo +nightlyはnightlyなRustのインストールが必要な感じでしょうか…?

はい。必要ですね...

@qryxip
Copy link
Member Author

qryxip commented Apr 19, 2023

あるいは同等のことをするサードパーティライブラリがあるなら、そちらに依存するのもありかもです。

Rustfmtくらいしか多分無いですね...

Rustfmtではないフォーマッタとしてはprettypleaseというものがあるのですが、これは機械的に生成された数千数万行のコードを破綻無くフォーマットするための用途で用いられており(例えば我々がonnxruntime-rsやopen_jtalk-rsで使っているbindgen)、group_importsにあたる設定は無かったんじゃないかと。

@Hiroshiba
Copy link
Member

なるほどです。
公式ライブラリに含まれてるとリリースまで時間がかかるという欠点もあるんですねー

流石にnightlyは難しいかなと思いました!
たまにその設定でformatかけてあげる、とかですかねぇ。。

@qryxip
Copy link
Member Author

qryxip commented Apr 19, 2023

nightly入れるとしたら、nightly-2023-04-20のような形で入れておきupdate_rust_toolchainワークフローによるstableの更新のときにnightlyも更新するといったことを考えてました。もちろんlintやbuildはstableのままで。

参考までに、サニタイザとかcargo-udepsとかの他のnightly依存のツールも避ける方針でしょうか? メリット次第?

@qryxip
Copy link
Member Author

qryxip commented Apr 19, 2023

流石にnightlyは難しいかなと思いました!

単純にここら辺のコスト感覚を共有しておきたいなと思いました。
私としては、手元で作業する我々は普通にstableのRustfmtを使っておいて、CIではnightlyのRustfmtでチェックするという感じで考えています (VSCodeとかの設定でRustfmtだけnightlyのを使うようにするというのも一応できはするんですけど)。

@qryxip
Copy link
Member Author

qryxip commented Apr 19, 2023

あと「nightlyなRustのインストールが必要」の時点で、もしかしたらですがコストについて齟齬が発生していそうな予感もしています。

私が考えているのはCIでのみnightlyにする方針ですが、仮に手元でやりたくなったときも

rustup install nightly

とだけ実行すれば60MiBくらいのダウンロードの後Nightly Rustが使えるようになります。pyenvやnvsにあたるものは標準搭載です。

@Hiroshiba
Copy link
Member

・コード実行するRustがnightlyになるのは避けたい
・フォーマットだけnightlyになるのはOK
・CIがnightlyもOK
って感じです。

ただ、開発者にとって「フォーマッターがないのにCIが落ちる」のはストレスになりそうです。
rustupをわかってないのですが、+nightlyを書いたときだけnightlyが使われる感じでしょうか?

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

はい、そうです

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 20, 2023

なるほどです!

nightlyは相当不安定なことがあり得るのでできれば避けたいかもです。(できればです)
フォーマット用の仕様が入ったり消えたりするのも少ししんどそうです。
betaに入ってるならbetaが良さそうですが、どうでしょう。

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

開発者のストレスについてですが、Reviewdogから言われる形であれば軽減できるかもしれません

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 20, 2023

どこを修正すればよいかわかるから、でしょうか?
個人的に気になってるのは「フォーマッターがないのにCIが落ちる」点でした。
cargo +beta fmt -- --config group_imports=StdExternalCrateでフォーマットしてください」と案内できるならまあ良いかなーと

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

案内というより、reviewdogならsuggestionまで出せるんじゃないかと思っています。
https://github.com/reviewdog/reviewdog#code-suggestions

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

betaに入ってるならbetaが良さそうですが、どうでしょう。

あ、betaには入っていないですね...状況はこんな感じです。
rust-lang/rustfmt#5083

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 20, 2023

なるほどです、suggestionまで出せるなら結構ありかもと思いました!!
betaに入ってないのもなるほどです。

CIに入れてreviewdogでsuggestion出してもらう、という体験をしてみたいです・・・!!!
(順番入れ替えだと結構広範囲になるはずなので、たぶんsuggestion出ないんじゃないかなーとちょっと思ってます)

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

あーsuggestion対象は選べても、そのsuggest内容は厳しいですね...10行離れてたりすると...

あとそれ以前の致命的な問題に気付いてしまいました。RustfmtのStdExternalCrate、Rust Analyzer式のものとは別物です。Rust Analyzer式に従うとこういう順序になるのですが、

struct A;

mod sub {
    struct B;

    mod sub {
        use std::cmp::Reverse;

        use itertools::Itertools;

        use crate::A;

        use self::sub::C;

        use super::B;

        mod sub {
            pub(super) struct C;
        }
    }
}

StdExternalCrateだとself::super::crate::が一緒にまとめられてしまいます...

        use std::cmp::Reverse;

        use itertools::Itertools;

        use self::sub::C;
        use super::B;
        use crate::A;

@qryxip
Copy link
Member Author

qryxip commented Apr 20, 2023

まあこのissueの主題とは別にClippyとかactionlintの方でreviewdog使うというのはありなのではと思ってきました。

sksat/action-clippyというのを用意している方がブログにこんなことを書かれていますし、このリポジトリにPRしに来てくれるRust初心者の体験をマシにできるんじゃないかなーと思っています。

ちなみに,clippy, clang-tidy の出力を reviewdog に喰わせて PR comment とかに流すための GitHub Action を作ってある. reviewdog は本当によい文化だと思う. CI がコケた時には赤いバツマークと共に status check failed とだけ表示される以上の情報が出てくるべきだ.

ArkEdge Space で正社員になって半年が経った - 重力に縋るな

@Hiroshiba
Copy link
Member

あ~なるほどです。
Rust Analyzerとずれるとなると、+nightlyなfmtをかけたコードを、通常のRust toolchainで見るとエラーやwarningになる感じでしょうか。
ちょっと厳しそうに思いました。nightlyが明けて正式導入されたらかなぁと・・・。

それはそれとして、reviewdogは興味があります!!
もしgroup_imports=StdExternalCrateが難しそうであれば、いったんこちらのissueは閉じて、reviewdogを導入していくって感じですかね。

@qryxip
Copy link
Member Author

qryxip commented Apr 21, 2023

Rust Analyzerとずれるとなると、+nightlyなfmtをかけたコードを、通常のRust toolchainで見るとエラーやwarningになる感じでしょうか。 ちょっと厳しそうに思いました。nightlyが明けて正式導入されたらかなぁと・・・。

いえ、Rust Analyzer式で揃えた筈のuseStdExternalCrateに弾かれるだけです。Rust Analyzer自体及びstableのRustfmtは何も言いません。

それはそれとして、reviewdogは興味があります!! もしgroup_imports=StdExternalCrateが難しそうであれば、いったんこちらのissueは閉じて、reviewdogを導入していくって感じですかね。

まあuseが現状混沌としているのは事実なので、手動でRust Analyzer式(機械によるフォーマットが現状不可)にしようかと。

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 21, 2023

なるほどです!!

nightlyのフォーマッターを導入するかの判断について優先順位が高い順に整理していくと

  • Rustに詳しくない開発者がストレスを感じるのは避けたい
    • フォーマット方法は必ず案内
    • nightly Rustの挙動が変でCIが落ちるのは避けたい
  • Rustに詳しい開発者がストレスを感じるのは避けたい
    • importのフォーマッターは導入したい
  • レビュワーがストレスを感じるのは避けたい
    • これはフォーマッターが導入されたにせよされなかったにせよほぼ変わらない

ということかなと。従うと

  • READMEでフォーマット方法を案内
    • nightlyのインストールとcargo +nightly fmt -- --config group_imports=StdExternalCrateを案内
  • CIの導入はどちらでも
    • nightly Rustが狂うこともあると思うのでCIは一長一短
    • errorレベルではなくwarnやinfoレベルならOK
  • 今のコードにgroup_imports=StdExternalCrateフォーマットする

ですかね!

@Hiroshiba
Copy link
Member

あ。

手動でRust Analyzer式

という提案頂いてたの見逃しました。
↑の流れならgroup_imports=StdExternalCrateのフォーマッターありかなと思いましたが、ちょっと良くない感じですか。

@qryxip
Copy link
Member Author

qryxip commented Apr 21, 2023

「Rust Analyzer式」のと互換性が無いですね...まとめるとこんな感じです。

group imports フォーマッタ Preserveの部分集合? StdExternalCrateの部分集合? Rust Analyzesr式の部分集合?
Preserve
(stableを含むRustfmtの既定の設定)
N/A (既定設定のRustfmtはすべてを受理し、何も行わない) ✔️ ✔️
StdExternalCrate
(nightly限定)
nightlyのRustfmt
Rust Analyzer式 存在しない

@qryxip
Copy link
Member Author

qryxip commented Apr 21, 2023

あとRust Analyzer式group importsですが、rust-lang/rust-analzyerのPR/issueコメント以外にドキュメントは多分存在しないですね...

Rust Analyzerは婉曲的に表現するとかなりフットワークが軽いので、しれっとデフォルトの挙動が変更されるかもしれません。
週一でリリースされるRust AnalyzerのChangelog
("drop support for …"という字面が"New Features"の方に含まれていたりします。多分"New Features"と"Fixes"と"Internal Improvements"の三つしか無いのでは?)

なのでRust Analyzer式で一度だけフォーマットし、CIの設定やドキュメントの記述はせずに、以後しばらくの間は(qwertyさんやSuitCaseさんも含めて)PRに対する突っ込みは一切入れない、というのが丸いのかなと今は思っています...

@Hiroshiba
Copy link
Member

なるほどです!
良い感じの一意なimport用フォーマッターが誰でも浸かる形でリリースされると嬉しそうだなーと思いました。

一旦closeで良さそうでしょうか?

@qryxip
Copy link
Member Author

qryxip commented Apr 21, 2023

まあ一旦closeしてもよさそうですね。

@qryxip qryxip closed this as completed Apr 21, 2023
@qryxip qryxip closed this as not planned Won't fix, can't repro, duplicate, stale Apr 21, 2023
@qryxip
Copy link
Member Author

qryxip commented Apr 21, 2023

まあRustのuseのスタイルは、できる表現が2015年から段々増えていったという事情もあって慣習の統一の望みは薄いですね.. 極端な話こんな書き方も今はできるわけで。

// Rust Analyzerが「なんだこれ」と思ってしまうのか、無視して別のgroupを作り始めてしまうという欠点はある
use {
    self::sub::C,
    super::B,
    crate::A,
    ::{
        anyhow::{bail, Context as _},
        itertools::Itertools as _,
        octocrab::{
            models::{
                repos::{Asset, Release},
                AssetId,
            },
            Octocrab,
        },
        std::{
            borrow::Cow,
            env,
            future::Future,
            path::{Path, PathBuf},
            sync::Arc,
            time::Duration,
        },
    },
};

Rustfmtが強権をふるえば話は別でしょうけど、version 1を宣言したRustfmtがデフォルトの挙動を変更できるはずがなく...

Rust Analyzerが"Reorder import groups"みたいなcode actionを用意すれば統一のきっかけにはなるかもしれない、という程度ですね。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants