rustdoc: Confusing intra-doc handling of Self in enum

[camelid]

This code works fine:

pub enum Foo {
    /// [Self::Bar::abc]
    Bar {
        abc: i32,
        xyz: i32,
    },
}

But this code's intra-doc link doesn't resolve:

pub enum Foo {
    Bar {
        abc: i32,
        /// [Self::Bar::abc]
        xyz: i32,
    },
}

warning: unresolved link to `Bar::Bar::abc`
 --> foo.rs:4:14
  |
4 |         /// [Self::Bar::abc]
  |              ^^^^^^^^^^^^^^ no item named `Bar` in scope
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default

Doesn't Self usually only refer to types, not enum variants? (Barring the proposed RFC to make enum variant types.)

[camelid]

Yeah, this definitely seems like a bug. I have to use Foo::Self::abc (or Foo::Bar::abc). Self::abc doesn't work because it looks for the enum variant as if it's a free-floating type:

warning: unresolved link to `Bar::abc`
 --> foo.rs:4:14
  |
4 |         /// [Self::abc]
  |              ^^^^^^^^^ no item named `Bar` in scope
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default

[jyn514]

@camelid do you know if this ever worked? I might have broken it in #76467.

[jyn514]

But also I agree that this seems like a misfeature, I'd be ok with getting rid of it. To do that, just delete variant_field and all references to it.

[hellow554]

hey @jyn514

you are correct. Bisecting this leads to b7ebc6b

So the fix is to get rid of the variant_field method completly (and of course delete any code that calls it)? Or am I misreading it?

[jyn514]

Yes, I think so. No one noticed this breaking for 12 weeks and Self always refers to the type in normal code, not the variant: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9655fe188d99f728e90e65a08164e825

[lucas-deangelis]

Hi, I don't know if anyone is already planning to work on it, but if not, would it be a good first issue? I'm still new to the language but I'd like to start contributing.

[camelid]

Yes, this should be a good first issue. You can claim it with @rustbot claim. Post here or on Zulip in #t-compiler/help if you have any questions :)

[lucas-deangelis]

Thanks! I'll have a look at it. @rustbot claim

[lucas-deangelis]

I've taken a deeper look at the issue, and at the doc on contributions to Rust. Correct me if I'm wrong, but I think I'm supposed to start with making a failing test case. I made a issue-82209.rs file in src/test/rustdoc/. Here's the file:

#![deny(broken_intra_doc_links)]
pub enum Foo {
    Bar {
        abc: i32,
        /// [Self::Bar::abc]
        xyz: i32,
    },
}

I'm not sure about the #![deny(broken_intra_doc_links)], but I couldn't get the test to fail without something like that, and it's more precise than the #![deny(warnings)] that I used at first. Does this look okay to you?

[jyn514]

@lucas-deangelis yes, that looks fine. I have a couple nits but they can wait until you make a PR to review, that test is enough to show what's going wrong.

[lucas-deangelis]

Thanks! I'll start working on the fix now.

[camelid]

Assigning P-medium and removing I-prioritize as discussed in the prioritization working group.

[lucas-deangelis]

I've tried removing variant_field and all references to it as @jyn514 suggested, however I still had the same behavior as before. There's a part of resolve with FIXME(jynelson): why is this different from variant_field?, but even after removing it, the link is still resolved to Bar::Bar::abc. I think some of the logic is somewhere else.

[jyn514]

Hmm, if that doesn't fix it the bug might be in the Self handling:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 870 in 15598a8

let name = self.cx.tcx.opt_item_name(self_id).map(|sym| sym.to_string());

You could try extending

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 837 to 845 in 15598a8

	} else if matches!(
	self.cx.tcx.def_kind(item.def_id),
	DefKind::AssocConst
	| DefKind::AssocFn
	| DefKind::AssocTy
	| DefKind::Variant
	| DefKind::Field
	) {
	self.cx.tcx.parent(item.def_id)

to also use the parent item for DefKind::Ctor or DefKind::Variant?

[lucas-deangelis]

This alone doesn't seem to fix it, however I've found that after adding DefKind::Ctor on the match, modifying the following part modify the error message for the resolved link:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 1016 to 1026 in 15598a8

	let is_lone_self = path_str == "Self";
	let is_lone_crate = path_str == "crate";
	if path_str.starts_with("Self::") || is_lone_self {
	if let Some(ref name) = self_name {
	if is_lone_self {
	path_str = name;
	} else {
	resolved_self = format!("{}::{}", name, &path_str[6..]);
	path_str = &resolved_self;
	}
	}

The problem here seems to be that name refers to the kind and not the enum.

[camelid]

This alone doesn't seem to fix it, however I've found that after adding DefKind::Ctor on the match

Did you try matching both DefKind::Ctor and DefKind::Variant?

[lucas-deangelis]

This is my naive attempt:

} else if matches!(
            self.cx.tcx.def_kind(item.def_id),
            DefKind::AssocConst
                | DefKind::AssocFn
                | DefKind::AssocTy
                | DefKind::Variant
                | DefKind::Field
                | DefKind::Ctor(_, _)
        ) {
            self.cx.tcx.parent(item.def_id)

I didn't add DefKind::Variant because it is already in the match, however, I think there's something here that I don't understand since you said "matching both", and here I'm matching either of them.

[camelid]

I didn't add DefKind::Variant because it is already in the match, however, I think there's something here that I don't understand since you said "matching both", and here I'm matching either of them.

What you wrote is what I meant :)

I meant match either Ctor or Variant (as in DefKind::Ctor(..) | DefKind::Variant), but I didn't realize that Variant was already in the match.

Does your attempt work?

[lucas-deangelis]

What you wrote is what I meant :)

Nice, thanks for confirming.

I meant match either Ctor or Variant (as in DefKind::Ctor(..) | DefKind::Variant), but I didn't realize that Variant was already in the match.

Does your attempt work?

My attempt doesn't work.

Right now I'm looking at resolve_link, since it's the part where the link is created. I think that by trying to understand more resolve_link, I can figure out from where exactly does the path comes from, and why in that case it gets resolved to Bar::Bar::abc instead of Foo::Bar::abc.

[jyn514]

@lucas-deangelis all the "preprocessing" of the link (turning Self into a path) happens before resolve_link is called, in

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 858 in a8486b6

let self_name = self_id.and_then(|self_id| {

.

[camelid]

Aha, I bet this is the issue:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 837 to 845 in a8486b6

	} else if matches!(
	self.cx.tcx.def_kind(item.def_id),
	DefKind::AssocConst
	| DefKind::AssocFn
	| DefKind::AssocTy
	| DefKind::Variant
	| DefKind::Field
	) {
	self.cx.tcx.parent(item.def_id)

For fields in enum variants, the parent is the variant, not the enum.

[lucas-deangelis]

@lucas-deangelis all the "preprocessing" of the link (turning Self into a path) happens before resolve_link is called, in

Thanks for the clarification.

For fields in enum variants, the parent is the variant, not the enum.

So if I understand correctly, if we encounter a variant, we should return the parent of the parent of item.def_id?

[jyn514]

So if I understand correctly, if we encounter a variant, we should return the parent of the parent of item.def_id?

If you encounter a field that's in a variant. Basically make the check recursive (since fields may not be in a variant, e.g. struct S { x: usize }).

[lucas-deangelis]

Thanks, I'll try that.

[lucas-deangelis]

I think I'm making some progress. I added a new condition to check if we encounter a field that's in a variant before the matches! line 837:

else if (matches!(self.cx.tcx.def_kind(item.def_id), DefKind::Field) && matches!(self.cx.tcx.def_kind(self.cx.tcx.parent(item.def_id).unwrap()), DefKind::Variant)) {
            self.cx.tcx.parent(item.def_id).and_then(|item_id| self.cx.tcx.parent(item_id))
} else if matches!(
            self.cx.tcx.def_kind(item.def_id),
            DefKind::AssocConst
                | DefKind::AssocFn
                | DefKind::AssocTy
                | DefKind::Field
                | DefKind::Variant
) {

I'll have to find a way to handle the call to .unwrap() better once I get this working. Now the error I get is:

error: unresolved link to `Foo::Bar::abc`
 --> rust/src/test/rustdoc/issue-82209.rs:5:14
  |
5 |         /// [Self::Bar::abc]
  |              ^^^^^^^^^^^^^^ the enum `Foo` has no variant or associated item named `abc`
  |

Am I correct in assuming that this is now an error in the link resolution?

[jyn514]

Hmm, is this still with the calls to variant_field removed? Try adding those back.

[lucas-deangelis]

Hmm, is this still with the calls to variant_field removed? Try adding those back.

It was, and now with it back it works. Thanks. Should I make a pull request now?

[jyn514]

That would be great, thanks!

[lucas-deangelis]

And it's merged! Thank you @jyn514 and @camelid for the help on the issue, @hellow554 for some of the initial work and @Dylan-DPC for the rollups.

[jyn514]

You're very welcome! Thank you for tackling the fix :)