contributing: how to make tools apply the desired grouping of imports

[japaric]

the convention adopted in the project is not the default behavior of rust-analyzer nor rustfmt. this PR documents how to make those tools apply the desired grouping of imports.

[djc]

Thanks!

[codecov[bot]]

Codecov Report

Merging #1529 (eca015d) into main (7edbfb9) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1529   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files          74       74           
  Lines       15113    15113           
=======================================
  Hits        14579    14579           
  Misses        534      534           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[japaric]

after some local testing I noticed that both tools treat module imports as if they were type imports so this

use crate::client::handy;
use crate::client::{ClientConfig, ResolvesClientCert};

becomes

use crate::client::{handy, ClientConfig, ResolvesClientCert};

and, similarly, this

use crate::sign;
use crate::verify;
use crate::versions;

becomes

use crate::{sign, verify, versions};

so neither tool can't automate / enforce the existing convention :-(

[djc]

We don't prescribe the separation of module and type/trait symbols in the style guide, right? I think putting those in the same group is fine.

[ctz]

I don't think where we ended up on import ordering has much deep reasoning behind it, and I think I'd support changing it if the reasons justify that. "The ecosystem tooling doesn't do that by default" seems like a good reason?

Naturally choosing the time to do this is key: when we don't have any big branches outstanding (hah!). I would suggest just before the first 0.22 release, after all features for that are landed.

[japaric]

after doing more local testing (see #1524 ) an empty rustfmt.toml changes the default settings of rustfmt (which I found bewildering). to apply just the group_imports = "StdExternalCrate" setting I had to use this rustfmt.toml file:

array_width = 60
attr_fn_like_width = 70
chain_width = 40
disable_all_formatting = false
edition = "2015"
fn_call_width = 60
fn_params_layout = "Tall"
force_explicit_abi = true
group_imports = "StdExternalCrate" # the only non-default value!
hard_tabs = false
match_arm_leading_pipes = "Never"
match_block_trailing_comma = false
max_width = 100
merge_derives = true
newline_style = "Auto"
remove_nested_parens = true
reorder_imports = true
reorder_modules = true
short_array_element_width_threshold = 10
single_line_if_else_max_width = 50
single_line_let_else_max_width = 50
struct_lit_width = 18
struct_variant_width = 35
tab_spaces = 4
use_field_init_shorthand = false
use_small_heuristics = "Default"
use_try_shorthand = false

so if y'all want to use non-default formatting settings I'd suggest committing such a rustfmt.toml file . personally, I think sticking to rustfmt's and rust-analyzer's defaults is the least friction for new contributors and the easiest thing to enforce in CI (as these rustfmt settings require nightly) .

[djc]

I think we'd prefer to stick to the default rustfmt config, but it would be nice to use rustfmt to globally clean up import style/order.

[cpu]

Here's an attempt to summarize the state of things as I understand it:

1. Our documented grouping is non-standard.
2. ~The described change to CONTRIBUTING.md in this PR doesn't work like we'd hope, you need to fill out all the defaults in a rustfmt.toml in addition to the one customized group_imports setting.~ Using group_imports in .rustfmt.toml also requires nightly tooling to use.
3. We're not especially tied to the import grouping we're using today.
4. We'd generally prefer to use the default rustfmt settings.
5. If we do a codebase wide re-arranging, it should happen after the 0.22 release.

If that summary is accurate then I propose we close this PR and file an issue for doing a reformat after 0.22 to use default grouping, along with updating the CONTRIBUTING.md to reflect the new grouping preferences.

WDYT?

[japaric]

you need to fill out all the defaults in a rustfmt.toml in addition to the one customized group_imports setting

minor point here: I totally missed the .rustfmt.toml in the root of the repository. It's sufficient to add the custom group_imports settings there; it's not necessary to create a new rustfmt.toml pre-filled with a bunch of other settings. (One still needs a nightly toolchain to use that group_imports setting.)

[cpu]

minor point here: I totally missed the .rustfmt.toml in the root of the repository. It's sufficient to add the custom group_imports settings there

Ahh! Ok thanks, I missed that too. I will edit my earlier comment.

One still needs a nightly toolchain to use that group_imports setting.

:+1:

[ctz]

We'd generally prefer to use the default rustfmt settings.

I think the rustfmt defaults are to leave imports alone? I think I'd prefer to have rustfmt manage the order and grouping of imports, so there is less thought and review capacity needed when it comes to formatting.

FWIW I just reviewed the result of adding:

group_imports="StdExternalCrate"
imports_granularity="Module"

to our .rustfmt.toml, and it looks pretty tidy to me. It does mean some warnings when running stable cargo fmt, but that does not undo the formatting.

[cpu]

I think I'd prefer to have rustfmt manage the order and grouping of imports, so there is less thought and review capacity needed when it comes to formatting.

Agreed :+1:

It does mean some warnings when running stable cargo fmt, but that does not undo the formatting.

Ah interesting! That doesn't sound particularly onerous then.

[djc]

SGTM.

[japaric]

I think I'd prefer to have rustfmt manage the order and grouping of imports, so there is less thought and review capacity needed when it comes to formatting.

you mean adding those group_imports and imports_granularity settings to .rustfmt.toml and having CI run cargo +nightly-2023-10-19 fmt -- --check?

I think that could increase friction in two ways: people that run cargo fmt manually will get a warning if they use a stable toolchain ("rustfmt: these settings are unstable") and people that let their editors run cargo fmt for them won't notice the warning (e.g. emacs and vs-code won't show formatting warnings) and then they may find that CI rejects their PR due to formatting errors despite the code being correctly formatted by a stable rustfmt.

I think the ideal scenario would be formatting the code without contributor or reviewer intervention. Ideally, this would be an extra commit that bors / GHA adds to a PR prior to running CI and merging it; or, alternatively, it could be some post merge action that pushes a formatting commit to the main branch. however, I'm not aware of any CI configuration that does either.

P.S. I'm always wary of using nightly tools / features as they can create issues irrelevant of code changes. e.g. a nightly release may not include rustfmt / clippy if the build of that tool happened to fail in rust-lang CI (though there's a workaround for that), or a tool decided to change the behavior of an unstable feature, which is totally fair since the feature is unstable (though this seems unlikely for these particular rustfmt features)

[cpu]

alternatively, it could be some post merge action that pushes a formatting commit to the main branch.

That approach makes me a little bit nervous because it implies CI has write permission to the repo and I think we usually scope it to read-only

[ctz]

you mean adding those group_imports and imports_granularity settings to .rustfmt.toml and having CI run cargo +nightly-2023-10-19 fmt -- --check?

The first, but not the second -- CI would continue to use stable rustfmt to validate future PRs. I'm not sure I want to be exposed to rustfmt nightly regressions on an ongoing basis.

The short term goal would be to get to a consistent baseline -- which seems to me to be a net benefit compared to today, where we have a mix of styles. That naturally has its own issues:

• main would likely regress
• people running nightly rustfmt would have those regressions fixed in unrelated changes; this would demand a bit of commit hygiene.

But hopefully that would be short-term once those options become stabilised.

I don't think I want CI to have commit access, as it massively changes the level of trust we need to have in it. The other option is to have a standard pre-commit hook people can use, but that again raises the work for first-time contributors.

[cpu]

That plan sounds reasonable to me

[ctz]

FWIW I just reviewed the result of adding:

This is #1577

[cpu]

Closing in favour of https://github.com/rustls/rustls/pull/1577