Add document-unstable default-feature

[bjoernQ]

This adds a default feature to include the cfg(doc) - this makes it possible to opt-out and generate docs including just the public-API-surface (and helps in using e.g. cargo-semver-checks to check the public API surface)

Background

Since #12 the unstable macro expands to this

#[cfg(any(doc, feature = "unstable"))]
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = " This is unstable"]
#[doc = "# Stability\n\n**This API is marked as unstable** and is only available when the `unstable`\ncrate feature is enabled. This comes with no stability guarantees, and could be changed\nor removed at any time."]
pub fn something_unstable() {
    todo!()
}
#[cfg(not(any(doc, feature = "unstable")))]
#[allow(dead_code)]
#[doc = " This is unstable"]
#[doc = "# Stability\n\n**This API is marked as unstable** and is only available when the `unstable`\ncrate feature is enabled. This comes with no stability guarantees, and could be changed\nor removed at any time."]
pub(crate) fn something_unstable() {
    core::panicking::panic("not yet implemented")
}

This is great and nice since documentation generated from this example will include the something_unstable function without the need to select the unstable feature. It's a great default and probably what most users need or want.

However, since rustdoc unconditionally sets the doc config flag it is not possible to generate documentation without the unstable API included.

While it's probably a niche use-case to generate docs for the stable API-surface only (most likely just for manual inspection) it becomes more important when using tools for semver-checks (e.g. cargo-semver-checks or public-api) - these tools usually rely on generating API-docs in JSON format for further processing and the unstable part of the API shouldn't get checked.

The only way I see to accomplish that is to have a way to emit code from the unstable macro without cfg(doc)

Drawbacks

Features add complexity. If (for some reason) users included instability with default-features set to false they will see a difference in the generated documentation.

Alternatives

Unfortunately, I don't see any real alternatives since rustdoc unconditionally sets the doc config.

The only (quite hacky) alternative I could think of is to pre-process the generated JSON and remove the unstable parts. Unfortunately, the rustdoc JSON format makes this an unpleasant experience especially because we would need to inspect the docs to see if they include a well known pattern. Ideally, I would like to avoid this pre-processing step.

Implementation

I think a default-feature which opt-in to including cfg(doc) is the most straight-forward way to do this.

I am not too confident about the naming of this new feature (i.e. happy to change it) or if you would prefer a non-default feature to opt-out of including cfg(doc)

[joshka]

Can you please take one step back and describe the problem this solves? I'm missing context here to understand it.

[bjoernQ]

Can you please take one step back and describe the problem this solves? I'm missing context here to understand it.

Sorry for the way too brief PR-description - I updated it and hopefully it now explains the motivation better. Let me know if the description is still lacking details

[joshka]

Sorry for the way too brief PR-description - I updated it and hopefully it now explains the motivation better. Let me know if the description is still lacking details

No problem. If I had a dollar for every time I'd jumped into a solution assuming that the other person had the context,...

I'm going to have to think about this one a bit more, but my initial impression of the solution is that adding a feature flag that all downstream consumers of the library for documentation seems a little heavy. I think that will happen here unless I'm mistaken about how that works. Can you confirm that this approach would break downstream uses of the library?

Sometimes speaking in generalities hides the true problem a bit. What actual concrete problems does this cause for you in your daily use. I can see that you're describing the part which you're saying is a problem (inability to turn off generation of docs), but I'd like to see what you're doing that needs this (e.g. what commands are you running, what's the result, and how does this lib cause that). You've sort of alluded to that the json generated is a problem, but I'd like to capture the details a bit more. (This may also surface other alternative solutions).

[bjoernQ]

This will not break things if

• the user supplies the unstable feature when generating the documentation (most users probably will do that - they might have more cfg-gates in their code on the feature)
• the user doesn't opt out of the default features (probably not many will do that - the crate had no default features before - on the other hand some users tend to disable default-features "by default")

But yes - if users opt out of instability's default feature and don't pass the unstable feature when generating docs they will end up with documentation missing the unstable part of the API (which should be easy to spot). Actual code should never be affected by this change.

To illustrate things a bit

cargo doc would currently include the unstable API items.

With this new feature flag (enabled by default) it would be the same.

When adding instability with no-default features enabled (instability = { version = "0.37.0", default-features = false } that above command would generate documentation without the unstable API items but cargo doc --features=unstable would also include the unstable API in that case.

No rush with this: I was able to work around things, but I guess ideally having a way to opt-out of having unstable API items documented always is useful to other's as well

[MabezDev]

I'm going to have to think about this one a bit more, but my initial impression of the solution is that adding a feature flag that all downstream consumers of the library for documentation seems a little heavy. I think that will happen here unless I'm mistaken about how that works. Can you confirm that this approach would break downstream uses of the library?

What about a --cfg instead of a feature? We only want this for semver checking our API (the stable parts). The workaround @bjoernQ implemented here is really not ideal because we're now at the mercy of rustdoc json format changes :(.

I think given that cargo semver checks is making its way into cargo itself, there should be some way to do this, whether its via feature flag/ cfg or some other mechanism.

[bjoernQ]

I changed this PR to do what Scott suggested.

The new cfg is instability_exclude_unstable_docs which will not include unstable parts in docs if enabled.

I think it's very unlikely that a user will set this by mistake.

I didn't change the original PR description since it would make the conversation so far look weird

[MabezDev]

Gentle ping @joshka, no rush in particular to land this, but this is a feature we want so I'd just like to know if this kind of functionality will ever be accepted.

[joshka]

Thanks for the reminder on this. It had slipped my mind.

I initially looked at this on my phone and misunderstood how this worked a bit. I thought that it was adding a new feature flag to the generated code, but it was actually just for this library.

To clarify this a bit:

• currently this library emits code which always includes the unstable docs when running rustdoc (#[cfg(any(doc, feature = "unstable-xxx"))]
• your original commit suggested gating this behind a "document-unstable" feature on this crate, which generally removed doc from that config check when not enabled.
• the updated version uses a cfg flag for this instead of a feature flag

I've got a couple of other thoughts about this that I can't quite articulate right tonight (I'll swing back to this in the next couple of days though). A preview though, is that a feature flag might be better than a compiler flag for this.

[joshka]

There's a couple of things which confused me on initially reading this:

• Naming the variables cfg/not_cfg makes it more difficult / confusing to read and maintain as they inherently clash with the #[cfg] macro. This actually caught me on the initial reading of this (I'm pretty sure I read it on a phone), so not a hypothetical misunderstanding, an actual one. A longer name here would help this a bit.
• There's a few too many levels of indirection to easily make out how this works from looking at it at a glance. We're in a proc macro impl, checking a config flag, to generate two token streams each with feature flag checks which are then used in a token stream directly below.
• the extracted part of this adds 12 lines to avoid repeating 9 lines of code. It seems like it would be clearer to instead repeat the code here if cfg!(instability_exclude_unstable_docs) { quote! { ... } } else quote! { ... } and remove that indirection.

[bjoernQ]

• Naming the variables cfg/not_cfg makes it more difficult / confusing to read

Agreed - tried to come up with a better naming

• There's a few too many levels of indirection to easily make out how this works from looking at it at a glance. We're in a proc macro impl, checking a config flag, to generate two token streams each with feature flag checks which are then used in a token stream directly below.

I see - unfortunately we cannot just inline the cfg-gating on instability_disable_unstable_docs - it would be emitted in the resulting code which is not what we want here

[joshka]

Same note about variable name here as above and adding 5 extra lines (including the newline) to avoid repeating 5 lines.

[joshka]

As a general rule, I like tests to be fairly linear and simple when possible. I don't like to have to think about an expectation being several different things based on a flag like this. Often this means not calling any code that has logic like this and being WET (Write Everything Twice) instead of DRY.

Here that means adding an extra #[cfg] on a copy of the test and copying the body with the modified bit to show how it works.

It's possible that we might need to introduce something like https://crates.io/crates/trybuild to help test both this functionality in enabled and disabled mode properly as I don't see an easy way to otherwise test both approaches in a single test run. I note that the addition to the CI splits that into two runs.

[bjoernQ]

As a general rule, I like tests to be fairly linear and simple when possible. I don't like to have to think about an expectation being several different things based on a flag like this. Often this means not calling any code that has logic like this and being WET (Write Everything Twice) instead of DRY.

I'm absolutely on the same page about that

It's possible that we might need to introduce something like https://crates.io/crates/trybuild to help test both this functionality in enabled and disabled mode properly as I don't see an easy way to otherwise test both approaches in a single test run. I note that the addition to the CI splits that into two runs.

Would be awesome to have something like that. Unfortunately, I don't see a way to pass feature flags to trybuild - maybe something similar exists but haven't found anything yet

[joshka]

Yeah, this is something that falls at the edges of my knowledge, so solutions are very much about exploring rather than synthesizing. Making the tests simple and easy to understand / fix when they break is the main point of this.

I'm fine with the extra CI to do this (unless there's some obvious other way that would make it possible to run this without that needed). That can definitely be out of scope for getting this PR over the line and something to think about for later.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.74%. Comparing base (c9ae887) to head (cd8265d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
- Coverage   89.93%   83.74%   -6.19%     
==========================================
  Files           4        4              
  Lines         626      363     -263     
==========================================
- Hits          563      304     -259     
+ Misses         63       59       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bjoernQ]

Thanks for the reminder on this. It had slipped my mind.

I initially looked at this on my phone and misunderstood how this worked a bit. I thought that it was adding a new feature flag to the generated code, but it was actually just for this library.

To clarify this a bit:

• currently this library emits code which always includes the unstable docs when running rustdoc (#[cfg(any(doc, feature = "unstable-xxx"))]
• your original commit suggested gating this behind a "document-unstable" feature on this crate, which generally removed doc from that config check when not enabled.
• the updated version uses a cfg flag for this instead of a feature flag

I've got a couple of other thoughts about this that I can't quite articulate right tonight (I'll swing back to this in the next couple of days though). A preview though, is that a feature flag might be better than a compiler flag for this.

Thanks for getting back to this!

It seems at least you are not considering functionality like this to be something you absolutely don't want, right?

I agree a feature flag is something I'd personally would prefer - it's more discoverable and "less magic"

[joshka]

It seems at least you are not considering functionality like this to be something you absolutely don't want, right?

I can see how this is reasonable to have. The rationale is sensible. You don't want to have code marked as unstable to interact with tooling that would treat changes to be breaking (e.g. cargo-semver).

I agree a feature flag is something I'd personally would prefer - it's more discoverable and "less magic"

My main concerns are around whether the feature flag would suffer problems due to feature unification. I think this might also happen with the current approach of compiler flags too though. I need to test out a few scenarios on this to understand it better. My concern is if you have a workspace with unstable functions in crate A, and crate B calls those functions, can you generate docs for A (and use cargo-semver etc.), or does this break the workspace somehow.

[bjoernQ]

My main concerns are around whether the feature flag would suffer problems due to feature unification. I think this might also happen with the current approach of compiler flags too though. I need to test out a few scenarios on this to understand it better. My concern is if you have a workspace with unstable functions in crate A, and crate B calls those functions, can you generate docs for A (and use cargo-semver etc.), or does this break the workspace somehow.

I see where you are coming from, and feature unification is definitely something to consider. For the cfg flags I don't think people can run into issues since libraries cannot set them themselves

Indeed, the default-feature document-unstable as done in the initial commit is a problem in this situation:

Crate A - uses `instablility` with default features = false to not build the docs including the unstable part of the API
     |
     uses Crate B - uses `instablility` with default features enabled

In this case - assuming Crate B is not under your control - there is no way to opt-out of the default-feature now

One way to solve this via features would be to invert the meaning of the feature-flag and name it like no-document-unstable (and not making it a default feature). While a library using the crate could set it and cause problems this way I'd assume people won't do that and rather enable the feature only when running rustdoc

So, my gut feeling is that the cfg-flag to opt out of having unstable parts of the API documented might be "safer" since it should never give any surprising results

[MabezDev]

It seems to me there are only two choices here, an inverted feature flag or passing a --cfg flag. I think either would suit our use case, so it's a question of what's best for the ratatui project. @joshka Do you have any input here?

I'd love to move this forward, as our hack where we manually parse json doc is tiresome (breaks with compiler updates), and unfortunately a bit error prone (we're seeing some failures in our CI related to unstable API changes: https://github.com/esp-rs/esp-hal/actions/runs/15438663420/job/43451004700?pr=3505 :().

Please let us know if this feature is still wanted, and the requirements you have for the implementation, we'd be happy to accommodate!

[MabezDev]

Any progress on a decision being made here? We're on our third cycle of dealing with rustdoc json output changed (esp-rs/esp-hal#3742), we'd really like to avoid another :(.

[joshka]

Let me take a deeper look at it soon (I'll try to get to it tomorrow) and I'll get back to you.

[joshka]

Having thought about this a bit more. I think the --cfg flag is the way to go here as avoiding feature unification makes sense.

How about we call it instability_disable_unstable_docs (or possibly it would be better named in the positive instability_document_unstable_items with an effective default of true unless set to false.

Some edge cases to think about:

• If your lib A imports lib B and with the doc setting disabled, and lib B has a link to an unstable item in another item, does this fail in some way (e.g. generating docs for your workspace)?

/// lib A

/// Calls [`lib_b::foo`] to foo the foo
#[unstable("aaa")]
fn aaa() {}

/// lib B

#[unstable(feature = "foo")]
fn foo() {}

/// Bar calls [`foo`] to baz
fn bar() {}

Regarding testing, I'd expect a unit test that gates on the cfg flag, and checks that the generated code for a function (or whatever other item is simplest) when turned on does not include the doc attributes at all.

That is to say, yep. Go ahead with implementing this. Let's get it over the line.

Apologies again for the delay.

[joshka]

Thanks for submitting this PR and bearing with me getting over the line. I've just merged it and it should be available shortly in 0.3.8