Regression: missing_debug_implementations linting on private items when used as parameter in trait impl

[Manishearth]

(As a lint regression it is not a true "breaking" regression, but it's still strange behavior, categorizing as a regression.)

Code

(Also reproducible with cargo check --no-default-features in https://github.com/unicode-org/icu4x/tree/3a00b1efc12d8fac2aa84b04f85508e3b75bca4c/provider/datagen)

#[deny(missing_debug_implementations)]
#[allow(unused)]

pub mod foo {
    struct Bar;

    impl<'a> TryFrom<Bar> for u8 {
        type Error = ();
        fn try_from(o: Bar) -> Result<Self, ()> {
            unimplemented!()
        }
    }
}

[09:57:34] मanishearth@manishearth-glaptop2 ~/sand/crates/bar O_o 
$ cargo +nightly c
    Checking bar v0.1.0 (/home/manishearth/sand/crates/bar)
error: type does not implement `Debug`; consider adding `#[derive(Debug)]` or a manual implementation
 --> src/lib.rs:5:5
  |
5 |     struct Bar;
  |     ^^^^^^^^^^^
  |
note: the lint level is defined here
 --> src/lib.rs:1:8
  |
1 | #[deny(missing_debug_implementations)]
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `bar` (lib) due to previous error

Version it worked on

It most recently worked on: nightly-2023-05-06

Version with regression

rustc --version: nightly-2023-05-07

[Manishearth]

searched nightlies: from nightly-2023-05-05 to nightly-2023-05-07
regressed nightly: nightly-2023-05-07
searched commit range: f9a6b71...a77c552
regressed commit: 31a4f2d

bisected with cargo-bisect-rustc v0.6.6

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2023-05-05 --end 2023-05-07 -- check --no-default-features 

[Manishearth]

Regression comes from #110907

[Manishearth]

The issue is that Bar now appears in the type parameters of the the TryFrom impl of u8. However that doesn't mean it's reachable, this is effectively a "private" TryFrom impl: there is no way to invoke this impl to get this type.

[petrochenkov]

In theory #110907 wasn't supposed to change the algorithm, only to make the collected data more detailed.
So if we didn't reach type parameters previously, then we shouldn't reach them now, and vice versa.

cc @Bryanskiy, we need to figure out why there's a difference.

[petrochenkov]

However that doesn't mean it's reachable, this is effectively a "private" TryFrom impl: there is no way to invoke this impl to get this type.

Type inference is sometimes smarter than you'd expect, see e.g. examples in #57344, so we may have to conservatively consider generic arguments of impls as reachable.

[petrochenkov]

In general, the reachable set is an upper limit rather than something that can be determined precisely, so we can overshoot slightly (or not so slightly in cases where macros 2.0 are in scope), so lints based on it should generally be ready to require doc comments / stability attributes / copy or debug imps on items where they are not strictly necessary.

[Manishearth]

Yeah, that seems fair. Stepping back, perhaps the question becomes: should this lint be really dealing with reachability or actual "intentional" publicity?

[petrochenkov]

Reachable means "objects of this type may be accessible to other crates, even if the type itself cannot be named", so it's also public of sorts, and you may want to pass such objects to dbg!().

[petrochenkov]

https://internals.rust-lang.org/t/privacy-and-its-interaction-with-docs-lints-and-stability/2880

[Manishearth]

Yes, I understand, this is why I am talking about "intentional" publicity. Such types may end up being passed to dbg!(), so yes, there are potentials for false negatives here, but currently we have a lot of false positives for types that cannot be constructed in such ways. And I think it's definitely a grey area for such types in the first place; normally you can call very limited things on these unnameable types anyway, it seems fine for me for us to not nudge people too hard in that direction.

[petrochenkov]

This should be fixed in #111425, by restricting the lint to nominally pub items only.

[Manishearth]

Thanks!

[kpreid]

I found this issue while reviewing newly-occurring-on-nightly lints in my code, and I observe that:

The documentation of missing_debug_implementations doesn't say that it cares at all about visibility or reachability. If it is intended to apply strictly to pub items then I think that should be documented to avoid confusion, but it isn't so documented, so was that in fact intended? (Happy to write a docs PR once I know what's intended.)