Manifest checks

[epage]

The API extends past the rust code to the manifest

Possible checks include

• Explicit feature removed (major breaking)
• Implicit feature (optional dependency) removed (informative)
  • This is separate because pre-1.60's namespaced features, crates couldn't hide these
• Implicit feature added via removing dep: reference to optional dependency (informative)
• Explicit feature no longer implies another explicit feature (major breaking)
• Feature no longer implies an implicit feature (informative)

[obi1kenobi]

The primary challenge in this issue is coming up with a good design with respect to the experience we offer users.

Two open questions we should consider before embarking toward implementing this feature:

• I haven't looked at the sparse index design. Can we always get a manifest file even with a sparse index? Or would we need to run cargo metadata somehow instead?
• Currently we allow users to specify their own rustdoc JSON instead of relying on c-s-c to generate it. In that case, are manifest checks disabled? Or do we allow or require users to also specify a manifest file?

[epage]

I haven't looked at the sparse index design. Can we always get a manifest file even with a sparse index? Or would we need to run cargo metadata somehow instead?

You can't get the full manifest from the index (sparse or git). You could use cargo metadata in the temp projects that are created.

Currently we allow users to specify their own rustdoc JSON instead of relying on c-s-c to generate it. In that case, are manifest checks disabled? Or do we allow or require users to also specify a manifest file?

What are the use cases for this feature?

I wonder if the approach to take is to split up the library logic so someone can make a custom tool using only the rustdoc side of things while the high level command only works on packages and not on rustdoc json.

[obi1kenobi]

Yeah, maybe the answer is that we remove that feature. It's relatively rarely used, generally when one's CI already builds rustdoc JSON (for either current or baseline) in a separate step and wants to avoid a (potentially expensive) rebuild inside c-s-c. For context, today each rustdoc JSON build takes ~15x as long as running all the checks combined — and if the baseline JSON isn't cached, then we do two rustdoc JSON builds.

Or maybe we change this feature to expect a "bundle" file of some sort instead of a rustdoc JSON file. This bundle could hold the rustdoc JSON + manifest, and in the future when we do cross-crate analysis it could also hold re-exported dependency crates' rustdoc JSON files as well. Our baseline caching logic will have to be updated to also cache manifest info anyway, so perhaps the bundling logic gets reused from there.

We have options, this is surmountable.

[obi1kenobi]

#1007 implements getting manifest information from cargo metadata and implements the first manifest lint: a check for features that have been removed.

That means the lints described at the top of this issue are now unblocked and possible to implement, possibly with small additional schema tweaks.

@epage quick question for you: do you still think this 👇 should be informative-only?

Implicit feature (optional dependency) removed (informative)

• This is separate because pre-1.60's namespaced features, crates couldn't hide these

I'm asking because that comment was made relatively soon after 1.60 had shipped. But today we're 20+ Rust releases and 2.5+ years after 1.60 shipped, so it might be a good idea to be a bit more strict by default.

What do you think?

[epage]

We almost removed implicit features in Edition 2024 but had to back it out because the approach we took needed more design work...

Implicit features are weird because

• Some people do have old enough MSRVs
• Some people haven't updated to not expose them
• Some people don't know they are exposed

They don't show up in most workflows for maintainers, so they are easy to overlook or then not know how to remove them.

[obi1kenobi]

Agreed on the complexity and non-obviousness — that's why I wanted to make this stricter than just an informative (warn) lint.

I propose making that lint error level by default. What do you think?

My reasoning is that warn won't block publishing so for most workflows today, people would just miss it. Nobody looks at warnings in CI etc. Given the footgun potential of breaking an implicit feature without realizing, I think an error-level lint that forces folks to notice and make an explicit choice to proceed or not would be better.

[epage]

More so my point was there are reasonable situations where the author does not intent for this to be a breaking change. Granted, there is lint control now so they can always change it and this becomes a question of defaults. I guess in that case, error makes sense. It would help if the UX were clear that this is reasonable for the user to override. I can see users having a hard time understanding when a lint is reasonably overriden. Take "removing pub item` and "removing implicit feature". Those are on different scales and a user might not be confident in making calls on that.

[obi1kenobi]

That makes sense, thank you. We can work on the messaging around the lints to give users good advice 👍