rustfmt

rustfmt copied to clipboard

TLDR, for a given config:

group_imports = [
    ["$std::*", "proc_macro::*"],
    ["*"],
    ["my_crate::*", "crate::*::xyz"],
    ["$crate::*"],
]

The following order would be set:

use proc_macro::Span;
use std::rc::Rc;

use rand;

use crate::abc::xyz;
use my_crate::a::B;
use my_crate::A;

use self::X;
use super::Y;
use crate::Z;

This PR also contains three auxiliary patches for proc-macro #[config_type]:

• 0a462a0 allows one to use #[config_type(skip_derive(Debug, ...))], also impl Copy removed from most of types where unneeded;
• 1d3e43e is for deriving serde::{Serialize, Deserialize};
• b195129 impl Display for singled-fielded enums.

These are not really mandatory but simplify the final commit itself. Probably the proc macro could be refactored further but I think it shall be enough for now.

Implementation notes:

• Thanks @dbr for the proposed syntax, the difference is usage of arrays instead of tuples as toml doesn't support these https://github.com/toml-lang/toml/issues/219;
• toml does not support fielded enums at all, that's why manual impl Deserialize for GroupImportsTactic is required (https://github.com/toml-rs/toml-rs/issues/302 https://github.com/toml-rs/toml/issues/364);
• don't really want to introduce an additional crate for wildcards so regex is basically used with a hack: escape(user_pattern).replace("\\*", ".*"), performance should not be concern here anyway;
• group_imports function (i.e for StdExternalCrate) could be re-implemented with group_imports_custom but I would rather prefer leaving it separate;
• $crate alias doesn't mean usage of macro-specific $crate import.

Resolves #4693, resolves #4788, resolves #5550

Hint: one may build a group for all crates within the workspace with cargo-metadata:

WORKSPACE_INCLUDES=$(cargo metadata --offline --format-version 1 | jq -r '.workspace_members[]' | awk '{ print $1 }' | sed 's|-|_|' | sed 's|^\(.*\)$|"\1::\*",|' | tr '\n' ' ' | sed 's|, $||')
echo "[${WORKSPACE_INCLUDES}]"

which for example for Rocket produces this:

["rocket::*", "rocket_codegen::*", "rocket_http::*", "rocket_db_pools_codegen::*", "rocket_db_pools::*", "rocket_sync_db_pools_codegen::*", "rocket_sync_db_pools::*", "rocket_dyn_templates::*", "rocket_guide_tests::*"]

Wow, this is exactly what we need! Is there any updates about this?)

@l4l I think you need to rebase your PR to get the tests working.

Found a failing test case:

conf:

group_imports = [
    ["$std::*", "proc_macro::*"],
    ["*"],
    ["workspace*::*"],
    ["$crate::*"],
]

Input:

use std::fmt;

use serde::de;

use workspace_p1::m1;
use workspace_p2::m2;
use workspace_p3 as p3;
use workspace_p4 as p4;

Expected:

use std::fmt;

use serde::de;

use workspace_p1::m1;
use workspace_p2::m2;
use {workspace_p3 as p3, workspace_p4 as p4};

Actual:

use std::fmt;

use serde::de;
use {workspace_p3 as p3, workspace_p4 as p4};

use workspace_p1::m1;
use workspace_p2::m2;

Will probably be useful to add a $pub which covers pub(crate) & pub uses.

@utkarshgupta137 Thanks for continuing to help review this and for checking up on various test cases.

Will probably be useful to add a $pub which covers pub(crate) & pub uses.

I don't want to extend the scope of this PR. We can consider adding that feature in a follow up PR

For me it works as expected, it's not expected to match xyz with xyz::* (only with xyz or xyz*). However the case for the following conf:

group_imports = [
    ["$std::*", "proc_macro::*"],
    ["*"],
    ["workspace*"],
]

works indeed in a quite unexpected way (there's the same result). There's no simple way to split includes, reorder and then somehow "preserve" original granularity. I don't think this behavior need to be changed. The actual problem here is missing import_granularity (e.g Crate) option which places all of the workspace_p* modules in the third block.

Is there any chance to get this reviewed and merged? Seems like this has been stuck in limbo for no good reason when it's an excellent feature a lot of us are waiting for 😃

Experience report: I tried using this option in a decently-sized personal project (~18 kLoC) with the following config:

# ...
group_imports = [["*"], ["$crate::*"]]
# ...

I only ran into one surprising "issue": local module imports were grouped with external crate imports:

  mod register;
  mod view;

  use glam::{const_vec3, Vec2, Vec3};
  use mountain_river::high_scores::{HighScore, HighScores, HIGH_SCORES_PER_GROUP};
+ pub use register::RegisterHighScore;
  use std::{array, borrow::Cow};
  use triplicata::{
      // ...
  };
-
- pub use register::RegisterHighScore;
  pub use view::ViewHighScores;

Since this feature works by matching on import paths, this doesn't really seem like a bug. This was easily fixed by prepending self:: to both local imports.

Also, note that reexports were moved into a group containing normal imports. This is slightly unfortunate, but I believe that this is an acceptable price to pay for consistency.

Otherwise, everything seems to have gone quite smoothly!

It seems this PR got forgotten, so kind reminder @ytmimi @calebcartwright

If anything else you needed from me or have any concerns feel free to ping. If it's better to split-out the proc-macro part that's also not a problem.

@l4l thanks for reaching out! I wouldn't say this has been forgotten, it's just been put on the back burner while we tend to some higher priority items. We spent a lot of time getting ready for let-else support, and we just recently released some experimental support for let-chains. The next big ticket item for rustfmt is implementing style_edition to give the project more flexibility moving forward. I hope you understand and I appreciate your patience on this.

In the interim, maybe you can add additional tests to see how this new option interacts with other import options like imports_granularity. If there are any edge cases you can think of having good tests for those would also be helpful. While Caleb and I can't spend time reviewing this right now it doesn't mean that this PR has to go unreviewed. Thank you to everyone who's already chimed in and shared their thoughts. I'd sure welcome reviews from anyone who's interested in seeing this land continue to provide feedback on how this PR could be improved or how the implementation could be simplified. It would definitely be a great help for when Caleb and I are able to revisit this.

I'd also like to add that I understand this is important to you and many others, and I personally think it looks like a cool feature!

Some random drive-by notes:

1. It would be a nice convenience if a string is interchangeable with a single-item list in the config. That is, group_imports = ["$std", "*"] would act the same as group_imports = [["$std"], ["*"]]
2. I think regex may be better to use here rather than wildcards because it allows e.g. myproject_(macros|cli)::.* or rustc_.*. In which case, I think it is better to suggest using .* directly rather than doing replace("*", ".*"), which would turn .* in to ..*
3. How is the fallback supposed to work? In the opening example:

group_imports = [
    ["$std::*", "proc_macro::*"],
    ["*"],
    ["my_crate::*", "crate::*::xyz"],
    ["$crate::*"],
]

I haven't looked at the algorithm but I assume it's trying to match from most specific to most general somehow. It would be more straightforward IMO to always match first to last, and then add a meta $other or $fallback matcher where everything goes if it isn't picked up by another group. E.g. the above could be written as the following (including my other suggestions):

group_imports = [
    ["$std::.*", "proc_macro::.*"],
    "$other",
    ["my_crate::.*", "crate::.*::xyz"],
    ["$crate::.*"],
]

Very useful PR!

It's possible to use globset as a frontend to regex engine.

It supports "or" globs (myproject_{macros,cli}::*), cc @tgross35.