- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
[rustdoc] Generic impls #52585
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.
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
[rustdoc] Generic impls #52585
Conversation
| Meta: could you prefix PR names with "rustdoc", like  Otherwise when I see a notification about a PR to rust-lang/rust named like "Generic impls" I get and immediate reaction and have to go and look what the PR does. | 
| @petrochenkov oups sorry, changed it! :) | 
| The job  Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact  | 
| I think I just broke all urls haha. | 
dcde482    to
    0f73cd5      
    Compare
  
    | The job  Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact  | 
467cf84    to
    bc8e933      
    Compare
  
    | Tests passed. | 
| ☔ The latest upstream changes (presumably #52368) made this pull request unmergeable. Please resolve the merge conflicts. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks so much for finally getting this put together. I have a couple questions and i'm currently waiting on a docs build locally, but overall this looks good!
cc @eddyb and @oli-obk to double-check the final compiler code, and @Aaron1011 as the original owner of synthetic impls.
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this filter out libcore specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It panics getting libcore information apparently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, at some point since the original implementation, it doesn't panic if you take out this condition any more. I just tried it locally and it made everything just fine.
        
          
                src/librustdoc/core.rs
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can filter this down to just the ones with blanket impls? That may require some overhead here but would also cut down on the traits we iterate over when gathering impls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand you. I do this call here because it otherwise would need to be done on every item (and also because @eddyb thinks it's better this way, which I agree).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i understand why you wanted to cache the traits. I'm asking if it's possible to store less traits in this, by filtering out traits that don't have a blanket/generic impl.
bc8e933    to
    bcb5c6f      
    Compare
  
            
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | self.get_auto_trait_impls(did, &def_ctor, Some(name)) | ||
| } | ||
|  | ||
| fn get_real_ty<F>(&self, def_id: DefId, def_ctor: &F, real_name: &Option<Ident>, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this function signature should follow rustfmt block indent style (especially since it looks like the function below already follows that
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | }) | ||
| .into_iter() | ||
| let auto_traits: Vec<_> = | ||
| self.cx.send_trait | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's some funky formatting here. revert this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double-check: i think @oli-obk wanted you to revert this section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn I thought I did! >< Might have been lost in my multiple tests...
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | let mut traits = Vec::new(); | ||
| if self.cx.crate_name != Some("core".to_string()) && | ||
| self.cx.access_levels.borrow().is_doc_reachable(def_id) { | ||
| if let ty::TyAdt(_adt, _) = ty.sty { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we only doing this for adts? wouldn't it be practical to also see impls on tuples, array, slices, ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! But the answer lies on @eddyb side here. Or at least I recall him asking me to only perform this on Adt items. Maybe I got it wrong?
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | let real_name = name.clone().map(|name| Ident::from_str(&name)); | ||
| let param_env = self.cx.tcx.param_env(def_id); | ||
| for &trait_def_id in self.cx.all_traits.iter() { | ||
| if !self.cx.access_levels.borrow().is_doc_reachable(trait_def_id) || | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to prefilter all_traits during creation by their reachability instead of doing this over and over again for every single type in a crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I discussed a bit earlier with @QuietMisdreavus so I'll do it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can't, it depends on the context so we have to make this check in here (access_levels is updated through the code).
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | let generics = infcx.tcx.generics_of(impl_def_id); | ||
| let trait_ref = infcx.tcx.impl_trait_ref(impl_def_id).unwrap(); | ||
|  | ||
| if !match infcx.tcx.type_of(impl_def_id).sty { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match infcx.tcx.type_of(impl_def_id).sty {
    ::rustc::ty::TyParam(_) => {},
    _ => return,
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| let substs = infcx.fresh_substs_for_item(DUMMY_SP, def_id); | ||
| let ty2 = ty.subst(infcx.tcx, substs); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need both ty and ty2, so just name it ty to reduce what the reader needs to keep in the mental cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need ty afterwards:
for_: ty.clean(self.cx),
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | if !may_apply { | ||
| return | ||
| } | ||
| self.cx.generated_synthetics.borrow_mut() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we do this way earlier (like where we're early aborting usually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because a trait can have multiple generic impls and we only know at this point that the current impl matches.
| @QuietMisdreavus Which implementation is invalid? If you're talking about  | 
| None of them are valid.  | 
| The fix isn't quite there. The impl signatures are still nonsensical, and they don't tell the whole story. Here's how  It declares a type param  One of the goals of the original synthetic impls was that you could copy/paste them into your own code and they would be perfectly valid. Some of the original synthetic-impl code got moved into librustc (looks like  | 
31ec977    to
    06e124d      
    Compare
  
    | The job  Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact  | 
06e124d    to
    7a3c7b2      
    Compare
  
    | Note about the current version: We've scaled back this implementation so it just displayed the original blanket impl, instead of unifying all the constraints on the various types and impls involved. This can create some confusing situations regarding things like  (Reading through the code right now...) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to pull out the blanket-impl collection into its own function, and just call that from get_auto_trait_impls. As-is, it feels a little out of place.
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, at some point since the original implementation, it doesn't panic if you take out this condition any more. I just tried it locally and it made everything just fine.
        
          
                src/librustdoc/clean/auto_trait.rs
              
                Outdated
          
        
      | }) | ||
| .into_iter() | ||
| let auto_traits: Vec<_> = | ||
| self.cx.send_trait | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double-check: i think @oli-obk wanted you to revert this section?
| With that last push, i think we're ready to go! r=me pending travis. | 
| @bors: r=QuietMisdreavus | 
| 📌 Commit 06364bd has been approved by  | 
| } | ||
| } | ||
|  | ||
| pub fn get_blanket_impls<F>( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the wrong file IMO.
| let trait_ref = infcx.tcx.impl_trait_ref(impl_def_id).unwrap(); | ||
|  | ||
| match infcx.tcx.type_of(impl_def_id).sty { | ||
| ::rustc::ty::TypeVariants::TyParam(_) => {}, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typical way to match this is ty::TyParam(_). Also, you want to match on trait_ref.self_ty().sty instead.
| .clean(self.cx)), | ||
| }), | ||
| }); | ||
| debug!("{:?} => {}", trait_ref, may_apply); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover random debug! (if you want to keep it, you should adjust the message).
| let ty = self.get_real_ty(def_id, def_ctor, &real_name, generics); | ||
| let predicates = infcx.tcx.predicates_of(impl_def_id); | ||
|  | ||
| traits.push(Item { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no way to do this? Something like build_impl?
[rustdoc] Generic impls Fixes #33772. r? @QuietMisdreavus
| ☀️ Test successful - status-appveyor, status-travis | 





Fixes #33772.
r? @QuietMisdreavus