Skip to content
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

Resolve Self in a submodule when the type is not in scope #84999

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 57 additions & 5 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
ns: Namespace,
module_id: DefId,
extra_fragment: &Option<String>,
self_id: Option<DefId>,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
if let Some(res) = self.resolve_path(path_str, ns, module_id) {
match res {
Expand Down Expand Up @@ -515,6 +516,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// primitives.
resolve_primitive(&path_root, TypeNS)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
.or_else(|| self_id.map(|id| Res::Def(self.cx.tcx.def_kind(id), id)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the wrong fix; it will use other items in the same scope if they happen to have the same name. This should come first and only then resolve_primitive and resolve_path. This is #84827 (comment); please add it as a test case.

.and_then(|ty_res| {
let (res, fragment, side_channel) =
self.resolve_associated_item(ty_res, item_name, ns, module_id)?;
Expand Down Expand Up @@ -680,7 +682,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
let result = match ns {
Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from),
Namespace::TypeNS | Namespace::ValueNS => {
self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res)
self.resolve(path_str, ns, module_id, extra_fragment, None).map(|(res, _)| res)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't passed self_id to check_full_res, because it had no role in solving the bug, so it call resolve with None instead. But maybe we should ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should; can you add self_id as a parameter to check_full_res? You may have to also add it to some of the error variants.

The worst that happens is the diagnostic is wrong though, so if it's complicated it's ok to save it for a follow-up. I think this is a case that would be affected:

struct Foo {
	foo: i32,
}

mod bar {
	impl crate::Foo {
		/// Baz the [macro@Self::foo].
		pub fn baz(&self) {}
	}
}

}
};

Expand Down Expand Up @@ -837,7 +839,37 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
Some(if parent_kind == DefKind::Impl { parent } else { item.def_id.expect_real() })
} else {
Some(item.def_id.expect_real())
};
}
.map(|curr_id| match self.cx.tcx.def_kind(curr_id) {
DefKind::Impl => {
match curr_id
.as_local()
.map(|local_id| self.cx.tcx.hir().item(rustc_hir::ItemId { def_id: local_id }))
{
Some(rustc_hir::Item {
kind:
rustc_hir::ItemKind::Impl(rustc_hir::Impl {
self_ty:
rustc_hir::Ty {
kind:
rustc_hir::TyKind::Path(rustc_hir::QPath::Resolved(
_,
rustc_hir::Path {
res: rustc_hir::def::Res::Def(_, def_id),
..
},
)),
..
},
..
}),
..
}) => *def_id,
_ => curr_id,
}
}
_ => curr_id,
Comment on lines +844 to +871
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all wrong, it will break for impls from foreign crates. Use the logic below from self_name instead.

(This is what I removed initially in #76467.)

});

// FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly
let self_name = self_id.and_then(|self_id| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self_name should no longer be used at all. Everything that touches it is wrong.

Expand Down Expand Up @@ -879,7 +911,15 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
// NOTE: if there are links that start in one crate and end in another, this will not resolve them.
// This is a degenerate case and it's not supported by rustdoc.
for md_link in markdown_links(&doc) {
let link = self.resolve_link(&item, &doc, &self_name, parent_node, krate, md_link);
let link = self.resolve_link(
&item,
&doc,
&self_name,
parent_node,
krate,
md_link,
self_id,
);
if let Some(link) = link {
self.cx.cache.intra_doc_links.entry(item.def_id).or_default().push(link);
}
Expand Down Expand Up @@ -1023,6 +1063,7 @@ impl LinkCollector<'_, '_> {
parent_node: Option<DefId>,
krate: CrateNum,
ori_link: MarkdownLink,
self_id: Option<DefId>,
) -> Option<ItemLink> {
trace!("considering link '{}'", ori_link.link);

Expand Down Expand Up @@ -1131,6 +1172,7 @@ impl LinkCollector<'_, '_> {
},
diag_info.clone(), // this struct should really be Copy, but Range is not :(
matches!(ori_link.kind, LinkType::Reference | LinkType::Shortcut),
self_id,
)?;

// Check for a primitive which might conflict with a module
Expand Down Expand Up @@ -1285,6 +1327,7 @@ impl LinkCollector<'_, '_> {
key: ResolutionInfo,
diag: DiagnosticInfo<'_>,
cache_resolution_failure: bool,
self_id: Option<DefId>,
) -> Option<(Res, Option<String>)> {
// Try to look up both the result and the corresponding side channel value
if let Some(ref cached) = self.visited_links.get(&key) {
Expand All @@ -1302,7 +1345,7 @@ impl LinkCollector<'_, '_> {
}
}

let res = self.resolve_with_disambiguator(&key, diag);
let res = self.resolve_with_disambiguator(&key, diag, self_id);

// Cache only if resolved successfully - don't silence duplicate errors
if let Some(res) = res {
Expand Down Expand Up @@ -1333,6 +1376,7 @@ impl LinkCollector<'_, '_> {
&mut self,
key: &ResolutionInfo,
diag: DiagnosticInfo<'_>,
self_id: Option<DefId>,
) -> Option<(Res, Option<String>)> {
let disambiguator = key.dis;
let path_str = &key.path_str;
Expand All @@ -1341,7 +1385,13 @@ impl LinkCollector<'_, '_> {

match disambiguator.map(Disambiguator::ns) {
Some(expected_ns @ (ValueNS | TypeNS)) => {
match self.resolve(path_str, expected_ns, base_node.expect_real(), extra_fragment) {
match self.resolve(
path_str,
expected_ns,
base_node.expect_real(),
extra_fragment,
self_id,
) {
Ok(res) => Some(res),
Err(ErrorKind::Resolve(box mut kind)) => {
// We only looked in one namespace. Try to give a better error if possible.
Expand Down Expand Up @@ -1384,6 +1434,7 @@ impl LinkCollector<'_, '_> {
TypeNS,
base_node.expect_real(),
extra_fragment,
self_id,
) {
Ok(res) => {
debug!("got res in TypeNS: {:?}", res);
Expand All @@ -1400,6 +1451,7 @@ impl LinkCollector<'_, '_> {
ValueNS,
base_node.expect_real(),
extra_fragment,
self_id,
) {
Ok(res) => Ok(res),
Err(ErrorKind::AnchorFailure(msg)) => {
Expand Down
15 changes: 15 additions & 0 deletions src/test/rustdoc/issue-84827.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![deny(broken_intra_doc_links)]

// @has issue_84827/struct.Foo.html
// @has - '//a[@href="struct.Foo.html#structfield.foo"]' 'Self::foo'

pub struct Foo {
pub foo: i32,
}

pub mod bar {
impl crate::Foo {
/// [`Self::foo`].
pub fn baz(&self) {}
}
}