Rustdoc: Fix macros 2.0 and built-in derives being shown at the wrong path

[danielhenrymantilla]

Fixes #74355

• waiting on author + draft PR since my code ought to be cleaned up w.r.t. the way I avoid the .unwrap()s:

  • dummy items may avoid the first ?,

  • but within the module traversal some tests did fail (hence the second ?), meaning the crate did not possess the exact path of the containing module (extern / impl blocks maybe? I'll look into that).

r? @jyn514

[jyn514]

This is awful, but I don't know a better way to do it.

[jyn514]

Ugh, I really wish rustdoc didn't use its own data structures (#76382). That said, if this works I won't block on it, but it's unambiguously a hack.

Suggested change

	for path_segment in macro_parent_module.data {
	let path_segment = path_segment.to_string();
	cur_mod = cur_mod.mods.iter_mut().find(|module| {
	matches!(
	module.name, Some(symbol)
	if symbol.with(|mod_name| mod_name == path_segment)
	)
	})?;
	}
	// HACK: rustdoc has no way to lookup `doctree::Module`s by their HirId
	// HACK: instead, lookup the module by its name, by looking at each path segment one at a time
	// WARNING: this will probably break in the presence of re-exports or shadowing.
	for path_segment in macro_parent_module.data {
	let path_segment = path_segment.to_string();
	cur_mod = cur_mod.mods.iter_mut().find(|module| {
	matches!(
	module.name, Some(symbol)
	if symbol.with(|mod_name| mod_name == path_segment)
	)
	})?;
	}

@GuillaumeGomez before I sign off on this, do you know a better way to do this?

[danielhenrymantilla]

That said, if this works I won't block on it, but it's unambiguously a hack.

Definitely! The reason I've submitted this code (despite my not having cleaned that try block elsewhere) was for you people to tell me if there was some obvious tool I had missed in places like here, given my lack of experience with rustdoc and rustc internals 👶

[jyn514]

dummy items may avoid the first ?,

Sorry, I wasn't thinking when I said this originally - dummy items are always impls, not macros. A ? wouldn't have worked for fake items, those will ICE instead before the function returns. I'm not sure when parent would return None for a macro.

but within the module traversal some tests did fail (hence the second ?), meaning the crate did not possess the exact path of the containing module (extern / impl blocks maybe? I'll look into that).

Yeah, this will break for macros defined in an impl since those are unnamed. I don't think those macros can be publicly re-exported but I could be wrong.

[Dylan-DPC-zz]

@danielhenrymantilla any updates?

[danielhenrymantilla]

@Dylan-DPC still have an issue with the links in the rendered documentation, so I have to fix that before the PR is mergeable. I have been quite busy the last 2 weeks, so I have been unable to make much progress in the meantime, but now I should be able to 🙂

[Dylan-DPC-zz]

Cool, no issues :)

[RalfJung]

@danielhenrymantilla what is the status of this? #74355 is the last thing blocking the stabilization of #73394, so if there's any way I can help move this forward, please let me know. :) However, I have zero experience working on rustdoc... or even running rustdoc, for that matter.^^

[jyn514]

@RalfJung the review comments need to be addressed, especially the tests.

In general I'm not super happy with the approach this takes, but I don't have a better one in mind, so I'm ok with merging this temporarily until #77828 is fixed.

[Dylan-DPC-zz]

@danielhenrymantilla any updates?

[jyn514]

Now that you added the comment, I think this isn't true in general and there will be times the assertion fails, because cur_mod_def_id is always a module and macro_parent may not be. I think looking for the nearest module is the correct behavior.

[danielhenrymantilla]

Technically that should be caught by the loop logic: if one of the ancestors is not a module, we will be hitting one of the two continue 'exported macros; bail points, and thus never reach that part of the code. That being said, I am open to suggestions to make these things clearer (e.g., we could .try_fold() and Err rather than for + continue 'outer 🤷)

[jyn514]

Ok, but wouldn't it be simpler to only look up the parent module in the first place? Then you wouldn't have to skip the segments you don't care about, because you care about all of them.

[danielhenrymantilla]

We could add a "fail faster" branch where we'd compare the immediate parent and the parent module, and bail if they're not equal; but even then we'd still need to check that that module in and of itself is reachable / nameable, so most of the logic would still be here. I think the micro optimization is not worth the extra branch; but I keep thinking that a try_fold() for the inner loop may be interesting, since it avoids the need to continue the 'outer loop:

Example (assuming postfix match for the ergonomics)

for def in krate.exported_macros {
    let put_macro_in = |module| module.macros.push((def, None));
    // The `def` of a macro in `exported_macros` should correspond to either:
    //  - a `#[macro_export] macro_rules!` macro,
    //  - a built-in `derive` (or attribute) macro such as the ones in `::core`,
    //  - a `pub macro`.
    // Only the last two need to be fixed, thus:
    if def.ast.macro_rules {
        put_macro_in(&mut top_level_module);
        continue;
    }
    let tcx = self.cx.tcx;
    // Note: this is not the same as `.parent_module()`. Indeed, the latter looks
    // for the closest module _ancestor_, which is not necessarily a direct parent
    // (since a direct parent isn't necessarily a module, c.f. #77828).
    let macro_parent_def_id = {
        use rustc_middle::ty::DefIdTree;
        tcx.parent(tcx.hir().local_def_id(def.hir_id).to_def_id()).unwrap()
    };
    let macro_parent_path = tcx.def_path(macro_parent_def_id);
    // HACK: rustdoc has no way to lookup `doctree::Module`s by their HirId. Instead,
    // lookup the module by its name, by looking at each path segment one at a time.
    macro_parent_path.data.try_fold(&mut top_level_module, |(cur_mod, path_segment)| {
        // Path segments may refer to a module (in which case they belong to the type
        // namespace), which is _necessary_ for the macro to be accessible outside it
        // (no "associated macros" as of yet). Else we bail.
        let path_segment_ty_ns = match path_segment.data {
            rustc_hir::definitions::DefPathData::TypeNs(symbol) => symbol,
            _ => return Err(()),
        };
        // Descend into the child module that matches this path segment, if any.
        cur_mod.mods.iter_mut().find(|child| child.name == Some(path_segment_ty_ns)).ok_or(())
    }).match {
        Ok(module) => {
            let module_def_id =
                tcx.hir().local_def_id(module.id).to_def_id()
            ;
            assert_eq!(module_def_id, macro_parent_def_id);
            put_macro_in(&mut module);
        },
        Err(()) => continue,
    }
}

[jyn514]

Hmm, ok. I'm fine with either the current code or the try_fold.

[jyn514]

r=me once Petrochenkov signs off on the resolve changes.

[petrochenkov]

In theory #[rustc_macro_transparency = "transparent"] pub macro foo { ... } is also possible and needs update called, but that's not a case used by the standard library, and all of this is a hack anyway, so it should be ok for now.

[petrochenkov]

I'm ok with this.
r? @jyn514

[jyn514]

@bors r=jyn514,petrochenkov

[bors]

📌 Commit bceb173 has been approved by jyn514,petrochenkov

[bors]

⌛ Testing commit bceb173 with merge 7a19392...

[bors]

☀️ Test successful - checks-actions
Approved by: jyn514,petrochenkov
Pushing 7a19392 to master...