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

Add module_style lint to style #7543

Merged
merged 1 commit into from Aug 25, 2021
Merged

Conversation

DevinR528
Copy link
Contributor

changelog: Add new [module_style] style lint

This is a configurable (no mod file/mod file) lint that determines if mod.rs is used consistently or if mod.rs is never used (using the new mod layout).

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 7, 2021
let p = p.to_string_lossy();
let path = Path::new(p.trim_start_matches(trim_to_src.as_ref()));

if path.ends_with("mod.rs") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not trigger for something_mod.rs.

I would just emit the lint right here. It's okay to emit for every module. Also, you should be able to get a Span using Span::new(file.start_pos, file.start_pos, SyntaxContext::root()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't Path::ends_with is different that str::ends_with, it only considers whole segments.

I can't emit until I know if there are any missing mod.rs files this is the mod_file check. I could still collect all the folder names (after src) and collect all the mod.rs file paths and remove all the folders that contain mod files, emitting for all the folders that are left after. I would have to keep more info around and haven't figured out all the details but it would probably work, then the error messages would at least be more consistent.

@camsteffen
Copy link
Contributor

I think it would be simpler to make this two complimentary restriction lints - mod_file and self_named_mod_file (enable the lint for the style you want to ban). In either case, I don't think the Ignore variant is necessary since it is the same as allowing the lint.

@Manishearth
Copy link
Member

So we discussed this in the meeting yesterday and it's not really clear to me if this is an accepted style preference; in my experience codebases are fine with mixing them since the approaches sometimes have their own benefits. I think this should be a restriction lint, making it possible for crates to opt in but out of the picture by default.

@camsteffen
Copy link
Contributor

I did see the meeting and my suggestion is in addition to that. (I should have clarified that)

@Manishearth
Copy link
Member

Oh, I wasn't responding to you, I had just forgotten to update the PR 😄

@camsteffen
Copy link
Contributor

Oh, I wasn't responding to you

Oh I see how it is! 😆

clippy_lints/src/module_style.rs Show resolved Hide resolved
/// stuff.rs
/// lib.rs
/// ```
pub MOD_FILES,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on improving the lint name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mod_module_files and the other could be self_named_module_files 🤷 I was hoping when I looked up the RFC that introduced the new module system there would be a good name for it but there is not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camsteffen thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. I think @DevinR528's idea is good. Another approach is mod_file_within_dir and mod_file_outside_dir but I don't like that better.

@@ -4,7 +4,7 @@ error: `mod.rs` files are required, found `/bad/inner.rs`
LL | pub mod stuff;
| ^
|
= note: `-D clippy::self-named-mod-files` implied by `-D warnings`
= note: `-D clippy::self-named-module-files` implied by `-D warnings`
= help: create a `/bad/inner/mod.rs` file and remove `/bad/inner.rs`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the suggestion simple be "move bad/inner.rs to bad/inner/mod.rs, rather than "create", "remove"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed both error messages to the above suggestion

@DevinR528
Copy link
Contributor Author

Should I squash the commits or are there any more things to fix?

@Manishearth
Copy link
Member

Nah I should just review this 😄

@Manishearth
Copy link
Member

Squashing would be appreciated though!

@DevinR528
Copy link
Contributor Author

Squashed!

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost there!

return;
};

let mut segments = FxHashSet::default();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we have some comments on what these variables contain

}
}

fn check_mod_file_exists(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: also document the purpose of the function

especially since this does a different kind of "check" than the other one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this function to process_paths_for_mod_files, as you pointed out there is no check there so it didn't make much sense hope this works

Add tests for disallowed_mod in ui-cargo test section

Use correct algorithm to determine if mod.rs is missing

Move to two lints and remove config option

Switch lint names so they read "warn on ..."

Emit the same help info for self_named_mod_file warnings

Bail when both lints are Allow

Reword help message for both module_style lints
@Manishearth
Copy link
Member

@bors r+

Thanks for your patience!

@bors
Copy link
Collaborator

bors commented Aug 25, 2021

📌 Commit 31738b1 has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Aug 25, 2021

⌛ Testing commit 31738b1 with merge de80188...

@bors
Copy link
Collaborator

bors commented Aug 25, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing de80188 to master...

@bors bors merged commit de80188 into rust-lang:master Aug 25, 2021
@DevinR528 DevinR528 deleted the disallowed-mod branch August 26, 2021 11:52
@DevinR528
Copy link
Contributor Author

Thanks for helping me along!

@lopopolo
Copy link

lopopolo commented Sep 4, 2021

This lint is going into the style group which means it will be enabled by default. Has a crater run been performed to assess how much churn this will cause?

@jplatte
Copy link
Contributor

jplatte commented Sep 4, 2021

@lopopolo If you have a look at the description and discussion above, you will see that this lint was never enabled by default. Originally it was a style lint that would only ever trigger when it was configured via clippy.toml, after feedback it was changed to be two separate restriction lints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants