Repository navigation
Audit lint pass names and functionality for 1.0 #21761
Description
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).
Activity
pnkfelix commentedon Feb 5, 2015
1.0 polish, P-high.
DanielKeep commentedon Feb 6, 2015
I went through the lints I could find in the compiler and checked the following, as per RFC 344:
lint-name" or "allowlint-nameitems".In addition I added another rule:
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.rsbox_pointersWhat about
rc_pointers/arc_pointers? What's the actual goal with this lint?dead_codedeprecatedexceeding_bitshiftsThis reads as "allow bitshifts which exceed", which seems a strange phrasing without indicating what is being exceeded.
Possible alternative:
width_exceeding_bitshifts.fat_ptr_transmutesThe "
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
dstfrom the "no abbreviations" rule becausedynamically_sized_type_pointer_transmutesfeels a bit too verbose.improper_ctypesThis 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_implementationsmissing_debug_implementationsmissing_docsThere's an unusual abbreviation in "
docs".Rename to
missing_documentation?non_camel_case_typesnon_shorthand_field_patternsnon_snake_casenon_upper_case_globalsThis actually checks non-
mutstatics andconsts, which are not necessarily global. In fact, it explicitly does not check mutablestatics, although those may be going away soon.Rename to
non_upper_case_constants?overflowing_literalspath_statementsI'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_fnsAnother case of an un-delineated adjective having a slightly weird reading. Not wrong, just weird.
Possible alternative:
private_non_mangled_fns.raw_pointer_deriveThis 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_recursionunknown_crate_typesunknown_lintsunreachable_codeunsafe_blocksunsigned_negationunstable_featuresunused_allocationThis 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_assignmentsunused_attributesunused_comparisonsunused_extern_cratesunused_featuresunused_import_bracesunused_importsunused_must_useThis one fits the rules, but comes across as confusing.
Possible alternatives:
unused_must_use_values,unused_values_marked_must_use.unused_mutThis 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_qualificationsunused_resultsAn aside: at first, I thought this was referring to
Result.unused_expressionsorunused_expression_resultswould be clearer, though much longer.unused_typecastsI'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_unsafeThis 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_variablesvariant_size_differencesThe 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.warningswhile_trueApplies 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.rsbad_styleunusedpnkfelix commentedon Feb 7, 2015
While "fat pointer" may not be a concept presented to end-users, I think
Unsized(or at least?Sized) must be. So maybetransmutes_of_pointer_to_unsized?DanielKeep commentedon Feb 7, 2015
@pnkfelix I considered that, but my concern is that
?Sizeddoesn'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 commentedon Feb 8, 2015
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 commentedon Feb 10, 2015
Regarding the interaction with stable Rust, I think that we should adopt a plan like this:
Cargo.tomlfileThe 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
-Wallnot being able to refer to "all".Manishearth commentedon Feb 10, 2015
#[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 commentedon Apr 2, 2015
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 commentedon Apr 2, 2015
Also opened rust-lang/rfcs#1029
Valloric commentedon Apr 3, 2015
@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.