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

anti pattern: #![deny(warnings)] #46

Closed
llogiq opened this issue Dec 21, 2016 · 5 comments · Fixed by #47
Closed

anti pattern: #![deny(warnings)] #46

llogiq opened this issue Dec 21, 2016 · 5 comments · Fixed by #47

Comments

@llogiq
Copy link
Contributor

llogiq commented Dec 21, 2016

Through my work with clippy, I've probably had more exposure to the negative consequences than others, so I'm going to write up something in the next few days.

@llogiq llogiq mentioned this issue Dec 21, 2016
@nrc nrc closed this as completed in #47 Jan 4, 2017
@sanmai-NL
Copy link

@llogiq: Hear me Rust bard,

I can't help but be very criticial of this guideline. Let me explain why.

By disallowing the compiler to build with warnings, a crate author opts out of Rust's famed stability.

Rust's stability is rather mythical at this point. Only the syntax and standard library API are stable, and only on a best effort basis. There have been numerous breaking changes. That can be good, as Rust is still a young project. In the future, the Rust epochs system will be the principled solution to managing backward compatibility. At present, if there is any reason to not forbid all warnings during a build given #![deny(warnings)] in the source code, then this directive can be overridden with the relevant command line parameters of rustc.

Sometimes new features or old misfeatures need a change in how things are done, thus lints are written that warn for a certain grace period before being turned to deny.

This grace period is a courtesy from those responsible for the change to the affected. It is not an obligation of the affected themselves to adhere to that grace period. With this guideline, you imply that those affected by breaking changes to lint checking should not enforce the changed lint checking logic. But they normally want to. There's a reason they started out with #![deny(warnings)].

The proper way to prevent broken builds due to dependencies with strict linting is to apply rustc --cap-lints to dependencies.

Furthermore, crates that supply additional lints (e.g. rust-clippy) can no longer be used unless the annotation is removed.

That should be elaborated.

Moreover, the downsides of the alternative solutions need to be pointed out as well. Changing command line parameters to opt in to strict lint checking doesn't really solve the problem of forward compatibility. After all, when and where should these command line parameters be set then? If during CI, then the build will break still after a breaking change to lint checks. Explicitly listing numerous lints in the source code is not scalable across many crates. Furthermore, it results in another kind of forward incompatibility risk, other than breakage: risk of not keeping current with new lints. Lints are added for a reason, they address real world quality issues. Grace periods are only a compromise in the interest of keeping things going without maintenance, but it is certainly preferable to aggressively perform all lint checks available and address the results head-on.

@sanmai-NL
Copy link

sanmai-NL commented Nov 17, 2017

@llogiq / @nrc: Can you reopen, to give others more opportunity to join the discussion?

@epage
Copy link

epage commented Jan 30, 2018

In my rust2018 post, I propose using Epochs on warnings groups. I'm going to be posting on the epoch issue to find out the right venue for discussion.

@ghost
Copy link

ghost commented Jun 2, 2019

Furthermore, crates that supply additional lints (e.g. rust-clippy) can no longer be used unless the annotation is removed.

from: https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deny-warnings.md#drawbacks

Is that still true? it doesn't seem to be, or I don't know what they meant, but I've only tested this example:

#![deny(clippy::pedantic, clippy::all, clippy::correctness, clippy::nursery)]
//^ no effect on: unreachable_patterns

#![deny(warnings)]
//^ this works!

fn main() {
    let num = Some(4);

    match num {
        Some(x) if x < 5 => println!("less than five: {}", x),
        Some(x) => println!("{}", x),
        None => (),
    }

    match num {
        Some(x) => println!("{}", x),
        Some(x) if x < 5 => println!("less than five: {}", x), // warning: unreachable pattern
        None => (),
    }                                                                                                                         
}

@Artoria2e5
Copy link

Artoria2e5 commented Apr 5, 2021

I think the "additional lints" refer to the possibility of the new lints breaking compilation.


WRT whether it is an anti-pattern… Being an ex-packager and seeing all the packages broken due to a well-intentioned -Wall -Werror has lead me to firmly see anything similar as actively harmful. Some compilation flags are only useful for development and have no place in your user's build instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants