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.

Sign up for GitHub

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

Jump to bottom

Lints sometimes don't trigger, with no discernable pattern #46695

Open
sgrif opened this issue Dec 12, 2017 · 5 comments
Open

Lints sometimes don't trigger, with no discernable pattern #46695

sgrif opened this issue Dec 12, 2017 · 5 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@sgrif
Copy link
Contributor

sgrif commented Dec 12, 2017

This has cropped up in Diesel for a long time, either manifesting as having to change some unrelated code because a lint started triggering that wasn't before, or occasionally noticing "oh hey you need to implement Copy on this type, why didn't the lint trigger?"

I finally have a case that I can report though, which I noticed before it started failing, and very clearly should be triggering. diesel-rs/diesel#1375 adds #[deny(missing_docs)] to the query_dsl module. However, query_dsl::boxed_dsl::BoxedDsl is a public item which has no documentation on it, and should be triggering the lint. I can even go in and add #[deny(missing_docs)] to that trait directly, and the lint still doesn't fire.

I'm not sure what other information I can add -- This is not specific to this one lint in this one case, we've had tons of cases of lints which should have always been failing randomly starting to fail due to changes in completely unrelated code.
@sgrif
Copy link
Contributor Author

sgrif commented Dec 12, 2017

This is potentially a more general report of #46164

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 12, 2017 •
edited
Loading

Both missing_docs and missing_copy_implementations are based on set of items "reachable from outside of the crate", so if this set changes somehow, new warnings may appear.
However I don't see recent changes in that area (https://github.com/rust-lang/rust/commits/master/src/librustc_privacy/lib.rs).

I should also add, the "reachable" set is very unreliable for crates defining macros 2.0, but it doesn't seem to be relevant for the linked Diesel crate.

@sgrif
Copy link
Contributor Author

sgrif commented Dec 12, 2017

This is an item which is 100% reachable from outside the crate. I've also observed this with lints unrelated to public items, such as random lints from clippy -- I can try to pull up some examples, I've only noticed those when they randomly start failing for no apparent reason

@sgrif
Copy link
Contributor Author

sgrif commented Dec 12, 2017

Here's another example: https://github.com/diesel-rs/diesel/pull/1350/files#diff-ed7f6c154205d63bf776e00598c41eb9

The only code that changed is some imports, as a trait got renamed. The method being referenced is fundamentally unchanged, and is also irrelevant to this lint that suddenly started triggering. This lint should have always been failing. Glancing through the source of that lint in clippy, I don't see any filtering there that would have changed as a result of this code change.

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-lints 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 Jan 30, 2018
HeroicKatora added a commit to nabijaczleweli/image that referenced this issue Apr 18, 2020
@HeroicKatora
Address lint of missing Copy
2105a62 
This lint (missing_copy_implementations) no longer seems to trigger in
more recent version of Rust. Some issues [#46695][1] seem to indicate
that this is based on detection of externally visible types that might
have improved—note that the types in question are private to their
module.

It should not be any risk to add this impl as it is not externally
visible. Consider the scenario where the types are made public by
extracting the modules into their own crate. In this case, the
discussion on the exhaustiveness of these representations will need to
occur in any case and be more SemVer restrictive than the Copy impl.

[1]: (rust-lang/rust#46695)
@Enselic Enselic added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Sep 24, 2023
@jieyouxu
Copy link
Member

Hi @sgrif, is this still happening on latest stable/nightly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Enselic @sgrif @pietroalbini @petrochenkov @jieyouxu