Create Item::full_name method to include name of reexports

[GuillaumeGomez]

I originally wanted to make Item::name field private and implement full_name as name replacement directly. Not sure if it's a good idea though. What do you think?

cc @camelid
r? @notriddle

[rust-highfive]

Some changes occurred in clean/types.rs.

cc @camelid

[notriddle]

I originally wanted to make Item::name field private and implement full_name as name replacement directly.

This sounds like a good idea to me.

[GuillaumeGomez]

Except that in some cases, we filter on Item.name.is_none, so it would be a problem to always return the name of the reexport. I can still make the field private and provide both methods though.

[notriddle]

Where and why is that done? If it's for the search index, maybe a method returning bool could be added just for that.

[GuillaumeGomez]

If an Item doesn't have a name, it doesn't have a page for itself and it's how it's filtered in html/render and why reexports don't have a page on their own.

[notriddle]

It seems like there should be a function whose job is just to determine whether something gets its own page or not, separately from the considering whether something has a name or not. After all, there are multiple considerations that go into this:

• Only an item with a name will get a page (obviously).
• The item must be a direct descendant of a module; associated items can have names, but still get no pages to themselves.
• Non-#[doc(inline)] re-exports don't get their own pages.

The question of whether something gets a page is a "responsibility," in the SRP sense. I really don't want to be tweaking the logic behind the name field, when the thing actually being changed isn't the name, but rather the decision about whether something gets its own page or not.

[GuillaumeGomez]

The item must be a direct descendant of a module; associated items can have names, but still get no pages to themselves.

With this, reexports should get their own page since they are direct descendant of a module (the reexport itself).

[notriddle]

The point I’m trying to make is to distill the logic behind whether something gets its own page. Right now, this logic is spread out all over the codebase. We barely even have a name for this!

In pseudo-rust, here’s that bulleted list:

fn has_page(self: &Item) -> bool {
    if self.full_name().is_none() { return false }
    if self.parent_item().is_mod() { return false }
    if self.is_use() { return false }
    true
}

[GuillaumeGomez]

Hum... I could write a function which handles that I guess. That'd be interesting. :)

[notriddle]

Of course, we should get @camelid's input on all this, too.

[GuillaumeGomez]

Absolutely! Let's wait for them then.

[camelid]

Sorry, I didn't realize you were waiting on me! I've been busier with other stuff recently. What is it that you wanted my feedback on?

[GuillaumeGomez]

We wanted your opinion on "should we add a method which returns true if the the item should get its own documentation page instead of basing it on whether it has a name or not". :)

[bors]

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

[GuillaumeGomez]

Fixed merge conflict.

[camelid]

We wanted your opinion on "should we add a method which returns true if the the item should get its own documentation page instead of basing it on whether it has a name or not". :)

My opinion is that sounds like a great idea. @notriddle's pseudo-Rust at #96864 (comment) looks like a good approach. My only question is with this part, which seems incorrect:

    if self.parent_item().is_mod() { return false }

Is that supposed to be if self.parent_item().is_struct() || self.parent_item().is_enum() || ...?

[camelid]

Does s refer to the y or the x in pub use x as y;?

[GuillaumeGomez]

Yes. Simple is created with new_simple which returns an Import. The Import struct contains the source (so x in this case) and the kind (our Simple("y")).

[notriddle]

if self.parent_item().is_mod() { return false }

Sorry. That should have been if !self.parent_item().is_mod() { return false }.

Only the children of modules get their own page, because only modules get their own directory to put stuff in.

[GuillaumeGomez]

So currently, the filtering of which item should get its own page is here. Another place is in print_sidebar. The last place being here. In all cases, we need to check the item to call the correct function. I can eventually centralize it with a function taking a callback, but I'm not sure we can do much better. At least the logic would be in one place only. What do you think?

EDIT: as for the has_page method, it'd look like this:

pub(crate) fn is_parent_a_mod(&self) -> bool {
    let def_id = match self.item_id {
        DefId(def_id) => def_id,
        Auto { .. } | Blanket { .. } => return true,
        ItemId::Primitive(_, _) => return false,
    };
    if let Some(def_id) = def_id.as_local() {
        let hir = tcx.hir();
        let parent_def_id = hir.get_parent_item(hir.local_def_id_to_hir_id(def_id));

        matches!(hir.opt_def_kind(parent_def_id), Some(DefKind::Mod))
    } else {
        false
    }
}

pub(crate) fn has_page(&self, tcx: TyCtxt<'_>) -> bool {
    !self.is_stripped() && !self.is_import() && parent.is_mod() && self.full_name().is_some()
}

[bors]

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

[jyn514]

It's been a while - @GuillaumeGomez are you planning to follow up on this?

[GuillaumeGomez]

No I don't, or at least not before long. Closing then.