Exclude dependencies of `std` for diagnostics #135278

tgross35 · 2025-01-09T06:27:10Z

Currently crates in the sysroot can show up in diagnostic suggestions, such as in #135232. To prevent this, duplicate all_traits into visible_traits which only shows traits in non-private crates.

Setting #![feature(rustc_private)] overrides this and makes items in private crates visible as well, since rustc_private enables use of std's private dependencies.

This may be reviewed per-commit.

Fixes: #135232

rustbot · 2025-01-09T06:27:18Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

tgross35 · 2025-01-09T06:28:39Z

Unsure whether adding a query is okay or if this should just be a filtering method on tcx. I am still figuring out a good way to test this so

@rustbot author

tgross35 · 2025-01-09T06:42:14Z

Another option is to drop stdlib_private_dep and just use private_dep in the query. I didn't do this originally because I thought private_dep was more tightly coupled to the public_private_dependencies feature, but maybe this isn't the case.

Open to suggestions here.

tgross35 · 2025-01-13T03:03:45Z

Okay, I think I have something workable here. I dropped stdlib_private_dep in favor of reusing is_private_dep, and added a repro.

@rustbot ready

SparrowLii

This change makes sense, but I'm not sure it is the best for diagnostics' design.
@compiler-errors Could you help have a review?

SparrowLii · 2025-01-13T03:56:31Z

compiler/rustc_middle/src/ty/context.rs

    pub fn all_traits(self) -> impl Iterator<Item = DefId> + 'tcx {
        iter::once(LOCAL_CRATE)
-            .chain(self.crates(()).iter().copied())
+            .chain(self.visible_crates(()).iter().copied())


Do we have to change this? It makes sense to analyze trait candidates from private dependencies IMO, we just need to exclude it from diagnostics

And if we not change it here, we can avoid to add a new query since visible_crates only be used in the query all_diagnostic_items

yeah, this seems wrong. all_traits is also used by things like SMIR and rustdoc in ways that are not just diagnostics. if you want to distinguish "all visible traits" from "all traits", and then use the former in diagnostics, then it should be more explicit.

I wasn't sure about this one, but yeah that makes sense. I guess there are a couple questions - should the query be kept or should it just filter as-needed? My original thought was that there could be other existing or new places where crates is used but visible_crates would be more correct, but maybe this isn't very true.

Then either way, should all_traits be split into visible_traits, or is filtering in all_diagnostic_items sufficient to cover this case? (I didn't realize that would affect the output here, so didn't actually test that on its own)

I think the filtering should be done as needed. if we expose visible_traits as just a function, it doesn't matter if the filtering is expensive b/c we only call it on the diagnostic codepath (i.e. failed compilation).

Then either way, should all_traits be split into visible_traits, or is filtering in all_diagnostic_items sufficient to cover this case? (I didn't realize that would affect the output here, so didn't actually test that on its own)

probably worth just testing yourself, idk off the top of my head lol

Just tested, seems like all_diagnostic_items doesn't have any effect on the relevant output here so it has to filter somewhere else too.

So I got rid of the query and split all_traits into all_traits and visible_traits that does the filtering on the fly. I also replaced all_traits in a few other places that deal with errors that seem reasonable (?), still waiting for my tests to finish and see if that affects much.

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

compiler-errors · 2025-01-13T04:10:01Z

@bors try @rust-timer queue

Exclude dependencies of `std` for diagnostics Currently crates in the sysroot can show up in diagnostic suggestions, such as in rust-lang#135232. To prevent this, introduce a query `visible_crates` that excludes crates marked `is_private_dep` and use this query in user-facing areas of code. Setting `#![feature(rustc_private)]` overrides this to just use all crates, since `rustc_private` enables use of `std`'s private dependencies. Notably, this changes `all_traits` to use `.visible_crates(())` rather than `.crates(())`. This is not strictly required and filtering could happen later as-needed; however, I cannot think of any cases where traits from private dependencies should be relevant in trait selection or diagnostics, so filtering at this earlier point seems more likely to avoid similar issues in the future. This may be reviewed per-commit. Fixes: rust-lang#135232

bors · 2025-01-13T04:11:12Z

⌛ Trying commit a04ed09 with merge dbe10d0...

rustbot · 2025-01-13T05:00:27Z

HIR ty lowering was modified

cc @fmease

compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs

tgross35 · 2025-01-13T05:50:23Z

(^ spurious CI failure)

SparrowLii · 2025-01-13T07:08:01Z

@bors try @rust-timer queue

bors · 2025-01-14T10:38:52Z

📌 Commit ed63539 has been approved by SparrowLii

It is now in the queue for this repository.

bors · 2025-01-14T14:15:43Z

⌛ Testing commit ed63539 with merge 8c39ce5...

bjorn3 · 2025-01-14T14:35:38Z

Alright, figured it out. addr2line, which I was using for my tests, was apparently already a private dependency. I am not sure why this is (possibly related to it being optional in std?)

#111076 marked all dependencies of libstd as private except for those that are actually part of the public interface of libstd to fix a similar diagnostic issue. This wasn't done for liballoc and other sysroot crates however. Should probably be fixed.

bjorn3 · 2025-01-14T14:39:06Z

compiler/rustc_middle/src/ty/util.rs

@@ -876,6 +876,11 @@ impl<'tcx> TyCtxt<'tcx> {
    /// [public]: TyCtxt::is_private_dep
    /// [direct]: rustc_session::cstore::ExternCrate::is_direct
    pub fn is_user_visible_dep(self, key: CrateNum) -> bool {
+        // `#![rustc_private]` overrides defaults to make private dependencies usable.
+        if self.features().enabled(sym::rustc_private) {


However, this is a very internal feature, so it doesn't seem worth doing any additional filtering here.

Not really. There are a lot of external custom drivers that need to enable this feature.

Are you suggesting that it is worth filtering here to only expose crates that are dependencies of std? Or just that the message is inaccurate.

I think the override for rustc_private should be removed. Even with rustc_private enabled you generally shouldn't do something like extern crate libc; from the sysroot.

bors · 2025-01-14T16:59:19Z

☀️ Test successful - checks-actions
Approved by: SparrowLii
Pushing 8c39ce5 to master...

tgross35 · 2025-01-14T18:36:07Z

#111076 marked all dependencies of libstd as private except for those that are actually part of the public interface of libstd to fix a similar diagnostic issue. This wasn't done for liballoc and other sysroot crates however. Should probably be fixed.

I was not aware of this history but that makes sense, I will update the others. In that case, do you think it makes sense to also revert ed63539? Assuming yes, since it would no longer be needed.

bjorn3 · 2025-01-14T18:39:11Z

If the Cargo.toml changes are enough, then yes. It would make sense to me to revert that commit.

tgross35 · 2025-01-14T20:11:24Z

Opened #135501 as an experiment, but I have been unsuccessful keeping the test fixed just by setting public = false.

rust-timer · 2025-01-14T20:32:30Z

Finished benchmarking commit (8c39ce5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-3.1%, -0.8%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-3.1%, -0.8%]	4

Cycles

Results (primary 2.6%, secondary -3.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.3%, 3.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-4.6%, -2.6%]	6
All ❌✅ (primary)	2.6%	[2.3%, 3.0%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 763.379s -> 762.502s (-0.11%)
Artifact size: 326.08 MiB -> 326.12 MiB (0.01%)

Replace `public_test_dep!` by placing optionally public items into new modules, then controlling what is exported with the `public-test-deps` feature. This is nicer for automatic formatting and diagnostics. This is a reland of 2e2a925 ("Eliminate the use of `public_test_dep!`"), which was reverted in 47e50fd ('Revert "Eliminate the use of..."') due to a bug exposed at [1]. This was fixed in [2], so the cleanup should be able to be applied again. [1]: rust-lang/rust#128691 [1]: rust-lang/rust#135278

Replace `public_test_dep!` by placing optionally public items into new modules, then controlling what is exported with the `public-test-deps` feature. This is nicer for automatic formatting and diagnostics. This is a reland of 2e2a925 ("Eliminate the use of `public_test_dep!`"), which was reverted in 47e50fd ('Revert "Eliminate the use of..."') due to a bug exposed at [1]. This was fixed in [2], so the cleanup should be able to be applied again. [1]: rust-lang/rust#128691 [2]: rust-lang/rust#135278

The change between 0.1.143 and 0.1.144 includes refactoring that was in compiler-builtins before, but had to be reverted before landing in rust-lang/rust because the traits were leaking into diagnostics [1]. Recently a fix for this issue was merged [2] so the cleanup is reapplied here. This also acts as a regression test for [2]. [1]: rust-lang#128691 (comment) [2]: rust-lang#135278

Update `compiler-builtins` to 0.1.144 The change between 0.1.143 and 0.1.144 includes refactoring that was in compiler-builtins before, but had to be reverted before landing in rust-lang/rust because the traits were leaking into diagnostics [1]. Recently a fix for this issue was merged [2] so the cleanup is reapplied here. This also acts as a regression test for [2]. [1]: rust-lang#128691 (comment) [2]: rust-lang#135278

Update `compiler-builtins` to 0.1.144 The change between 0.1.143 and 0.1.144 includes refactoring that was in compiler-builtins before, but had to be reverted before landing in rust-lang/rust because the traits were leaking into diagnostics [1]. Recently a fix for this issue was merged [2] so the cleanup is reapplied here. This also acts as a regression test for [2]. [1]: rust-lang#128691 (comment) [2]: rust-lang#135278 try-job: test-various

rustbot assigned SparrowLii Jan 9, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 9, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025

tgross35 mentioned this pull request Jan 9, 2025

Windows x86: Change i128 to return via the vector ABI #134290

Open

tgross35 force-pushed the ignore-std-dep-crates branch 3 times, most recently from 6a3e3c6 to a04ed09 Compare January 13, 2025 02:45

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 13, 2025

SparrowLii reviewed Jan 13, 2025

View reviewed changes

compiler-errors reviewed Jan 13, 2025

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 13, 2025

tgross35 force-pushed the ignore-std-dep-crates branch from a04ed09 to 871d75a Compare January 13, 2025 05:00

tgross35 commented Jan 13, 2025

View reviewed changes

compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs Outdated Show resolved Hide resolved

tgross35 force-pushed the ignore-std-dep-crates branch 2 times, most recently from 35ad309 to c0eb4af Compare January 13, 2025 05:26

This comment has been minimized.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2025

bjorn3 reviewed Jan 14, 2025

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 14, 2025

bors merged commit 8c39ce5 into rust-lang:master Jan 14, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 14, 2025

bors mentioned this pull request Jan 14, 2025

Initial support for dynamically linked crates #134767

Draft

tgross35 deleted the ignore-std-dep-crates branch January 14, 2025 18:36

tgross35 mentioned this pull request Jan 14, 2025

[WIP] Mark sysroot dependencies private #135501

Draft

tgross35 mentioned this pull request Jan 15, 2025

Eliminate the use of public_test_dep! rust-lang/compiler-builtins#747

Merged

tgross35 mentioned this pull request Jan 16, 2025

Update compiler-builtins to 0.1.144 #135560

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude dependencies of `std` for diagnostics #135278

Exclude dependencies of `std` for diagnostics #135278

tgross35 commented Jan 9, 2025 •

edited

Loading

rustbot commented Jan 9, 2025

tgross35 commented Jan 9, 2025

tgross35 commented Jan 9, 2025

tgross35 commented Jan 13, 2025

SparrowLii left a comment •

edited

Loading

SparrowLii Jan 13, 2025

compiler-errors Jan 13, 2025

tgross35 Jan 13, 2025

compiler-errors Jan 13, 2025

compiler-errors Jan 13, 2025

tgross35 Jan 13, 2025

compiler-errors commented Jan 13, 2025

This comment has been minimized.

bors commented Jan 13, 2025

rustbot commented Jan 13, 2025

This comment has been minimized.

tgross35 commented Jan 13, 2025

SparrowLii commented Jan 13, 2025

bors commented Jan 14, 2025

bors commented Jan 14, 2025

bjorn3 commented Jan 14, 2025 •

edited

Loading

bjorn3 Jan 14, 2025

tgross35 Jan 14, 2025

bjorn3 Jan 15, 2025

bors commented Jan 14, 2025

tgross35 commented Jan 14, 2025

bjorn3 commented Jan 14, 2025

tgross35 commented Jan 14, 2025

rust-timer commented Jan 14, 2025

Exclude dependencies of std for diagnostics #135278

Exclude dependencies of std for diagnostics #135278

Conversation

tgross35 commented Jan 9, 2025 • edited Loading

rustbot commented Jan 9, 2025

tgross35 commented Jan 9, 2025

tgross35 commented Jan 9, 2025

tgross35 commented Jan 13, 2025

SparrowLii left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jan 13, 2025

This comment has been minimized.

bors commented Jan 13, 2025

rustbot commented Jan 13, 2025

This comment has been minimized.

tgross35 commented Jan 13, 2025

SparrowLii commented Jan 13, 2025

bors commented Jan 14, 2025

bors commented Jan 14, 2025

bjorn3 commented Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 14, 2025

tgross35 commented Jan 14, 2025

bjorn3 commented Jan 14, 2025

tgross35 commented Jan 14, 2025

rust-timer commented Jan 14, 2025

Overall result: no relevant changes - no action needed

Exclude dependencies of `std` for diagnostics #135278

Exclude dependencies of `std` for diagnostics #135278

tgross35 commented Jan 9, 2025 •

edited

Loading

SparrowLii left a comment •

edited

Loading

bjorn3 commented Jan 14, 2025 •

edited

Loading