Uuid 1.12.0 contains a downstream build breaking change

[weiznich]

Steps to reproduce:

git clone https://github.com/diesel-rs/diesel
cd diesel
# make sure to checkout an affected commit
git checkout 25cff1dcb6b24d144d2b39e8e03eb3d5582e9ea9
cargo xtask tidy --keep-going
# build fails here
cargo update -p uuid --precise=1.11.0
cargo xtask tidy --keep-going
# build is now successful

Example build that's broken: https://github.com/diesel-rs/diesel/actions/runs/12827455942/job/35769450341?pr=4434#step:8:120

(Fortunately that only breaks test code in diesel, so it's relatively easy for us to fix)

[KodrAus]

Without digging in properly I’m not entirely sure what the source of breakage is there. Does that FromSql trait piggyback off From<T>? We added some new From impls on Uuid in the latest release.

[weiznich]

The FromSql implementation doesn't use any From impl at all for uuid::Uuid. See here for the actual implementation:

https://github.com/diesel-rs/diesel/blob/fa871e70693b8e1b35d40b05dafe900dbcd62786/diesel/src/pg/types/uuid.rs#L16-L20

In fact it doesn't even have any generic bounds at all. See the documentation for a list of all possible FromSql impls, but I cannot see how any other impl than FromSql<diesel::sql_types::Uuid, Pg> could relevant here as the call in question is fully qualified to that types:

https://github.com/diesel-rs/diesel/blob/25cff1dcb6b24d144d2b39e8e03eb3d5582e9ea9/diesel/src/pg/types/uuid.rs#L59

Also note that I already merged a fix to the diesel master branch so you would need to checkout to reproduce the problem: diesel-rs/diesel@25cff1d

[KodrAus]

Thanks! I’ll have a play with it and see if I can figure out what caused it.

[weiznich]

I gave git bisect a try and that pointed to f570b57 as commit that introduced this. I'm honestly not sure how adding these new trait impls can cause type inference to fail there in diesel. Possibly because PartialEq<T> for uuid::Uuid is not unambiguous anymore? Otherwise this still feels like something rustc should be able to resolve, given that this is also rather well constrained from diesels side.

Anyway: We fixed that in diesel and I would not expect that this particular usage function call is something that someone outside of diesel does often, so I wouldn't expect any other breakage from this.

[obi1kenobi]

👋 Hi, maintainer of cargo-semver-checks here.

If you're up for it, I'd love your thoughts on what (if anything) you would have found useful for an automated tool to flag here.

I believe the new version is 1.12.0 not 0.12.0, so this was a minor release. Adding a new impl of an existing trait on an existing type is one of those "breaking but not SemVer-major" changes, because it breaks inference. So this release followed Rust SemVer norms, even though the breakage is regrettable.

It feels to me like some variant of "maintainers should know about it ahead of time, but shouldn't have to make a major bump over it" might be good. Perhaps a warning-level lint? What do you think?

Your feedback would inform how (if at all) we tackle this breaking change item on our lint wish list: obi1kenobi/cargo-semver-checks#5

[KodrAus]

I believe the possible inference breakage is going from:

impl Trait<B> for A {}

to:

impl<T> Trait<T> for A {}

where T may just be the closed set B and C from an additional impl Trait<C> for A {}, or by making the original impl generic with a bound that covers B.

It's difficult to tell when this happens because impl blocks can appear anywhere and writing impl blocks is such a fundamental activity.

If cargo-semver-checks were able to flag this then I would call them out in release notes, but I would never consider this a breaking library change requiring a major bump myself.

[obi1kenobi]

Makes sense! Thank you!

I think it makes sense to add warning-level lints about adding trait impls that may break inference. We'll make them produce a warning without requiring a major bump.