Fix false positives for extra_unused_type_parameters

[mkrasnitski]

Don't lint external macros. Also, if the function body is empty, any type parameters with bounds on them are not linted. Note that only the body needs be empty - this rule still applies if the function takes any arguments.

fixes #10318
fixes #10319
changelog: none

[rustbot]

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

[mkrasnitski]

r? @flip1995 since you originally reviewed #10028 as well

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit 8789b37 has been approved by flip1995

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8789b37 with merge 595f783...

[flip1995]

This also makes me wonder if we should lint type params with bounds in the first place or if that is a bit too much for a warn-by-default lint. We could make this configurable.

Nominating for the next meeting.

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 595f783 to master...

[flip1995]

This also makes me wonder if we should lint type params with bounds in the first place or if that is a bit too much for a warn-by-default lint. We could make this configurable.

Nominating for the next meeting.

Result of the meeting: With this fix, this should be good to go as-is into beta next week. I'll run a lintcheck with this lint before the sync, but if nothing suspicious comes up, there's nothing left to do.

[mkrasnitski]

Sounds good - is lintcheck like a crater run but for clippy?

[flip1995]

Yes, exactly. There is a predefined list in the lintcheck dir in the Clippy repo, but also https://github.com/matthiaskrgr/clippy-lintcheck which does things on a slightly bigger scale.

[flip1995]

FP in CXX:

    /// `CxxString` is not constructible via `new`. Instead, use the
    /// [`let_cxx_string!`] macro.
    pub fn new<T: Private>() -> Self {
        unreachable!()
    }

Private is a public trait in a private module, so that it can't be implemented for external types. Maybe checking if a bound is_exported would address this FP.

---

Pretty much the same thing in matrixmultiply:

    /// Decide how many threads to use in each loop
    pub(crate) fn new<K>(m: usize, k: usize, n: usize, max_threads: usize) -> Self
        where K: GemmKernel

GemmKernel is a pub(crate) trait in a private module.

---

In protobuf:

    pub fn new<O>(name: &'static str) -> GeneratedOneofDescriptorData
    where
        O: OneofFull,

I think this can be removed. Maybe it is in generated code and that's why it is there? 🤔

However, this made me notice that this lint should respect the avoid_breaking_exported_api conf value and not lint on public / exported functions.

---

Tokio-io:

pub use self::async_read::AsyncRead;
pub use self::async_write::AsyncWrite;

fn _assert_objects() {
    fn _assert<T>() {}
    _assert::<Box<dyn AsyncRead>>();
    _assert::<Box<dyn AsyncWrite>>();
}

My best guess is, that this asserts that the AsyncRead and AsyncWrite traits are object safe.

So maybe extend the fix in this PR to not only type params with bounds, but general params.

---

@mkrasnitski Will you get to fixing the above issues today / tomorrow before the sync? If not, I would move it to pedantic for now and then we will make it warn-by-default (complexity) one release cycle later.

---

I ran this lint on the most popular > 400 crates regarding downloads and dependencies that are out there.
crates.toml < Those are the crates

[mkrasnitski]

Thanks for finding these. I can take care of this today (don't want to wait another 6 weeks to have the fix land).

[mkrasnitski]

I'm unsure how to get the required LocalDefId for each of the bounds to check if they're is_exported. Would you happen to know? Should I be using their TraitRef in some way?

[mkrasnitski]

I don't think the matrixmultiply example is a false positive. Since new is also marked pub(crate), I think the lint should apply there. There's a #[cfg] branch where K goes unused, so I think it's a valid application of the lint.

So, I think I should maybe check for cases where the function is exported but the bound is not - this by itself is pretty rare I would guess, since you can't just bound on a private trait, as that produces a compiler error saying you aren't allowed to leak the private trait. I'm actually surprised that you're allowed to leak the trait if it's inside of a private module.

[flip1995]

So, I think I should maybe check for cases where the function is exported but the bound is not - this by itself is pretty rare I would guess

I think that would be overkill. Just checking if the bound is exported should be enough. Maybe this introduces a FN in this case, but as you said, this is pretty rare and then we err on the safe side.

Should I be using their TraitRef in some way?

That sounds reasonable to me.

[mkrasnitski]

Opened #10392 to address the new FPs.