Add `Item::from_def_id_and_kind` to reduce duplication in rustdoc #77820

jyn514 · 2020-10-11T14:56:39Z

This makes it harder to make typos, and also makes it much more clear what's intentionally different rather than a typo (look for what_rustc_thinks).

Found this while working on #76998, I really didn't want to add const_visibility in 20 different places.

r? @GuillaumeGomez

src/librustdoc/clean/types.rs

jyn514 · 2020-10-11T16:06:10Z

@petrochenkov do you understand this error?

error: internal compiler error: compiler/rustc_middle/src/ty/query/mod.rs:105:1: `tcx.visibility(DefId(0:4 ~ foo[8787]::Bar))` unsupported by its crate
query stack during panic:
#0 [visibility] computing visibility of `Struct`
end of query stack

I can change it to use the hir::Visibility instead, but do you know why it's necessary?

jyn514 · 2020-10-11T16:07:40Z

It's coming from this code:

impl Clean<Item> for doctree::Struct<'_> {
    fn clean(&self, cx: &DocContext<'_>) -> Item {
        Item::from_hir_id_and_kind(
            self.id,
            StructItem(Struct {
                struct_type: self.struct_type,
                generics: self.generics.clean(cx),
                fields: self.fields.clean(cx),
                fields_stripped: false,
            }),
            cx,
        )
    }
}

jyn514 · 2020-10-11T16:10:07Z

Hmm, this comes from impl Default for Providers. Maybe rustdoc forgot to call providers::provide somewhere?

jyn514 · 2020-10-11T16:12:40Z

Looks like the crate that should provide this is

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

Line 204 in d902752

visibility => { cdata.get_visibility(def_id.index) }

petrochenkov · 2020-10-11T16:14:19Z

@jyn514

error: internal compiler error: compiler/rustc_middle/src/ty/query/mod.rs:105:1: tcx.visibility(DefId(0:4 ~ foo[8787]::Bar)) unsupported by its crate

IIRC, the visibility query is only implemented for other crates, but not for the current crate (LOCAL_CRATE).
You could try implementing it for the local crate, it would be generally useful, I think.
Every time Visibility::from_hir is used, that new query could be used instead.
It's preferable to do that as a separate PR because there may be performance effects.

petrochenkov · 2020-10-21T22:39:12Z

#78077 has landed.

jyn514 · 2020-10-22T01:32:21Z

Rebased and updated. This is still panicking on most tests:

error: internal compiler error: compiler/rustc_middle/src/hir/map/mod.rs:477:41: couldn't find hir id HirId { owner: DefId(0:0 ~ union[8787]), local_id: 0 } in the HIR map

Backtrace:

   8: rustc_middle::util::bug::bug_fmt
             at ./compiler/rustc_middle/src/util/bug.rs:14:5
   9: rustc_middle::hir::map::Map::get::{{closure}}
             at ./compiler/rustc_middle/src/hir/map/mod.rs:477:41
  10: core::option::Option<T>::unwrap_or_else
             at ./library/core/src/option.rs:427:21
  11: rustc_middle::hir::map::Map::get
             at ./compiler/rustc_middle/src/hir/map/mod.rs:477:9
  12: rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::opt_item_name::{{closure}}
             at ./compiler/rustc_middle/src/ty/mod.rs:2796:32
  13: core::option::Option<T>::and_then
             at ./library/core/src/option.rs:662:24
  14: rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::opt_item_name
             at ./compiler/rustc_middle/src/ty/mod.rs:2794:9
  15: rustdoc::clean::types::Item::from_def_id_and_kind
             at ./src/librustdoc/clean/types.rs:127:19
  16: rustdoc::clean::types::Item::from_hir_id_and_kind
             at ./src/librustdoc/clean/types.rs:118:9
  17: <rustdoc::doctree::Module as rustdoc::clean::Clean<rustdoc::clean::types::Item>>::clean
             at ./src/librustdoc/clean/mod.rs:265:33
  18: rustdoc::clean::utils::krate
             at ./src/librustdoc/clean/utils.rs:44:22

I think the issue is that opt_item_name doesn't check for the crate root, only item_name:

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

Lines 2793 to 2797 in cee5521

    
           pub fn opt_item_name(self, def_id: DefId) -> Option<Ident> { 
        
               def_id 
        
                   .as_local() 
        
                   .and_then(|def_id| self.hir().get(self.hir().local_def_id_to_hir_id(def_id)).ident()) 
        
           }

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

Lines 2919 to 2934 in cee5521

    
           pub fn item_name(self, id: DefId) -> Symbol { 
        
               if id.index == CRATE_DEF_INDEX { 
        
                   self.original_crate_name(id.krate) 
        
               } else { 
        
                   let def_key = self.def_key(id); 
        
                   match def_key.disambiguated_data.data { 
        
                       // The name of a constructor is that of its parent. 
        
                       rustc_hir::definitions::DefPathData::Ctor => { 
        
                           self.item_name(DefId { krate: id.krate, index: def_key.parent.unwrap() }) 
        
                       } 
        
                       _ => def_key.disambiguated_data.data.get_opt_name().unwrap_or_else(|| { 
        
                           bug!("item_name: no name for {:?}", self.def_path(id)); 
        
                       }), 
        
                   } 
        
               } 
        
           }

I'll try rewriting item_name in terms of opt_item_name and see if that helps.

jyn514 · 2020-11-17T14:48:00Z

@Manishearth FYI - Guillaume and I discussed offline that there's no way to get precisely the previous behavior back. I can recover pub(self) (in theory, I need to work out exactly how) and pub(super), but the previous behavior with super::b::super will always be normalized to pub(super) now - the information about how it was written just isn't present in the metadata, only in the HIR.

Manishearth · 2020-11-17T15:12:02Z

Yeah that's fine, I care about self and super only, the others are rare enough

jyn514 · 2020-11-17T15:13:12Z

@bors r=petrochenkov rollup=never

I want to be able to bisect if this causes regressions.

bors · 2020-11-17T15:13:14Z

📌 Commit 46a99790cdf4dc633fe8b295b506b15c7ad72f1c has been approved by petrochenkov

Visibility needs much less information than a full path, since modules can never have generics. This allows constructing a Visibility from only a DefId. Note that this means that paths are now normalized to their DefPath. In other words, `pub(self)` or `pub(super)` now always shows `pub(in path)` instead of preserving the original text.

This also uses an exhaustive match to avoid future similar bugs.

- Add `Item::from_hir_id_and_kind` convenience wrapper - Make name parameter mandatory `tcx.opt_item_name` doesn't handle renames, so this is necessary for any item that could be renamed, which is almost all of them. - Override visibilities to be `Inherited` for enum variants `tcx.visibility` returns the effective visibility, not the visibility that was written in the source code. `pub enum E { A, B }` always has public variants `A` and `B`, so there's no sense printing `pub` again. - Don't duplicate handling of `Visibility::Crate` Instead, represent it as just another `Restricted` path.

It was completely unused.

jyn514 · 2020-11-17T20:16:42Z

@bors r=petrochenkov rollup=never

bors · 2020-11-17T20:16:43Z

📌 Commit 0e1a302 has been approved by petrochenkov

bors · 2020-11-18T03:56:23Z

⌛ Testing commit 0e1a302 with merge c4f836a...

bors · 2020-11-18T06:21:45Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c4f836a to master...

jyn514 · 2020-11-18T13:40:34Z

@petrochenkov thank you so much for all your help, I wouldn't have been able to do this without you :)

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 11, 2020

rust-highfive assigned GuillaumeGomez Oct 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2020

jyn514 force-pushed the from-inner branch from 956eced to 782dbbf Compare October 11, 2020 15:13

petrochenkov reviewed Oct 11, 2020

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

petrochenkov self-assigned this Oct 11, 2020

petrochenkov 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 Oct 11, 2020

This comment has been minimized.

Sign in to view

jyn514 mentioned this pull request Oct 13, 2020

Make tcx.visibility() work for items in the local crate #77889

Closed

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2020

This comment has been minimized.

Sign in to view

jyn514 mentioned this pull request Oct 18, 2020

[WIP] Get rid of rustdoc::doctree #78082

Closed

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 21, 2020

jyn514 force-pushed the from-inner branch from a8808c4 to 8182a1d Compare October 22, 2020 01:30

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 17, 2020

jyn514 mentioned this pull request Nov 17, 2020

regression: Rustdoc shows pub(self) as pub(in a::b) #79139

Closed

This comment has been minimized.

Sign in to view

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 17, 2020

jyn514 added 4 commits November 17, 2020 15:13

Fix broken handling of MacroDef in Map::attrs

c9a17b1

This also uses an exhaustive match to avoid future similar bugs.

Remove dead code in rustdoc::doctree

0e1a302

It was completely unused.

jyn514 force-pushed the from-inner branch from 46a9979 to 0e1a302 Compare November 17, 2020 20:16

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2020

bors merged commit c4f836a into rust-lang:master Nov 18, 2020

rustbot added this to the 1.50.0 milestone Nov 18, 2020

jyn514 deleted the from-inner branch November 18, 2020 13:39

camelid mentioned this pull request Dec 17, 2020

rustdoc: Private function is rendered with a verbose visibility #80101

Closed

jyn514 mentioned this pull request Jan 22, 2021

Wrong pub modifier in each function of trait documentation #81274

Closed

jyn514 mentioned this pull request Feb 14, 2021

Move collection of items accessible directly or through reexports from rustc_privacy to rustc_resolve #82064

Closed

matthiaskrgr mentioned this pull request Mar 22, 2024

ICE: pattern has unexpected type #122809

Closed

Add Item::from_def_id_and_kind to reduce duplication in rustdoc #77820

Add Item::from_def_id_and_kind to reduce duplication in rustdoc #77820

Uh oh!

Conversation

jyn514 commented Oct 11, 2020

Uh oh!

Uh oh!

Uh oh!

jyn514 commented Oct 11, 2020

Uh oh!

jyn514 commented Oct 11, 2020

Uh oh!

jyn514 commented Oct 11, 2020

Uh oh!

jyn514 commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Oct 11, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Oct 21, 2020

Uh oh!

jyn514 commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth commented Nov 17, 2020

Uh oh!

jyn514 commented Nov 17, 2020

Uh oh!

bors commented Nov 17, 2020

Uh oh!

This comment has been minimized.

jyn514 commented Nov 17, 2020

Uh oh!

bors commented Nov 17, 2020

Uh oh!

bors commented Nov 18, 2020

Uh oh!

bors commented Nov 18, 2020

Uh oh!

jyn514 commented Nov 18, 2020

Uh oh!

Uh oh!

Add `Item::from_def_id_and_kind` to reduce duplication in rustdoc #77820

Add `Item::from_def_id_and_kind` to reduce duplication in rustdoc #77820

jyn514 commented Oct 11, 2020 •

edited

Loading

jyn514 commented Oct 22, 2020 •

edited

Loading

jyn514 commented Nov 17, 2020 •

edited

Loading