Rust: Uplift lints from clippy to rustc

[oli-obk]

cc @Manishearth @rust-lang/lang

As discussed in the Clippy 1.0 RFC, we would like to uplift some lints from clippy to rustc. Full rationale can be found in https://github.com/rust-lang/rfcs/blob/b9c8471887f308223c226642cad3a8290731b942/text/0000-clippy-uno.md#compiler-uplift

The list of correctness lints in clippy follows. I have checked the boxes of lints that I think should be uplifted

Renamings happening during uplift have been added via -> new_name annotations

• [ ] [for_loop_over_option](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#for_loop_over_option): Checks for for loops over Option values.
• [x] [eq_op -> same_operands](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#eq_op): Checks for equal operands to comparison, logical and
  bitwise, difference and division binary operators (==, >, etc., &&,
  ||, &, |, ^, - and /).
• [x] [iter_next_loop -> for_val_in_iter_next](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#iter_next_loop): Checks for loops on x.next().
• [x] [deprecated_semver -> incorrect_semver_strings](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#deprecated_semver): Checks for #[deprecated] annotations with a since
  field that is not a valid semantic version.
• [x] [drop_copy -> dropping_copy_types](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#drop_copy): Checks for calls to std::mem::drop with a value
  that derives the Copy trait
• [x] [not_unsafe_ptr_arg_deref -> deref_ptr_arg_in_safe_fns](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref): Checks for public functions that dereferences raw pointer
  arguments but are not marked unsafe.
• [x] [logic_bug -> unused_boolean_operands](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#logic_bug): Checks for boolean expressions that contain terminals that
  can be eliminated.
• [x] [clone_double_ref -> cloning_references](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#clone_double_ref): Checks for usage of .clone() on an &&T.
• [x] [almost_swapped -> incorrect_swaps](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#almost_swapped): Checks for foo = bar; bar = foo sequences.
• [ ] [possible_missing_comma](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#possible_missing_comma): Checks for possible missing comma in an array. It lints if
  an array element is a binary operator expression and it lies on two lines.
• [x] [wrong_transmute -> undefined_transmutes](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#wrong_transmute): Checks for transmutes that can't ever be correct on any
  architecture.
• [ ] [invalid_regex](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#invalid_regex): Checks regex creation
  (with Regex::new,RegexBuilder::new or RegexSet::new) for correct
  regex syntax.
• [x] [bad_bit_mask -> unused_bitmasks](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#bad_bit_mask): Checks for incompatible bit masks in comparisons.
• [x] [drop_ref -> dropping_references](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#drop_ref): Checks for calls to std::mem::drop with a reference
  instead of an owned value.
• [ ] [derive_hash_xor_eq](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#derive_hash_xor_eq): Checks for deriving Hash but implementing PartialEq
  explicitly or vice versa.
• [ ] [useless_attribute](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_attribute): Checks for extern crate and use items annotated with
  lint attributes
• [x] [temporary_cstring_as_ptr](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr): Checks for getting the inner pointer of a temporary
  CString.
• [x] [min_max -> incorrect_clamps](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#min_max): Checks for expressions where std::cmp::min and max are
  used to clamp values, but switched so that the result is constant.
• [x] [unit_cmp -> unit_comparisons](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unit_cmp): Checks for comparisons to unit.
• [x] [reverse_range_loop -> incorrect_reversed_ranges](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#reverse_range_loop): Checks for loops over ranges x..y where both x and y
  are constant and x is greater or equal to y, unless the range is
  reversed or has a negative .step_by(_).
• [ ] [erasing_op](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#erasing_op): Checks for erasing operations, e.g. x * 0.
• [ ] [suspicious_op_assign_impl](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl): Lints for suspicious operations in impls of OpAssign, e.g.
  subtracting elements in an AddAssign impl.
• [ ] [float_cmp](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#float_cmp): Checks for (in-)equality comparisons on floating-point
  values (apart from zero), except in functions called *eq* (which probably
  implement equality for a type involving floats).
• [x] [zero_width_space -> zero_width_spaces](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#zero_width_space): Checks for the Unicode zero-width space in the code.
• [x] [fn_to_numeric_cast_with_truncation -> truncating_fn_ptr_to_int_casts](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_with_truncation): Checks for casts of a function pointer to a numeric type not large enough to store address.
• [ ] [suspicious_arithmetic_impl](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#suspicious_arithmetic_impl): Lints for suspicious operations in impls of arithmetic operators, e.g.
  subtracting elements in an Add impl.
• [ ] [approx_constant](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#approx_constant): Checks for floating point literals that approximate
  constants which are defined in
  std::f32::consts
  or
  std::f64::consts,
  respectively, suggesting to use the predefined constant.
• [x] [while_immutable_condition -> unused_while_loop_conditions](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#while_immutable_condition): Checks whether variables used within while loop condition
  can be (and are) mutated in the body.
• [ ] [never_loop](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#never_loop): Checks for loops that will always break, return or
  continue an outer loop.
• [x] [nonsensical_open_options -> unused_open_options](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#nonsensical_open_options): Checks for duplicate open options as well as combinations
  that make no sense.
• [x] [forget_copy -> forgetting_copy_types](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#forget_copy): Checks for calls to std::mem::forget with a value that
  derives the Copy trait
• [x] [if_same_then_else -> unused_branching](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#if_same_then_else): Checks for if/else with the same body as the then part
  and the else part.
• [ ] [cast_ptr_alignment](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#cast_ptr_alignment): Checks for casts from a less-strictly-aligned pointer to a
  more-strictly-aligned pointer
• [x] [ifs_same_cond -> unused_branching](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#ifs_same_cond): Checks for consecutive ifs with the same condition.
• [x] [out_of_bounds_indexing](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#out_of_bounds_indexing): Checks for out of bounds array indexing with a constant
  index.
• [ ] [modulo_one](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#modulo_one): Checks for getting the remainder of a division by one.
• [x] [inline_fn_without_body -> inline_fns_without_body](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#inline_fn_without_body): Checks for #[inline] on trait methods without bodies
• [x] [cmp_nan -> unused_comparisons](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#cmp_nan): Checks for comparisons to NaN.
• [ ] [ineffective_bit_mask](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#ineffective_bit_mask): Checks for bit masks in comparisons which can be removed
  without changing the outcome.
• [x] [infinite_iter -> infinite_iterators](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#infinite_iter): Checks for iteration that is guaranteed to be infinite.
• [x] [mut_from_ref -> returning_mut_ref_from_ref](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mut_from_ref): This lint checks for functions that take immutable
  references and return
  mutable ones.
• [x] [unused_io_amount -> unused_io_amounts](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_io_amount): Checks for unused written/read amount.
• [x] [invalid_ref -> undefined_references](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#invalid_ref): Checks for creation of references to zeroed or uninitialized memory.
• [ ] [serde_api_misuse](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#serde_api_misuse): Checks for mis-uses of the serde API.
• [x] [forget_ref -> forgetting_references](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#forget_ref): Checks for calls to std::mem::forget with a reference
  instead of an owned value.
• [x] [absurd_extreme_comparisons -> unused_comparisons](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#absurd_extreme_comparisons): Checks for comparisons where one side of the relation is
  either the minimum or maximum value for its type and warns if it involves a
  case that is always true or always false. Only integer and boolean types are
  checked.
• [ ] [for_loop_over_result](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#for_loop_over_result): Checks for for loops over Result values.
• [x] [iterator_step_by_zero -> iter_step_by_zero](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#iterator_step_by_zero): Checks for calling .step_by(0) on iterators,
  which never terminates.
• [ ] [enum_clike_unportable_variant](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#enum_clike_unportable_variant): Checks for C-like enumerations that are
  repr(isize/usize) and have values that don't fit into an i32.

[oli-obk]

@rfcbot fcp merge

please raise concerns about specific lints directly with rfcbot

[rfcbot]

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged teams:

• [ ] @Zoxc
• [x] @cramertj
• [x] @eddyb
• [x] @estebank
• [ ] @michaelwoerister
• [ ] @nagisa
• [x] @nikomatsakis
• [x] @oli-obk
• [ ] @petrochenkov
• [ ] @pnkfelix
• [ ] @varkor

Concerns:

• naming conventions (https://github.com/rust-lang/rust/issues/53224#issuecomment-412378013)
• some lints are opinionated on how the target platform ought to work (https://github.com/rust-lang/rust/issues/53224#issuecomment-412107556)
• the descriptions of lints are a little too concise for decision making. (https://github.com/rust-lang/rust/issues/53224#issuecomment-412097880)
• tying compiler to stdlib implementation details (https://github.com/rust-lang/rust/issues/53224#issuecomment-412097880)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Mark-Simulacrum]

https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_io_amount seems like it should be replaced with a #[must_use] on those, but I'm not sure if we can do that for trait methods yet.

[nrc]

cc @rust-lang/compiler

[eddyb]

@Mark-Simulacrum You should be able to do it without any special support on the method declaration in the trait, because that's what paths and method calls resolve to, but if you want to stick it on method definitions in impls, that's significantly harder.

[oli-obk]

The problem is not that the return value is unused, it is usually used via ?, because Result is already must_use. The problem is that the Ok value is unused, and I don't think that's solvable with must_use

[nikomatsakis]

for_loop_over_option: Checks for for loops over Option values.

I feel like classifying this as correctness is a bit of a stretch? It seems like type check ought to resolve most of the bugs that would occur here.

We may still want to uplift it, though, I'm not sure.

UPDATE: ok, my mistake, I didn't realize the meaning of the checked boxes. =)

[nagisa]

Checks for transmutes that can't ever be correct on any architecture.

Related: https://github.com/rust-lang/rust/issues/50842

---

Checks for creation of references to zeroed or uninitialized memory.

IMO description is wrong, as the example shows creation of a zero-reference, not a reference to zeroed memory.

---

@rfcbot concern tying compiler to stdlib implementation details

Many of these lints are libstd specific (mostly pertaining to iterators). What mnemonic does clippy use to figure out what libstd items are what and whether the actual libstd is used at all and not e.g. use my_funky_crate as std;? How would the compiler deal with this without making half of the libstd a language item?

---

@rfcbot concern the descriptions of lints are a little too concise for decision making.

For example, it is unclear from description of eq_op whether it does act on generic code or non-builtin-types, where all of the operators can be overridden to have arbitrary side effects.

[nagisa]

A good way to test your questions is to use https://play.rust-lang.org/. On the right hand side, there’s a “Tools” dropdown which has Clippy in it.

---

@rfcbot concern some lints are opinionated on how the target platform ought to work

For example, the mut_from_ref lint prevents one from writing code like this, which is fairly common in my, admittedly C-centric, embedded programming experience:

// Gets a mutable reference to resource’s register bank
unsafe fn get_resource(x: &ResourceLock) -> &mut Resource {
    // THE_RESOURCE is linked by the linker into well known hardware operated range of memory.
    #[link_section="something_wellknown"]
    static mut THE_RESOURCE: Resource;
    take_resource_lock(x); // later released by the caller once they’re finished with the Resource
    &mut THE_RESOURCE
}

If it wasn’t for @japaric and WG-embedded, this is how I’d write my embedded code if I wasn’t really interested in exploring the design avenues that exact moment.

---

One interesting idea that came to mind was an ability for rustc to use different default lint levels depending on the target platform. The code above, is infinitely unlikely to occur on hosted targets, so the lint can be enabled there, but not on bare-metal targets where the code might be entirely valid and is quite likely to show up.

[Manishearth]

Many of these lints are libstd specific (mostly pertaining to iterators). What mnemonic does clippy use to figure out what libstd items are what and whether the actual libstd is used at all and not e.g. use my_funky_crate as std;?

Clippy doesn't try to solve this, anything found behind a std path is fair game. It tries to use core though as much as possible though (rustc does this too -- ranges and iteration goes through ::core)

The robust solution here is to add a #[diagnostic_item = "foo"] like #[lang_item] which you can check against.

The code above, is infinitely unlikely to occur on hosted targets, so the lint can be enabled there, but not on bare-metal targets where the code might be entirely valid and is quite likely to show up.

I feel like that's enough of a special case that you'll want to turn it off. Idk.

[petrochenkov]

@rfcbot concern naming conventions

Lints in rustc follow some naming conventions.
A number of the lints proposed for uplifting doesn't follow the conventions.
Supposedly naming for the new lints needs to be harmonized with existing rustc lints.

[oli-obk]

I added renaming suggestions to all lints with a checkbox if the name didn't follow the rules

[oli-obk]

concern the descriptions of lints are a little too concise for decision making.

For example, it is unclear from description of eq_op whether it does act on generic code or non-builtin-types, where all of the operators can be overridden to have arbitrary side effects.

Unless explicitly stated, all lints are as generic as possible. So in the eq_op case, we don't care about the type at all. Generic parameters and custom types are fair game. I'm aware that you could be missing a Mul<i32> impl and thus use a + a instead of 2 * a. If this is too opinionated, we can adjust the lint during uplift and leave the rest to clippy or not uplift it at all.

The list is a suggestion by me, not reviewed by anyone else. I am absolutely fine with crossing items off the list if they are too opinionated.

[withoutboats]

I feel vaguely uneasy about this because this is a lot of lints, being uplifted all at once, without very clear (to me) explanation of what rules determine if a lint belongs in the compiler or not.

What I'd prefer to do is establish consensus on the guidelines and then entrust other people to make the call on whether individual lints fit those guidelines.

[oli-obk]

The general guidelines suggested by the RFC are

Some correctness lints should probably belong in the compiler, whereas style/complexity/etc lints should probably belong in Clippy. Lints may be incubated in Clippy, of course.

I don't think the compler will want all correctness lints here, however if the lint is about a common enough situation where it being not a bug is an exceedingly rare case (i.e. very low false positive frequency) it should probably belong in the compiler.

This of course depends on the rules for correctness lints:

Probable bugs

Yes this is a very short and general definition, but the space for correctness lints is just too big to create a guideline for them (as evidenced by the variance of the lints in the OP).

[Manishearth]

What I'd prefer to do is establish consensus on the guidelines

We _did_, the clippy RFC outlines the correctness lints as potential
uplifts, they best match the degree of severity and accuracy of rustc lints.

But there are a lot of these, so we wanted the compiler team to pick out
the ones they really liked.

On Tue, Aug 14, 2018 at 8:31 AM boats [e-mail] wrote:

I feel vaguely uneasy about this because this is a lot of lints, being
uplifted all at once, without very clear (to me) explanation of what rules
determine if a lint belongs in the compiler or not.

What I'd prefer to do is establish consensus on the guidelines and then
entrust other people to make the call on whether individual lints fit those
guidelines.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/53224#issuecomment-412913304,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABivSBaYBDdF-Z8WngGyjBd0zlZEdxd3ks5uQu01gaJpZM4V15Oa
.

[varkor]

Unless explicitly stated, all lints are as generic as possible. So in the eq_op case, we don't care about the type at all. Generic parameters and custom types are fair game. I'm aware that you could be missing a Mul impl and thus use a + a instead of 2 * a. If this is too opinionated, we can adjust the lint during uplift and leave the rest to clippy or not uplift it at all.

I think eq_op specifically for types such as integers is fine, but if applied in the general case, it makes assumptions about the semantics of the operations that I don't think are reasonable, considering there's no requirement that the operations respect any algebraic laws.

(I'll take a better look at the others soon.)

[goodmanjonathan]

If I may contribute to the bikeshed a bit, these are the renames I would like to see:

• absurd_extreme_comparisons -> unused_comparisons (existing in rustc)
• almost_swapped -> incorrect_swaps
• bad_bit_mask -> unused_bitmasks
• clone_double_ref -> cloning_references
• cmp_nan -> unused_comparisons (existing in rustc)
• deprecated_semver -> incorrect_semver_strings
• drop_copy -> dropping_copy_types
• drop_ref -> dropping_references
• eq_op -> same_operands
• fn_to_numeric_cast_with_truncation -> truncating_fn_ptr_as_int
• forget_copy -> forgetting_copy_types
• forget_ref -> forgetting_references
• if_same_then_else -> unused_branching
• ifs_same_cond -> unused_branching
• infinite_iter -> infinite_iterators
• inline_fn_without_body -> inline_fns_without_body
• invalid_ref -> undefined_references
• iter_next_loop -> for_val_in_iter_next
• iterator_step_by_zero -> iter_step_by_zero
• logic_bug -> unused_short_circuits
• min_max -> incorrect_clamps
• mut_from_ref -> returning_mut_ref_from_ref
• nonsensical_open_options -> unused_file_options
• not_unsafe_ptr_arg_deref -> deref_ptr_arg_in_safe_fns
• reverse_range_loop -> incorrect_reversed_ranges
• unit_cmp -> unit_comparisons
• unused_io_amount -> unused_io_amounts
• while_immutable_condition -> while_immutable_val
• wrong_transmute -> undefined_transmutes
• zero_width_space -> zero_width_spaces

The out_of_bounds_indexing lint should be deprecated now that const_err is warn by default. temporary_cstring_as_ptr seems fine as is.

Many of these names were originally proposed by @clarcharr (rust-lang-nursery/rust-clippy#2845, rust-lang-nursery/rust-clippy#2846). They came up with some naming rules there too.

[Centril]

Sorry again for the land grab, but...
...I think that uplifting lints to the compiler, while not being directly about the language specification (and other Rust compilers are not expected to have the same set of lints...), is not like diagnostics because it is highly normative about what Rust code should and should not look like.
...Also, the RFC policy - language design document states that this falls under T-lang's purview. (I know this hasn't been exercised for some time, but now it is, and I like sticking to rules...)
...And as @withoutboats's noted, this is a lot of lints
...So I'm adding T-lang to the party.
...Again, sorry about that... :)

EDIT: Apparently rfcbot wasn't appreciative; I'll have to fix the bot first ^,-

[Manishearth]

I'm more okay with it here because this isn't an RFC (I was _very_ wary of making the clippy RFC into a three team RFC). Also "a lot of lints" is correct and more eyeballs is good here :smile:

That said I will reiterate that while that document does state that lints fall under the lang team, not only has this never (?) been exercised, AIUI it's always been the compiler team doing this in practice; so we may want to change that policy.

(Also, by that argument the style RFCs also fall under the lang team.)

Don't want to discuss this here, but just want to bring up that this may be something the teams may wish to change, given that AFAICT theory and practice seem to not match here.

[varkor]

@rfcbot concern overly-aggressive stylistic lints

Some of the lints discourage patterns that seem reasonable in practice.

• eq_op: sensible for integer and boolean types, but not generic types.
• bad_bit_mask: same concern as for eq_op.
• infinite_iter: while it's probably not good practice, I'm not sure it's something the compiler itself should warn about.
• mut_from_ref: the description claims it's "trivially unsound", but it seems naïvely like such a signature could be plausible for some sort of allocator.

---

@rfcbot concern possible false positives

With the current descriptions, some of the lints seem like they could behave incorrectly in some cases. If these are correct, the descriptions should be updated.

• ifs_same_cond: this ignores function calls in the condition, but there are other ways to cause effects within a condition. Does this lint definitely not have false positives?
• logic_bug: could the comment about ignoring short-circuiting behaviour be elaborated? What expressions will it warn against that might be effectful?
• absurd_extreme_comparisons: usize should probably be excluded?

---

Some other comments, but not blocking concerns.
not_unsafe_ptr_arg_deref: seems reasonable, though it could be helpful to address the limitations in "Known problems".

• wrong_transmute: the description doesn't go into which transmutes are actually forbidden.
• zero_width_space: where exactly is this caught? Is it caught inside strings for instance? Are there other characters it'd similarly make sense to forbid (such as other zero-width whitespace characters)?

All the other suggested lints seem sensible to me.

[Centril]

Since I don't have time to fix @rfcbot... I'll just have to:

@rfcbot cancel
@rfcbot merge

-- To save y'all some time I'll tick the boxes that were already ticked..

[rfcbot]

@Centril proposal cancelled.

[rfcbot]

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @Centril
• [x] @Zoxc
• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @estebank
• [x] @joshtriplett
• [x] @michaelwoerister
• [ ] @nagisa
• [x] @nikomatsakis
• [x] @nrc
• [x] @oli-obk
• [x] @petrochenkov
• [x] @pnkfelix
• [ ] @scottmcm
• [ ] @varkor
• [ ] @withoutboats

Concerns:

• guidelines (https://github.com/rust-lang/rust/issues/53224#issuecomment-426454093)
• infinite_iter (https://github.com/rust-lang/rust/issues/53224#issuecomment-417830969)
• naming conventions 2 (@petrochenkov’s concern) (https://github.com/rust-lang/rust/issues/53224#issuecomment-415100630)
• overly-aggressive stylistic lints (https://github.com/rust-lang/rust/issues/53224#issuecomment-418545482)
• possible false positives (https://github.com/rust-lang/rust/issues/53224#issuecomment-418545482)
• some lints are opinionated on how the target platform ought to work 2 (https://github.com/rust-lang/rust/issues/53224#issuecomment-415100630)
• the descriptions of lints are a little too concise for decision making 2 (https://github.com/rust-lang/rust/issues/53224#issuecomment-415100630)
• tying compiler to stdlib implementation details 2 (https://github.com/rust-lang/rust/issues/53224#issuecomment-415100630)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nagisa]

To ensure that concerns are retained:

---

@rfcbot concern tying compiler to stdlib implementation details 2

Many of these lints are libstd specific (mostly pertaining to iterators). What mnemonic does clippy use to figure out what libstd items are what and whether the actual libstd is used at all and not e.g. use my_funky_crate as std;? How would the compiler deal with this without making half of the libstd a language item?

---

@rfcbot concern the descriptions of lints are a little too concise for decision making 2

For example, it is unclear from description of eq_op whether it does act on generic code or non-builtin-types, where all of the operators can be overridden to have arbitrary side effects.

---

@rfcbot concern some lints are opinionated on how the target platform ought to work 2

For example, the mut_from_ref lint prevents one from writing code like this, which is fairly common in my, admittedly C-centric, embedded programming experience:

// Gets a mutable reference to resource’s register bank
unsafe fn get_resource(x: &ResourceLock) -> &mut Resource {
    // THE_RESOURCE is linked by the linker into well known hardware operated range of memory.
    #[link_section="something_wellknown"]
    static mut THE_RESOURCE: Resource;
    take_resource_lock(x); // later released by the caller once they’re finished with the Resource
    &mut THE_RESOURCE
}

If it wasn’t for @japaric and WG-embedded, this is how I’d write my embedded code if I wasn’t really interested in exploring the design avenues that exact moment.

---

@rfcbot concern naming conventions 2 (@petrochenkov’s concern)
Lints in rustc follow some naming conventions.
A number of the lints proposed for uplifting doesn't follow the conventions.
Supposedly naming for the new lints needs to be harmonized with existing rustc lints.

[arielb1]

mut_from_ref: the description claims it's "trivially unsound", but it seems naïvely like such a signature could be plausible for some sort of allocator.

Sure. This would complain about RefCell<T> and any other type that implements a RefCell/Mutex, so all the implementors would have to #[allow] the lint.

[arielb1]

How would the compiler deal with this without making half of the libstd a language item?

#[rustc_XYZ] attributes? I agree that these sound like a better idea than doing name comparisons.

[Manishearth]

How would the compiler deal with this without making half of the libstd a language item?

I've suggested this before somewhere, we should have a concept of "diagnostic item" like lang item, which isn't required by the compiler but helps lints and diagnostics find things.

There are a bunch of diagnostics that could already benefit from this.

[oli-obk]

@petrochenkov I have addressed the naming convention issues

@nagisa wrt descriptions being too concise for decision making. Assume that there are no additional checks. The lint addresses all code that matches the description.

@nagisa wrt platform opinions, we can silence some lints on non-host platforms, but your example makes me want to lint this even more. While it is sound, it is most certainly unidiomatic and seems fragile/easy to get wrong. Especially with the ownership based embedded work that is going on.

We could just silence the lint if any arguments contain an UnsafeCell.

[leonardo-m]

What about the Clippy lint "match_overlapping_arm"? It could go well with the recent exhaustive_integer_patterns Nightly feature.

[clarfonthey]

Re: naming conventions, I strongly encourage you all to take a look at rust-lang-nursery/rust-clippy#2845 where I tried to augment some of the existing conventions for use in clippy.

[petrochenkov]

@rfcbot resolved naming conventions

[clarfonthey]

Also this is just one example of where naming conventions might conflict: this post suggests dropping_copy_types whereas I suggested drop_copy_types. The lint is specifically looking for calls to drop, and should explicitly name the function. "Dropping" copy types is completely natural and happens all the time. :p

[clarfonthey]

If people find it reasonable, I have a lot on my plate right now but I could propose another RFC specifically for lint naming conventions with lessons from clippy. I wouldn't necessarily block all uplifts until then but it may be worthwhile to have a larger discussion on this.

[joshtriplett]

I'd like to see for_loop_over_option and for_loop_over_result uplifted as well. (Note: Accidentally clicked the checkbox for the former, so I then un-clicked it.)

[joshtriplett]

@rfcbot concern infinite_iter

I think the infinite_iter lint might be excessively opinionated about desirable style. Looping over an infinite iterator doesn't seem like an inherent problem. On the other hand, an infinite iterator fed to collect definitely is.

[Manishearth]

I guess you're talking about cases where you're iterating over a receiver? Yeah, that makes sense.

@clarcharr Yeah, this sounds great! I don't think there needs to be a rush, and we can rename lints in clippy later too in a backwards compat way.

[joshtriplett]

I think possible_missing_comma should be uplifted as well.

[leonardo-m]

Are some Clippy (or Rustc warnings) good to become errors (instead of warnings) in future Rust versions?

[clarfonthey]

I should add that iter_step_by_zero will panic at runtime now, rather than looping infinitely. May not even be worth having a lint for something so specific.

[leonardo-m]

Avoiding one panic caused by zero step sounds good.

[oli-obk]

@nagisa are your concerns resolved?

[varkor]

(Reposting a previous comment because rfcbot doesn't take edits into account.)

@rfcbot concern overly-aggressive stylistic lints

Some of the lints discourage patterns that seem reasonable in practice.

• eq_op: sensible for integer and boolean types, but not generic types.
• bad_bit_mask: same concern as for eq_op.
• infinite_iter: while it's probably not good practice, I'm not sure it's something the compiler itself should warn about.
• mut_from_ref: the description claims it's "trivially unsound", but it seems naïvely like such a signature could be plausible for some sort of allocator.

---

@rfcbot concern possible false positives

With the current descriptions, some of the lints seem like they could behave incorrectly in some cases. If these are correct, the descriptions should be updated.

• ifs_same_cond: this ignores function calls in the condition, but there are other ways to cause effects within a condition. Does this lint definitely not have false positives?
• logic_bug: could the comment about ignoring short-circuiting behaviour be elaborated? What expressions will it warn against that might be effectful?
• absurd_extreme_comparisons: usize should probably be excluded?

---

Some other comments, but not blocking concerns.
not_unsafe_ptr_arg_deref: seems reasonable, though it could be helpful to address the limitations in "Known problems".

• wrong_transmute: the description doesn't go into which transmutes are actually forbidden.
• zero_width_space: where exactly is this caught? Is it caught inside strings for instance? Are there other characters it'd similarly make sense to forbid (such as other zero-width whitespace characters)?

All the other suggested lints seem sensible to me.

[leonardo-m]

Some lints could become errors in Rustc, like: wrong_transmute,
fn_to_numeric_cast_with_truncation, out_of_bounds_indexing, inline_fn_without_body.

[nagisa]

are your concerns resolved?

Assume that there are no additional checks. The lint addresses all code that matches the description.

I still think that this is not nearly enough. What I was really looking for is a well thought out list of corner cases (most of the lints’ descriptions neglect to mention even the obvious ones) & some sort of list if pros and cons we could amend as a part of decision making process. Most of the lints only specify the obvious case where they fire, but then there must exist samples of code which are not as clear-cut. Where exactly is the line drawn? I don’t want (and hardly have the attention span) to think about possible corner cases for 50 different lints at once.

Since making a decision for this many lints at once is infeasible. Perhaps an issue for each lint or a small number of lints (grouped according to some criteria) would be better, so that discussion does not mix this much?

I would be willing to mark my checkbox here provided that the purpose of this tracking issue is to select lints we want to consider for inclusion to rustc, each on their own merit.

platform opinions, we can silence some lints on non-host platforms, but your example makes me want to lint this even more. While it is sound, it is most certainly unidiomatic and seems fragile/easy to get wrong. Especially with the ownership based embedded work that is going on.

There are multiple different approaches we could take, sure. Just like I elaborated above, I think this tracking issue is not a great location to come to a consensus on that.

[scottmcm]

Did I miss a concise list somewhere of "here's why these but not the other are getting uplifted"?

Part of me feels that, even where I agree with the lints, that a bunch of these ought to be generalized if they'd go into rustc itself. For example, .step_by(0) feels like it should be some sort of "what inputs are sane" attribute on parameters that could be checked more generally. Or things like .unwrap_of(foo()) (https://github.com/rust-lang/rfcs/issues/2536) is roughly the same problem as .resize(n, HashMap::new()) (https://github.com/rust-lang/rfcs/issues/2551), so there could be a "did you want the lazy version of this?" attribute.

[JeanMertz]

If there is an agreement on naming conventions, and the rules that are used to determine if a lint should be uplifted, wouldn't it make more sense to split this issue into one single issue per proposed lint uplift?

That way, the discussion can stay focussed on that one lint, potential improvements can be discussed (and linked to PRs implementing that improvement), and when a specific lint is ready to be uplifted, it can be, without blocking on any discussion on other lints that are more debatable.

It would also make it easier to link back to decisions in the past for specific lints when talking about them in the future.

I'm a bystander here, but interested in this process, and having a hard time with all the different discussions crossing. I'm also concerned that it might leave some concerns unnoticed, because of the noise (although that's less of a concern with the rfcbot keeping track).

The obvious downside is having to comment on multiple issues when discussing multiple lints. So this would only make sense if there is an agreement on the general way to move forward with _all_ uplift proposals (so naming convention, level of documentation, and a general set of rules to determine if a lint can be uplifted or not). If there is no such agreement yet, then I would propose to postpone this issue, and figure that out first, in a separate issue, without the noise of talking about specific lint implementations.

[Centril]

I very much agree with @JeanMertz and @nagisa here; going through such a large list of lints at once is a daunting task and delays reaching consensus by a lot. I'd much rather we split things up and consider each lint one by one.

[withoutboats]

@rfcbot concern guidelines

I agree that trying to filter through these lints in a single thread is unworkable, but I am also not thrilled by the idea of each lint being considered individually without establishing a clear consensus regarding guidelines; that would result in inconsistent decision making.

When I brought this up in August, @oli-obk pointed out this guidance from the clippy 1.0 RFC:

I don't think the compler will want all correctness lints here, however if the lint is about a common enough situation where it being not a bug is an exceedingly rare case (i.e. very low false positive frequency) it should probably belong in the compiler.

This is good guidance, but my interpretation of this guidance would put quite a few of these lints out of the question, especially several of those highlighted in @varkor's previous comment. I'm not certain if this was an oversight or if I am applying the "very low false positive" standard more stringently than other users.

My own experience with the compiler is that I essentially never have a false positive warning, and when I do I am doing something really exceptionally strange. For example, I have had to allow dead code for a field that is never accessed because I want its destructor to run when this object is dropped: this is about the only time I've ever had to permanently allow a lint. I would perceive a change to this situation as a real drawback: false positives make me feel (wrongly) less confident about my understanding of the code and whether or not it is correct.

On the other hand, I have some objections outside of just the "low false positive" standard. Some of these lints are also indirect, highlighting that your code is probably wrong but without highlighting the problematic code. Take mut_from_ref for example: based on the signature of this function, it determines that you probably have UB in the function. But it doesn't identify the point in the function where the UB was introduced. Is this actually a net benefit as a part of the ordinary user flow (as opposed to in some static analysis tool users opt into explicitly)? I'm not so certain, I think it would be better if on-by-default lints always pointed you at the specific code you need to change to appease them.

So I think overall the next step should be to determine what the guidelines for correctness lints in the compiler actually are. For me, the requirements I prefer are:

1. The lint identifies a genuine bug; not bad code but a bug.
2. The lint has essentially no false positives.
3. The lint can identify exactly the code it considers to contain the bug.

Once we have consensus on the guidelines, people can go through individual lints and make a decision following those guidelines.

[leonardo-m]

The lint is identifies a genuine bug;

If it's a bug, then the compiler should raise a compilation error. If we talk about compiler warnings they are allowed to be about things that aren't bugs.

[Manishearth]

Btw, clippy's relicense is going through, so uplifting lints into Rust will be easy now.

https://github.com/rust-lang-nursery/rust-clippy/pull/3269

[lu-zero]

Would be possible to add https://github.com/rust-lang/rust/issues/54905 as well? :)

[Centril]

@withoutboats

To get this unstuck... Perhaps we can "evolve" the guidelines through the first lints we deal with... i.e. let's establish policy through precedent and do it more "by example".

Some of these lints are also indirect, highlighting that your code is probably wrong but without highlighting the problematic code. Take mut_from_ref for example: based on the signature of this function, it determines that you probably have UB in the function. But it doesn't identify the point in the function where the UB was introduced. Is this actually a net benefit as a part of the ordinary user flow (as opposed to in some static analysis tool users opt into explicitly)? I'm not so certain, I think it would be better if on-by-default lints always pointed you at the specific code you need to change to appease them.

I do think that if the compiler lints and determines that you probably have UB in the function, even if it is indirect, it is better than nothing. While it might not point directly point at the problem and a possible solution, it will hopefully at least make the user raise some questions which then leads to them searching for answers from various resources. The lint can also offer a link which provides more context in the description.

So I think overall the next step should be to determine what the guidelines for correctness lints in the compiler actually are. For me, the requirements I prefer are:

1. The lint identifies a genuine _bug_; not _bad code_ but a bug.

2. The lint has essentially no false positives.

3. The lint can identify exactly the code it considers to contain the bug.

I agree with 2, but I'm not so sure about 1 (for reasons noted below) and 3 (for reasons aforementioned).

We already have lints that identifies bad code such as non_standard_style, unused_attribute, etc. It seems to me that it is OK to lint against "bad code" but the bar for such lints are higher while the bar for identifying true bugs are lower.

[joshtriplett]

What's the current state of this? What are the next steps?

Is the plan to uplift clippy lints one by one? (Is there a finalized list?)

Does anyone want to take on the tasks of 1) working through the concerns one by one and 2) updating the RFC accordingly? (Is that the right next step here?)

[Centril]

Is the plan to uplift clippy lints one by one? (Is there a finalized list?)

imo that's the only feasible way to get this thing through.

I think we should go through each lint one by one in a separate issue with FCPs and then gradually coalesce around a way of applying our policy. That seems more efficient... we don't have to treat the lints as a package deal.

[Centril]

@rfcbot close

We discussed this sometime ago on a language team meeting and came to the conclusion that we don't think considering all lints together is going anywhere. Rather, we would be open to people filing issues or PRs to uplift particular lints on a case-by-case basis. When doing so, please the language team (@rust-lang/lang) and possibly also the compiler team. If you cannot ping the entire team, ping some folks in it and we can forward it. In some cases, we might just accept a PR or we might start a language team FCP. When deciding on whether to file a PR or issue, consider however that we might not accept all uplifts.

I'm leaving this issue open as a tracking issue.

[Centril]

Oops... wrong command;

@rfcbot cancel

[rfcbot]

@Centril proposal cancelled.

[nagisa]

Should we close this?

[Manishearth]

It might be worth opening a separate tracking issue that tracks these things, though.