cargo-semver-checks: `#[doc(hidden)]` result in false positives
Steps to reproduce the bug with the above code
It is possible to use #[doc(hidden)] to implement seemingly-breaking changes without actually breaking semver compatibility.
Since cargo-semver-checks only uses the json rustdoc output to form its analysis, the claim that cargo-semver-checks has no false positives should probably have this shortcoming noted in the readme.
e.g. these two versions of the same code below are technically semver compatible:
pub struct Foo {}
pub struct Bar {}
#[doc(hidden)]
pub struct Foo {}
pub struct Bar {}
as merely hiding a type (or as is more often, a method) from the documentation doesn’t break the semver contract.
or, for a more interesting case, the following:
pub struct Fo {}
pub struct Foo {}
// The previous release included a typo in the type name so the following is kept for backwards-compatibility:
#[doc(hidden)]
pub type Fo = Foo;
There is a lot more dark magic that can done that would break docs but preserve backwards compatibility. Relying solely on the docs is a great and efficient way of implementing straightforward semver breakage cases, but I don’t think it’s fair to claim that any tool using this approach has no false positives.
Actual Behaviour
A breaking change is reported.
Expected Behaviour
No breaking change should be reported (though I realize this is fundamentally not possible for this crate as it is written). As such, the “no false positives” claim should be removed or at least the caveats thoroughly mentioned.
Additional Context
No response
Debug Output
No response
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 20 (11 by maintainers)
If there are different usages of
#[doc(hidden)], perhaps there may need to be some configuration to help the crate authors indicate what is acceptable in their case?PyO3, for example, has a lot of
#[doc(hidden)]items exported from thepyo3::impl_module. This module is explicitly documented as implementation details which are semver exempt.It might be good enough for PyO3’s case to have an option
--allow-semver-exempt=pyo3::impl_or something like this, which the crate authors could use to specify semver-exempt namespaces.#[doc(hidden)]is often used precisely to exclude something from the public API, i.e. exclude it from semantic versioning guarantees. Many times,#[doc(hidden)]is the only option, when there is no technical alternative in Rust (e.g. macros polluting the top-level namespace, internal dependencies between crates, generated code, etc.)As such, it seems wrong to me that changes in hidden items are detected as “breaking semver”. Shouldn’t hidden items be treated as non-existing from a public API perspective?
As such, I disagree with this conclusion:
Foowas part of the public API and is now removed from it. This is a semver-breaking change in my opinion.If you want to communicate that a previously existing symbol should no longer be used,
#[deprecated]is the better choice.Fo/Footypo example.If on the other hand the presence of the symbol is a deal-breaking bug, the symbol should be removed, and the next semver-incompatible version should be released.
Some popular crates follow this understanding, for example syn.
This view is further backed by the Rust API guidelines:
The main use case for PyO3 is types which are supposed to be implemented, i.e. cannot be sealed, but never manually, i.e. only via proc-macro generated code.
The above proposal sounds correct to me; I can’t think of any uses in PyO3 which would not fit in the above.
Maybe as proposal for a mechanistic definition: Allow listing a path prefix means that these items are completely removed before any checks are performed. Anything publically visible referencing them (and not removed itself in the previous step) is an error by itself.
At least for our use case, the re-export itself is most likely an error and we would benefit from it being reported, independent of any changes. As a corollary, changes could be reported as well.
I think just publically exposing these items outside of the crate and allowing to list them could be reported as a potential semver hazard. Thereby, I would say changes can be reported as this is strictly less of a requirement.
Definitely because it affects types we expect our uses to access.
This is reported directly in
fields_stripped, so theirs a relativly easy “native” way to do it.I don’t even remember why we do this. Can we get rid of private items?