Pre-RFC: module style enforcement lint

language design
1

Motivation

I wasn't a big fan of the change in RFC 2126 that made mod.rs files optional (allowing foo.rs and foo/) as I think the problem it aimed to solve was relatively trivial compared to the new problems it created (especially now that IDE plugins like IntelliJ-Rust have a "convert module to directory" intention).

That is, mainly, jumping back and forth between a module file and its submodules (which is pretty common since they should contain related code) now usually means scanning the list of files in the parent directory (since nearly every file manager and every IDE sorts folders above files). Because of this same issue, you cannot tell at a glance which modules have submodules or not as you have to perform a visual diff between the files and folders in a given directory.

I'm also just a curmudgeon who avoids change when I can (heck, I still use old Reddit).

However, I can get used to the new module style; the real issue is that while RFC 2126 promised lints to cover the other changes, this part seems to have been left by the wayside, and so there's no way to enforce consistency by default. This results in projects having to enforce this either procedurally in code review or by cobbling together a script to enforce their chosen module style in CI.

For proprietary projects in a rapid prototyping phase, this kind of thing is easily forgotten and so you can sometimes end up with messy projects that mix and match both mod.rs files and RFC 2126's style (which I will hereafter refer to as "2015 edition style" and "2018 edition style", respectively; while the new style didn't require 2018 edition, the two are strongly associated), which is honestly the worst of both worlds.

Proposal

Henceforth, I'd like to propose three related lints for inclusion directly in rustc (as Clippy is easy to forget to run and thus runs into the same procedural issues in projects that are moving quickly). The first two are pretty straightforward:

  • edition_2015_module_style, allowed by default, which lints against mod.rs files in the crate/current module tree
  • edition_2018_module_style, allowed by default, which lints against having both foo.rs and foo/ in same

The names are open for bikeshedding, although I chose these names because the way lints work, users will want to #![deny(edition_2015_module_style)] at the crate root to enforce the 2018 module style and vice-versa. These lints will be cargo fixable.

The third lint, mixed_module_style, is warn by default and fires if the crate mixes and matches both 2015 and 2018 styles; it will not be cargo fixable but instead recommend in help messages that the user choose either #![deny(edition_2015_module_style)] or #![deny(edition_2018_module_style)] to place at the crate root.

Alternatives

Single-Lint Solution

For simplicity of implementation, I figured the mixed_module_style lint could just visit the whole module tree once, determining the style for each module and then generating a lint warning on an edge trigger (e.g. the last module it saw used 2018 style but the current module uses 2015 style), however that might produce false-positives from the user's perspective as it would generate two warnings if it saw three modules with 2018, 2015, 2018 styles even though the middle module is the only odd one out.

Instead, the mixed_module_style lint could pull double duty and visit the whole crate up front to determine the style used by the majority of the modules, and then generate lint warnings for any that don't follow the majority. This of course means visiting the whole crate twice, but since it's only looking at modules this is likely a non-issue for compilation times.

Then, mixed_module_style could be cargo fixable and the user wouldn't have to manually pick a style to enforce.

The only issue is if there's a tie; the choice here should probably not be arbitrary but it's also not obvious what it should be; maybe whichever style it saw first, or maybe the edition the crate was compiled with.

Rustfmt Formatting Rule

This could instead be an option in rustfmt.toml:

module_style = "<2015 | 2018>"

This is also something most users would probably want to configure once workspace-wide rather than at the root of every crate in their project, which may be another reason to put this in rustfmt instead.

However, I don't think rustfmt currently supports any formatting rules that require moving/renaming files so this may require some hacking to implement, compared to the proposed lints which I already have a vague mental plan for implementing. It also somewhat runs into the same procedural issues as mentioned above, though it has the benefit of IDE support for being run automatically on save which wouldn't require any user intervention.

Although an issue with rustfmt doing this automatically is that it may not check the renamed/moved file into version control which means the user may accidentally commit broken code without intending to. I think though that this could be fixed/papered-over in, e.g. IntelliJRust as it just has to be aware that rustfmt can move files around now and to automatically check-in any that got moved with IDEA's Git integration.

Meanwhile, a solution like a #[deny(...)]'d lint would force user intervention and also remind them which module style the project is using so that they can remember to adhere to it.

Unresolved Questions

Is there any good reason to allow configuring these lints anywhere but the crate root? It's kind of antithetical to this proposal to choose one style for one module subtree and a different style for another. (Just thought of one: generated code.)

11 Likes
2

Personally I feel like a lint like this, since it is about style more than anything else, definitely belongs in clippy, not rustc.

I also feel that tying the lint names to the editions doesn’t make much sense, and it would be better to pick names that are more descriptive, and, as such, doesn’t require knowledge of the history (which would be even less relevant in the future).

But I should probably mention I don’t experience this as a problem, since module introductions aren’t all that common.

8 Likes
3

Personally I feel like a lint like this, since it is about style more than anything else, definitely belongs in clippy, not rustc.

It was already discussed that this should be a lint in rustc itself during the stabilization of the module style change: https://github.com/rust-lang/rust/issues/53125#issuecomment-411174227

The intention is to primarily point people to foo.rs . We want to keep foo/mod.rs for compatibility, so this change doesn't break existing projects. And we can provide appropriate lints (for people who want to write idiomatic Rust 2018 code) that encourage renaming the files accordingly, to help migrate the ecosystem.

(Emphasis mine; I'm interpreting this "we" to mean "the lang team of rustc"; Clippy is not mentioned anywhere in this discussion.)

I also feel that tying the lint names to the editions doesn’t make much sense

The other changes from RFC 2126, namely the absolute/relative path changes and making extern crate optional, are already part of the rust_2018_compatibility and rust_2018_idioms lint groups, respectively. I would bundle this under the latter. Also see the above quote; associating the new module style with the 2018 edition was intended from the start.

But I should probably mention I don’t experience this as a problem, since module introductions aren’t all that common.

To be fair, that's not really a good counterargument to introducing a new lint. Certainly a lot of lints rustc and/or Clippy have don't trigger for experienced users; you just don't write code that trigger them because you know better.

In my experience this issue happens when you have developers switching back and forth between various projects that use both styles and lose track of which one a project is using. Ideally you'd just pick one and stick with it but again, enforcing things procedurally is hard to keep consistent, not everyone has time to do sweeping style refactors of a dozen different projects (especially with no decent way to automate it), and if Rust is about one thing it's about building things most systems projects try to handle procedurally into the compiler itself.

4

mixed_module_style lint makes a lot of sense. You'll likely find people who have strong opinions on which style is better, but mixing both is definitely messy.

8 Likes
6

Are you talking about the single lint solution or mixed_module_style combined with the lints to enforce a specific module style?

7

I do think having a lint for mixed style seems reasonable.

1 Like
8

Any input on my ideas as to how it might work? (See Alternatives - Single Lint Solution)

What about the lints to enforce a specific style?

9

I would suggest hooking into module import, into the code that finds a module, and recording which style was seen. Then, if you see the other style, emit a warning at that point.

The lints to enforce a specific style (which should be mutually exclusive) could then change that warning from "warn if seeing a different style than the first one seen" to "warn if seeing a style different than the one specified with the lint".

10

I'm not sure I follow. What is the "other style" we're comparing to? The style of the module's parent? What of a situation like this, where subtrees of the modules are internally consistent, but siblings at the crate's root are not consistent?

src/
  - a/
    - aa/
        aaa.rs
      aa.rs

  - b/
    - ba/
        mod.rs
        baa.rs
     mod.rs
     bb.rs

   lib.rs
   a.rs
11

I would imagine that the import order of a and b would set expectations and warn on the second one imported.

12

Right, that's what I was talking about here under "Alternatives / Single Lint Solution":

13

especially now that IDE plugins like IntelliJ-Rust have a "convert module to directory" intention

Historical note: now was quite a bit ago, before 2018 edition.

In addition to the lint, I'd personally would like to see a clear statement from lang/libs team which style is recommended (or a statement that neither is recommended, and that they are both exactly equivalent).

5 Likes
14

Personally, I actually do mix the two styles, though typically not within a single crate.

A "directory" module -- that is, one with just $($vis)? mod submodule; and optionally a pub use self::{}; -- is always mod.rs. This is the kind of module that wouldn't exist in a model with implicit file discovery (which I still agree is the wrong choice for Rust, due primarily to the convenience for conditionally included platform dependent implementation modules).

A "leaf" submodule -- that is, one with no mod submodule;, though potentially inline submodules -- is always foo.rs, even though mod.rs is theoretically allowed. This typically isn't what people talk about when mixing foo/mod.rs and foo.rs styles.

Where the choice comes in is "implementation" modules -- those with some nontrivial amount of implementation in it, which also have helper submodules in separate files. My general approach recently has been that if the module entirely encapsulates its submodules -- that is, no pub mod, and does include some implementation code in this file -- then it should be foo.rs. If any submodule is publicly exposed to the use tree, then it should be mod.rs.

So for me, at least, foo.rs modules are primarily for growing a "leaf module" into multiple files, without changing the fact that it looks like a leaf module. If it's not a leaf module, I migrate it to use mod.rs.

(I still have a half-written blog post about what a different choice for nonleaf foo.rs could've been made, and how it might've been more consistent (TL;DR mod foo always finds ./foo.rs or ./foo/mod.rs, period, end of story) but I doubt it'll ever get finished since it's not really a productive value-add, just an exploration of what mental model of module mounting the new module style broke.)

2 Likes
15

@josh can comment on whether this is still relevant, but the comment I quoted here from in the stabilization discussion for the new style clearly states that they wanted to push people towards the new style.

16

While it's true that it was technically discussed during stabilization, that doesn't mean it's the iron law. It really hasn't worked out that way. I for one never plan on migrating to the new style because I think it's worse.

In any case, isn't this a distraction of the overall idea here? When I read your proposal, it didn't sound like you were trying to push one style over the other. More or less, you 1) want to avoid inconsistent styles and 2) want folks to be able to pick their style explicitly through a lint. As others have mentioned, I think (1) is probably something most can get on board with. (2) sounds like it belongs in Clippy to me personally, but I confess that I don't really understand the decision process for where a lint is supposed to ultimately live. Maybe that's where the discussion should head. Not sure.

4 Likes
17

Right, yes the primary idea is to allow people to choose a style and enforce it. Unfortunately the discussion in this thread seems to be somewhat orbiting around orthogonal issues to the proposal rather than primarily addressing it directly and I'm getting caught up in that.

My issues with putting this in Clippy rather than rustc:

  • The other lints suggested by RFC 2126 are part of rustc so it would be weird to leave this one out.
  • Clippy is easy to forget to run; we can set it up in CI but I'd like to be able to #![deny(mixed_module_style)] and know that it's actually enforced. I don't want to rely on procedure (running cargo clippy) to ensure this lint is run, because that's part of the problem to begin with (forgetting procedures).
  • Not everyone uses Clippy. Originally, this proposal was just going to be a Github repo with a README ranting about the new style and about half-measures, along with a couple of shell scripts that people could copy into their projects to enforce their chosen module style in a precommit hook or CI. But I was like, "you know what? no, I'm just going to fix this the right way, for everyone."
18

I think you’re leaning to much on what the RFC said some years ago. Since then, we’ve gained implementation experience, and I don’t think there is a clear direction in the ecosystem towards the new style. Effectively I think that makes this more of a style issue than a correctness issue, so clippy is, in my opinion a better fit.

5 Likes
19

I don't see any major issue with adding allow-by-default style lints, given that they should be extremely easy to implement.

20

Would that require an actual RFC or should I just open a PR?

21

A bit off topic: Does some know why this is bad?