False positive: Items moved to another crate + re-exported from there are reported as missing

[chrysn]

Steps to reproduce the bug with the above code

I'm moving two traits and a type from the coap-handler-implementations (v0.3.5) crate to the coap-handler (v0.1.4) crate, and pub use them in their old places.

Diff on coap-handler-implementations

diff --git a/Cargo.toml b/Cargo.toml
index 6d3d517..6cb891b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -36,0 +37,3 @@ gitlab = { repository = "chrysn/coap-handler-implementations" }
+
+[patch.crates-io]
+coap-handler = { path = "../coap-handler/" }
diff --git a/src/wkc.rs b/src/wkc.rs
index eafe20b..4adc7c7 100644
--- a/src/wkc.rs
+++ b/src/wkc.rs
@@ -1,9 +1 @@
-//! Traits and tools to implement a .well-known/core resource easily
-//!
-//! This tries to be future-proof for building also CoRAL responses, without going out of its way
-//! for that.
-//!
-//! The [Reporting] trait is a bit of mis-fit for coap_handler_implementations, and should not be
-//! too tightly coupled with the remainign components. It is a candidate for moving in with
-//! coap-handler or a to-be created coap-discovery crate (when it and its components have matured),
-//! while NotReporting etc. would stay here.
+//! Tools to implement a .well-known/core resource easily
@@ -14,48 +6,2 @@ use coap_message::{MutableWritableMessage, ReadableMessage};
-/// A property an advertised resource can have many of.
-///
-/// This corresponds to target attributes in Link Format, and also to properties in CoRAL without
-/// being very final yet.
-///
-/// This is a single type with static string out-references, but likely to be generalized later
-/// into a trait (but right now it's insufficiently known what it'll need to produce).
-#[non_exhaustive]
-#[derive(Copy, Clone)]
-pub enum Attribute {
-    Observable,
-    Interface(&'static str),
-    ResourceType(&'static str),
-    Title(&'static str),
-    Ct(u16), // Single only -- array could be added in an own option
-    Sz(usize),
-}
-
-/// A entry produced by Reporting, corresponding to a single link in a Link Format file.
-pub trait Record {
-    type PathElement: AsRef<str>;
-    type PathElements: Iterator<Item = Self::PathElement>;
-    type Attributes: Iterator<Item = Attribute>;
-
-    /// List of path segments (equivalent to Uri-Path option values) leading to the indicated
-    /// resoruce
-    fn path(&self) -> Self::PathElements;
-
-    /// Link relation (or None to default to the implicit "hosts")
-    ///
-    /// Note that the allowed character set is limited compared to full UTF-8 strings.
-    fn rel(&self) -> Option<&str>;
-
-    /// Target attributes of the link
-    fn attributes(&self) -> Self::Attributes;
-}
-
-/// Indicates that this resource can produce output for a .well-known/core resource.
-pub trait Reporting {
-    type Record<'res>: Record
-    where
-        Self: 'res;
-    type Reporter<'res>: Iterator<Item = Self::Record<'res>>
-    where
-        Self: 'res;
-
-    fn report(&self) -> Self::Reporter<'_>;
-}
+#[deprecated(note = "Use these through coap_handler instead")]
+pub use coap_handler::{Attribute, Record, Reporting};
@@ -261,0 +208,3 @@ pub fn write_link_format(
+                // New attributes are ignored -- as a matter of best practice, when a new attribute
+                // is added, it should be covered here.
+                _ => (),

Diff on coap-handler

diff --git a/README.md b/README.md
index 23010f0..b2256c8 100644
diff --git a/src/lib.rs b/src/lib.rs
index 3de4eea..5dedb67 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -32,0 +33,4 @@
+//!
+//! The main item of this crate is the [Handler] trait. The [Reporting] trait can be implemented in
+//! addition to [Handler] to ease inclusion of the rendered resource in discovery, eg. through
+//! `/.well-known/core` as implemented in [coap-handler-implementations].
@@ -37,0 +42 @@ pub mod implementations;
+mod wkc;
@@ -39,0 +45 @@ pub use handler::Handler;
+pub use wkc::{Attribute, Record, Reporting};
diff --git a/src/wkc.rs b/src/wkc.rs
new file mode 100644
index 0000000..1dacbea
--- /dev/null
+++ b/src/wkc.rs
@@ -0,0 +1,53 @@
+//! Traits  to implement a .well-known/core resource easily
+//!
+//! This tries to be future-proof for building also CoRAL responses, without going out of its way
+//! for that.
+
+/// A property an advertised resource can have many of.
+///
+/// This corresponds to target attributes in Link Format, and also to properties in CoRAL without
+/// being very final yet.
+///
+/// This is a single type with static string out-references, but likely to be generalized later
+/// into a trait (but right now it's insufficiently known what it'll need to produce).
+#[non_exhaustive]
+#[derive(Copy, Clone)]
+pub enum Attribute {
+    Observable,
+    Interface(&'static str),
+    ResourceType(&'static str),
+    Title(&'static str),
+    Ct(u16), // Single only -- array could be added in an own option
+    Sz(usize),
+}
+
+/// A entry produced by Reporting, corresponding to a single link in a Link Format file.
+pub trait Record {
+    type PathElement: AsRef<str>;
+    type PathElements: Iterator<Item = Self::PathElement>;
+    type Attributes: Iterator<Item = Attribute>;
+
+    /// List of path segments (equivalent to Uri-Path option values) leading to the indicated
+    /// resoruce
+    fn path(&self) -> Self::PathElements;
+
+    /// Link relation (or None to default to the implicit "hosts")
+    ///
+    /// Note that the allowed character set is limited compared to full UTF-8 strings.
+    fn rel(&self) -> Option<&str>;
+
+    /// Target attributes of the link
+    fn attributes(&self) -> Self::Attributes;
+}
+
+/// Indicates that this resource can produce output for a .well-known/core resource.
+pub trait Reporting {
+    type Record<'res>: Record
+    where
+        Self: 'res;
+    type Reporter<'res>: Iterator<Item = Self::Record<'res>>
+    where
+        Self: 'res;
+
+    fn report(&self) -> Self::Reporter<'_>;
+}

Then running, in -implementations:

cargo semver-checks check-release

Actual Behaviour

    Updating index
     Parsing coap-handler-implementations v0.3.5 (current)
     Parsing coap-handler-implementations v0.3.5 (baseline)
    Checking coap-handler-implementations v0.3.5 -> v0.3.5 (no change)
   Completed [   0.108s] 40 checks; 38 passed, 2 failed, 0 unnecessary

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.17.1/src/queries/enum_missing.ron

Failed in:
  enum coap_handler_implementations::wkc::Attribute, previously in file /home/chrysn/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/coap-handler-implementations-0.3.5/src/wkc.rs:23

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.17.1/src/queries/trait_missing.ron

Failed in:
  trait coap_handler_implementations::wkc::Reporting, previously in file /home/chrysn/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/coap-handler-implementations-0.3.5/src/wkc.rs:52
  trait coap_handler_implementations::wkc::Record, previously in file /home/chrysn/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/coap-handler-implementations-0.3.5/src/wkc.rs:33
       Final [   0.110s] semver requires new major version: 2 major and 0 minor checks failed

Expected Behaviour

I'd expect either of two behaviors:

• Just pass

• Point out that by replacing a type (I'm not sure what the situation with a trait is) with a pub used one, there comes up the risk of a third party impl'ing something for both the types, creating a similar situation with why we can't unify ! with core::convert::Infallible.

  I'd argue that this should only be reported as a minor issue, because there are situations in which this is practically ruled out: Like in this case, releases happen simultaneously, so a user would need to have implemented something on the -implementations trait, update, -handlers but not -implementation, add a second impl, and then update -- that borders malice.

Either way: The item is not gone, but reported as gone.

Generated System Information

Software version

cargo-semver-checks 0.17.1

Operating system

Linux 6.0.0-3-amd64

Command-line

/home/chrysn/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.67.0 (8ecd4f20a 2023-01-10)

Compile time information

• Profile: release
• Target triple: x86_64-unknown-linux-gnu
• Family: unix
• OS: linux
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,sse,sse2
• Host: x86_64-unknown-linux-gnu

Build Configuration

No response

Additional Context

No response

[obi1kenobi]

Hi! Thanks for the report, and sorry for the inconvenience. This is definitely a bug and "just pass" is indeed the expected outcome.

The underlying cause is that somewhat recently, there was a change in the rustdoc JSON format where items defined in foreign crates are no longer inlined into the crate that re-exports them. As you can imagine, that means cargo-semver-checks really can't find the re-exported item because it really isn't present in the file presented to the tool.

Of course, that doesn't make this any less of a bug. We're working on a design that should fix it, but doing this right and without completely wrecking linting performance is a bit complicated so it's taking a bit of time. Apologies!

Hopefully this should be a one-off false-positive, in that once the next coap-handler-implementations version is released, the baseline will move forward to that version and the false-positive error will stop happening. Please update us on this issue if that doesn't end up being the case for any reason.

[chrysn]

Thanks a bunch for your quick response, this makes me much less anxious about continuing on this path :-)

[obi1kenobi]

Of course! Thanks for opening the thoughtfully-written issue, and please keep them coming if anything else is less than optimal. We're doing our best to wrangle all the edge cases that pop up :)

[thomaseizinger]

How difficult is this to implement? We don't allow merges with CI failures but from what I understand, the current workaround is to accept the false-positive and immediately do a release to "fix" things?

I am happy to have a go at implementing this as I'd rather not want to build local workarounds and libp2p/rust-libp2p#3350 is not pressing.

[obi1kenobi]

It's a difficult one, unfortunately.

As of somewhat recently, rustdoc no longer "inlines" foreign items that are relevant, like re-exported items, implemented foreign traits, etc. This was done for sensible reasons in rustdoc, but isn't something we originally planned for in our design. The assumption that there's only a single crate's rustdoc to be generated and loaded is pretty deeply embedded and won't be easy to change.

I'd say it's on the "week or two of focused work" kind of timescale. The queries are all fine and won't need to change, but we'll need to allow the Trustfall adapter implementation to build and load new rustdoc JSONs dynamically — ideally without replicating the rustdoc-building code once per adapter version. We'll need to special-case alloc/std/core rustdoc since it can't be generated by the usual rustdoc process and must be downloaded as a component — but it's only available for nightly so we'll need to figure out which nightly corresponds to which stable. And testing will be a bit tricky, too.

If you're still interested, I'd be happy to point you in the right direction. The work will be not just here but also in https://github.com/obi1kenobi/trustfall-rustdoc-adapter which holds the Trustfall adapters for the various rustdoc JSON format versions (one per branch).

[thomaseizinger]

Thanks for outlining the approach. I am afraid that is a bit too big in scope for me to tackle.

[obi1kenobi]

Item IDs (type Id) are not guaranteed to be stable between rustdoc JSON files or even invocations over the same project, and the paths portion of rustdoc is not a satisfactory replacement and is tentatively planned to be removed when a better solution is available.

I've discussed this issue with folks that work on rustdoc, and I believe they understand the severity of the problem and are interested in helping come up with a reasonable and more permanent solution.