fix(dcutr): rename error types to bypass cargo-semver-checks hickup

[thomaseizinger]

Description

cargo semver-checks is still missing features in regards to properly detecting renamed exports. To make our CI pass again, we remove the renamed export, replace it with type-aliases and deprecate them to point users types exported under a module which now follows the conventions set in #2217.

Notes

Links to any relevant issues

Open Questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Lol, I was about to say "damn it doesn't work" until I released that the failure this time is actually correct and I did break the API with that change. Oh the irony 😄

[thomaseizinger]

Unfortunately, it actually doesn't work. I am guessing because of the same issue, cargo semver-checks now complains that I am removing a type even though that type was never exposed (under that name).

Any more ideas @obi1kenobi? Otherwise, I'd suggest that we merge #3214 but without the rename of the inner type but instead deprecate it for the ones scoped under the new inbound and outbound modules. Thoughts @mxinden?

[obi1kenobi]

The false-positive will go away as soon as you merge this PR, and there are no semver violation here as far as I can tell so it should be safe to ignore cargo-semver-checks in this specific instance. I recommend merging this PR. I am agnostic as to whether you also use this opportunity to rename the types as in #3214, since I don't think that will have a bearing on cargo-semver-checks assuming you choose to merge this PR as well.

The semver-checking in CI for this PR is using the 0.8.0 version as a baseline. Because of the renaming re-export, that causes an incorrect view of what types are available under which names, and false-positives happen. But as soon as 0.8.1 gets published, that version will then become the baseline for future runs, and the false-positive will go away.

Here's how I know: this false-positive is such that testing a version against itself reports semver violations (which are obviously impossible). Testing v0.8.0 against itself reports false-positive violations, but testing v0.8.1 (this PR) against itself finds none (correctly so).

Here's v0.8.0 being tested against itself. This is using the exact same rustdoc JSON file being used as both the baseline and the current version, as you can see in the flags below:

$ cargo semver-checks check-release --manifest-path ./protocols/dcutr/Cargo.toml --baseline-rustdoc ./target/semver-checks/registry-libp2p_dcutr-0_8_0/target/semver-checks/target/doc/libp2p_dcutr.json --current-rustdoc ./target/semver-checks/registry-libp2p_dcutr-0_8_0/target/semver-checks/target/doc/libp2p_dcutr.json
    Checking <unknown> v0.8.0 -> v0.8.0 (no change)
   Completed [   0.010s] 22 checks; 20 passed, 2 failed, 0 unnecessary

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.14.0/src/queries/enum_variant_added.ron

Failed in:
  variant UpgradeError:MissingStatusField in /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:124
  variant UpgradeError:MissingReservationField in /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:126
  variant UpgradeError:InvalidReservationExpiration in /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:130
  variant UpgradeError:ParseStatusField in /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:140

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It 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.14.0/src/queries/enum_variant_missing.ron

Failed in:
  variant UpgradeError::MissingStatusField, previously in file /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:124
  variant UpgradeError::MissingReservationField, previously in file /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:126
  variant UpgradeError::InvalidReservationExpiration, previously in file /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:130
  variant UpgradeError::ParseStatusField, previously in file /.../.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:140
       Final [   0.013s] semver requires new major version: 2 major and 0 minor checks failed

And here's v0.8.1 (this PR) being tested against itself in the same way:

$ cargo semver-checks check-release --manifest-path ./protocols/dcutr/Cargo.toml --baseline-rustdoc ./target/semver-checks/target/doc/libp2p_dcutr.json 
     Parsing libp2p-dcutr v0.8.1 (current)
    Checking libp2p-dcutr v0.8.1 -> v0.8.1 (no change)
   Completed [   0.008s] 22 checks; 22 passed, 0 unnecessary

Both of these commands were executed from the root of the repo after a single cargo semver-checks check-release --manifest-path ./protocols/dcutr/Cargo.toml command with no other arguments, which generated the rustdoc files in the target directory that I then specified explicitly as above. You should be able to run the same commands on your machine and verify my result.

[mergify]

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

[thomaseizinger]

Thanks for the detailed explanation @obi1kenobi ! That is what I thought would happen, great to have it confirmed!

@mxinden Can you prioritise this so that our CI is green again? :)

[thomaseizinger]

I think we should get away with not renaming the inner type which would make this PR smaller. I'll do that tomorrow.

If you happen to have time for a release before that @mxinden , I'd appreciate it if you could include that :)

(Just on mobile atm so can't push code.)

[obi1kenobi]

I think we should get away with not renaming the inner type which would make this PR smaller.

Not sure I understood this part. For cargo-semver-checks, its confusion is around what the type is named when it's declared as pub enum ..., and it is ignorant of type aliases. So if the inner type you had in mind was the pub enum ... name, that one probably needs to remain renamed.

In any case, you should be able to locally test any changes with cargo-semver-checks as I described above to see if they would resolve the false-positive after being merged and released. I'd recommend doing that with any changes, just to be safe.

[thomaseizinger]

It seems to work :)

Both of these commands are executed on the current branch!

❯ cargo semver-checks check-release --package libp2p-dcutr
    Updating index
     Parsing libp2p-dcutr v0.8.1 (current)
     Parsing libp2p-dcutr v0.8.0 (baseline)
    Checking libp2p-dcutr v0.8.0 -> v0.8.1 (minor change)
   Completed [   0.073s] 22 checks; 21 passed, 1 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.13.1/src/queries/enum_missing.ron

Failed in:
  enum libp2p_dcutr::UpgradeError, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/inbound.rs:120
  enum libp2p_dcutr::UpgradeError, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:118
       Final [   0.075s] semver requires new major version: 1 major and 0 minor checks failed

❯ cargo semver-checks check-release --manifest-path ./protocols/dcutr/Cargo.toml --baseline-rustdoc ./target/semver-checks/target/doc/libp2p_dcutr.json
     Parsing libp2p-dcutr v0.8.1 (current)
    Checking libp2p-dcutr v0.8.1 -> v0.8.1 (no change)
   Completed [   0.070s] 22 checks; 22 passed, 0 unnecessary

[thomaseizinger]

Okay, I think I have arrived at the absolute minimal fix for this problem!

I think this is what we should merge and release. I'll deal with the deprecations separately in #3214. @mxinden I also removed the bump of the lib in misc/metrics and libp2p because none of these libraries need the new version. I think releasing a new patch version should be good enough. Nobody necessarily needs to update to that. We just need to release it so we have a green CI again :)

[thomaseizinger]

@obi1kenobi I've ran the commands you used as well and the latter one now passes without problems! Can you maybe double check that I actually did this correctly?

[mxinden]

Thanks for looking so deep into this @thomaseizinger and @obi1kenobi.

We need to release a new breaking version of libp2p-dcutr anyways due to #3170. I missed to properly bump the libp2p-dcutr version in #3170. This is now done in #3225. #3225 is all green.

Any objections simply proceeding with #3225? What would be the benefit of this pull request and the additional patch release of libp2p-dcutr?

[thomaseizinger]

We need to get rid of the export rename, otherwise we have the same problem on the next release!

If you are already bumping it there, then we don't need to release it but either way, this PR needs to merge :)

[obi1kenobi]

The cargo-semver-checks invocation is correct and I've replicated its result locally. It's good to go.

Right now I think cargo-semver-checks completely ignores type aliases, which means I'm not 100% how this resolves the false-positives, and since the underlying types aren't renamed, there's potential that similar false-positives could happen in the future if another pub use for those types is added. At this point it's just a race between that possibly happening and me actually fixing the underlying issues here.

I think this is fine to merge, even though renaming the underlying types might have been just a touch safer.

[thomaseizinger]

Right now I think cargo-semver-checks completely ignores type aliases, which means I'm not 100% how this resolves the false-positives, and since the underlying types aren't renamed, there's potential that similar false-positives could happen in the future if another pub use for those types is added. At this point it's just a race between that possibly happening and me actually fixing the underlying issues here.

I think this is fine to merge, even though renaming the underlying types might have been just a touch safer.

I am planning to land #3214 shortly after, meaning the type aliases are only a short-term fix in any case and not planned to be part of the public API for long. It is good to know that they are ignored for now, will keep it in mind!

[mergify]

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

[mxinden]

Needs updates to changelog and version. Otherwise good to go from my end.

[thomaseizinger]

Damn, I forgot to update the pull-request description. It is outdated :(