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

Uplift lints from clippy to rustc #53224

Closed
32 of 49 tasks
oli-obk opened this issue Aug 9, 2018 · 60 comments
Closed
32 of 49 tasks

Uplift lints from clippy to rustc #53224

oli-obk opened this issue Aug 9, 2018 · 60 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2018

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

@oli-obk oli-obk added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 9, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 9, 2018

@rfcbot fcp merge

please raise concerns about specific lints directly with rfcbot

@rfcbot
Copy link

rfcbot commented Aug 9, 2018

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

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 9, 2018
@Mark-Simulacrum
Copy link
Member

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
Copy link
Member

nrc commented Aug 9, 2018

cc @rust-lang/compiler

@eddyb
Copy link
Member

eddyb commented Aug 10, 2018

@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
Copy link
Contributor Author

oli-obk commented Aug 10, 2018

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

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented Aug 10, 2018

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

Related: #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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 10, 2018
@nagisa
Copy link
Member

nagisa commented Aug 10, 2018

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 10, 2018
@Manishearth
Copy link
Member

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
Copy link
Contributor

@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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 12, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 13, 2018

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 14, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

oli-obk commented Aug 14, 2018

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
Copy link
Member

Manishearth commented Aug 14, 2018 via email

@varkor
Copy link
Member

varkor commented Aug 14, 2018

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
Copy link
Contributor

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/rust-clippy#2845, rust-lang/rust-clippy#2846). They came up with some naming rules there too.

@Centril
Copy link
Contributor

Centril commented Aug 15, 2018

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 ^,-

@lu-zero
Copy link
Contributor

lu-zero commented Oct 23, 2018

Would be possible to add #54905 as well? :)

@Centril
Copy link
Contributor

Centril commented Nov 20, 2018

@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
Copy link
Member

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
Copy link
Contributor

Centril commented Apr 11, 2019

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 Centril added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jan 9, 2020
@Centril
Copy link
Contributor

Centril commented Jan 9, 2020

@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
Copy link
Contributor

Centril commented Jan 9, 2020

Oops... wrong command;

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Jan 9, 2020

@Centril proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 9, 2020
@nagisa
Copy link
Member

nagisa commented Aug 18, 2020

Should we close this?

@Manishearth
Copy link
Member

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

ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Sep 22, 2020
…-obk

Uplift `temporary-cstring-as-ptr` lint from `clippy` into rustc

The general consensus seems to be that this lint covers a common enough mistake to warrant inclusion in rustc.
The diagnostic message might need some tweaking, as I'm not sure the use of second-person perspective matches the rest of rustc, but I'd like to hear others' thoughts on that.

(cc rust-lang#53224).

r? @oli-obk
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 27, 2020
Uplift `temporary-cstring-as-ptr` lint from `clippy` into rustc

The general consensus seems to be that this lint covers a common enough mistake to warrant inclusion in rustc.
The diagnostic message might need some tweaking, as I'm not sure the use of second-person perspective matches the rest of rustc, but I'd like to hear others' thoughts on that.

(cc rust-lang#53224).

r? `@oli-obk`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 22, 2023
…lints, r=fee1-dead

Rename `{drop,forget}_{copy,ref}` lints to more consistent naming

This PR renames previous uplifted lints in rust-lang#109732 to more consistent naming.

I followed the renaming done [here](rust-lang#53224) and also advocated in this [clippy issue](rust-lang/rust-clippy#2845):
   - `drop_copy` to `dropping_copy_types`
   - `forget_copy` to `forgetting_copy_types`
   - `drop_ref` to `dropping_references`
   - `forget_ref` to `forgetting_references`
oli-obk pushed a commit to oli-obk/miri that referenced this issue May 23, 2023
…fee1-dead

Rename `{drop,forget}_{copy,ref}` lints to more consistent naming

This PR renames previous uplifted lints in rust-lang/rust#109732 to more consistent naming.

I followed the renaming done [here](rust-lang/rust#53224) and also advocated in this [clippy issue](rust-lang/rust-clippy#2845):
   - `drop_copy` to `dropping_copy_types`
   - `forget_copy` to `forgetting_copy_types`
   - `drop_ref` to `dropping_references`
   - `forget_ref` to `forgetting_references`
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jul 18, 2023
…fee1-dead

Rename `{drop,forget}_{copy,ref}` lints to more consistent naming

This PR renames previous uplifted lints in rust-lang/rust#109732 to more consistent naming.

I followed the renaming done [here](rust-lang/rust#53224) and also advocated in this [clippy issue](rust-lang/rust-clippy#2845):
   - `drop_copy` to `dropping_copy_types`
   - `forget_copy` to `forgetting_copy_types`
   - `drop_ref` to `dropping_references`
   - `forget_ref` to `forgetting_references`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests