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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented May 6, 2021

fix #84827

id of Self is passed as argument to resolve, and used if we cannot re…
e31b68d 
…solve the string by any othe way. Also if self_id refers to an Impl, it is replaced by the DefId of the object it refers to
@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2021
@tdelabro
Copy link
Contributor Author

tdelabro commented May 6, 2021

r? @jyn514

@tdelabro
Copy link
Contributor Author

tdelabro commented May 6, 2021 •
edited
Loading

So this fix the bug, no more broken link in this situation.
Nevertheless it slightly increase the complexity to the whole process of resolving intra links.

@jyn514 and multiple comments in the code pointed out that we should maybe use self_id more, instead of strings. But when I tried to invert those two lines:

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

it broke multiple tests. So I kept things the way there are, with just this new extra backup way to resolve the link if all the previous ones have failed.

I'm pretty sure this can be improved in an other MR tho

tdelabro
tdelabro commented May 6, 2021
View reviewed changes
src/librustdoc/passes/collect_intra_doc_links.rs
@@ -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) {}
	}
}

@jyn514 jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 9, 2021
jyn514
jyn514 suggested changes May 9, 2021
View reviewed changes
src/librustdoc/passes/collect_intra_doc_links.rs
@@ -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.

src/librustdoc/passes/collect_intra_doc_links.rs
@@ -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
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) {}
	}
}

src/librustdoc/passes/collect_intra_doc_links.rs
}
}
_ => curr_id,
});

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

src/librustdoc/passes/collect_intra_doc_links.rs
Comment on lines +844 to +871
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,
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.)

@jyn514
Copy link
Member

jyn514 commented May 9, 2021

I won't have time to review any follow-ups.

r? @Manishearth

@rust-highfive rust-highfive assigned Manishearth and unassigned jyn514 May 9, 2021
@jyn514 jyn514 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 May 9, 2021
@bors
Copy link
Contributor

bors commented May 15, 2021

☔ The latest upstream changes (presumably #85328) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710
Copy link
Member