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

upper_case_acronyms should not suggest breaking changes to APIs #6974

Closed
sunshowers opened this issue Mar 25, 2021 · 6 comments
Closed

upper_case_acronyms should not suggest breaking changes to APIs #6974

sunshowers opened this issue Mar 25, 2021 · 6 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied P-high Priority: High

Comments

@sunshowers
Copy link

sunshowers commented Mar 25, 2021

Hi there! Thanks for writing clippy and all the useful suggestions it provides :)

Unfortunately, there's one lint that I've not been happy about: this new upper_case_acronyms lint that's on by default in Rust 1.51. I'm here to complain about two separate issues:
(1) I have aesthetic issues with the suggestions it makes.
(2) The more fundamental issue -- it routinely suggests breaking changes to stable, public APIs.

For example, the code here:

https://github.com/withoutboats/camino/blob/8f8d61dc91051b8a2ed013eb2f1fc34c03403bdb/src/lib.rs#L1595-L1629

has enums that are correctly and appropriately named, and match the corresponding names in std. However, the lint complains with:

image

Issue 1: the changes it suggests are aesthetically bad. UNC in particular is widely understood in the Windows world, and renaming it to Unc looks ugly and pattern-matches in my head to "Uncle".

Issue 2: Even if you disagree about the aesthetics, the lint recommends breaking changes to a public API. A lint like this couldn't possibly be high-value enough to suggest a breaking change to an API.

Please disable this lint by default, or at least for public APIs.

Meta

  • cargo clippy -V: clippy 0.1.51 (2fd73fa 2021-03-23)
  • rustc -Vv:
rustc 1.51.0 (2fd73fabe 2021-03-23)
binary: rustc
commit-hash: 2fd73fabe469357a12c2c974c140f67e7cdd76d0
commit-date: 2021-03-23
host: x86_64-unknown-linux-gnu
release: 1.51.0
LLVM version: 11.0.1
@sunshowers sunshowers added the C-bug Category: Clippy is not doing the correct thing label Mar 25, 2021
@giraffate
Copy link
Contributor

Thanks for your report!

This seems to be fixed by #6805.

@sunshowers
Copy link
Author

Thanks! Looks like that's not in the version of clippy released with Rust 1.51.0. Are there plans to do a patch release with this fix?

@giraffate
Copy link
Contributor

Hmm, looking at the PR and issues, it doesn't seem to be discussed so far.

@sunshowers
Copy link
Author

I really would suggest considering a patch release.

@matthiaskrgr
Copy link
Member

Thanks for bringing this to our attention!

pub enum Wasd {
    SomeNAME,
    ANotherNAME,
}

This ONLY lints on stable right now (beta is already fixed, yay)

There are other things already on the point-release pile, maybe we can just sneak this one in...? 😅
I'm not sure what the release team thinks about this....
clippy topic thread
https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/upper_case_acronyms.20FP.20for.20pub.20items.20on.20stable

point release thread
https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/1.2E51.2E1.20point.20release

@matthiaskrgr matthiaskrgr added I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied P-high Priority: High labels Mar 26, 2021
fenollp added a commit to fenollp/libremarkable that referenced this issue Mar 27, 2021
Signed-off-by: Pierre Fenoll <pierrefenoll@gmail.com>
brainstorm added a commit to umccr/htsget-rs that referenced this issue Apr 4, 2021
@flip1995
Copy link
Member

flip1995 commented May 4, 2021

Since Thursday is the new release and this is fixed in the current beta, I'll close this. Obviously there won't be a point release until Thursday.

@flip1995 flip1995 closed this as completed May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied P-high Priority: High
Projects
None yet
Development

No branches or pull requests

4 participants