Add adapter support for a Module item type.

[nmathewson]

Hello!

This is prerequisite for solving
obi1kenobi/cargo-semver-checks#482

This is my first attempt at using trustfall, graphql, or the rustdoc_types crate, so I certainly may have missed some things. Please do not assume I know what I am doing in the slightest. 😉

The patch is against rustdoc-v27. I think it should work similarly with the other branches since the Module struct is the same across all the supported versions of rustdoc, but I don't know how you prefer to handle porting patches like this.

[nmathewson]

I cited 0.16.0 here because 0.11.0 does not have the is_stripped field.

[nmathewson]

It's possible that we don't even want to consider the top-level module (the one with is_crate=true) to be an item? But I think it's okay.

[obi1kenobi]

I think we probably do want to consider it a module, since that both matches how Rust thinks about it and also because otherwise it'll be hard (perhaps even impossible) to answer "what items are at top level in the crate."

[obi1kenobi]

This is excellent work, truly A+ stuff 💯 I'm very impressed, because I know the sad state the Trustfall docs (and this project's docs) are in relative to the complexity involved here 😅 Thank you so much! 🙌

There's just one little entirely non-obvious thing missing, and that's to update supported_item_kind() in adapter/mod.rs to say that ItemEnum::Module(..) is a supported item kind. Without that, module items won't be indexed by our "look up items by name" optimization code. That in turn would lead to the strange behavior that enumerating all modules works fine but looking them up by their importable path incorrectly returns no results. This would be great to have a test case for, if you're up for it.

But again, there's zero way you could have spotted that, and the codebase didn't do anything whatsoever to direct you toward it. Sorry! And thank you for bearing with it 😁

Don't worry about porting this to earlier rustdoc versions. I have a script for that that does it mostly automagically, since the code is almost always identical or extremely close to it. (Yet another reason to bind to the semantics of the underlying data set instead of its bytes-on-disk representation.)

If you feel up for it, a subsequent PR adding docs around the sticking points you ran into would be super, super helpful and much appreciated. Also definitely good to mention that contributors shouldn't worry about porting their PRs to prior rustdoc versions and that I'm happy to sort that out.

[obi1kenobi]

I think we can trigger this .expect() with an item re-export from an external crate. Probably best to use .filter_map() to discard missing items from the iterator until we get a chance to properly handle cross-crate items at some point in the future.

[obi1kenobi]

This is an entirely reasonable design here. But if you're up for it, I think we can take advantage of an advanced little Trustfall trick and do something clever and more ergonomic.

Here's the key observation that generally takes some getting used to when starting to use Trustfall: when modelling a data source in Trustfall, we don't actually have to map the underlying data representation one-to-one. We can model the semantic representation of the underlying data, instead of matching how it is syntactically represented in the underlying data source.

Here's what I mean.

Consider when we'd use the is_crate property: it's when we want to find the top-level module of the crate. The query for that might look like something like this:

{
    Crate {
        item {
            ... on Module {
                is_crate @filter(op: "=", value: ["$true"])
                
                < ... etc ... >
            }
        }
    }
}

This makes sense, and is a one-to-one match for the data representation in the underlying JSON file. But it's also a bit clunky, no?

Consider this alternative (names subject to bikeshedding):

{
    Crate {
        root_module {
            # this is already of type Module and represents the crate root
            < ... same stuff as the "etc." in the earlier query ... >
        }
    }
}

Now, root_module isn't an explicit relationship that is ever named as such in the underlying representation — everything in the JSON file goes through the item index — but this is what the actual, truly-underlying data looks like, semantically. Nothing is stopping us from implementing such an edge in Trustfall, and it's often both a query ergonomics improvement and it gives us some more decoupling from the specific implementation choices of the JSON representation.

I know that was a lot to take in at once. Did that make sense? Happy to answer any questions, or even hop on a quick video call to chat about it, if that would be helpful.

[nmathewson]

I think this is a fine idea, but I'm not 100% sure how to actually build this. I guess this would mean adding a "root_module" edge in the graphql file, and then adding a "root_module" handler in resolve_crate_items, but at that point I'm afraid I'm lost. The best I can think to do, based on copy-and-paste, is something like:

	"root_module" =>
	    Box::new(
		optimizations::item_lookup::resolve_crate_items(adapter, contexts, resolve_info).filter(|item| {
                  // check if item is a module with is_crate == true
                })
	    ),

but I'm not really sure that's right.

[obi1kenobi]

That would work just fine, well done!

We can do slightly better by taking advantage of the JSON format structure: the JSON file has a key called "root" at top level whose value is the item ID of the root module. We can look up that ID in the crate index directly, which will let us avoid iterating through all items in the crate.

[nmathewson]

Okay. I tried this:

	"root_module" => resolve_neighbors_with(contexts, move |vertex| {
	    let current_crate = adapter.current_crate;
	    let previous_crate = adapter.previous_crate;

	    let origin = vertex.origin;
	    let item_index = match origin {
                Origin::CurrentCrate => &current_crate.inner.index,
                Origin::PreviousCrate => {
		    &previous_crate
                        .expect("no previous crate provided")
                        .inner
                        .index
                }
	    };

	    let module = item_index.get(&Id("root".into())).expect("crate had no root module");
	    Box::new(std::iter::once(origin.make_item_vertex(module)))
	}),

but I'm afraid I ran into lifetime issues. Do I need to pass current_crate and previous_crate as arguments into this function manually?

[obi1kenobi]

This is a common issue: adapter itself doesn't necessarily live long enough to be moved in, but adapter.current_crate and adapter.previous_crate are &'a references so they live long enough. Assign them outside the closure and move them into it instead of moving adapter — almost all the other edge implementations elsewhere do the same thing for the same reason.

[nmathewson]

Ah, that did it!

[obi1kenobi]

Given the new edge, I'd remove this property since I think it won't be that useful — if someone wants the crate-level module, they can get it directly via the new edge.

[nmathewson]

Hm. What if you wanted to get all of the modules except for the root module? Would that justify this property?

[obi1kenobi]

That's still doable and in a way that more closely maps to the semantic data model (the query says "not the root module" explicitly here) instead of mapping to the is_crate field which is mostly for JSON representation purposes, not something Rust modules inherently have.

Here's how:

{
    Crate {
        root_module {
            id @tag
        }

        item {
            ... on Module {
                id @filter(op: "!=", value: ["%id"])
                
                < ... these are all modules that aren't the root ... >
            }
        }
    }
}

[obi1kenobi]

I think we probably do want to consider it a module, since that both matches how Rust thinks about it and also because otherwise it'll be hard (perhaps even impossible) to answer "what items are at top level in the crate."

[obi1kenobi]

Throw a check for supported_item_kind here after resolving the item and before making the vertex, that's causing the error you're seeing in the test. Rustdoc's Import item type (which, confusingly, represents re-exports) isn't part of our schema at the moment so we shouldn't represent it as a vertex.

Now that I think of it, we should perhaps modify make_item_vertex() to call that function internally and return Option<Vertex> instead of Vertex... But that can go in a separate PR if you'd like, just to keep the scope here limited.

[nmathewson]

okay, I think I did something for each of our open threads here; please let me know if there's more. I'll also think about documentation a bit and circle around to it soon.

[obi1kenobi]

Two minor nits and this is good to merge.

[obi1kenobi]

Given the new edge, I'd remove this property since I think it won't be that useful — if someone wants the crate-level module, they can get it directly via the new edge.

[obi1kenobi]

Nicely done! Thanks for putting this together!

[obi1kenobi]

I've ported this PR to the other rustdoc versions, and queued up new patch releases for all supported rustdoc versions. When those patch releases make their way through the pipeline, you'll be able to use them in cargo-semver-checks by just running cargo update first.

It's possible it you might have to use --precise because some of our other dependencies raised MSRV to 1.70 and we haven't done that yet so we can't use those newer versions yet.