Propose lint candidates derived from CodeRabbit review findings#263
Draft
leynos wants to merge 2 commits into
Draft
Propose lint candidates derived from CodeRabbit review findings#263leynos wants to merge 2 commits into
leynos wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Analyse a corpus of 192 CodeRabbit review findings (83 in Rust files) and distil the recurring, mechanically detectable finding classes not already scheduled in `docs/roadmap.md` into seven candidate lints. Recommend three for immediate scheduling (`test_helper_must_return_result`, `assertion_missing_message`, and `no_direct_env_in_tests`), two as follow-ups (`error_context_discarded` and `no_drop_to_silence_unused`), and defer two (`fallible_gap_before_guard` pending the ownership-shape infrastructure, `parameterizable_duplicate_tests` pending clone detector and rstest hygiene experience). Clusters already covered by rustc (E0761), Clippy (`missing_errors_doc`), or scheduled roadmap work (§2.2, §7, §8) are recorded as excluded so the corpus does not need re-analysis. Link the RFC from `docs/contents.md` under a new "Requests for comments" section.
Rename the `no_direct_env_in_tests` candidate to `no_std_env_operations` and widen its scope from test contexts to all code. Direct environment manipulation in tests is a symptom of a production-side design gap: code that reads `std::env` ambiently forces its tests to mutate process-global state. The lint now targets the mechanism everywhere, mirroring how `no_std_fs_operations` pushes filesystem access behind injectable handles, with context-sensitive remedies: inject an environment handle (for example `&dyn mockable::Env`) in production code, and construct a stub such as `MockEnv` in tests. Split severity per operation (mutation deny-by-default, reads warn), keep the sanctioned-boundary allowlist, and replace the guard-crate open question with whether the help text should bless `mockable::Env` by name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch adds RFC 0001, which distils a corpus of 192 CodeRabbit review
findings (83 targeting Rust files) into seven candidate Whitaker lints that
are mechanically detectable, not covered by Clippy or rustc, and not already
scheduled in the roadmap. The RFC recommends three candidates for immediate
scheduling (
test_helper_must_return_result,assertion_missing_message,and
no_std_env_operations), two as follow-ups (error_context_discardedand
no_drop_to_silence_unused), and defers two(
fallible_gap_before_guardandparameterizable_duplicate_tests) againstblocking roadmap infrastructure. Clusters already covered by rustc E0761,
clippy::missing_errors_doc, or scheduled roadmap work (§2.2, §7, §8) arerecorded as excluded so the corpus does not need re-analysis.
no_std_env_operationsdeliberately widens the original test-scopedenvironment candidate: direct environment manipulation in tests is a symptom
of production code reading
std::envambiently, so the lint enforces theinjectable-handle pattern (for example
&dyn mockable::Env) acrossproduction and test code alike, using
no_std_fs_operationsas itsimplementation template.
Review walkthrough
docs/rfcs/0001-coderabbit-derived-lint-candidates.md
for the full analysis: the exclusion screen against existing coverage, the
cluster summary table, per-candidate detection sketches and configuration
surfaces, and the tiered recommendation.
docs/contents.md
for the new "Requests for comments" index section linking the RFC.
Validation
make check-fmt: pass.make test: 1453 passed, 3 skipped, 0 failed.make typecheck: pass (cargo check --workspace --all-targets --all-featureswith-D warnings).make lint: pass (cargo docandcargo clippywith-D warnings).make markdownlint: 69 files linted, 0 errors.make nixie: all Mermaid diagrams validated successfully.Notes
(
~/docs/coderabbit-sample-findings.txt); the RFC records the clustercounts and severity profiles so the analysis is reproducible without it.
design document was dropped during rebase because the identical fix landed
on
mainindependently.