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

deny(warning) is not an anti-pattern for libs #260

Open
roblabla opened this issue Jun 13, 2021 · 4 comments
Open

deny(warning) is not an anti-pattern for libs #260

roblabla opened this issue Jun 13, 2021 · 4 comments
Labels
A-anti_pattern Area: Content about Anti-Patterns C-amendment Category: Amendments to existing content C-needs discussion Area: Something that is not clear to everyone if it fixes something/adds valuable content C-outdated Category: Content that is outdated and needs a rework good first issue Good for newcomers
Projects

Comments

@roblabla
Copy link

roblabla commented Jun 13, 2021

The main reason why deny(warning) is considered an antipattern, according to the patterns guide, is that a crate author opts out of Rust's stability guarantees, since Rust is allowed to add new warnings to new versions. Thus, new versions of rust may break the crate.

However, rust currently caps the the max lint level to "warn" when building dependencies. Thus, even if a crate specifies #![deny(warning)], the warnings will not be upgraded to denies when it is built as a dependency. See this comment:

I'm actually not sure if that antipattern still applies given that cap-lints exists. The patterns book is old and partially maintained, and cap-lints was not always a thing.

Again, if a crate is a dependency there is no way to cause it to fail to compile by tweaking lint levels.

It's not a huge deal if a crate fails to compile under deny(warnings), if you're building the crate itself you probably are developing on it (not using it as a dep). Except perhaps for binary crates.

As such, deny(warning) is actually an entirely fine pattern for libraries to use. It may be worth amending the deny(warning) antipattern crate to explain that it only applies to binary crates.

CC #46

@roblabla roblabla changed the title deny(warning) is not an anti-pattern deny(warning) is not an anti-pattern for libs Jun 13, 2021
@est31
Copy link

est31 commented Jun 13, 2021

Wasn't there a problem that it possibly makes your library fail to build on crater?

@roblabla
Copy link
Author

I feel crater should probably use rustc's cap-lint too if it doesn't already.

@Nemo157
Copy link

Nemo157 commented Jun 14, 2021

Even if the reasoning in the book is wrong, #![deny(warnings)] is still an anti-pattern that makes contributors lives harder for no reason. It is bad enough and widespread enough that I have had to add --cap-lints warn to my global RUSTFLAGS to workaround the issue (which is itself bad because it downgrades all the deny-by-default warnings to just warn, but that is a lesser evil).

https://reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/ has some discussion, and mentions that crater does use cap-lints for the beta runs, but this runs into the same issue I mention that it also disables the deny-by-default lints. Given the forward compat lifecycle found through a deny stage it seems very unfortunate if crater would not pick up forward compat issues.

@MarcoIeni
Copy link
Collaborator

MarcoIeni commented Jun 17, 2021

So from my understanding we could:

  • fix the not correct statement in the rust book.
  • say that this makes developer life more difficult.

Am I right?

@simonsan simonsan added C-needs discussion Area: Something that is not clear to everyone if it fixes something/adds valuable content C-outdated Category: Content that is outdated and needs a rework good first issue Good for newcomers A-anti_pattern Area: Content about Anti-Patterns C-amendment Category: Amendments to existing content labels Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-anti_pattern Area: Content about Anti-Patterns C-amendment Category: Amendments to existing content C-needs discussion Area: Something that is not clear to everyone if it fixes something/adds valuable content C-outdated Category: Content that is outdated and needs a rework good first issue Good for newcomers
Projects
Content
Awaiting triage
Development

No branches or pull requests

5 participants