avoid suggesting traits from private dependencies

[Qelxiros]

fixes #142676
fixes #138191

r? @tgross35

[tgross35]

Oh that's a wonderfully small fix.

Were you able to reproduce this with something other than object and compiler_builtins? The test here is good to have, but it would also be good to extend it with an aux build that puts some known trait into scope (in case object/builtins ever wind up with a different API or not being deps). As an example of that, check out the tests at https://github.com/rust-lang/rust/tree/bc4376fa73b636eb6f2c7d48b1f731d70f022c4b/tests/ui/privacy/pub-priv-dep.

Just make sure it actually fails without the fix since I know this is a bit touchy.

[tgross35]

I think it might be better to rename all_traits to all_visible_traits (and update its usage) here https://github.com/rust-lang/rust/blob/2f3d599989883d0be932df3dceed65a6ec3274f8/compiler/rustc_hir_typeck/src/method/suggest.rs#L4387C15-L4389, then have that function call visible_traits

rust/compiler/rustc_middle/src/ty/context.rs

Lines 2320 to 2335 in bc4376f

	/// All traits in the crate graph, including those not visible to the user.
	pub fn all_traits(self) -> impl Iterator<Item = DefId> {
	iter::once(LOCAL_CRATE)
	.chain(self.crates(()).iter().copied())
	.flat_map(move |cnum| self.traits(cnum).iter().copied())
	}
	/// All traits that are visible within the crate graph (i.e. excluding private dependencies).
	pub fn visible_traits(self) -> impl Iterator<Item = DefId> {
	let visible_crates =
	self.crates(()).iter().copied().filter(move |cnum| self.is_user_visible_dep(*cnum));
	iter::once(LOCAL_CRATE)
	.chain(visible_crates)
	.flat_map(move |cnum| self.traits(cnum).iter().copied())
	}

. That seems likely to catch future problems that don't go through suggest_valid_traits.

@estebank any opinions here?

[estebank]

Maybe renaming all_traits to all_traits_inclusing_private or something else verbose? visible_traits is what most people want/need so it should have the shorter, more findable name.

[tgross35]

The actual tcx methods you mean? That seems reasonable to me to avoid the footgun. It's only named in ~10 places so @Qelxiros feel free to either do that in a second commit in this PR, or as a separate PR.

[Qelxiros]

I'm happy to add it here. As a side note, visible_traits above uses TyCtxt::is_user_visible_dep, which allows imports from direct private dependencies. Using that instead of is_private_dep still removes the erroneous suggestions, is more in line with my mental model of what I'm trying to do here, and aligns with the documentation of is_user_visible_dep:

rust/compiler/rustc_middle/src/ty/util.rs

Lines 838 to 839 in bc4376f

	/// dependency, or a [direct] private dependency. This is used to decide whether the crate can
	/// be shown in `impl` suggestions.

I also think that this will preserve the compiler's ability to make suggestions like these for library developers. Am I missing anything here?

[rustbot]

HIR ty lowering was modified

cc @fmease

[Qelxiros]

I'm assuming that ping is due to the function rename, see #143038 (comment) for context

[Qelxiros]

@estebank @tgross35 anything preventing merging this?

[tgross35]

LGTM as well, thank you for the changes!

One request if possible, are you able to adjust the history a bit? Split commits like:

1. Add the tests and run+bless it so we have the current failures in history (this is purely nice to have, feel free to disregard this if it's tricky)
2. Make compiler-builtins a private dep
3. Add the fix and update the test (everything else)

This is so we will be able to backport only the library changes (second commit) to beta, which should fix #143215.

[tgross35]

help: trait CastFrom which provides cast_from_lossy is implemented but not in scope; perhaps you want to import it

Awesome to see the failure reproduced in the first commit since I was having trouble getting it to work locally. Thank you for all the work here!

@bors r+

[bors]

📌 Commit 92e5103 has been approved by tgross35

It is now in the queue for this repository.

[tgross35]

@rust-lang/libs nominating commit 208ab58 1e6e4bb for backport, which should fix #143215. The remaining commits aren't needed in beta since they are diagnostic changes.

@rustbot label +I-libs-nominated +beta-nominated

(updated team labels for the backport)

[tgross35]

@bors r-

[tgross35]

Since read_at is only available on Unix via FileExt, I guess that makes the rest of the diagnostic not cross-platform (which is preexisting). I think you could make this cross-platform by adding something like

// By default, the `read` diagnostic suggests `std::os::unix::fs::FileExt::read_at`. Add 
// something more likely to be recommended to make the diagnostic cross-platform. 
trait DecoyRead {
    fn read1(&self) {}
}
impl<T> DecoyRead for Vec<T> {}

[tgross35]

@bors2 try jobs=test-various

[rust-bors]

⌛ Trying commit 6b824e8 with merge 9508371…

To cancel the try build, run the command @bors2 try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9508371 (9508371619dabdc5448e5949c21860d8f0afee61, parent: 6677875279b560442a07a08d5119b4cd6b3c5593)

[tgross35]

@bors r+

[bors]

📌 Commit 6b824e8 has been approved by tgross35

It is now in the queue for this repository.

[tgross35]

Removing the nomination since #143660 fixes it instead