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.

Sign up for GitHub

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

Jump to bottom

Import formatting #1143

Open
y86-dev opened this issue Feb 17, 2025 · 17 comments
Open

Import formatting #1143

y86-dev opened this issue Feb 17, 2025 · 17 comments
Labels
• misc Related to other topics (e.g. CI).

Comments

@y86-dev
Copy link
Member

y86-dev commented Feb 17, 2025

In review the following question came up: How should we format our imports?

Like this:

use crate::alloc::flags;
use crate::container_of;
use crate::page::PAGE_SIZE;
use crate::prelude::*;
use crate::str::CString;
use crate::sync::Arc;
use crate::types::ForeignOwnable;
use crate::types::Opaque;
use core::cell::UnsafeCell;
use core::marker::PhantomData;
use core::ptr::addr_of;
use core::ptr::addr_of_mut;

Or merged?:

use crate::{
    alloc::flags,
    container_of,
    page::PAGE_SIZE,
    prelude::*,
    str::CString,
    sync::Arc,
    types::{ForeignOwnable, Opaque},
};
use core::{
    cell::UnsafeCell,
    marker::PhantomData,
    ptr::{addr_of, addr_of_mut},
};

The merged form is worse for rebasing, but in my opinion easier to parse at a glance.
@ojeda ojeda added the • misc Related to other topics (e.g. CI). label Feb 17, 2025
@kloenk
Copy link
Member

kloenk commented Feb 20, 2025 •
edited
Loading

Also related is the rustfmt option for this imports_granularity
(and my personal opinion is Module, but thats just my opinion)

@Lymkwi
Copy link

Lymkwi commented Feb 20, 2025

Not a technical argument, but speaking from experience, i have observed more Rust codebases in the wild using the merged format. It will also scale better when dozens of structure have to be pulled, and should allow IDEs to collapse the groupings.

On a technical note, it's perfectly understandable to want to keep the granular layout. rustfmt has a lot of options to configure that anyways.

@kloenk
Copy link
Member

kloenk commented Feb 20, 2025

Also group_imports could be a nice thing to add. I very much favor StdExternalCrate there, so that it's easy to see what is imported from where.
Maybe we could even somehow get kernel to be in the Std group as it's kinda our standard library, but that's more nitpicky

@y86-dev
Copy link
Member Author

y86-dev commented Feb 20, 2025

@kloenk

Also related is the rustfmt option for this imports_granularity

Ah good to see that there is an option for that (although it is unstable).

(and my personal opinion is Module, but thats just my opinion)

My personal opinion would be Crate.

@metaspace's opinion probably is Item, if I understood him correctly in the email thread.

Not a technical argument, but speaking from experience, i have observed more Rust codebases in the wild using the merged format. It will also scale better when dozens of structure have to be pulled, and should allow IDEs to collapse the groupings.

On a technical note, it's perfectly understandable to want to keep the granular layout. rustfmt has a lot of options to configure that anyways.

The imports_layout option seems interesting, but if we want to optimize for merge conflicts, I think that the only choice is to use one item per line.

@Lymkwi
Copy link

Lymkwi commented Feb 20, 2025

i ran a bit of grep -Er "use (.*)::{.*;$" ./ on rust-next's rust/ folder, showing that we currently group a lot of imports by Module.

i guess i'll create a patch quickly to add item and fix imports.

@nbdd0121
Copy link
Member

I'd vote for Module. Specifically, different subsystems of kernel crate should be on a separate import line.

@y86-dev
Copy link
Member Author

y86-dev commented Feb 20, 2025

i ran a bit of grep -Er "use (.*)::{.*;$" ./ on rust-next's rust/ folder, showing that we currently group a lot of imports by Module.

i guess i'll create a patch quickly to add item and fix imports.

We haven't yet decided on a convention, so I think it's a bit early to create a patch. But when we find a consensus, we will gladly accept your patch :)

@y86-dev
Copy link
Member Author

y86-dev commented Feb 20, 2025

I'd vote for Module. Specifically, different subsystems of kernel crate should be on a separate import line.

In the long run, we will split the kernel crate into subsystem crates, so we will get that later for free. Do we still want to have Module granularity thereafter?

@Lymkwi
Copy link

Lymkwi commented Feb 20, 2025

In the long run, we will split the kernel crate into subsystem crates, so we will get that later for free. Do we still want to have Module granularity thereafter?

i don't know how code splitting is shaping up, but it is very likely that each subsystem might end up having a dedicated crate, and modules within those. Worst case scenario, if we no longer have enough modules once that happens such that we no longer write import groupings by module, the top-level of the crate would still be considered a module, and individual items from submodules can remain on one line. i doubt we will run into such a situation however.

while i'm at it, what LKML thread are you referring to for the comment about using Item? LKML is a vast soup and the attention i can pay to it is minimal

@y86-dev
Copy link
Member Author

y86-dev commented Feb 20, 2025

In the long run, we will split the kernel crate into subsystem crates, so we will get that later for free. Do we still want to have Module granularity thereafter?

i don't know how code splitting is shaping up, but it is very likely that each subsystem might end up having a dedicated crate, and modules within those. Worst case scenario, if we no longer have enough modules once that happens such that we no longer write import groupings by module, the top-level of the crate would still be considered a module, and individual items from submodules can remain on one line. i doubt we will run into such a situation however.

Yes that could happen. But my answer to Gary was "if the reason for choosing to use Module granularity is to import subsystem-specific items in a single group, then we will already achieve that by splitting the crate".

while i'm at it, what LKML thread are you referring to for the comment about using Item? LKML is a vast soup and the attention i can pay to it is minimal

The one linked at the top of the issue: https://lore.kernel.org/rust-for-linux/dd63fcde-ba4c-4a6e-9bde-1af5af37e91b@proton.me/

@Lymkwi
Copy link

Lymkwi commented Feb 20, 2025 •
edited
Loading

when everyone on the MLs is done picking what they'd like, here's how to implement it:

laying things down flat at the level of modules (without removing current groups) would boil down to adding:

imports_granularity = "Module/Item/Whatever"

to .rustfmt.toml. Then, switch over to a nightly toolchain and add unstable_features=true (because rustfmt 1.78 is too old to have the opt-in command-line argument --unstable-features). When you're done, you'll have modified 57 files. Comment the imports granularity option out and revert to the normal build toolchain.

Now, 57 files is a hell of a lot, and i do not think anybody wants to send a single patch for that, nor a series. i guess it would be wiser to just add the option commented out (because it will cause a ton of warnings and not be used by rustfmt on the current minimum working version of the toolchain) and leave that work to the next poor soul who actually runs make rustfmt. If we do that, the patch is a single line of commented code.

@y86-dev
Copy link
Member Author

y86-dev commented Feb 20, 2025

Now, 57 files is a hell of a lot, and i do not think anybody wants to send a single patch for that, nor a series.

What makes you think that? I think it's going to be fine. Especially since the series is machine-generated it is rather easy to review.

i guess it would be wiser to just add the option commented out (because it will cause a ton of warnings and not be used by rustfmt on the current minimum working version of the toolchain) and leave that work to the next poor soul who actually runs make rustfmt. If we do that, the patch is a single line of commented code.

That doesn't seem like an option to me, as Rust kernel code should always be formatted according to rustfmt.

@ojeda
Copy link
Member

ojeda commented Feb 21, 2025

Especially since the series is machine-generated it is rather easy to review.

Yeah, it should be fine. In the worst case, if a patch is too big and can be machine-generated, it could be exceptionally applied directly, for instance. But probably it is not that big.

The worst part is probably conflicts when backporting, but they are just imports, so it shouldn't be too bad, and perhaps we could backport the reformatting.

But what I would like is that we are really sure about the style we move to -- we should not change this again (if we do).

That doesn't seem like an option to me, as Rust kernel code should always be formatted according to rustfmt.

Yeah, it is likely we would be breaking someone's CI, so it would be best to avoid it if possible.

@Lymkwi
Copy link

Lymkwi commented Feb 21, 2025

What makes you think that? I think it's going to be fine. Especially since the series is machine-generated it is rather easy to review.

Having to deal with the maintainers of all subsystems affected by those 57 files could be a bother, but i have been told that because it's formatting it might not be a problem.

That doesn't seem like an option to me, as Rust kernel code should always be formatted according to rustfmt.

Gotcha.

But what I would like is that we are really sure about the style we move to -- we should not change this again (if we do).

i seem to understand that this discussion is primarily happening on the LKMLs. i have not seen a dedicated thread on the Zulip.

@y86-dev
Copy link
Member Author

y86-dev commented Feb 21, 2025

But what I would like is that we are really sure about the style we move to -- we should not change this again (if we do).

Agreed.

What makes you think that? I think it's going to be fine. Especially since the series is machine-generated it is rather easy to review.

Having to deal with the maintainers of all subsystems affected by those 57 files could be a bother, but i have been told that because it's formatting it might not be a problem.

It won't be an issue.

But what I would like is that we are really sure about the style we move to -- we should not change this again (if we do).

i seem to understand that this discussion is primarily happening on the LKMLs. i have not seen a dedicated thread on the Zulip.

The only people that have been discussing this on the LKML (AFAIK) have been Andreas and me. I created this issue to track it somewhere and not lose sight of it. If you want, you can also open a zulip topic about it.

@Lymkwi
Copy link

Lymkwi commented Feb 21, 2025

If you want, you can also open a zulip topic about it.

i would argue it's not my call to make but if this is beneficial to getting input from other people and nobody else wants to do it, i can (assuming i am allowed to create a topic in the General section; i assume anyone can?)

@metaspace
Copy link
Collaborator

metaspace commented Feb 21, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
• misc Related to other topics (e.g. CI).
Milestone
No milestone
Development

No branches or pull requests
6 participants
@metaspace @ojeda @nbdd0121 @Lymkwi @kloenk @y86-dev