diagnostics: do not suggest masked crate impls #109688

notriddle · 2023-03-28T03:51:06Z

Fixes #88696

rustbot · 2023-03-28T03:51:12Z

r? @b-naber

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

rustbot · 2023-03-28T03:51:15Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

b-naber

I think this is an improvement, but afaict only partially fixes the issue since this relies on the use of extern crate.

Can you maybe check that all private dependencies in std are marked #[doc(masked)]?

b-naber · 2023-03-28T08:52:37Z

tests/ui/suggestions/dont-suggest-doc-masked-impls/issue-88696.rs

+#![feature(doc_masked)]
+
+#[doc(masked)]
+extern crate masked_crate;


I don't understand why you include this extern crate statement here. miniz_oxide is #[doc(masked)] in std, so just using the example in the fixed issue as a test should suffice here.

I wanted to make sure that this test case didn't rely on the specifics of how libstd is implemented right now, so I copied the relevant parts of miniz_oxide into the test case.

When run without the fix, the error message lists both miniz_oxide and masked_crate.

b-naber · 2023-03-28T09:03:26Z

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

@@ -495,6 +496,25 @@ pub(in crate::rmeta) fn provide(providers: &mut Providers) {
            tcx.untracked().cstore.leak();
            tcx.arena.alloc_from_iter(CStore::from_tcx(tcx).iter_crate_data().map(|(cnum, _)| cnum))
        },
+        doc_masked_crates: |tcx, LocalCrate| {


Can you put this functionality in a function and only call that function in the closure here, please? I think keeping the provide functions short is preferable and is what seems to be done in most places in the compiler.

Okay. It's done.

b-naber · 2023-03-28T09:14:26Z

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

@@ -2006,6 +2006,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                    || !trait_pred
                        .skip_binder()
                        .is_constness_satisfied_by(self.tcx.constness(def_id))
+                    || self.tcx.is_doc_masked(def_id.krate)


hm I'm not sure whether relying on an attribute that seems to solely be used in rustdoc inside the trait system is such a good idea. 🤔 But I can't think of a better way to filter for crates we want to ignore here either, so I guess this is fine.

Still can you put a FIXME here that states that ideally we should't rely on an attribute for extern crate for this?

Okay, FIXME added.

b-naber · 2023-03-28T09:17:22Z

compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -1549,6 +1549,12 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
        self.root.tables.is_intrinsic.get(self, index)
    }

+    fn get_doc_masked_crates(self) -> Vec<CrateNum> {
+        let mut v: Vec<CrateNum> = self.root.doc_masked_crates.decode(self).collect();
+        v.sort_unstable();


Why do we need to sort here?

I added a comment describing why it needs sorted. It's so that binary search can be used later.

b-naber · 2023-03-28T09:40:46Z

Since this adds two queries:

@bors try @rust-timer queue

bors · 2023-03-28T09:40:58Z

⌛ Trying commit a28eba9 with merge 46d8dad086555c8fe75815dc84f0f04b9ea287af...

bors · 2023-03-28T11:41:30Z

☀️ Try build successful - checks-actions
Build commit: 46d8dad086555c8fe75815dc84f0f04b9ea287af (46d8dad086555c8fe75815dc84f0f04b9ea287af)

rust-timer · 2023-03-28T19:50:38Z

Finished benchmarking commit (46d8dad086555c8fe75815dc84f0f04b9ea287af): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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)	1.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	5.0%	[4.6%, 5.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-4.9%, -3.2%]	3
All ❌✅ (primary)	1.6%	[1.6%, 1.6%]	1

Cycles

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

b-naber · 2023-03-29T09:51:35Z

I looked again where #[doc(masked)| is actually used in combination with extern crate and it seems to be hardly used at all. Even in std the only crate for which we would exclude traits in suggestions is miniz_oxide afaict. So we would essentially only exclude traits for that particular crate in suggestions. Can you say whether that assessment is accurate?

notriddle · 2023-03-30T06:03:40Z

@b-naber That assessment seems accurate. The only times I've seen or heard of this bug, it's been miniz_oxide that does it.

b-naber · 2023-03-30T10:07:07Z

Looking at what traits are considered in find_similar_impl_candidates

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Line 2003 in f2d9a3d

.all_impls(trait_pred.def_id())

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

Lines 221 to 225 in f2d9a3d

    
               pub fn all_impls(self, trait_def_id: DefId) -> impl Iterator<Item = DefId> + 'tcx { 
        
                   let TraitImpls { blanket_impls, non_blanket_impls } = self.trait_impls_of(trait_def_id); 
        
                   blanket_impls.iter().chain(non_blanket_impls.iter().flat_map(|(_, v)| v)).cloned() 
        
               }

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

Lines 229 to 270 in f2d9a3d

    
           pub(super) fn trait_impls_of_provider(tcx: TyCtxt<'_>, trait_id: DefId) -> TraitImpls { 
        
               let mut impls = TraitImpls::default(); 
        
               // Traits defined in the current crate can't have impls in upstream 
        
               // crates, so we don't bother querying the cstore. 
        
               if !trait_id.is_local() { 
        
                   for &cnum in tcx.crates(()).iter() { 
        
                       for &(impl_def_id, simplified_self_ty) in 
        
                           tcx.implementations_of_trait((cnum, trait_id)).iter() 
        
                       { 
        
                           if let Some(simplified_self_ty) = simplified_self_ty { 
        
                               impls 
        
                                   .non_blanket_impls 
        
                                   .entry(simplified_self_ty) 
        
                                   .or_default() 
        
                                   .push(impl_def_id); 
        
                           } else { 
        
                               impls.blanket_impls.push(impl_def_id); 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
               for &impl_def_id in tcx.hir().trait_impls(trait_id) { 
        
                   let impl_def_id = impl_def_id.to_def_id(); 
        
                   let impl_self_ty = tcx.type_of(impl_def_id).subst_identity(); 
        
                   if impl_self_ty.references_error() { 
        
                       continue; 
        
                   } 
        
                   if let Some(simplified_self_ty) = 
        
                       fast_reject::simplify_type(tcx, impl_self_ty, TreatParams::AsCandidateKey) 
        
                   { 
        
                       impls.non_blanket_impls.entry(simplified_self_ty).or_default().push(impl_def_id); 
        
                   } else { 
        
                       impls.blanket_impls.push(impl_def_id); 
        
                   } 
        
               } 
        
               impls 
        
           }

I don't see why this would be exclusive to miniz_oxide (though I don't understand why this diagnostics problem doesn't occur more often either). Still adding two queries and all of this code to fix a corner-case diagnostics issue is not worth it in my opinion (especially since we would have to revert this PR once we remove either the #[doc(masked)] attribute on the extern crate miniz_oxide stmt or the extern crate miniz_oxide stmt itself). So unless you want to try another approach I would close this. Thanks a lot for your efforts in writing this PR and sorry about not having looked at where these changes would actually apply before requesting changes.

notriddle · 2023-03-30T13:59:44Z

The event that’s happening is that the compiler suggests a trait impl from a transitive dependency. It probably occurs all the time, but when it isn’t a private dependency of the standard library, is it considered a bug?

b-naber · 2023-03-30T21:30:48Z

The correct fix for this issue, I think, would be to not suggest traits from any private dependencies in general and not just for miniz_oxide in std. Now I don't know whether the compiler currently has sufficient information to infer whether a crate is a private dependency or not, so don't know whether this is fixable. I find that not having these traits suggested for miniz_oxide is an improvement, but it's really a diagnostics corner-case and I don't think that adding two queries and all of the rest of the code is worth it given the significance (or lack thereof) of the problem. I'm inclined to close this PR, but if you're convinced that this PR is worth merging and there's something I miss here, then I can assign somebody else for a review.

notriddle · 2023-03-30T21:42:39Z

No, you make a good call.

A proper fix will probably tie into RFC 1977, and require a different implementation tactic.

notriddle added 2 commits March 27, 2023 20:44

rustdoc: convert masked_crates to a query

5d38a15

diagnostics: do not suggest masked crate impls

a28eba9

rustbot assigned b-naber Mar 28, 2023

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 28, 2023

b-naber requested changes Mar 28, 2023

View reviewed changes

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 Mar 28, 2023

This comment has been minimized.

Sign in to view

notriddle force-pushed the notriddle/suggestion-masked branch from 58f292d to ced09d3 Compare March 28, 2023 16:15

Add comments and move get_sorted_masked_crates into a function

622e7eb

notriddle force-pushed the notriddle/suggestion-masked branch from ced09d3 to 622e7eb Compare March 28, 2023 16:44

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 28, 2023

notriddle requested a review from b-naber March 29, 2023 03:31

notriddle closed this Mar 30, 2023

notriddle deleted the notriddle/suggestion-masked branch March 30, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diagnostics: do not suggest masked crate impls #109688

diagnostics: do not suggest masked crate impls #109688

notriddle commented Mar 28, 2023

rustbot commented Mar 28, 2023

rustbot commented Mar 28, 2023

b-naber left a comment

b-naber Mar 28, 2023

notriddle Mar 28, 2023

b-naber Mar 28, 2023

notriddle Mar 28, 2023

b-naber Mar 28, 2023

notriddle Mar 28, 2023

b-naber Mar 28, 2023

notriddle Mar 28, 2023

b-naber commented Mar 28, 2023

This comment has been minimized.

bors commented Mar 28, 2023

bors commented Mar 28, 2023

This comment has been minimized.

This comment has been minimized.

rust-timer commented Mar 28, 2023

b-naber commented Mar 29, 2023

notriddle commented Mar 30, 2023

b-naber commented Mar 30, 2023

notriddle commented Mar 30, 2023

b-naber commented Mar 30, 2023

notriddle commented Mar 30, 2023

diagnostics: do not suggest masked crate impls #109688

diagnostics: do not suggest masked crate impls #109688

Conversation

notriddle commented Mar 28, 2023

rustbot commented Mar 28, 2023

rustbot commented Mar 28, 2023

b-naber left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Mar 28, 2023

This comment has been minimized.

bors commented Mar 28, 2023

bors commented Mar 28, 2023

This comment has been minimized.

This comment has been minimized.

rust-timer commented Mar 28, 2023

Overall result: no relevant changes - no action needed

b-naber commented Mar 29, 2023

notriddle commented Mar 30, 2023

b-naber commented Mar 30, 2023

notriddle commented Mar 30, 2023

b-naber commented Mar 30, 2023

notriddle commented Mar 30, 2023