Hey all,
I'm looking for feedback on an idea before I write up an RFC for it.
Motivation
Where I am coming from:
- I love how compiler warnings help me catch problems in my code.
- I want to automate maintaining crates, including reviewing PRs, as much as possible due to the proliferation of crates because of the trend to keep them small.
- I want to minimize human fallacy, in preparing or reviewing a PR, that exist with manual checks (running clippy, etc)
- I don't want to have to look at the logs, on success or failure
- I don't want the build breaking for things unrelated to the PR at hand
- Discourages contributors when they are held accountable for unrelated code or if they have to wait on the maintainer to fix it
- If there is an expectation of this happening, then it requires manually checking the logs to help the contributor through the PR process
The naive solution for warnings is either
#![deny(warnings)]
or
RUSTFLAGS=-D warnings cargo check
But when a new release of Rust comes out, there might be new warnings, breaking your PRs. Some recommend working around this by explicitly listing every warning. The problem is that it either goes stale or takes work to identify new warnings and update it in every crate.
This problem is not unique to Rust. Some fear enabling -Werror
but we don't want to go to the other extreme like gcc where -Wall
has been frozen out of concern for breaking people. The fact that they froze it show enough people care about minimizing breakages from new warnings.
Proposal
I feel like Epochs provide a good compromise between constantly evolving warning groups or warning groups going stale.
Warning groups would remain "frozen" during an epoch. New warnings cannot be added to a group. Warnings can be removed from a group when being removed from the compiler.
warning groups would have an -unstable
variant that represents what the group will be in the next release.
- Supports developers wanting the latest
- Helps developers preparing for a new epoch
- Helps avoid warnings from falling through the cracks when transitioning between epochs.
Alternatives
Not using groups
Mentioned above
Versioning warning groups
With Rust's release schedule, this would quickly lead to a unhelpfully large set of warning groups
Pin the rust compiler in the CI
So instead of locking down the warnings, we lock down the compiler
matrix:
include:
- rust: 1.24.0 # Locking down for consistent behavior
env: RUSTFLAGS=-D warnings
script:
- cargo check --tests
This is a good short term solution and might get us by in the long run.
The downsides:
- Maintaining the compiler version in all of your crates.
- Contributors get surprised with a breaking build for their PR, especially for
allow
s being turned intodeny
s.- A possible workaround is putting a
#![warn(warnings)]
in the code. It sounds like it won't lowerdeny
s towarn
and-D warnings
will still do the right thing?
- A possible workaround is putting a
Post warnings back to the PR
Idea from @skade
Write a tool that builds the code with warnings denied, captures the warnings, and reports them back to github. Run this in on Travis in after_script
.
Pros
- Doesn't require manual inspection of the logs
- Makes information available in a forum the maintainer is already interacting on
Cons
- Still requires the maintainer to sift through them to help guide the contributor on what they are accountable for
- Still requires the maintainer to immediately turn around fixes for the warnings so he doesn't have to sift through them on the next PR
- One time cost to implement a fairly special case tool but would hopefully be used widely.
EDIT: Added "Post warnings back to the PR"