semver-checks check-release detects non-existant changes in core-graphics crate

[jrmuizel]

Steps to reproduce the bug with the above code

Check out /~https://github.com/servo/core-foundation-rs and run cargo semver-checks check-release --verbose in the core-graphics directory.

Actual Behaviour

    Updating index
     Parsing core-graphics v0.22.3 (current)
warning: use of deprecated type alias `libc::int32_t`: Use i32 instead.
  --> core-graphics-types/src/base.rs:29:26
   |
29 | pub type CGError = libc::int32_t;
   |                          ^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: `core-graphics-types` (lib) generated 1 warning
 Documenting core-graphics v0.22.3 (/Users/jrmuizel/src/core-foundation-rs/core-graphics)
    Finished dev [unoptimized + debuginfo] target(s) in 0.66s
     Parsing core-graphics v0.22.3 (baseline)
    Updating crates.io index
 Documenting core-graphics v0.22.3
    Finished dev [unoptimized + debuginfo] target(s) in 0.93s
    Checking core-graphics v0.22.3 -> v0.22.3 (no change)
    Starting 22 checks, 0 unnecessary
        PASS [   0.167s]       major        auto_trait_impl_removed
        PASS [   0.049s]       major        derive_trait_impl_removed
        PASS [   0.001s]       major        enum_marked_non_exhaustive
        PASS [   0.020s]       major        enum_missing
        PASS [   0.008s]       major        enum_repr_c_removed
        PASS [   0.004s]       major        enum_repr_int_changed
        PASS [   0.004s]       major        enum_repr_int_removed
        PASS [   0.001s]       major        enum_struct_variant_field_missing
        PASS [   0.063s]       major        enum_variant_added
        PASS [   0.064s]       major        enum_variant_missing
        PASS [   0.048s]       major        function_missing
        PASS [   0.105s]       major        function_parameter_count_changed
        PASS [   0.156s]       major        inherent_method_missing
        FAIL [   0.004s]       major        method_parameter_count_changed
        PASS [   0.048s]       major        sized_impl_removed
        PASS [   0.001s]       major        struct_marked_non_exhaustive
        PASS [   0.038s]       major        struct_missing
        PASS [   0.004s]       major        struct_pub_field_missing
        PASS [   0.004s]       major        struct_repr_c_removed
        PASS [   0.001s]       major        struct_repr_transparent_removed
        PASS [   0.002s]       major        unit_struct_changed_kind
        PASS [   0.063s]       major        variant_marked_non_exhaustive
   Completed [   0.853s] 22 checks; 21 passed, 1 failed, 0 unnecessary

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: /~https://github.com/obi1kenobi/cargo-semver-check/tree/v0.13.0/src/queries/method_parameter_count_changed.ron

Failed in:
  core_graphics::path::CGPath now takes 1 parameters instead of 0, in :
  core_graphics::font::CGFont now takes 1 parameters instead of 0, in :
  core_graphics::event_source::CGEventSource now takes 1 parameters instead of 0, in :
  core_graphics::image::CGImage now takes 1 parameters instead of 0, in :
  core_graphics::context::CGContext now takes 1 parameters instead of 0, in :
  core_graphics::event::CGEvent now takes 1 parameters instead of 0, in :
  core_graphics::data_provider::CGDataProvider now takes 1 parameters instead of 0, in :
  core_graphics::color_space::CGColorSpace now takes 1 parameters instead of 0, in :
       Final [   1.330s] semver requires new major version: 1 major and 0 minor checks failed
       

Expected Behaviour

No changes should be detected. I think this might have something to do with the foreign_type! macro.

Generated System Information

Software version

cargo-semver-checks 0.13.0

Operating system

Mac OS X 10.15.6 (Darwin 19.6.0)

Command-line

/Users/jrmuizel/.cargo/bin/cargo-semver-checks semver-checks --bugreport

cargo version

> cargo -V
cargo 1.64.0 (387270bc7 2022-09-16)

Compile time information

• Profile: release
• Target triple: x86_64-apple-darwin
• Family: unix
• OS: macos
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,sse,sse2,sse3,ssse3
• Host: x86_64-apple-darwin

Additional Context

No response

[obi1kenobi]

Since I don't have a macOS machine, would you mind also submitting the two generated rustdoc JSON files from your build target directory? I don't believe the project builds on non-macOS, so I am not able to run the cargo semver-checks command and reproduce the problem.

[obi1kenobi]

Also, could you let me know what foreign-types crate version you have currently in your lockfile, on the off chance it's a similar sort of issue to #167?

[jrmuizel]

foreign-types is 0.3.2

[jrmuizel]

core-graphics.zip

[obi1kenobi]

Still looking into this, but so far I can confirm there is at least one bug here: the check seems to be correctly matching on the method but the error message is incorrectly naming the type whose method changed arity, instead of the method itself.

It's entirely possible there are more bugs here. I'll keep you posted.

Thank you for the report and for your patience as I debug this.

[obi1kenobi]

Second bug: I confirmed this is a false-positive report due to a logic error in the lint query.

As currently written, it assumes that methods by name are unique on each ImplOwner (the struct or enum that owns the impl block). This isn't actually true, and in this case there's an inherent type_id() method + the Any trait's blanket impl (for all T) which also has a type_id() method. The lint reports a false-positive because it compares the inherent type_id() to the <Self as Any>::type_id() method, which don't have the same signature.

To resolve, we'll need to reframe the check to look for "no method on the same ImplOwner with the same name takes the correct number of arguments" rather than the current logic which looks for "any method on the ImplOwner with the same name that doesn't take the correct number of arguments."

[obi1kenobi]

Fix published in v0.13.1. The current commit of core-graphics is now part of our integration testing suite (#198) to ensure there are no future false-positive regressions.

Thanks again for the report!

[jrmuizel]

Thanks!