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

Unify non-snake-case lints and non-uppercase statics lints; add lint groups #15773

Merged
merged 2 commits into from
Aug 30, 2014

Conversation

ftxqxd
Copy link
Contributor

@ftxqxd ftxqxd commented Jul 18, 2014

This unifies the non_snake_case_functions and uppercase_variables lints into one lint, non_snake_case. It also now checks for non-snake-case modules. This also extends the non-camel-case types lint to check type parameters, and merges the non_uppercase_pattern_statics lint into the non_uppercase_statics lint.

Because the uppercase_variables lint is now part of the non_snake_case lint, all non-snake-case variables that start with lowercase characters (such as fooBar) will now trigger the non_snake_case lint.

New code should be updated to use the new non_snake_case lint instead of the previous non_snake_case_functions and uppercase_variables lints. All use of the non_uppercase_pattern_statics should be replaced with the non_uppercase_statics lint. Any code that previously contained non-snake-case module or variable names should be updated to use snake case names or disable the non_snake_case lint. Any code with non-camel-case type parameters should be changed to use camel case or disable the non_camel_case_types lint.

This also adds support for lint groups to the compiler. Lint groups are a way of grouping a number of lints together under one name. For example, this also defines a default lint for naming conventions, named bad_style. Writing #[allow(bad_style)] is equivalent to writing #[allow(non_camel_case_types, non_snake_case, non_uppercase_statics)]. These lint groups can also be defined as a compiler plugin using the new Registry::register_lint_group method.

This also adds two built-in lint groups, bad_style and unused. The contents of these groups can be seen by running rustc -W help.

[breaking-change]

@alexcrichton
Copy link
Member

I'm a little uneasy about the continued proliferation of micro-lints, especially for various style criteria. I think I would rather push lints like these all under the same "non snake case" lint rather than a "non snake case X" lint.

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 18, 2014

I agree—the level of control over style lints right now is far too fine-grained. Do you think it would be a good idea to unify some other lints (like non_uppercase_pattern_statics into non_uppercase_patterns, uppercase_variables into non_snake_case) in the process?

(It should be noted that right now uppercase_variables only checks the first character of variable names, while merging it into non_snake_case would make it check the entire string. It is possible that quite a lot of existing code has non-snake-case variables that aren’t being checked by the current lint.)

@ftxqxd ftxqxd changed the title Add lints for non-snake-case lifetime parameters and module names Unify non-snake-case lints and non-uppercase statics lints Jul 19, 2014
@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 19, 2014

OK, I’ve merged the uppercase and snake case lints together. I’m not sure if this counts as a breaking change or not.

@alexcrichton
Copy link
Member

Thanks @P1start! I do think that this needs a [breaking-change] annotation because some lints are removed which may cause compilation failure downstream.

cc @brson, @aturon, I like the direction this is going, I'm not sure if we want to go all the way towards just one allow(style) lint or not or keep the separation as is.

@huonw
Copy link
Member

huonw commented Jul 19, 2014

I like the direction this is going, I'm not sure if we want to go all the way towards just one allow(style) lint or not or keep the separation as is.

Maybe we could have umbrella lints? So one still have fine-grained control of individual things, but the 'common' cases are covered by lints like #[allow(style)].

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 20, 2014

@huonw Maybe that could be done with a normal attribute that expands to a number of lint attributes? So something like #[disable_style_checks] could expand to #[allow(non_snake_case)] #[allow(non_camel_case_types)] #[allow(non_uppercase_statics)] (if that’s actually possible).

@huonw
Copy link
Member

huonw commented Jul 20, 2014

I don't like it; it's a whole new attribute to remember and use, and isn't very extensible, e.g. what if one wants #[deny(style)] to enforce the style guide, or what if we want an umbrella lint like #[deny(unused)] that collects unused_variable, unused_mutability and dead_code?

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 20, 2014

Perhaps the most flexible solution would be to allow lints to have parameters. All the style lints could be put into one lint, with parameters for fine-tuning. So #[deny(style(non_uppercase_statics = warn))] would deny all bad style, except only warn on non-uppercase statics. All the existing style lints would be removed. (@huonw, your spellck plugin would also presumably benefit from this.)

@alexcrichton
Copy link
Member

Yes @huonw I think that may be a good direction to start going in. Messaging that in terms of error messages may get tricky, but I think it may be good to have.

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 20, 2014

Should I add umbrella lints (#[allow(bad_style)]) to this PR, or put that in another PR? I’m beginning to get them working, so I might as well bunch them together with the current changes in this PR.

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Jul 21, 2014

Well, I’ve added lint groups/umbrella lints. I’ve also added two built-in lint groups named bad_style and unused. The name for bad_style is not very good, but I couldn’t think of anything better. (Something like #[forbid(names_that_go_against_the_naming_conventions)] would be perfect if not for the obvious flaw that it is too long.)

@ftxqxd ftxqxd changed the title Unify non-snake-case lints and non-uppercase statics lints Unify non-snake-case lints and non-uppercase statics lints; add lint groups Jul 21, 2014
@alexcrichton
Copy link
Member

@P1start this looks pretty good to me, I'd like to gain a few more opinions about umbrella lints, however, because this is a pretty big feature for the compiler.

@aturon
Copy link
Member

aturon commented Jul 27, 2014

My 2c: I think umbrella lints are a very useful idea, and I don't see a lot of downside. So +1 from me!

@nikomatsakis
Copy link
Contributor

Not quite +1: see http://discuss.rust-lang.org/t/umbrella-lints/307 (TLDR -- I am concerned about backwards compat)

@alexcrichton
Copy link
Member

This was discussed in yesterday's meeting and we decided to merge it, thanks for being patient @P1start!

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Aug 7, 2014

Updated; tests pass locally. I should probably also note that this change makes make spit out thousands of lines of warnings. (This would be fixed with a snapshot.)

@ftxqxd ftxqxd force-pushed the style-lints branch 2 times, most recently from 677333f to 7282c4d Compare August 29, 2014 10:10
@alexcrichton
Copy link
Member

Argh sorry @P1start, this totally fell under the radar! Feel free to ping PRs if they don't seem to have much activity to spur us into action!

ftxqxd added 2 commits August 30, 2014 09:10
This unifies the `non_snake_case_functions` and `uppercase_variables` lints
into one lint, `non_snake_case`. It also now checks for non-snake-case modules.
This also extends the non-camel-case types lint to check type parameters, and
merges the `non_uppercase_pattern_statics` lint into the
`non_uppercase_statics` lint.

Because the `uppercase_variables` lint is now part of the `non_snake_case`
lint, all non-snake-case variables that start with lowercase characters (such
as `fooBar`) will now trigger the `non_snake_case` lint.

New code should be updated to use the new `non_snake_case` lint instead of the
previous `non_snake_case_functions` and `uppercase_variables` lints. All use of
the `non_uppercase_pattern_statics` should be replaced with the
`non_uppercase_statics` lint. Any code that previously contained non-snake-case
module or variable names should be updated to use snake case names or disable
the `non_snake_case` lint. Any code with non-camel-case type parameters should
be changed to use camel case or disable the `non_camel_case_types` lint.

[breaking-change]
This adds support for lint groups to the compiler. Lint groups are a way of
grouping a number of lints together under one name. For example, this also
defines a default lint for naming conventions, named `bad_style`. Writing
`#[allow(bad_style)]` is equivalent to writing
`#[allow(non_camel_case_types, non_snake_case, non_uppercase_statics)]`. These
lint groups can also be defined as a compiler plugin using the new
`Registry::register_lint_group` method.

This also adds two built-in lint groups, `bad_style` and `unused`. The contents
of these groups can be seen by running `rustc -W help`.
@ftxqxd
Copy link
Contributor Author

ftxqxd commented Aug 29, 2014

Updated again; tests pass locally.

bors added a commit that referenced this pull request Aug 29, 2014
This unifies the `non_snake_case_functions` and `uppercase_variables` lints into one lint, `non_snake_case`. It also now checks for non-snake-case modules. This also extends the non-camel-case types lint to check type parameters, and merges the `non_uppercase_pattern_statics` lint into the `non_uppercase_statics` lint.

Because the `uppercase_variables` lint is now part of the `non_snake_case` lint, all non-snake-case variables that start with lowercase characters (such as `fooBar`) will now trigger the `non_snake_case` lint.

New code should be updated to use the new `non_snake_case` lint instead of the previous `non_snake_case_functions` and `uppercase_variables` lints. All use of the `non_uppercase_pattern_statics` should be replaced with the `non_uppercase_statics` lint. Any code that previously contained non-snake-case module or variable names should be updated to use snake case names or disable the `non_snake_case` lint. Any code with non-camel-case type parameters should be changed to use camel case or disable the `non_camel_case_types` lint.

This also adds support for lint groups to the compiler. Lint groups are a way of grouping a number of lints together under one name. For example, this also defines a default lint for naming conventions, named `bad_style`. Writing `#[allow(bad_style)]` is equivalent to writing `#[allow(non_camel_case_types, non_snake_case, non_uppercase_statics)]`. These lint groups can also be defined as a compiler plugin using the new `Registry::register_lint_group` method.

This also adds two built-in lint groups, `bad_style` and `unused`. The contents of these groups can be seen by running `rustc -W help`.

[breaking-change]
@bors bors closed this Aug 30, 2014
@bors bors merged commit ed2aad8 into rust-lang:master Aug 30, 2014
retep998 added a commit to retep998/rust that referenced this pull request Aug 30, 2014
Signed-off-by: Peter Atashian <retep998@gmail.com>
retep998 added a commit to retep998/rust that referenced this pull request Aug 31, 2014
Signed-off-by: Peter Atashian <retep998@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants