[Bug] dependabot crossterm update broke semver?

[joshua-mo-143]

Describe the bug

Hey there! First of all, thanks for making this crate.

I just wanted to let you know that it looks like the most recent patch caused a semver break, which inadvertently caused the latest compile for cargo-shuttle (a CLI for a dev platform) to break. Currently because of the semver break, it's impossible to compile the package and users are currently required to use --locked for dependencies. Is there any chance the 6.2.0 release could be yanked?

Thank you in advance

Steps to reproduce

Run cargo install cargo-shuttle

Logs (if applicable)

error[E0308]: mismatched types
  --> /home/glitch/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/shuttle-common-0.17.0/src/models/error.rs:27:43
   |
27 |             self.message.to_string().with(Color::Red)
   |                                      ---- ^^^^^^^^^^ expected `crossterm::style::Color`, found `comfy_table::Color`   |                                      |
   |                                      arguments to this method are incorrect
   |
   = note: `comfy_table::Color` and `crossterm::style::Color` have similar names, but are actually distinct types
note: `comfy_table::Color` is defined in crate `crossterm`
  --> /home/glitch/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/crossterm-0.26.1/src/style/types/color.rs:28:1
   |
28 | pub enum Color {
   | ^^^^^^^^^^^^^^
note: `crossterm::style::Color` is defined in crate `crossterm`
  --> /home/glitch/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/crossterm-0.25.0/src/style/types/color.rs:28:1
   |
28 | pub enum Color {
   | ^^^^^^^^^^^^^^
   = note: perhaps two different versions of crate `crossterm` are being used?
note: method defined here
  --> /home/glitch/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/crossterm-0.25.0/src/style/stylize.rs:78:8
   |
78 |     fn with(self, color: Color) -> Self::Styled {

Operating system

Pop!_OS (not applicable)

Comfy-table version

6.2.0

Additional context

All context has already been provided - please let me know if you need any other information.

[joshua-mo-143]

Actually apologies, please ignore. It appears there's a version conflict that I didn't realise was there 🤦

[aumetra]

I still believe that the upgrade from crossterm v0.25 to v0.26 was technically a semver breaking change.
Exposing their types on the public API tied your versioning to that of crossterm. They released a semver incompatible release with 0.26, so your upgrade is semver-incompatible as well.

To reference prior art from other crates, the signature crate has any public 0.x crates gated behind features which are semver exempt (per documentation; more of an informal contract) to enable minor releases that bump these 0.x crates without having to release a new major version.

But then again, if nobody notices, was it really breaking?

[Nukesor]

That's actually a good question 😅
I usually check crossterm's changelog to see if anything related to the re-exported types is changing.

I guess one could argue that re-exporting types from a dependency would implicitly result in the need for breaking change releases when that dependency introduces breaking changes?
To be honest, I never thought about it that way...

One thing though, the different versions wont be a problem, as long as users use the re-exported types when interacting with comfy-table. This only becomes a problem, if one uses types from crossterm directly and puts them into comfy-table.

[Nukesor]

I'm torn.

On one hand, if one uses both (comfy_table and crossterm) in the same project, it's very convenient to use crossterms exports directly, as one would otherwise have to handle different "versions" of Color/Attribute in the same file.
If one uses only one of both, it really doesn't matter.

One the other hand, this is indeed some kind of semver breaking change, since the user is forced to update crossterm in the same go, which breaks the build, even though it should be a minor update.

Here're the possible solutions I see:

1. Don't re-export, but rather mirror crossterms enums and convert them internally. This forces the user to handle crossterms and comfy_table's types seperately and would allow multiple versions of crossterm to exist in parallel, by forbidding interaction between both crates.
   This is most definitely a breaking change and needs a v7.

2. Ignore this and make it a "wontfix". I don't like this, it's obviously something people stumble upon and it will happen in the future. But being able to use the same types in a project is also super convenient!

3. Only update crossterm if there's a major version upgrade. I'm torn about this one. There won't come any breaking changes in comfy_table itself in the near future. The project is basically "done". Introducing breakin changes just for the sake of dependency bumps feels a bit dirty.

4. This is somewhat a mix of 1. and 2.. Mirror the types by default, but add a feature flag to not use the mirrored but rather the exported crossterm types. That way, users could opt-in to the features/stability level they want and we could properly document this behavior.

[Nukesor]

After reading through my own proposal a few times, I feel like 1. or 4. is the way to go.

I would still like to get some feedback from users that actually stumbled upon this issue.

[Nukesor]

I implemented the 4. solution in the linked PR. What do you think?

[oddgrd]

Hey @Nukesor! Thanks for the crate, we've really enjoyed it! We encountered this issue over a Shuttle because we were mixing Color from our crossterm dep and your re-export. We've untangled the types now (shuttle-hq/shuttle#961) to resolve this and also avoid it in the future, but it is indeed a bit of a footgun.

I'll take a look at your proposed solution and PR when I get a moment (feel free to ping me if I forget and you still need feedback!), but I just wanted to comment now to let you know that we have resolved it for now. Thanks again!

[Nukesor]

@oddgrd
The fact that you effectively disentangled the types tells me that this is indeed the correct thing to do :)

It would be awesome, if you could check out, whether the version on that branch works for you without any problems and need to upgrade! I'll do the same on my own project and if everything looks good, I'll go ahead and publish a v7 release :)

[Nukesor]

Tested it in my own project and it works perfectly fine with the changes in that MR :)