obi1kenobi / cargo-semver-checks

Scan your Rust crate for semver violations.
Apache License 2.0
1.17k stars 75 forks source link

Changed receiver type and return lifetime #961

Open cuviper opened 3 weeks ago

cuviper commented 3 weeks ago

Is this about an existing lint, or proposing a new one?

new

Known issues that might be causing this

Steps to reproduce the bug with the above code

I was curious if semver-checks could detect this intentional breaking change: https://github.com/indexmap-rs/indexmap/pull/304

$ git clone https://github.com/indexmap-rs/indexmap
$ cd indexmap
$ git checkout 244bd81
$ cargo semver-checks --baseline-rev @^

Actual Behaviour

     Parsing indexmap v2.2.1 (current)
      Parsed [   2.700s] (current)
     Parsing indexmap v2.2.0 (baseline)
      Parsed [   2.695s] (baseline)
    Checking indexmap v2.2.0 -> v2.2.1 (patch change)
     Checked [   0.027s] 89 checks: 89 pass, 0 skip
     Summary no semver update required

Expected Behaviour

I hoped it would detect the same change that cargo public-api diff finds:

$ cargo public-api diff @^..@
[...]
Changed items in the public API
===============================
-pub fn indexmap::map::raw_entry_v1::RawOccupiedEntryMut<'a, K, V, S>::into_key(&mut self) -> &mut K
+pub fn indexmap::map::raw_entry_v1::RawOccupiedEntryMut<'a, K, V, S>::into_key(self) -> &'a mut K

This could probably be two different lints, one for the changed receiver and one for the return lifetime.

Generated System Information

Software version

cargo-semver-checks 0.35.0

Operating system

Linux 6.10.10-200.fc40.x86_64

Command-line

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

cargo version

> cargo -V
cargo 1.81.0 (2dbb1af80 2024-08-20)

Compile time information

Build Configuration

No response

Additional Context

No response

obi1kenobi commented 3 weeks ago

Thanks! Agreed, probably two desirable lints here as you said: receiver changed + return lifetime change.

It's possible we might be able to catch these before we implement all of #149.

For the "receiver changed" lint, I'd like to make sure our approach is robust to the possible future changes from the "arbitrary receiver types" RFC. I'll need to think about the edge cases there.

cuviper commented 2 weeks ago

FWIW, I remembered there was also an intentional receiver change way back in https://github.com/rust-lang/rust/pull/39466, between Rust 1.15.0 and 1.15.1. Maybe these examples aren't particularly motivating since they were on purpose though. :)

obi1kenobi commented 2 weeks ago

They are very useful actually: they've given me ideas on how to break up the "type changed in incompatible fashion" problem space into useful and easier-to-implement components.

It's always good to see how breaking changes happen in the world, intentional or not.

On Wed, Oct 9, 2024, 2:23 AM Josh Stone @.***> wrote:

FWIW, I remembered there was also an intentional receiver change way back in rust-lang/rust#39466 https://github.com/rust-lang/rust/pull/39466, between Rust 1.15.0 and 1.15.1. Maybe these examples aren't particularly motivating since they were on purpose though. :)

— Reply to this email directly, view it on GitHub https://github.com/obi1kenobi/cargo-semver-checks/issues/961#issuecomment-2401034688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5MSV52AMS7CT5MQDTSRTZ2RZO7AVCNFSM6AAAAABPGVQ73CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRGAZTINRYHA . You are receiving this because you commented.Message ID: @.***>