[DISCUSS] Method of #![deny(current_warnings)]

[CAD97]

I find this tower annoying:

example-warn/src/lib.rs

Lines 1 to 36 in 5cbaf77

	#![deny(const_err,
	dead_code,
	illegal_floating_point_literal_pattern,
	improper_ctypes,
	non_camel_case_types,
	non_shorthand_field_patterns,
	non_snake_case,
	non_upper_case_globals,
	no_mangle_generic_items,
	overflowing_literals,
	path_statements,
	patterns_in_fns_without_body,
	plugin_as_library,
	private_in_public,
	private_no_mangle_fns,
	private_no_mangle_statics,
	renamed_and_removed_lints,
	stable_features,
	unconditional_recursion,
	unions_with_drop_fields,
	unknown_lints,
	unreachable_code,
	unreachable_patterns,
	unused_allocation,
	unused_assignments,
	unused_attributes,
	unused_comparisons,
	unused_features,
	unused_imports,
	unused_macros,
	unused_must_use,
	unused_mut,
	unused_parens,
	unused_unsafe,
	unused_variables,
	while_true)]

Reasoning:

• 36 (and more in the future) lines of code in the root before the interesting part
• This prevents local compilation from continuing after a warning
  • Depending on the warning, it can be detected before real errors and prevent them from showing
  • Often times when iterating I care more about correctness than eliminating warnings right now
  • It's beneficial to the compiler UI to have a visual separation between warnings and errors

I fully agree that warnings should be treated as errors on CI. So how do we get cargo build to treat warnings as errors? By the RUSTFLAGS environment variable!

With RUSTFLAGS=-D warnings you can deny the warning group warnings. (For the same reason that we don't use !#[deny(warnings)], this is dangerous. Though we're still technically not free from warnings introduced by new versions of the compiler; deprecation of symbols can introduce deprecation warnings. (As such that one should probably be excluded from our exclusion list.)) With RUSTFLAGS=-D const_err -D dead_code ... you can specify warning (groups) individually.

[epage]

I personally exclusively develop with warnings on :)

An alternative method for allowing both is a dev feature (documented in the book). So some of my contributors do this:

cargo test --features "dev"

[epage]

As you mention, with RUSTFLAGS, we're trading a tower in each of our top-level .rs files with a tower in each of our CI files ... and not a good way for our users to run it.

I'm going to be writing up an RFC for warning groups to be split in two, normal and -unstable. The normal ones do not change on epoch boundaries.

See rust-lang/rust#44581 (comment)

[epage]

In preparing for writing my RFC, I've been giving this more thought, including workarounds and alternative approaches.

One alternative is to do:

matrix:
  include:
  - rust: 1.24.0  # Locking down for consistent behavior
    env: RUSTFLAGS=-D warnings
    install:
    script:
    - cargo check --tests

Pros

• Does not break with time
• Does not get in the way of rapid iteration when prototyping ideas
• Easy to manually enable (though might be stricter if the user is on a different compiler version)
• Less copy / paste of large warning groups
• No RFC process / wait :)
• Easily customize targets or warnings (I'm assuming you can do -D warnings -A dead-code to subtract checks)

Cons

• Manual updating of rust version (also done for rustfmt)
• Enforcement is delayed until the PR
• User won't know about allowed warnings until their PR
  • We could #![warn(warnings)] but I assume that will lower all denys to warn and I doubt we want that.

Thoughts?

[CAD97]

A generic #![warn(warnings)] does not override a #[deny] by default: [play]. A crate noting that they deny warnings in CONTRIBUTING would be enough to inform contributors to address or #[allow] warnings in their contributions.

Pinning to a compiler version has the benefit of also eliminating problems with other forms of warning creep. Bumping CI requirements should be done in the repository history along with necessary fixes for the bumped requirements (if as simple as #[allow]s, otherwise they're actual improvements which could be landed separately). The only problem I see is potential for false positives in the pinned version, which is uncommon but happens (link includes many non-false-positive reports but I couldn't do better).

[epage]

I also decided to ping "users" about it before I wrote up an RFC

https://users.rust-lang.org/t/handling-warnings-in-a-ci/15669

Even if the RFC happens, it'll probably be worthwhile to switch to the pinned compiler until stable groups are available.