Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint: Changing the type of a pub struct's pub field #148

Open
passcod opened this issue Oct 15, 2022 · 6 comments
Open

Lint: Changing the type of a pub struct's pub field #148

passcod opened this issue Oct 15, 2022 · 6 comments
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations

Comments

@passcod
Copy link

passcod commented Oct 15, 2022

Steps to reproduce the bug with the above code

Actual Behaviour

All tests pass

Expected Behaviour

I've deliberately broken the public API in that branch. On this public type:

 pub struct BinFile {
-    pub base_name: CompactString,
+    pub base_name: String,
     pub source: PathBuf,
     pub dest: PathBuf,
     pub link: Option<PathBuf>,
 }

This should fail a semver check, obviously. (I tried asking cargo-semverver to bring me confidence but it didn't work, so I recruited the good people of Discord instead:)

image

Additional Context

Background: I'd added cargo-semver-checks as an optional step in cargo-bins/release-pr after reading this article.

Debug Output

Software version

cargo-semver-checks 0.12.0

Operating system

Linux 5.19.7-arch1-1

Command-line

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

cargo nightly version

> cargo +nightly -V 
cargo 1.66.0-nightly (b8f30cb23 2022-10-10)

Compile time information

  • Profile: release
  • Target triple: x86_64-unknown-linux-gnu
  • Family: unix
  • OS: linux
  • Architecture: x86_64
  • Pointer width: 64
  • Endian: little
  • CPU features: adx,aes,avx,avx2,bmi1,bmi2,fma,fxsr,lzcnt,pclmulqdq,popcnt,rdrand,rdseed,sse,sse2,sse3,sse4.1,sse4.2,ssse3,xsave,xsavec,xsaveopt,xsaves
  • Host: x86_64-unknown-linux-gnu
@passcod passcod added the C-bug Category: doesn't meet expectations label Oct 15, 2022
@obi1kenobi
Copy link
Owner

Hi! Thanks for checking out cargo-semver-checks and for opening this issue.

I'm sorry our inadequate README FAQ section didn't cover this situation better. If it did, it could have saved you some confusion and debugging time. That's my bad.

Unfortunately, this is a case of a missing lint rule. There are many ways to break semver, and cargo-semver-checks doesn't have lints for all of them yet — including this one. There's a long list of not-yet-implemented lints in #5 that we are working our way through, and we encourage contributing new lints and are happy to offer mentorship as well.

I just merged a PR that adds more information to the FAQ about this: https://github.com/obi1kenobi/cargo-semver-check#does-cargo-semver-checks-have-false-positives-or-false-negatives

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations and removed C-bug Category: doesn't meet expectations labels Oct 15, 2022
@obi1kenobi obi1kenobi changed the title Breaking the API (by changing a type) passes semver-checks Lint: Changing the type of a pub struct's pub field Oct 15, 2022
@obi1kenobi
Copy link
Owner

Currently blocked on #149.

@XAMPPRocky
Copy link

FWIW I'm using a release-plz which uses cargo-semver-checks for version bumps, and this missing lint is the most often occurring bug we have with using cargo-semver-checks for octocrab

@obi1kenobi
Copy link
Owner

As painful as I know this issue is, unfortunately I suspect I won't manage to get to it in the rest of this year. I have an approach in mind that should work, but it's a very substantial amount of work that I think is unlikely to fit in my available time or take priority over other cargo-semver-checks needs, like #120.

Of course, that's all barring some kind of major change. For example, like large corporate donor(s) setting up a substantial monthly GitHub Sponsors contribution comparable to my consulting rates, which would let me reallocate my time away from consulting and toward more cargo-semver-checks work.

This is probably not what you wanted to hear, and I am sorry about that. I hope sharing more detail, instead of just keeping quiet, is not something anyone in the community would see as a negative.

@XAMPPRocky
Copy link

This is probably not what you wanted to hear, and I am sorry about that.

Oh not at all, as another open source maintainer I very much understand the time being a limited resource. I just wanted to report back on our experience so that you have some data to make decisions when prioritising work.

I hope sharing more detail, instead of just keeping quiet, is not something anyone in the community would see as a negative.

I can only speak for myself, but I definitely prefer a "no not this year" to silence, communication is always better, even when it's communicating limitations and not just features.

FWIW I would help contribute this lint myself if not for #149 that task seems large in design and requires a lot of project knowledge I don't have to be able to contribute.

@obi1kenobi
Copy link
Owner

Thank you for the kind words, much appreciated!

I agree #149 is not the best issue for a community contribution, unless someone is willing to put in enough work to effectively become a co-maintainer (which I'd be very open to!). The full long-term-stable solution to #149 would end up touching practically ever part of cargo-semver-checks, the underlying Trustfall adapter, and likely even require a moderately-sized feature addition to Trustfall itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

3 participants