Skip to content

Add document-unstable default-feature #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joshka merged 5 commits into from
Jul 11, 2025
Merged

Add document-unstable default-feature #23

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff73d8a
Add `document-unstable` default-feature
bjoernQ Apr 24, 2025
34fc2b0
Remove feature, use `instability_exclude_unstable_docs` config
bjoernQ Apr 30, 2025
2ac6d59
Rename cfg, reorganize
bjoernQ Jul 7, 2025
cd8265d
ci: use RUST_FLAGS env instead of --config
joshka Jul 11, 2025
80876f9
docs: add docs for instability_disable_unstable_docs
joshka Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ jobs:
run: cargo generate-lockfile
- name: cargo test --locked
run: cargo test --locked --all-features --all-targets
- name: cargo test with instability_exclude_unstable_docs
run: RUSTFLAGS="--cfg instability_exclude_unstable_docs" cargo test --locked --all-features --all-targets
- name: cargo test --doc
run: cargo test --locked --all-features --doc
minimal-versions:
Expand Down Expand Up @@ -80,6 +82,8 @@ jobs:
run: cargo +nightly update -Zdirect-minimal-versions
- name: cargo test
run: cargo test --locked --all-features --all-targets
- name: cargo test with instability_exclude_unstable_docs
run: RUSTFLAGS="--cfg instability_exclude_unstable_docs" cargo test --locked --all-features --all-targets
- name: Cache Cargo dependencies
uses: Swatinem/rust-cache@v2
os-check:
Expand All @@ -100,6 +104,8 @@ jobs:
run: cargo generate-lockfile
- name: cargo test
run: cargo test --locked --all-features --all-targets
- name: cargo test with instability_exclude_unstable_docs
run: RUSTFLAGS="--cfg instability_exclude_unstable_docs" cargo test --locked --all-features --all-targets
- name: Cache Cargo dependencies
uses: Swatinem/rust-cache@v2
coverage:
Expand Down
3 changes: 3 additions & 0 deletions build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn main() {
println!("cargo:rustc-check-cfg=cfg(instability_exclude_unstable_docs)");
}
110 changes: 84 additions & 26 deletions src/unstable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,24 @@ impl UnstableAttribute {
.into_iter()
.map(|ident| quote! { #[allow(#ident)] });

#[cfg(not(instability_exclude_unstable_docs))]
let (cfg, not_cfg) = (
quote! { #[cfg(any(doc, feature = #feature_flag))] },
quote! { #[cfg(not(any(doc, feature = #feature_flag)))] },
);

#[cfg(instability_exclude_unstable_docs)]
let (cfg, not_cfg) = (
quote! { #[cfg(feature = #feature_flag)] },
quote! { #[cfg(not(feature = #feature_flag))] },
);

quote! {
#[cfg(any(doc, feature = #feature_flag))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = #feature_flag)))]
#item

#[cfg(not(any(doc, feature = #feature_flag)))]
#not_cfg
#(#allows)*
#hidden_item
}

@joshka joshka May 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bjoernQ Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

Expand All @@ -78,8 +90,15 @@ impl UnstableAttribute {
pub fn expand_impl(&self, mut item: impl Stability + ToTokens) -> TokenStream {
let feature_flag = self.feature_flag();
self.add_doc(&mut item);

#[cfg(not(instability_exclude_unstable_docs))]
let cfg = quote! { #[cfg(any(doc, feature = #feature_flag))] };

@joshka joshka May 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#[cfg(instability_exclude_unstable_docs)]
let cfg = quote! { #[cfg(feature = #feature_flag)] };

quote! {
#[cfg(any(doc, feature = #feature_flag))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = #feature_flag)))]
#item
}
Expand Down Expand Up @@ -107,6 +126,7 @@ impl UnstableAttribute {
.map_or(String::from("unstable"), |name| format!("unstable-{name}"))
}
}

#[cfg(test)]
mod tests {
use pretty_assertions::assert_eq;
Expand Down Expand Up @@ -144,6 +164,20 @@ mod tests {
const WITH_FEATURES_DOC: &str = "# Stability\n\n**This API is marked as unstable** and is only available when the `unstable-experimental`\ncrate feature is enabled. This comes with no stability guarantees, and could be changed\nor removed at any time.";
const ISSUE_DOC: &str = "The tracking issue is: `#123`.";

fn cfg_attributes(feature: &str) -> (TokenStream, TokenStream) {
#[cfg(not(instability_exclude_unstable_docs))]
return (
quote! { #[cfg(any(doc, feature = #feature))] },
quote! { #[cfg(not(any(doc, feature = #feature)))] },
);

#[cfg(instability_exclude_unstable_docs)]
return (
quote! { #[cfg(feature = #feature)] },
quote! { #[cfg(not(feature = #feature))] },
);
}

#[test]
fn expand_with_feature() {
let item: syn::ItemType = parse_quote! { pub type Foo = Bar; };
Expand All @@ -152,13 +186,15 @@ mod tests {
issue: None,
};
let tokens = unstable.expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable-experimental");
let expected = quote! {
#[cfg(any(doc, feature = "unstable-experimental"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable-experimental")))]
#[doc = #WITH_FEATURES_DOC]
pub type Foo = Bar;

#[cfg(not(any(doc, feature = "unstable-experimental")))]
#not_cfg

@joshka joshka May 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bjoernQ May 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 joshka May 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[allow(dead_code)]
#[doc = #WITH_FEATURES_DOC]
pub(crate) type Foo = Bar;
Expand All @@ -174,14 +210,16 @@ mod tests {
issue: Some("#123".to_string()),
};
let tokens = unstable.expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
#[doc = #ISSUE_DOC]
pub type Foo = Bar;

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
#[doc = #ISSUE_DOC]
Expand All @@ -194,13 +232,15 @@ mod tests {
fn expand_public_type() {
let item: syn::ItemType = parse_quote! { pub type Foo = Bar; };
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub type Foo = Bar;

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) type Foo = Bar;
Expand All @@ -216,15 +256,17 @@ mod tests {
}
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub struct Foo {
pub field: i32,
}

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) struct Foo {
Expand All @@ -243,16 +285,18 @@ mod tests {
}
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub enum Foo {
A,
B,
}

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) enum Foo {
Expand All @@ -269,13 +313,15 @@ mod tests {
pub fn foo() {}
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub fn foo() {}

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) fn foo() {}
Expand All @@ -291,15 +337,17 @@ mod tests {
}
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub trait Foo {
fn bar(&self);
}

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) trait Foo {
Expand All @@ -315,13 +363,15 @@ mod tests {
pub const FOO: i32 = 42;
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub const FOO: i32 = 42;

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) const FOO: i32 = 42;
Expand All @@ -335,13 +385,15 @@ mod tests {
pub static FOO: i32 = 42;
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub static FOO: i32 = 42;

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) static FOO: i32 = 42;
Expand All @@ -357,15 +409,17 @@ mod tests {
}
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub mod foo {
pub fn bar() {}
}

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(dead_code)]
#[doc = #DEFAULT_DOC]
pub(crate) mod foo {
Expand All @@ -381,13 +435,15 @@ mod tests {
pub use crate::foo::bar;
};
let tokens = UnstableAttribute::default().expand(item);

let (cfg, not_cfg) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
pub use crate::foo::bar;

#[cfg(not(any(doc, feature = "unstable")))]
#not_cfg
#[allow(unused_imports)]
#[doc = #DEFAULT_DOC]
pub(crate) use crate::foo::bar;
Expand All @@ -401,8 +457,10 @@ mod tests {
impl Default for crate::foo::Foo {}
};
let tokens = UnstableAttribute::default().expand_impl(item);

let (cfg, _) = cfg_attributes("unstable");
let expected = quote! {
#[cfg(any(doc, feature = "unstable"))]
#cfg
#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))]
#[doc = #DEFAULT_DOC]
impl Default for crate::foo::Foo {}
Expand Down
Loading