Use real Spans when resolving intra-doc links

[pitaj]

Fixes #78696

Tested this works locally by modifying bufreader.rs like so:

 /// data loss.
 ///
-/// [`TcpStream::read`]: Read::read
+/// [`TcpStream::read`]: crate::net::TcpStream::read
 /// [`TcpStream`]: crate::net::TcpStream

@rustbot modify labels: T-doc, A-intra-doc-links

r? @jyn514

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jyn514]

It looks like something is going wrong I didn't expect:

 ---- [rustdoc] rustdoc/intra-link-cross-crate-crate.rs stdout ----

error: htmldocck failed!
status: exit code: 1
command: "/usr/bin/python2.7" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/intra-link-cross-crate-crate" "/checkout/src/test/rustdoc/intra-link-cross-crate-crate.rs"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
5: @has check failed
	`XPATH PATTERN` did not match
	// @has outer/fn.f.html '//a[@href="../inner/fn.g.html"]' "crate::g"

I'm not sure exactly what - you'd have to debug it. I normally debug issues something like this:

1. Run rustdoc on the file without going through compiletest: rustdoc +stage1 src/test/rustdoc/intra-link-cross-crate-crate.rs
2. See what the differences are between the generated file and the expected. It will be in doc/ somewhere, in this case outer.
3. If you're not sure what's wrong after seeing the difference, look at the debug output: RUSTDOC_LOG=rustdoc::passes::collect=debug rustdoc +stage1 src/test/rustdoc/intra-link-cross-crate-crate.rs. If this gives a warning about logging being disabled, set debug-logging = true and rebuild, then run it again.

[jyn514]

See what the differences are between the generated file and the expected.

BTW, if you rebase over #78752 it will do this for you automatically. You'll need to install tidy and delta though.

[pitaj]

Okay so the difference here boils down to the following:

- <a class="fn" href="fn.f.html" title="outer::f fn">f</a></td><td class="docblock-short"><p>Links to <a href="../inner/fn.g.html" title="crate::g">crate::g</a></p>
+ <a class="fn" href="fn.f.html" title="outer::f fn">f</a></td><td class="docblock-short"><p>Links to [crate::g]</p>

As the error says, the link to inner::g is no longer there. The documentation for inner::f refers to inner::g as [crate::g], and this is copied to the documentation in the outer crate. But then in the outer crate, it tries to resolve it as if it's a member of the outer crate. (You can prove this by adding a new g to the outer crate). Curiously, no warning or error is emitted by rustdoc when this happens, it just skips it entirely. I think this is because in report_diagnostic, all non-locals are skipped.

I'm not sure where to go from here. One question is even if we want to leave the link text as crate::g when IMO it should really read as inner:f since we're in a different crate now.

I added some debug statements and found this, not sure how useful it is.

[src/librustdoc/passes/collect_intra_doc_links.rs:1605] &path_str = "$crate::g"
[src/librustdoc/passes/collect_intra_doc_links.rs:1605] &disambiguator = None
[src/librustdoc/passes/collect_intra_doc_links.rs:1605] &dox = "Links to [crate::g]"
[src/librustdoc/passes/collect_intra_doc_links.rs:1605] &link_range = Some(
    10..18,
)
[src/librustdoc/passes/collect_intra_doc_links.rs:1605] &kinds = [
    NotResolved {
        module_id: DefId(18:0 ~ inner[ea92]),
        partial_res: None,
        unresolved: "$crate::g",
    },
    NotResolved {
        module_id: DefId(18:0 ~ inner[ea92]),
        partial_res: Some(
            Def(
                Mod,
                DefId(0:0 ~ outer[8787]),
            ),
        ),
        unresolved: "g",
    },
    NotResolved {
        module_id: DefId(18:0 ~ inner[ea92]),
        partial_res: None,
        unresolved: "$crate::g",
    },
]
[src/librustdoc/passes/collect_intra_doc_links.rs:1548] cx.as_local_hir_id(item.def_id) = None

[jyn514]

@pitaj if you resolve $crate on its own, does anything useful come up? Right now you can't test it like [$crate] because it will be stripped out:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 961 in b385598

if path_str.contains(|ch: char| !(ch.is_alphanumeric() || ":_<>, ".contains(ch))) {

But you could change that to allow $ and see if [$crate] does anything useful.

[jyn514]

Also, what's the span of the item? It should have a span in inner, not from outer.

I'm not sure where to go from here. One question is even if we want to leave the link text as crate::g when IMO it should really read as inner:f since we're in a different crate now.

I don't think we should ever change the link text on re-exports. inner::f is how the link is resolved, but it might not be what the author intended - consider a facade crate that re-exports both:

pub fn g() {}
/// Link to [crate::g]
pub fn f() {}

// outer
pub use inner::g;
pub use inner::f;

[pitaj]

Also, what's the span of the item? It should have a span in inner, not from outer.

[src/librustdoc/passes/collect_intra_doc_links.rs:1546] item = Item {
    source: Span {
        filename: Real(
            Named(
                "/home/peter/Dev/rust-78797-ws/inner/src/lib.rs",
            ),
        ),
        cnum: crate18,
        loline: 4,
        locol: 0,
        hiline: 4,
        hicol: 10,
        original: /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:4:1: 4:11 (#0),
    },
    name: Some(
        "f",
    ),
    attrs: Attributes {
        doc_strings: [
            DocFragment {
                line: 0,
                span: /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:3:1: 3:24 (#0),
                parent_module: None,
                doc: "Links to [crate::g]",
                kind: SugaredDoc,
            },
        ],
        other_attrs: [],
        cfg: None,
        span: Some(
            /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:3:1: 3:24 (#0),
        ),
        links: [],
        inner_docs: false,
    },
    inner: FunctionItem(
        Function {
            decl: FnDecl {
                inputs: Arguments {
                    values: [],
                },
                output: Return(
                    Tuple(
                        [],
                    ),
                ),
                c_variadic: false,
                attrs: Attributes {
                    doc_strings: [],
                    other_attrs: [],
                    cfg: None,
                    span: None,
                    links: [],
                    inner_docs: false,
                },
            },
            generics: Generics {
                params: [],
                where_predicates: [],
            },
            header: FnHeader {
                unsafety: Normal,
                constness: NotConst,
                asyncness: NotAsync,
                abi: Rust,
            },
            all_types: [],
            ret_types: [],
        },
    ),
    visibility: Public,
    def_id: DefId(18:3 ~ inner[ea92]::f),
    stability: None,
    deprecation: None,
}

It does have the correct span, at least in the item here. The resolve call also has the correct span, but resolve_path returns None

[src/librustdoc/passes/collect_intra_doc_links.rs:364] &path_str = "$crate::g"
[src/librustdoc/passes/collect_intra_doc_links.rs:364] &span = /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:4:1: 4:11 (#0)
[src/librustdoc/passes/collect_intra_doc_links.rs:364] &ns = TypeNS
[src/librustdoc/passes/collect_intra_doc_links.rs:364] &current_item = Some(
    "f",
)
[src/librustdoc/passes/collect_intra_doc_links.rs:364] &module_id = DefId(18:0 ~ inner[ea92])
[src/librustdoc/passes/collect_intra_doc_links.rs:364] &extra_fragment = None
[src/librustdoc/passes/collect_intra_doc_links.rs:366] self.resolve_path(path_str, span, ns, module_id) = None

[jyn514]

Can you try resolving $crate on its own and see whether that resolves at all?

[pitaj]

I haven't tried it on its own, but $crate does seem to resolve to a module.

I did a little more digging, and I found that resolve gets a ty_res

[src/librustdoc/passes/collect_intra_doc_links.rs:428] &path_root = "$crate"
[src/librustdoc/passes/collect_intra_doc_links.rs:450] &ty_res = Def(
    Mod,
    DefId(0:0 ~ inner[8787]),
)

But because that resolves to a module, no further resolution is performed in the following block, and res is None

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 432 to 562 in dc06a36

	let res = match ty_res {
	Res::PrimTy(prim) => Some(
	self.resolve_primitive_associated_item(prim, ns, module_id, item_name, item_str),
	),
	Res::Def(DefKind::Struct | DefKind::Union | DefKind::Enum | DefKind::TyAlias, did) => {
	debug!("looking for associated item named {} for item {:?}", item_name, did);
	// Checks if item_name belongs to `impl SomeItem`
	let assoc_item = cx
	.tcx
	.inherent_impls(did)
	.iter()
	.flat_map(|&imp| {
	cx.tcx.associated_items(imp).find_by_name_and_namespace(
	cx.tcx,
	Ident::with_dummy_span(item_name),
	ns,
	imp,
	)
	})
	.map(|item| (item.kind, item.def_id))
	// There should only ever be one associated item that matches from any inherent impl
	.next()
	// Check if item_name belongs to `impl SomeTrait for SomeItem`
	// This gives precedence to `impl SomeItem`:
	// Although having both would be ambiguous, use impl version for compat. sake.
	// To handle that properly resolve() would have to support
	// something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
	.or_else(|| {
	let kind =
	resolve_associated_trait_item(did, module_id, item_name, ns, &self.cx);
	debug!("got associated item kind {:?}", kind);
	kind
	});
	if let Some((kind, id)) = assoc_item {
	let out = match kind {
	ty::AssocKind::Fn => "method",
	ty::AssocKind::Const => "associatedconstant",
	ty::AssocKind::Type => "associatedtype",
	};
	Some(if extra_fragment.is_some() {
	Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
	} else {
	// HACK(jynelson): `clean` expects the type, not the associated item
	// but the disambiguator logic expects the associated item.
	// Store the kind in a side channel so that only the disambiguator logic looks at it.
	self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
	Ok((ty_res, Some(format!("{}.{}", out, item_str))))
	})
	} else if ns == Namespace::ValueNS {
	debug!("looking for variants or fields named {} for {:?}", item_name, did);
	match cx.tcx.type_of(did).kind() {
	ty::Adt(def, _) => {
	let field = if def.is_enum() {
	def.all_fields().find(|item| item.ident.name == item_name)
	} else {
	def.non_enum_variant()
	.fields
	.iter()
	.find(|item| item.ident.name == item_name)
	};
	field.map(|item| {
	if extra_fragment.is_some() {
	let res = Res::Def(
	if def.is_enum() {
	DefKind::Variant
	} else {
	DefKind::Field
	},
	item.did,
	);
	Err(ErrorKind::AnchorFailure(
	AnchorFailure::RustdocAnchorConflict(res),
	))
	} else {
	Ok((
	ty_res,
	Some(format!(
	"{}.{}",
	if def.is_enum() { "variant" } else { "structfield" },
	item.ident
	)),
	))
	}
	})
	}
	_ => None,
	}
	} else {
	None
	}
	}
	Res::Def(DefKind::Trait, did) => cx
	.tcx
	.associated_items(did)
	.find_by_name_and_namespace(cx.tcx, Ident::with_dummy_span(item_name), ns, did)
	.map(|item| {
	let kind = match item.kind {
	ty::AssocKind::Const => "associatedconstant",
	ty::AssocKind::Type => "associatedtype",
	ty::AssocKind::Fn => {
	if item.defaultness.has_value() {
	"method"
	} else {
	"tymethod"
	}
	}
	};
	if extra_fragment.is_some() {
	Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
	} else {
	let res = Res::Def(item.kind.as_def_kind(), item.def_id);
	Ok((res, Some(format!("{}.{}", kind, item_str))))
	}
	}),
	_ => None,
	};
	res.unwrap_or_else(|| {
	if ns == Namespace::ValueNS {
	self.variant_field(path_str, current_item, module_id)
	} else {
	Err(ResolutionFailure::NotResolved {
	module_id,
	partial_res: Some(ty_res),
	unresolved: item_str.into(),
	}
	.into())
	}
	})
	}

[src/librustdoc/passes/collect_intra_doc_links.rs:571] &res = None

[jyn514]

@pitaj that match shouldn't be hit at all, it's for associated items. This should be going through

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 381 to 383 in dc06a36

	_ => {
	return Ok((res, extra_fragment.clone()));
	}

, the only reason it wouldn't is if rustc_resolve returned an error.

[pitaj]

I see. I'll try $crate on its own tonight or tomorrow.

[pitaj]

I approached the just-crate link a little different. I change inner to this:

/// Links to [crate]
/// Links to [crate::g]
pub fn f() {}
pub fn g() {}

And changed the replacement to also handle a path of just crate:

                 }
-            } else if path_str.starts_with("crate::") {
+            } else if path_str.starts_with("crate::") || path_str == "crate" {
                 // Resolve `crate` relative to the original scope of the item, not the current scope.

It does resolve, but the link in outer ends up incorrectly pointing to outer. Here's some context. Somehow even though resolve_path gets called with the correct span (pointing to inner), we end up with a resolution to outer.

[src/librustdoc/passes/collect_intra_doc_links.rs:366] "fn resolve" = "fn resolve"
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &path_str = "$crate"
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &span = /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:5:1: 5:11 (#0)
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &ns = TypeNS
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &current_item = Some(
    "f",
)
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &module_id = DefId(18:0 ~ inner[ea92])
[src/librustdoc/passes/collect_intra_doc_links.rs:366] &extra_fragment = None
[compiler/rustc_resolve/src/lib.rs:3202] module = Some(Def(Mod, DefId(18:0 ~ inner[ea92])))
[compiler/rustc_resolve/src/lib.rs:3203] parent_scope = ParentScope {
    module: Some(Def(Mod, DefId(18:0 ~ inner[ea92]))),
    expansion: ExpnId(
        0,
    ),
    macro_rules: Empty,
    derives: [],
}
[compiler/rustc_resolve/src/lib.rs:3214] self.resolve_path(&Segment::from_path(path), Some(ns), parent_scope, false,
                  path.span, CrateLint::No) = Module(
    Module(
        Some(Def(Mod, DefId(0:0 ~ outer[8787]))),
    ),
)
[compiler/rustc_resolve/src/lib.rs:3204] path = Path {
    span: /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:5:1: 5:11 (#0),
    segments: [
        PathSegment {
            ident: $crate#0,
            id: NodeId(18),
            args: None,
        },
    ],
    tokens: None,
}
[compiler/rustc_resolve/src/lib.rs:3204] res = Def(
    Mod,
    DefId(0:0 ~ outer[8787]),
)
[src/librustdoc/passes/collect_intra_doc_links.rs:342] resolver.resolve_str_path_error(span, &path_str, ns, module_id) = Ok(
    (
        Path {
            span: /home/peter/Dev/rust-78797-ws/inner/src/lib.rs:5:1: 5:11 (#0),
            segments: [
                PathSegment {
                    ident: $crate#0,
                    id: NodeId(18),
                    args: None,
                },
            ],
            tokens: None,
        },
        Def(
            Mod,
            DefId(0:0 ~ outer[8787]),
        ),
    ),
)
[src/librustdoc/passes/collect_intra_doc_links.rs:368] self.resolve_path(path_str, span, ns, module_id) = Some(
    Def(
        Mod,
        DefId(0:0 ~ outer[8787]),
    ),
)

[jyn514]

@petrochenkov do you know what's going wrong here? $crate is resolving as the outer crate even though the span is from inner.

[pitaj]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[petrochenkov]

resolve_str_path_error doesn't apply the passed span to path segments, only to the whole path.
The span should be applied to individual segments for $crate to work, that means using Ident::new instead of Ident::from_str/Ident::with_dummy_span.

[pitaj]

So what's my next step here? I need to replace all those instances with Ident::new? Were the changes I've already made necessary? I don't see how the ident information gets passed through resolve_path and therefore resolve_str_path_error. Does that interface need to change?

[petrochenkov]

So what's my next step here? I need to replace all those instances with Ident::new?

Yes.

Were the changes I've already made necessary?

I think yes.

I don't see how the ident information gets passed through resolve_path and therefore resolve_str_path_error. Does that interface need to change?

No, as you can see resolve_str_path_error splits the path into components by itself.

[pitaj]

I've replaced all instances I could find, but I may have done so too naively, as it is still failing in the same way.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[jyn514]

@pitaj I think you need to change resolve_str_path_error. The changes in your most recent commit don't hurt anything but I also don't think they have any effect.

rust/compiler/rustc_resolve/src/lib.rs

Lines 3201 to 3202 in 09c9c9f

	segments: iter::once(Ident::with_dummy_span(kw::PathRoot))
	.chain(path_str.split("::").skip(1).map(Ident::from_str))

[pitaj]

I replaced those too, and locally it didn't make a difference. I expect the CI run to fail the same way.

[petrochenkov]

I didn't consider that crate info is currently lost during span decoding from metadata, unless you are decoding a whole macro.
It means supplying a decoded span with $crate won't work.

Then I guess you'll have to remove crate:: from paths and resolve the remaining part of the path by passing module_id pointing to the desired crate's root to resolve_str_path_error.

[jyn514]

@petrochenkov that's the hack that broke this in the first place: #78696. Is it possible to load that metadata even for things that aren't macros?

[petrochenkov]

Is it possible to load that metadata even for things that aren't macros

Yes, and we need to do that eventually, but right now it will break a lot of assumptions in resolve and other places.

[petrochenkov]

@jyn514
Could you give a summary, in what cases do you need to resolve things as if they were written in a different location, introducing "rustdoc hygiene"?
I have no idea what the code in #78696 attempts to convey, and very little time or desire to dig into this myself.

[jyn514]

@petrochenkov there are a few cases that hack handles, all of them dealing with cross-crate re-exports:

// crate1
//! Link to [crate::f]
pub fn f() {}

// crate2
pub use crate1;

Here crate should still refer to crate1, but rustc_resolve thinks it refers to crate2. So the rustdoc hack resolves it relative to crate1 by setting module_id appropriately.

// crate1
//! Link to [crate::u8]
pub mod u8 {}

// crate2
pub use crate1;

Here removing crate:: altogether means rustdoc incorrectly thinks it has ambiguity (between the u8 in scope and the primitive). So rustdoc adds self:: at the start to disambiguate the one in scope. This doesn't actually have anything to do with rustc_resolve, it's specifically because rustdoc requires you to disambiguate primitives.

3. This is the case that breaks today.

// Uncomment this and the warning will go away
// use crate::io::Read;

pub mod io {
    pub trait Read {
        fn read(&mut self);
    }
}

pub mod bufreader {
    //! [`crate::TcpStream::read`]
    use crate::io::Read;
}

pub struct TcpStream;

impl crate::io::Read for TcpStream {
    fn read(&mut self) {
    }
}

Here Read is in scope in bufreader, where the link was written, but not in the crate root. Because module_id is set to the crate root, rustdoc mistakenly thinks Read is not in scope, and doesn't resolve TcpStream::read.

There are a few paths forward:

1. Wait for the larger fix in rustc_metadata that loads the info for $crate for items other than macros.
2. Add more hacks on top of this; have an original_module passed to resolve_associated_trait item that's different than module_id and only used for the traits in scope.
3. Add an even bigger hack to make case 3. work in the current crate, but not outside of it: skip replacing crate:: only if it's the local crate. This is just a bandaid and will still break if you re-export the item.

Both 2 and 3 make me nervous - I'm ok with them as a temporary workaround, but 3. doesn't actually help very much and 2. make me nervous other things have been missed. I would slightly prefer to wait for 1. but if @pitaj wants to take on 2. I'd be willing to review it.

[pitaj]

Given that there is already a hack that allows that case to work (use super:: instead) I'd rather wait for (1) than implement another hack.

I would be interested in trying my hand at (1) if there's mentoring available.