Audit lint pass names and functionality for 1.0

[alexcrichton]

We have a huge number of lints, and all of their names are at least decisions we cannot change. We have a naming convention RFC which dictates what each lint should be called, and we need to audit all names as some may have slipped through the cracks (we have a few new ones since the RFC was accepted).

More generally, we need to decide what lint functionality we're going to have in the compiler. Do we want to ship every single lint as-is today, or do we want to cut back on the number of lints that we're shipping with?

And as a final point, we should make a concrete decision about whether the functionality of lints can be altered in stable Rust. A lint may have a bug, for example, which issues new warnings in a new release, thereby breaking crates with #![deny(warnings)].

Nominating for 1.0-beta as we may want to decide to remove some lints (but if we don't, nominating for 1.0-polish).

[pnkfelix]

1.0 polish, P-high.

[DanielKeep]

I went through the lints I could find in the compiler and checked the following, as per RFC 344:

• Lint names should make sense when read as "allow lint-name" or "allow lint-name items".
• Lint names should state the bad thing being checked for.
• Lints that apply to arbitrary items should not have a category in their name.
• Lints that mention a grammatical class should mention said class and use the plural form.
• Lints that catch redundant constructs should use the term "unused".
• Use snake case.

In addition I added another rule:

• Words should not be abbreviated. Language keywords may be excepted. This rule was derived from common usage of un-abbreviated forms of words.

I have tried to apply these as blindly as possible, even in cases where I feel the lint name is perfectly reasonable, just to ensure each lint's name is considered for any possible infraction.

And yes, I know they're guidelines, not rules... but guidelines are really just loosely enforced rules. :P

Lints Checked

Comments are included only in cases of problems or suggestions.

src/librustc/lint/builtin.rs

• box_pointers

  What about rc_pointers/arc_pointers? What's the actual goal with this lint?

• dead_code

• deprecated

• exceeding_bitshifts

  This reads as "allow bitshifts which exceed", which seems a strange phrasing without indicating what is being exceeded.

  Possible alternative: width_exceeding_bitshifts.

• fat_ptr_transmutes

  The "ptr" abbreviation is not used in other lints. For consistency, it should be renamed, or all other lints mentioning pointers should be renamed to use the abbreviation. It is worth noting, however, that abbreviations appear to be relatively uncommon in lint names.

  Rename to fat_pointer_transmutes?

  A side issue is that I'm not aware of "fat pointer" being a concept in the language, as presented to users. If I understand correctly, this is only ever going to apply to DST pointers. In this case, perhaps it should be renamed further.

  Rename to dst_pointer_transmutes?

  I'm excepting dst from the "no abbreviations" rule because dynamically_sized_type_pointer_transmutes feels a bit too verbose.

• improper_ctypes

  This implies the problem is with ctypes. In truth, the problem is that you're using something which is not flagged #[repr(ctype)] where it should be, or a type which may not have a well-defined meaning in C.

  Possible alternatives: non_ffi_safe_types, ffi_unsafe_types.

• missing_copy_implementations

• missing_debug_implementations

• missing_docs

  There's an unusual abbreviation in "docs".

  Rename to missing_documentation?

• non_camel_case_types

• non_shorthand_field_patterns

• non_snake_case

• non_upper_case_globals

  This actually checks non-mut statics and consts, which are not necessarily global. In fact, it explicitly does not check mutable statics, although those may be going away soon.

  Rename to non_upper_case_constants?

• overflowing_literals

• path_statements

  I'm unsure, but should this be unused_path_expressions? It's about a piece of code that doesn't do anything. I feel this name is also marginally clearer than the current one, which initially confused me.

• private_no_mangle_fns

  Another case of an un-delineated adjective having a slightly weird reading. Not wrong, just weird.

  Possible alternative: private_non_mangled_fns.

• raw_pointer_derive

  This one violates the plural rule, and is also inelegantly named in general. The problem suggests that the problem is derive-ing the implementation of a raw pointer, rather than the implementation of something containing a raw pointer.

  Sadly, a more accurate name would likely be "raw pointer-containing derived implementations", which is a little unwieldy.

  Possible alternatives: derive_involving_raw_pointers (sort-of violates plural class rule), raw_pointer_derive_implementations (to be consistent with existing uses of un-abbreviated word), derive_using_raw_pointers.

• unconditional_recursion

• unknown_crate_types

• unknown_lints

• unreachable_code

• unsafe_blocks

• unsigned_negation

• unstable_features

• unused_allocation

  This violates the plural rule.

  Rename to: unused_allocations?

  As an aside, I can't work out what this one is actually checking for... it mentions ty::AutoPtr, but I don't know what that is.

• unused_assignments

• unused_attributes

• unused_comparisons

• unused_extern_crates

• unused_features

• unused_import_braces

• unused_imports

• unused_must_use

  This one fits the rules, but comes across as confusing.

  Possible alternatives: unused_must_use_values, unused_values_marked_must_use.

• unused_mut

  This violates the plural rule.

  Possible alternatives: unused_muts, unused_mutability.

• unused_parens

  "parens" is not a keyword, and thus its abbreviation is inconsistent.

  Rename to unused_parentheses?

• unused_qualifications

• unused_results

  An aside: at first, I thought this was referring to Result. unused_expressions or unused_expression_results would be clearer, though much longer.

• unused_typecasts

  I'm on the fence about this. I've never used "typecast" as a word in this context before, but Wikipedia does list it as a synonym for "type conversion". My other thought is: "if there are type casts, what other kinds of cast are they distinct from?"

  I'm not aware of any other kind of "cast" in Rust at the language level, so should this be unused_casts?

• unused_unsafe

  This violates the plural rule, applies to a specific item but doesn't mention it, and is inconsistent with the existence of unsafe_blocks.

  Rename to unused_unsafe_blocks?

• unused_variables

• variant_size_differences

  The description says that it checks for variants which have "widely" different sizes, but that's not reflected in the name.

  Possible alternative: excessive_variant_size_differences.

• warnings

• while_true

  Applies to a specific item, but doesn't mention it. That said, you could also argue that it applies to exactly one thing, thus it shouldn't be plural.

  Rename to while_true_loops?

src/librustc/lint/context.rs

• Group bad_style
• Group unused

[pnkfelix]

A side issue is that I'm not aware of "fat pointer" being a concept in the language, as presented to users. If I understand correctly, this is only ever going to apply to DST pointers. In this case, perhaps it should be renamed further.

Rename to dst_pointer_transmutes?

While "fat pointer" may not be a concept presented to end-users, I think Unsized (or at least ?Sized) must be. So maybe transmutes_of_pointer_to_unsized ?

[DanielKeep]

@pnkfelix I considered that, but my concern is that ?Sized doesn't mean "unsized", it means "not statically sized". I know there have been requests for Rust to gain actually unsized types for things like C strings, which would be thin pointers.

Also, just to be clear, I'm uncertain whether I should just go ahead with the proposed changes, and make a PR, move all this into an RFC, or keep this here. Honestly, the RFC thing only just occurred to me since it was "oh, this is fixing stuff that doesn't match the already accepted guidelines" as opposed to "let's deliberately break everything". I'm happy to go with whatever. :P

[Manishearth]

Note: If some of these lints need a place to go, I can take them as part of rust-clippy. If necessary, I can put some more work into the library and publish it.

[nikomatsakis]

Regarding the interaction with stable Rust, I think that we should adopt a plan like this:

• All stable code can use any lint that it wants to.
• We should add an option to rustc that caps all lints at warn.
• Cargo should supply this option when building dependencies or installing software
  • This might be configurable in the Cargo.toml file

The idea here is that if I am using somebody's package, I don't want to have compilation fail because we added a new lint in the meantime. But if I am editing my own source code, I absolutely do want compilation to fail. I feel like this bridges the gap and avoids the problem of -Wall not being able to refer to "all".

[Manishearth]

#[feature(deny)]? Makes sense.

That being said I'm a tad wary of making stability guarantees that depend on Cargo. But I guess that leads to a cleaner solution.

[nikomatsakis]

To expand on the point I made here, we can even go further and say that specifying lints which are no longer included are also not errors etc. It seems like we should reserve the right to change/remove/refine lints as we please, in which case this is not a milestone blocker. Closing issue for now in favor of more specific proposals.

[nikomatsakis]

Also opened rust-lang/rfcs#1029

[Valloric]

we can even go further and say that specifying lints which are no longer included are also not errors

@nikomatsakis At least some compiler output should be presented in this case. If rustc silently accepts lints that don't exist and thus don't do anything, the user can accidentally mistype the name of an important lint they care about and think they're being protected by it, when in fact no such protection is present.