Skip to content

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

Merged
bors merged 4 commits into
rust-lang:masterfrom
jyn514:from-inner
Nov 18, 2020
Merged

Add Item::from_def_id_and_kind to reduce duplication in rustdoc#77820
bors merged 4 commits into
rust-lang:masterfrom
jyn514:from-inner

Conversation

@jyn514

Copy link
Copy Markdown
Member

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

@jyn514jyn514 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-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2020
Comment threadsrc/librustdoc/clean/types.rs Outdated
Comment threadsrc/librustdoc/clean/types.rs Outdated
@petrochenkovpetrochenkov self-assigned this Oct 11, 2020
@petrochenkovpetrochenkov 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
@jyn514

Copy link
Copy Markdown
MemberAuthor

@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

Copy link
Copy Markdown
MemberAuthor

It's coming from this code:

implClean<Item>for doctree::Struct<'_>{fnclean(&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

Copy link
Copy Markdown
MemberAuthor

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

@jyn514

jyn514 commented Oct 11, 2020

Copy link
Copy Markdown
MemberAuthor

Looks like the crate that should provide this is

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

@petrochenkov

Copy link
Copy Markdown
Contributor

@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.

@jyn514

This comment has been minimized.

@petrochenkov

This comment has been minimized.

@jyn514

This comment has been minimized.

@petrochenkov

This comment has been minimized.

@jyn514

This comment has been minimized.

@jyn514

This comment has been minimized.

@petrochenkovpetrochenkov 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
@bors

This comment has been minimized.

@petrochenkovpetrochenkov 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
@petrochenkov

Copy link
Copy Markdown
Contributor

#78077 has landed.

@jyn514

jyn514 commented Oct 22, 2020

Copy link
Copy Markdown
MemberAuthor

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:

pubfnopt_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())
}

pubfnitem_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

jyn514 commented Nov 17, 2020

Copy link
Copy Markdown
MemberAuthor

@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

Copy link
Copy Markdown
Member

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

@jyn514

Copy link
Copy Markdown
MemberAuthor

@bors r=petrochenkov rollup=never

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

@bors

bors commented Nov 17, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 46a99790cdf4dc633fe8b295b506b15c7ad72f1c has been approved by petrochenkov

@borsbors 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 labels Nov 17, 2020
@bors

This comment has been minimized.

@borsbors 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
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

Copy link
Copy Markdown
MemberAuthor

@bors r=petrochenkov rollup=never

@bors

bors commented Nov 17, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 0e1a302 has been approved by petrochenkov

@borsbors 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

bors commented Nov 18, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 0e1a302 with merge c4f836a...

@bors

bors commented Nov 18, 2020

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2020
@bors
bors merged commit c4f836a into rust-lang:masterNov 18, 2020
@rustbotrustbot added this to the 1.50.0 milestone Nov 18, 2020
@jyn514
jyn514 deleted the from-inner branch November 18, 2020 13:39
@jyn514

Copy link
Copy Markdown
MemberAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@jyn514@petrochenkov@bors@GuillaumeGomez@Manishearth@rust-highfive@rustbot