# Idiom lints

## Structure

* Tracking issue
* Code example that triggers the lint
* Code example that fixes it
* Migration supported
* Current state
* Rationale in a nutshell
* Concerns folks have raised
* Proposed resolution

## Terminology

* **Migration**: Hard error in Rust 2021, warning in earlier editions
* **Idiom lint**: Deny-by-default in Rust 2021, warning in earlier editions

Lint Groups https://doc.rust-lang.org/nightly/rustc/lints/groups.html

Specifically, the `rust-2018-idioms` group consists of these:
- bare-trait-objects
- elided-lifetimes-in-paths
- ellipsis-inclusive-range-patterns
- explicit-outlives-requirements
- unused-extern-crates

## BARE_TRAIT_OBJECTS

Tracking issue: https://github.com/rust-lang/rust/pull/81244, https://github.com/rust-lang/lang-team/issues/65

* Code example that triggers the lint

let x: Box<std::fmt::Debug>;

fn process(iter: &mut Iterator<Item = i32>)

* Current status: warn
* Code example that fixes it

let x: Box<dyn std::fmt::Debug>;

fn process(iter: &mut dyn Iterator<Item = i32>)

* Migration supported

 --> src/main.rs:2:12
  |
2 | let x: Box<std::fmt::Debug>;
  |            ^^^^^^^^^^^^^^^ help: use `dyn`: `dyn std::fmt::Debug`
  |

* Known issues
- Not detecting `dyn`s needed for associated items (upgrading `Foo::bar` to `<dyn Foo>::bar`) https://github.com/rust-lang/rust/issues/65371
* Rationale in a nutshell
- Many, many people get confused by the difference between traits and types without this. (Past-Scott thought that trait generics existed because it *looks* like you can pass a trait to a generic parameter. That confusion disappears if there's an error saying "that's a trait", and pointing to documentation about trait objects.)
- `Foo<a + b>` currently gets treated as `Foo<dyn a + b>`, leading to confusing errors (https://github.com/rust-lang/rust/pull/77502#discussion_r499172677), making it harder to suggest `Foo<{a + b}>`.
- `Animal::speek` currently needs to look for `<dyn Animal>::speek`, leading to confusing errors (https://github.com/rust-lang/lang-team/issues/65#issuecomment-766453773), making it harder to suggest `Animal::speak`.
- `let x: u32 = <Default>::default();` sends the compiler off on a tirade about object safety (https://github.com/rust-lang/rust/issues/51077), but if `dyn` was required this could just say "don't put traits in `<>`s".
- rustc could steer people towards *either* `dyn` or `impl`.
* Proposal: soft deprecation; edition error

## ELIDED_LIFETIMES_IN_PATHS

Maybe the in-band lifetimes tracking issue? https://github.com/rust-lang/rust/issues/44524

* Code example that triggers the lint

pub fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {}

pub fn fmt(&self) -> Ref<T> {}

* Code example that fixes it

pub fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {}

pub fn fmt(&self) -> Ref<'_, T> {}

* Migration supported

3 | fn make_foo(x: &i32) -> Foo {
  |                         ^^^- help: indicate the anonymous lifetime: `<'_>`

* Current state
* Not enabled by default in 2018: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0f5b391efe0e8d73eede6028678245ee
* Rationale in a nutshell
* It's important to know when lifetimes are tied together, because of the large impact that has on how the values can be produced and used.
* It's important to know when types contain lifetimes because that affects what you can do with them even when `.to_owned()`ing them or trying to put them into `Box`es.
* These are required in other places, like `impl` blocks, so requiring them everywhere is more consistent.
* Concerns folks have raised
* eddyb: This is important for "lifetime relationships", not for lifetimes themselves https://github.com/rust-lang/rust/issues/44524#issuecomment-422384048
* But Niko disagrees: https://github.com/rust-lang/rust/issues/44524#issuecomment-422505328
* Possibilities/Conversation Topics:
* Would be nice to have the "you could use `'_` here" lint to go along with this, as otherwise it may result in excess named lifetimes.
* Should it split to parameters vs return type? It might be expedient to move the parts along separately...
* What's the balance between what's in code vs what's in rustdoc here?
* Proposed resolution
- split the lint:
- participating in lifetime elision
- not participating in lifetime elision
* Notes
* Reasons in favor to at least warn
* nikomatsakis finds it really helpful to see where lifetimes are; very helpful esp. when people come with questions about why they're getting an error
* joshtriplett would like the reminder that the type has a lifetime, and wants to be required to acknowledge that. joshtriplett would also like to see that when reading other people's code.
* It's nice for the error messages to have something to point at.
* Reasons against
* it's annoying, cramertj hates writing `&mut Context<'_>` because he has to write it so often and knows the type very well
* as a newcomer, definitely useful; as an experienced user, not so much
* maybe address with a simpler syntax, if we plan to address? (e.g., something easier to type)
* completely hypothetical syntaxes: `&mut Formatter<'>`, or `&mut Formatter'`
* IDE usage may help, but do we want to rely on that? That only addresses *typing* it, in any case, and seeing it is also a kind of problem
* Random thought:
* Is there a material difference between the `'_` in `Foo<'_>` vs `&Foo<'_>`?
* Is it "covered" by the outer lifetime in some sense?
* Spitting the lint
* 1. `fn foo(x: Ref<i32>) -> Ref<u32>`
* Argument is participating in lifetime elision too, not just the return position.
* 2. `fn foo(x: Ref<i32>)` -- want it because it's kind of unclear if this is borrowed or not
* 3. `fn foo(x: &Ref<i32>)` -- feels 'covered' by the &, less important
* 4. `fn foo(x: &mut Ref<i32>)` -- may store to it, feels more important
* Group 1 and 2 together as `elided_lifetimes_in_paths`
* But 2 isn't tying lifetimes together, so Scott's not convinced that they're the same category.
* Group 3 as .. `elided_lifetimes_in_arguments` ("elided trivial lifetime"?)
* Settings:
* elided-lifetimes-in-paths
* elided-lifetimes-in-arguments
* Argument about splits
* Figuring out the right place to split is hard, need something we can explain to people if they are treated differently
* "Always need to show except in the case where it's covered by a reference (in a fn argument list)"

## ELLIPSIS_INCLUSIVE_RANGE_PATTERNS

* Tracking issue ???
* Current status: warn
* Code example that triggers the lint

match foo {
    'a' ... 'z' => 22,
    _ => 44,
}

* Code example that fixes it

match foo {
    'a' ..= 'z' => 22,
    _ => 44,
}

* Migration supported: :green_heart:
* Proposal: soft deprecation; edition error
* Why?
* Reclaim `...`
* Been quite a while
* Could deny-by-default on 2018
* Teaches people not the thing to use in a stronger way
* Doing on the new edition has a similar effect, but on a slower timescale

## EXPLICIT_OUTLIVES_REQUIREMENTS

* Tracking issue: ???
* Code example that triggers the lint

struct SharedRef<'a, T>
where
    T: 'a,
{
    data: &'a T,
}

* Code example that fixes it

struct SharedRef<'a, T>
{
    data: &'a T,
}

* Migration supported
* Current state: Allow
* Rationale in a nutshell
* Having a reference with lifetime `'a` to an item implicitly infers that the item itself lives for at least `'a`.
* Concerns folks have raised
* Some false positives: https://github.com/rust-lang/rust/issues/54630
* Does the bound still show in rustdoc?
* With an old MSRV, maybe people would want this even if it's not needed on current versions?
* Discussion topic:
* Is this even an edition thing? How about everywhere?
* Proposed resolution
* ~~Warn-by-default in new edition? Or just in all of them?~~
* Not for edition 2021, so worry about it on the tracking issue later.

## UNUSED_EXTERN_CRATES

* Tracking issue: ???
* Code example that triggers the lint

extern crate proc_macro;

* Code example that fixes it

//extern crate proc_macro;

* Current state: Allow
* Rationale in a nutshell
* extern crate isn't needed any more with the new path system
* Concerns folks have raised
* https://github.com/rust-lang/rust/issues/53964
* https://github.com/rust-lang/rust/issues/44294
* Proposed resolution
* In favor of warning *if we know it will work* to just delete it
* This implies cannot warn on Rust 2015, so edition related in this way
* Question from Niko:
* Does this work in 2015?
* Answer: some changes were backported to 2015, but not this

# Other lints not in `rust-2018-idioms`

These were added to https://github.com/rust-lang/rust/issues/80165 at some point, but were not considered idiom lints when the group was made for the previous (2018) edition.

## UNREACHABLE_PUB
* Tracking issue: ???
* Code example that triggers the lint

mod m {
    #[allow(unused)] 
    pub struct Item;
}

* Code example that fixes it

mod m {
    #[allow(unused)] 
    struct Item;
}

* Migration supported
* Current state: Allow
* Rationale in a nutshell
* Items that are not publically exported should not be marked as such
* Concerns folks have raised
* False positives: https://github.com/rust-lang/rust/issues/64762, https://github.com/rust-lang/rust/issues/52665
* This is commonly used for sealed stuff, no?
* Generally we feel we would be ok to have an allow if it's "worth a comment"
* Proposed resolution
* Soft deprecation, but not an edition issue

## MACRO_USE_EXTERN_CRATE

* Tracking issue: ???
* Code example that triggers the lint

#[macro_use]
extern crate serde_json;
fn main() {
    let _ = json!{{}};
}

* Code example that fixes it

fn main() {
    let _ = serde_json::json!{{}};
}

or

use serde_json::json;
fn main() {
    let _ = json!{{}};
}

* Migration supported
* Current state: Allow
* [Fix works in Rust 2015](https://play.rust-lang.org/?version=stable&mode=debug&edition=2015)
* Rationale in a nutshell
* Since Rust 2018, the preferred way to refer to macros from third-party crates is through their path.
* Concerns folks have raised
* Undecided whether this is actually the way to go (though with pub macro_rules, that may no longer be the case): https://github.com/rust-lang/rust/issues/52043
* Proposed resolution
* Not edition specific, can soft deprecate later if we want

## CLASHING_EXTERN_DECLARATIONS

mod m {
    extern "C" {
        fn foo();
    }
}

extern "C" {
    fn foo(_: u32);
}

- Current state:
- Warn-by-default
- Comments:
- Is there a legit use for this?
- We believe no, basically UB at the llvm level
- You can't really control which signature gets selected
- If there are multiple valid ways to call it, you can use `...`
- maybe you can use `link_name`
- The lint does not fire if the arguments are the same, so you can declare twice with compatible signatures
- One possible use case might be `Option<&T>` and `*const T`
- these could be distinct lints
- Motivation:
- This is a soundness hole
- We can't detect this 100% (most notably cross crates)
- Proposal:
- Bug fix, phase in
- if somebody raises a use during deny-by-default phase, then we can revisit

## CONST_ITEM_MUTATION

const FOO: [i32; 1] = [0];

fn main() {
    FOO[0] = 1;
}

- Current state:
- Warn-by-default
- Questions
- Is this actually common?
- It comes up on Discord decently often
- People want statics
- `{FOO}[0] = 1` wouldn't lint?
- Comments
- Not UB but almost certainly long
- "Has defined semantics but is almost guaranteed to be wrong"
- Proposed resolution
- Not edition but an interesting question
- Might fit "Deny by Default" in a similar way to overflowing literals (see above category for overflowing literals)

## TYPE_ALIAS_BOUNDS

type SendVec<T: Send> = Vec<T>;

// T: Iterator is required here
type SendVec<T: Iterator> = Vec<T::Item>;

// you can rewrite to
type SendVec<T> = Vec<<T as Iterator>::Item>;

// common case was `T`...
type SendVec<T> = Box<T>;

- Current state
- warn-by-default
- Questions:
- Do we need to know what this will do in order to turn this into an error?
- Comments:
- Don't want to flip-flop on semantics. If we deny now, only to relatively soon allow this, are we causing confusion?
- Also, we don't know what to implement in next edition, and it's not clear that having people remove bounds is actually putting us in a better position for the next edition.
- Proposed resolution
- Leave it for now

## Future Compatibility Warnings

Sense:

* These are mostly bug fixes that should apply independent of edition, we should just do this
* Someone might want to look in and see if there is some particular case to champion--
* would be looking for a case that is not actually a bug fix, sort of like bare traits, but more of a deprecation, and hence a candidate for edition error

Meeting conversation:
- Should we flip these for the edition because we can?
- Should we not flip them because they need to happen everywhere?
- Do we want a case-by-case decision so we can do the `cargo fix`-able ones?

### WHERE_CLAUSE_OBJECT_SAFETY

* Tracking issue: https://github.com/rust-lang/rust/issues/51443
* Concerns:
* Need review to decide if the fix is appropriate
* Has notable impact on the issue over time

### SAFE_PACKED_BORROWS

#[repr(packed)]
pub struct Unaligned<T>(pub T);

pub struct Foo {
    start: u8,
    data: Unaligned<u32>,
}

fn main() {
    let x = Foo { start: 0, data: Unaligned(1) };
    let y = &x.data.0;
}

### CENUM_IMPL_DROP_CAST

enum E {
    A,
}

impl Drop for E {
    fn drop(&mut self) {
        println!("Drop");
    }
}

fn main() {
    let e = E::A;
    let i = e as u32;
}

### COHERENCE_LEAK_CHECK

trait SomeTrait { }
impl SomeTrait for for<'a> fn(&'a u8) { }
impl<'a> SomeTrait for fn(&'a u8) { }

### CONST_EVALUATABLE_UNCHECKED

fn test<T>() {
    let _ = [0; foo::<T>()];
}

### ILLEGAL_FLOATING_POINT_LITERAL_PATTERN

let x = 42.0;

match x {
    5.0 => {}
    _ => {}
}

### INDIRECT_STRUCTURAL_MATCH

struct NoDerive(i32);
impl PartialEq for NoDerive { fn eq(&self, _: &Self) -> bool { false } }
impl Eq for NoDerive { }
#[derive(PartialEq, Eq)]
struct WrapParam<T>(T);
const WRAP_INDIRECT_PARAM: & &WrapParam<NoDerive> = & &WrapParam(NoDerive(0));
fn main() {
    match WRAP_INDIRECT_PARAM {
        WRAP_INDIRECT_PARAM => { }
        _ => { }
    }
}

### LATE_BOUND_LIFETIME_ARGUMENTS

struct S;

impl S {
    fn late<'a, 'b>(self, _: &'a u8, _: &'b u8) {}
}

fn main() {
    S.late::<'static>(&0, &0);
}

### MUTABLE_BORROW_RESERVATION_CONFLICT

let mut v = vec![0, 1, 2];
let shared = &v;
v.push(shared.len());

### NONTRIVIAL_STRUCTURAL_MATCH

#[derive(Copy, Clone, Debug)]
struct NoDerive(u32);
impl PartialEq for NoDerive { fn eq(&self, _: &Self) -> bool { false } }
impl Eq for NoDerive { }
fn main() {
    const INDEX: Option<NoDerive> = [None, Some(NoDerive(10))][0];
    match None { Some(_) => panic!("whoops"), INDEX => dbg!(INDEX), };
}

### PRIVATE_IN_PUBLIC

struct SemiPriv;

mod m1 {
    struct Priv;
    impl super::SemiPriv {
        pub fn f(_: Priv) {}
    }
}

### PROC_MACRO_DERIVE_RESOLUTION_FALLBACK

// foo.rs
#![crate_type = "proc-macro"]

extern crate proc_macro;

use proc_macro::*;

#[proc_macro_derive(Foo)]
pub fn foo1(a: TokenStream) -> TokenStream {
    drop(a);
    "mod __bar { static mut BAR: Option<Something> = None; }".parse().unwrap()
}

// bar.rs
#[macro_use]
extern crate foo;

struct Something;

#[derive(Foo)]
struct Another;

fn main() {}

### UNINHABITED_STATIC

enum Void {}
extern {
    static EXTERN: Void;
}

### UNSUPPORTED_NAKED_FUNCTIONS

#[naked]
pub fn f() -> u32 {
    42
}