diff --git a/Cargo.lock b/Cargo.lock index e88893dd..248a2d1f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2308,6 +2308,25 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "88f2ca584f7d9d359a09f616900be015302c959d58c2c2da6c075573352dfa0d" +[[package]] +name = "rstest_helper_should_be_fixture" +version = "0.2.5" +dependencies = [ + "dylint_linting", + "log", + "proptest", + "rstest", + "rstest-bdd", + "rstest-bdd-macros", + "rustc_lint", + "rustc_session", + "serde", + "toml 0.9.12+spec-1.1.0", + "trybuild", + "whitaker", + "whitaker-common", +] + [[package]] name = "rstest_macros" version = "0.26.1" @@ -2755,6 +2774,12 @@ dependencies = [ "xattr", ] +[[package]] +name = "target-triple" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "591ef38edfb78ca4771ee32cf494cb8771944bee237a9b91fc9c1424ac4b777b" + [[package]] name = "temp-env" version = "0.3.6" @@ -2788,6 +2813,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "termcolor" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06794f8f6c5c898b3275aebefa6b8a1cb24cd2c6c79397ab15774837a0bc5755" +dependencies = [ + "winapi-util", +] + [[package]] name = "termtree" version = "0.5.1" @@ -2952,6 +2986,21 @@ dependencies = [ "winnow 0.7.15", ] +[[package]] +name = "toml" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "81f3d15e84cbcd896376e6730314d59fb5a87f31e4b038454184435cd57defee" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime 1.1.1+spec-1.1.0", + "toml_parser", + "toml_writer", + "winnow 1.0.1", +] + [[package]] name = "toml_datetime" version = "0.7.5+spec-1.1.0" @@ -3016,6 +3065,21 @@ dependencies = [ "once_cell", ] +[[package]] +name = "trybuild" +version = "1.0.116" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47c635f0191bd3a2941013e5062667100969f8c4e9cd787c14f977265d73616e" +dependencies = [ + "glob", + "serde", + "serde_derive", + "serde_json", + "target-triple", + "termcolor", + "toml 1.1.2+spec-1.1.0", +] + [[package]] name = "type-map" version = "0.5.1" @@ -3462,6 +3526,7 @@ dependencies = [ "rstest", "rstest-bdd", "rstest-bdd-macros", + "rstest_helper_should_be_fixture", "rustc_hir", "rustc_lint", "rustc_session", diff --git a/Cargo.toml b/Cargo.toml index d4788b03..33c07fbd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ flate2 = "1.1" serde_json = "1.0.149" toml = "0.9.8" thiserror = "2" +trybuild = "1.0.116" log = "0.4.28" sha2 = "0.10.9" tar = "0.4.44" @@ -37,6 +38,7 @@ zip = "2" rstest = "0.26.1" rstest-bdd = "0.5.0" rstest-bdd-macros = "0.5.0" +proptest = "1" rustc_lexer = "0.1.0" whitaker_clones_core = { path = "crates/whitaker_clones_core", version = "0.2.5" } whitaker_sarif = { path = "crates/whitaker_sarif", version = "0.2.5" } diff --git a/README.md b/README.md index 02f42337..9df6fbda 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,8 @@ For version pinning, installation details, and configuration options, see the ## The Lints -Whitaker currently ships eight lints, with more on the way: +Whitaker currently ships nine standard lints plus one experimental lint that +requires explicit opt-in. | Lint | What it does | | ----------------------------- | ---------------------------------------------------------------------------------------------------------------------- | @@ -48,6 +49,10 @@ Whitaker currently ships eight lints, with more on the way: | `no_unwrap_or_else_panic` | Catches sneaky panics hidden inside `unwrap_or_else` closures. If you're going to panic, at least be upfront about it. | | `no_std_fs_operations` | Forbids `std::fs` operations, nudging you toward capability-based filesystem access via `cap_std`. | +Experimental lints are not enabled by default. The current experimental lint is +`rstest_helper_should_be_fixture`, which is available only when installer and +suite flows opt in with `--experimental` or the corresponding suite feature. + ## Features - **Localised diagnostics** — Messages available in English, Welsh (Cymraeg), diff --git a/crates/rstest_helper_should_be_fixture/Cargo.toml b/crates/rstest_helper_should_be_fixture/Cargo.toml new file mode 100644 index 00000000..50ba7a15 --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/Cargo.toml @@ -0,0 +1,44 @@ +[package] +name = "rstest_helper_should_be_fixture" +version = "0.2.5" +edition = "2024" +publish = false +description = "Dylint lint that bootstraps rstest helper fixture extraction checks" +license.workspace = true +repository.workspace = true +homepage.workspace = true +documentation.workspace = true + +[lib] +crate-type = ["cdylib", "rlib"] +test = false + +[features] +default = [] +dylint-driver = [ + "dep:whitaker-common", + "dep:dylint_linting", + "dep:log", + "dep:rustc_lint", + "dep:rustc_session", + "dep:serde", + "dep:whitaker", +] +constituent = ["dylint-driver", "dylint_linting/constituent"] + +[dependencies] +whitaker-common = { workspace = true, optional = true } +dylint_linting = { workspace = true, optional = true } +log = { workspace = true, optional = true } +rustc_lint = { workspace = true, optional = true } +rustc_session = { workspace = true, optional = true } +serde = { workspace = true, optional = true } +whitaker = { workspace = true, features = ["dylint-driver"], optional = true } + +[dev-dependencies] +proptest = { workspace = true } +rstest = { workspace = true } +rstest-bdd = { workspace = true } +rstest-bdd-macros = { workspace = true } +toml = { workspace = true } +trybuild = { workspace = true } diff --git a/crates/rstest_helper_should_be_fixture/src/driver.rs b/crates/rstest_helper_should_be_fixture/src/driver.rs new file mode 100644 index 00000000..815759c5 --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/src/driver.rs @@ -0,0 +1,208 @@ +//! Dylint driver bootstrap for the `rstest` helper fixture lint. +//! +//! The driver owns compiler integration and configuration loading. Pure +//! configuration normalization stays in small helper methods so it can be +//! tested without constructing rustc contexts. + +use log::debug; +use rustc_lint::{LateContext, LateLintPass}; +use serde::Deserialize; +use whitaker::SharedConfig; +use whitaker_common::attributes::AttributePath; +use whitaker_common::i18n::{Localizer, get_localizer_for_lint}; +use whitaker_common::rstest::RstestDetectionOptions; + +const LINT_NAME: &str = "rstest_helper_should_be_fixture"; + +const DEFAULT_PROVIDER_PARAM_ATTRIBUTES: &[&str] = + &["case", "values", "files", "future", "context"]; + +type ConfigLoadResult = Result; + +dylint_linting::impl_late_lint! { + pub RSTEST_HELPER_SHOULD_BE_FIXTURE, + Warn, + "repeated rstest helper calls should be extracted into fixtures", + RstestHelperShouldBeFixture::default() +} + +/// Configuration for the `rstest_helper_should_be_fixture` lint. +/// +/// Values are loaded from `dylint.toml` and normalized so threshold settings +/// keep repeated-helper semantics. +#[derive(Clone, Debug, Deserialize, Eq, PartialEq)] +#[serde(default, deny_unknown_fields)] +struct Config { + min_calls: usize, + min_distinct_tests: usize, + require_identical_fixture_arg_names: bool, + provider_param_attributes: Vec, + use_source_callee_fallback: bool, +} + +impl Default for Config { + fn default() -> Self { + Self { + min_calls: 2, + min_distinct_tests: 2, + require_identical_fixture_arg_names: false, + provider_param_attributes: DEFAULT_PROVIDER_PARAM_ATTRIBUTES + .iter() + .map(ToString::to_string) + .collect(), + use_source_callee_fallback: false, + } + } +} + +impl Config { + fn normalized(self) -> Self { + Self { + min_calls: self.min_calls.max(2), + min_distinct_tests: self.min_distinct_tests.max(2), + require_identical_fixture_arg_names: self.require_identical_fixture_arg_names, + provider_param_attributes: normalize_provider_attributes( + self.provider_param_attributes, + ), + use_source_callee_fallback: self.use_source_callee_fallback, + } + } + + fn detection_options(&self) -> RstestDetectionOptions { + let provider_param_attributes = self + .provider_param_attributes + .iter() + .flat_map(|attribute| { + [ + AttributePath::from(attribute.as_str()), + AttributePath::from(format!("rstest::{attribute}")), + ] + }) + .collect(); + RstestDetectionOptions::new(provider_param_attributes, self.use_source_callee_fallback) + } +} + +/// Lint pass bootstrap for repeated `rstest` helper extraction. +pub struct RstestHelperShouldBeFixture { + config: Config, + detection_options: RstestDetectionOptions, + localizer: Localizer, +} + +impl Default for RstestHelperShouldBeFixture { + fn default() -> Self { + let config = Config::default(); + let detection_options = config.detection_options(); + Self { + config, + detection_options, + localizer: Localizer::new(None), + } + } +} + +impl RstestHelperShouldBeFixture { + fn apply_loaded_crate_configuration( + &mut self, + config: ConfigLoadResult, + shared_config: SharedConfig, + ) { + let config = match config { + Ok(config) => config, + Err(error) => { + debug!( + target: LINT_NAME, + "failed to parse `{LINT_NAME}` configuration: {error}; using defaults" + ); + Config::default() + } + }; + + self.apply_crate_configuration(config, shared_config); + } + + fn apply_crate_configuration(&mut self, config: Config, shared_config: SharedConfig) { + debug!( + target: LINT_NAME, + "applying `{LINT_NAME}` configuration: min_calls={}, min_distinct_tests={}, \ + require_identical_fixture_arg_names={}, provider_param_attributes={:?}, \ + use_source_callee_fallback={}, locale={:?}", + config.min_calls, + config.min_distinct_tests, + config.require_identical_fixture_arg_names, + config.provider_param_attributes, + config.use_source_callee_fallback, + shared_config.locale(), + ); + self.config = config; + self.detection_options = self.config.detection_options(); + self.localizer = get_localizer_for_lint(LINT_NAME, shared_config.locale()); + } +} + +impl<'tcx> LateLintPass<'tcx> for RstestHelperShouldBeFixture { + fn check_crate(&mut self, _cx: &LateContext<'tcx>) { + self.apply_loaded_crate_configuration(load_configuration(), load_shared_config()); + } +} + +fn normalize_provider_attributes(attributes: Vec) -> Vec { + let mut normalized = Vec::new(); + for attribute in attributes + .into_iter() + .map(|attribute| attribute.trim().trim_start_matches("rstest::").to_owned()) + .filter(|attribute| !attribute.is_empty()) + { + if !normalized.contains(&attribute) { + normalized.push(attribute); + } + } + + if normalized.is_empty() { + return default_provider_param_attributes(); + } + + normalized +} + +fn default_provider_param_attributes() -> Vec { + DEFAULT_PROVIDER_PARAM_ATTRIBUTES + .iter() + .map(ToString::to_string) + .collect() +} + +fn load_configuration() -> ConfigLoadResult { + debug!(target: LINT_NAME, "loading `{LINT_NAME}` configuration"); + loaded_configuration(dylint_linting::config::(LINT_NAME)) +} + +fn load_shared_config() -> SharedConfig { + // SAFETY / NOTE: `SharedConfig::load` does not currently propagate I/O + // errors, so this named boundary documents the infallible call site + // pending https://github.com/leynos/whitaker/issues/233. + debug!(target: LINT_NAME, "loading shared Whitaker configuration"); + SharedConfig::load() +} + +fn loaded_configuration(loaded: Result, E>) -> ConfigLoadResult +where + E: std::fmt::Display, +{ + match loaded { + Ok(Some(config)) => { + debug!(target: LINT_NAME, "loaded explicit `{LINT_NAME}` configuration"); + Ok(config.normalized()) + } + Ok(None) => { + debug!(target: LINT_NAME, "no `{LINT_NAME}` configuration found; using defaults"); + Ok(Config::default()) + } + Err(error) => Err(error.to_string()), + } +} + +#[cfg(test)] +#[path = "driver_tests.rs"] +mod tests; diff --git a/crates/rstest_helper_should_be_fixture/src/driver_tests.rs b/crates/rstest_helper_should_be_fixture/src/driver_tests.rs new file mode 100644 index 00000000..ecc59bbe --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/src/driver_tests.rs @@ -0,0 +1,208 @@ +//! Unit tests for driver configuration normalization, loading boundaries, and +//! `rstest` detection option construction. +//! +//! NOTE: `SharedConfig::load` is treated as infallible at the driver call site +//! pending https://github.com/leynos/whitaker/issues/233. + +use proptest::prelude::*; +use rstest::rstest; +use whitaker::SharedConfig; + +use super::*; + +#[rstest] +fn default_configuration_matches_design() { + let config = Config::default(); + + assert_eq!(config.min_calls, 2); + assert_eq!(config.min_distinct_tests, 2); + assert!(!config.require_identical_fixture_arg_names); + assert_eq!( + config.provider_param_attributes, + ["case", "values", "files", "future", "context"] + ); + assert!(!config.use_source_callee_fallback); +} + +#[rstest] +fn deserializes_valid_configuration() { + let config: Config = toml::from_str::( + r#" + min_calls = 3 + min_distinct_tests = 4 + require_identical_fixture_arg_names = true + provider_param_attributes = ["case", "custom_provider"] + use_source_callee_fallback = true + "#, + ) + .expect("valid configuration should deserialize") + .normalized(); + + assert_eq!(config.min_calls, 3); + assert_eq!(config.min_distinct_tests, 4); + assert!(config.require_identical_fixture_arg_names); + assert_eq!( + config.provider_param_attributes, + ["case", "custom_provider"] + ); + assert!(config.use_source_callee_fallback); +} + +#[rstest] +fn rejects_unknown_configuration_fields() { + let result = toml::from_str::("unexpected = true"); + + assert!(result.is_err()); +} + +#[rstest] +fn normalizes_numeric_thresholds_to_two() { + let config = Config { + min_calls: 0, + min_distinct_tests: 0, + ..Config::default() + } + .normalized(); + + assert_eq!(config.min_calls, 2); + assert_eq!(config.min_distinct_tests, 2); +} + +#[rstest] +#[case::plain(vec!["case".to_string()], vec!["case"])] +#[case::qualified(vec!["rstest::values".to_string()], vec!["values"])] +#[case::mixed_equivalent_spellings( + vec!["case".to_string(), "rstest::case".to_string()], + vec!["case"] +)] +#[case::blank(vec![" ".to_string()], vec!["case", "values", "files", "future", "context"])] +fn normalizes_provider_attributes(#[case] input: Vec, #[case] expected: Vec<&str>) { + let normalized = normalize_provider_attributes(input); + let expected: Vec = expected.into_iter().map(ToString::to_string).collect(); + + assert_eq!(normalized, expected); +} + +#[rstest] +fn detection_options_expand_plain_and_rstest_qualified_provider_paths() { + let config = Config { + provider_param_attributes: vec!["case".to_string(), "custom".to_string()], + use_source_callee_fallback: true, + ..Config::default() + }; + let options = config.detection_options(); + let paths: Vec = options + .provider_param_attributes() + .iter() + .map(ToString::to_string) + .collect(); + + assert_eq!(paths, ["case", "rstest::case", "custom", "rstest::custom"]); + assert!(options.use_expansion_trace_fallback()); +} + +#[rstest] +fn lint_pass_default_derives_detection_options_from_config() { + let pass = RstestHelperShouldBeFixture::default(); + + assert_eq!(pass.config, Config::default()); + assert_eq!( + pass.detection_options.provider_param_attributes().len(), + DEFAULT_PROVIDER_PARAM_ATTRIBUTES.len() * 2 + ); +} + +#[rstest] +fn loaded_configuration_uses_default_when_config_is_absent() { + assert_eq!( + loaded_configuration::(Ok(None)).expect("missing config should default"), + Config::default(), + ); +} + +#[rstest] +fn loaded_configuration_returns_error_when_config_errors() { + assert_eq!( + loaded_configuration(Err("invalid config")).expect_err("invalid config should error"), + "invalid config", + ); +} + +#[rstest] +fn loaded_configuration_normalizes_present_config() { + let config = Config { + min_calls: 1, + min_distinct_tests: 1, + provider_param_attributes: vec!["rstest::case".to_string()], + ..Config::default() + }; + + assert_eq!( + loaded_configuration::(Ok(Some(config))) + .expect("present config should load") + .provider_param_attributes, + ["case"] + ); +} + +#[rstest] +fn applying_crate_configuration_initializes_pass_state() { + let mut pass = RstestHelperShouldBeFixture::default(); + let config = Config { + provider_param_attributes: vec!["custom".to_string()], + use_source_callee_fallback: true, + ..Config::default() + }; + + pass.apply_crate_configuration(config.clone(), SharedConfig::default()); + + assert_eq!(pass.config, config.normalized()); + assert!(pass.detection_options.use_expansion_trace_fallback()); + assert_eq!(pass.detection_options.provider_param_attributes().len(), 2); +} + +#[rstest] +fn check_crate_configuration_loads_and_normalizes_config() { + let mut pass = RstestHelperShouldBeFixture::default(); + let config = Config { + min_calls: 0, + min_distinct_tests: 1, + provider_param_attributes: vec!["rstest::case".to_string()], + use_source_callee_fallback: true, + ..Config::default() + }; + + pass.apply_loaded_crate_configuration( + loaded_configuration::(Ok(Some(config))), + SharedConfig::default(), + ); + + assert_eq!(pass.config.min_calls, 2); + assert_eq!(pass.config.min_distinct_tests, 2); + assert_eq!(pass.config.provider_param_attributes, ["case"]); + assert!(pass.detection_options.use_expansion_trace_fallback()); + assert_eq!(pass.detection_options.provider_param_attributes().len(), 2); +} + +proptest! { + #[test] + fn normalized_configuration_is_idempotent( + min_calls in 0usize..8, + min_distinct_tests in 0usize..8, + require_identical_fixture_arg_names in any::(), + provider_param_attributes in prop::collection::vec("[ a-z:]{0,24}", 0..8), + use_source_callee_fallback in any::(), + ) { + let config = Config { + min_calls, + min_distinct_tests, + require_identical_fixture_arg_names, + provider_param_attributes, + use_source_callee_fallback, + }; + let once = config.normalized(); + let twice = once.clone().normalized(); + + prop_assert_eq!(once, twice); + } +} diff --git a/crates/rstest_helper_should_be_fixture/src/lib.rs b/crates/rstest_helper_should_be_fixture/src/lib.rs new file mode 100644 index 00000000..a1c00964 --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/src/lib.rs @@ -0,0 +1,30 @@ +//! Experimental lint crate for `rstest` helper fixture extraction. +//! +//! This crate declares the `rstest_helper_should_be_fixture` Dylint lint and +//! wires its configuration defaults. The first implementation intentionally +//! stops before helper-call collection and diagnostic emission; later roadmap +//! items add the analysis that decides when a repeated helper call should +//! become an `rstest` fixture. +#![cfg_attr(feature = "dylint-driver", feature(rustc_private))] + +#[cfg(feature = "dylint-driver")] +mod driver; + +#[cfg(feature = "dylint-driver")] +pub use driver::*; + +#[cfg(not(feature = "dylint-driver"))] +mod stub { + //! Disabled-driver stub module. + //! + //! This module keeps non-driver builds valid when the `dylint-driver` + //! feature is off. It exposes + //! `rstest_helper_should_be_fixture_disabled_stub` as the inert public + //! symbol for that build mode. + + #[expect( + dead_code, + reason = "stub used when the driver feature is disabled; tracked in https://github.com/leynos/whitaker/issues/233" + )] + pub fn rstest_helper_should_be_fixture_disabled_stub() {} +} diff --git a/crates/rstest_helper_should_be_fixture/tests/ui.rs b/crates/rstest_helper_should_be_fixture/tests/ui.rs new file mode 100644 index 00000000..277bd698 --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/tests/ui.rs @@ -0,0 +1,11 @@ +//! Compile-time UI smoke tests for the `rstest_helper_should_be_fixture` +//! bootstrap crate. +//! +//! These trybuild cases intentionally expect no diagnostics while the lint is +//! in its registration and configuration-loading phase. + +#[test] +fn bootstrap_fixtures_compile_without_diagnostics() { + let cases = trybuild::TestCases::new(); + cases.pass("tests/ui/bootstrap_zero_diagnostic.rs"); +} diff --git a/crates/rstest_helper_should_be_fixture/tests/ui/bootstrap_zero_diagnostic.rs b/crates/rstest_helper_should_be_fixture/tests/ui/bootstrap_zero_diagnostic.rs new file mode 100644 index 00000000..384ed452 --- /dev/null +++ b/crates/rstest_helper_should_be_fixture/tests/ui/bootstrap_zero_diagnostic.rs @@ -0,0 +1,11 @@ +//! Zero-diagnostic fixture for the `rstest_helper_should_be_fixture` bootstrap. + +use rstest::rstest; + +fn main() {} + +#[rstest] +#[case::single_use("helper input")] +fn rstest_macro_fixture_compiles_without_diagnostics(#[case] value: &str) { + assert_eq!(value, "helper input"); +} diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 945ba433..8ad33cab 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1445,7 +1445,9 @@ Whitaker data directory keyed by toolchain and target: - `-t, --target-dir DIR` — Staging directory for built libraries - `-l, --lint NAME` — Build specific lint (repeatable) - `--individual-lints` — Build individual crates instead of the suite -- `--experimental` — Include experimental lints in the build (none currently) +- `--experimental` — Include experimental lints in the build. In suite mode + this enables feature-gated experimental lints on `whitaker_suite`; in + `--individual-lints` mode it adds crates from `EXPERIMENTAL_LINT_CRATES`. - `--toolchain TOOLCHAIN` — Override the detected toolchain - `--cranelift` — Install `rustc-codegen-cranelift` for the selected toolchain - `-j, --jobs N` — Number of parallel build jobs @@ -1543,6 +1545,26 @@ When `try_fast_path_installation` returns `Some`, `run_install` constructs a `FinishInstallContext` from the returned values and delegates to `finish_install_and_record_metrics`, skipping the full build pipeline. +#### Crate resolution + +`installer/src/resolution.rs` is the boundary for deciding which lint crates +the installer may build. `run_install` constructs `CrateResolutionOptions` from +the CLI flags before validating or resolving crate names: + +- `individual_lints` switches resolution from the aggregated suite to the + individual lint crate list. +- `experimental` is the explicit opt-in gate for experimental lints. In + individual-lint mode it allows crates from `EXPERIMENTAL_LINT_CRATES`; in + suite mode the build configuration maps it to suite feature flags. + +Call `validate_crate_names` before `resolve_crates` whenever names come from +user input. Validation rejects unknown names and uses `is_experimental_crate` +to detect explicit experimental lint requests. If a user asks for an +experimental crate without `--experimental`, validation returns +`InstallerError::ExperimentalLintRequiresFlag`; callers should surface that +error unchanged so the CLI reports the missing opt-in rather than treating the +crate as unknown or silently building it. + ## Standard vs Experimental Lints Whitaker categorizes lints into two tiers: @@ -1553,7 +1575,10 @@ Whitaker categorizes lints into two tiers: false positives or undergo breaking changes. They require explicit opt-in via the `--experimental` flag. -At present, all shipped Whitaker lints are standard. +The current experimental set contains `rstest_helper_should_be_fixture`. It is +feature-gated in the suite as `experimental-rstest-helper-should-be-fixture` +and listed in `installer/src/resolution.rs` so the installer can derive the +matching suite feature automatically. ### Adding a new lint @@ -1564,6 +1589,8 @@ New lints should typically start as experimental. To add a lint: 2. Add the crate name to `EXPERIMENTAL_LINT_CRATES` in `installer/src/resolution.rs` 3. Add a feature flag for the lint in `suite/Cargo.toml` under `[features]` +4. Add an optional suite dependency and gate its descriptor, lint declaration, + and combined pass entry behind that feature ### Promoting to standard diff --git a/docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md b/docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md new file mode 100644 index 00000000..781018a9 --- /dev/null +++ b/docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md @@ -0,0 +1,834 @@ +# Create the `rstest_helper_should_be_fixture` lint crate + +This ExecPlan (execution plan) is a living document. The sections `Constraints`, + `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, +and `Outcomes & Retrospective` must be kept up to date as work proceeds. + +Status: COMPLETE + +## Purpose / big picture + +Roadmap item 8.2.1 starts the first `rstest` fixture hygiene lint. After this +plan is approved and implemented, Whitaker will contain a new experimental +Dylint crate named `rstest_helper_should_be_fixture`. The crate will declare +and register the public lint `RSTEST_HELPER_SHOULD_BE_FIXTURE`, load its +configuration with sensible defaults, and be visible to the existing suite and +installer wiring without yet attempting the full call-site aggregation +algorithm. + +The observable result is structural rather than diagnostic-heavy: a maintainer +can build the new lint crate, see the lint name in Whitaker's registration +metadata, configure its defaults in `dylint.toml`, and run the unit, +behavioural, lint, and formatting gates successfully. The later roadmap items +8.2.2, 8.2.3, and 8.2.4 will add call-site collection, cross-test aggregation, +diagnostic emission, and UI pass/fail coverage. + +This plan must be approved before implementation starts. + +## Constraints + +- This plan covers roadmap item 8.2.1 only. Do not implement the full repeated + helper-call detector, cross-test grouping, final diagnostic wording, or UI + failure expectations that belong to 8.2.2 through 8.2.4. +- Keep the new lint experimental. It must not become part of the standard + default lint set until later tuning against real repositories. +- Reuse existing repository conventions for Dylint crates, suite registration, + installer resolution, configuration, `rstest` helpers, behaviour tests, and + documentation. +- Preserve the hexagonal boundary in a pragmatic Rust form: pure rule and + configuration policy should be testable without rustc or filesystem I/O, and + compiler/Dylint integration should stay in the driver adapter layer. +- Do not add external dependencies unless implementation proves there is no + reasonable existing workspace dependency or standard-library alternative. +- Do not mark roadmap item 8.2.1 done until implementation has landed, gates + pass, CodeRabbit has no unresolved concerns, and the implementation PR is + ready for review. +- Follow the repository rule that `make check-fmt`, `make lint`, and + `make test` are the commit gates for code changes. Run command suites + sequentially and capture output with `tee` into `/tmp`. +- Use `rstest` for unit tests and `rstest-bdd` for behavioural tests where + behaviour is user-meaningful. Property tests, Kani, or Verus are not required + for the 8.2.1 bootstrap because this step introduces no new algorithmic + invariant over a range of inputs. +- Keep every Rust source file at or below 400 lines and ensure each module + starts with a `//!` module-level purpose comment. + +## Tolerances + +- Scope: if the implementation requires more than 14 repository files or more + than 650 net code lines, stop and ask whether 8.2.1 should be split. +- Interface: if a public API in an existing crate must change incompatibly, + stop and present options. +- Dependencies: if a new third-party dependency is needed, stop and justify it + before adding it. +- Test strategy: if the implementation cannot provide both unit coverage and + behaviour coverage for configuration and registration, stop and explain the + missing observable behaviour. +- Diagnostics: if meaningful diagnostic emission becomes necessary to satisfy + tests, stop and confirm whether part of 8.2.2 or 8.2.3 should be pulled into + this task. +- Validation: if any of `make check-fmt`, `make lint`, or `make test` still + fails after two fix attempts, stop and document the failing command, log + path, and available choices. +- Review: if `coderabbit review --agent` reports concerns after a major + milestone, address them or record why they are out of scope before moving on. +- Ambiguity: if experimental suite wiring conflicts with the existing install + path, stop and ask whether the lint should be standalone-only for 8.2.1. + +## Risks + +- Risk: The existing suite has only a `dylint-driver` feature and derives + experimental feature names from + `installer::resolution::EXPERIMENTAL_LINT_CRATES`. Severity: medium. + Likelihood: medium. Mitigation: add a suite feature named + `experimental-rstest-helper-should-be-fixture` that enables the new optional + lint dependency, and keep the installer source of truth in sync. + +- Risk: A lint that does not yet emit diagnostics can look like dead code. + Severity: medium. Likelihood: medium. Mitigation: make the first milestone + observable through registration, configuration loading, suite metadata, + installer resolution, and tests that exercise those contracts. + +- Risk: Configuration schema drift can create user-facing documentation that + does not match runtime defaults. Severity: medium. Likelihood: low. + Mitigation: define a single `Config::default()` shape in the driver policy + layer, test deserialization with `rstest`, and copy the same defaults into + `docs/users-guide.md` and + `docs/lints-for-rstest-fixtures-and-test-hygiene.md` only if the + implementation changes the design. + +- Risk: `rstest` procedural macro expansion can hide source attributes from + late lint passes. Severity: medium. Likelihood: medium. Mitigation: for + 8.2.1, rely only on the existing `common::rstest` pure detection and + `whitaker::hir` span-recovery hooks. Defer macro-heavy call-site collection + and UI diagnostics to 8.2.2 through 8.2.4. + +- Risk: Full workspace gates are expensive and can expose unrelated failures. + Severity: medium. Likelihood: low. Mitigation: run targeted tests before full + gates, then run the required `make` gates sequentially with logs under + `/tmp`. If unrelated failures are discovered, record evidence and ask for + direction. + +## Progress + +- [x] (2026-05-18T18:36:20Z) Loaded the `leta`, `rust-router`, + `execplans`, `hexagonal-architecture`, `firecrawl-mcp`, `arch-crate-design`, + `rust-errors`, `en-gb-oxendict-style`, `commit-message`, and `pr-creation` + skills needed for planning, branch work, Rust crate boundaries, configuration + errors, documentation style, commits, and PR creation. +- [x] (2026-05-18T18:36:20Z) Created the Leta workspace for this checkout with + `leta workspace add`. +- [x] (2026-05-18T18:36:20Z) Renamed the local branch to + `8-2-1-create-the-rstest-helper-lint-crate`. +- [x] (2026-05-18T18:36:20Z) Used two Wyvern agents to inspect repository + crate/configuration patterns and roadmap/design-document requirements. +- [x] (2026-05-18T18:36:20Z) Used Firecrawl to confirm current Dylint + registration/configuration contracts and `rstest` fixture/test semantics from + upstream documentation. +- [x] (2026-05-18T18:36:20Z) Drafted this pre-implementation ExecPlan. +- [x] (2026-05-18T18:36:20Z) Ran `make fmt` to format the draft and reverted + unrelated formatter-only changes outside this plan file. +- [x] (2026-05-18T18:36:20Z) Validated the plan milestone with + `make check-fmt`, `make markdownlint`, `make lint`, and `make test`. +- [x] (2026-05-18T18:36:20Z) Reviewed and revised this plan after local + formatting and linting. +- [x] (2026-05-18T18:36:20Z) Ran `coderabbit review --agent` for the plan + milestone and cleared Markdown wrapping concerns. +- [x] (2026-05-18T18:36:20Z) Reran `coderabbit review --agent` and received + 0 findings. +- [x] (2026-05-18T19:15:16Z) Committed this plan after validation passed. +- [x] (2026-05-18T19:15:16Z) Pushed the branch and created draft PR + for the ExecPlan. +- [x] (2026-05-20T00:00:00Z) Received explicit user approval to implement + the planned functionality. +- [x] (2026-05-20T00:00:00Z) Created the experimental lint crate and driver + bootstrap with `RSTEST_HELPER_SHOULD_BE_FIXTURE`, configuration defaults, + normalization, and `RstestDetectionOptions` construction. +- [x] (2026-05-20T00:00:00Z) Wired the crate into installer and suite + experimental registration via `EXPERIMENTAL_LINT_CRATES` and the suite feature + `experimental-rstest-helper-should-be-fixture`. +- [x] (2026-05-20T00:00:00Z) Added unit coverage for configuration defaults, + TOML deserialization, unknown fields, numeric threshold normalization, and + provider-attribute normalization in the new crate. +- [x] (2026-05-20T00:00:00Z) Confirmed behaviour coverage for experimental + registration through installer `rstest-bdd` scenarios and suite registration + scenarios. Targeted run + `cargo nextest run -p whitaker_suite -p whitaker-installer --all-targets + --all-features` passed 527 tests. +- [x] (2026-05-20T00:00:00Z) Updated user, developer, design, roadmap, and + plan documentation. `docs/roadmap.md` now marks 8.2.1 done. +- [x] (2026-05-20T00:00:00Z) Ran `coderabbit review --agent` after the + crate/bootstrap milestone and received 0 findings. +- [x] (2026-05-20T00:00:00Z) Ran final `coderabbit review --agent`. It + reported two findings: clamp thresholds to at least 2 and avoid constructing + a full default config for provider fallback. Both findings were accepted and + fixed. +- [x] (2026-05-20T00:00:00Z) Reran `coderabbit review --agent` after the + fixes. It reported one trivial Rustdoc concern for the private `Config` + type, which was accepted and fixed. +- [x] (2026-05-20T00:00:00Z) Ran final local gates. `make check-fmt`, + `make markdownlint`, `make lint`, and `make test` all passed; `make test` + reported 1428 passed and 2 skipped. +- [x] (2026-05-20T00:00:00Z) Reran final gates after the Rustdoc review fix. + `make check-fmt`, `make markdownlint`, `make lint`, and `make test` all + passed; `make test` again reported 1428 passed and 2 skipped. +- [x] (2026-05-20T00:00:00Z) Committed the implementation as `813d2cc`, + pushed it to `origin/8-2-1-create-the-rstest-helper-lint-crate`, and updated + draft PR for implementation + review. +- [x] (2026-05-20T00:00:00Z) Attempted one final CodeRabbit confirmation on + the pushed branch. It was still blocked by a recoverable rate-limit error. +- [x] (2026-05-20T00:00:00Z) Fixed the post-review installer policy issue + where explicit `--lint rstest_helper_should_be_fixture` requests could bypass + the documented `--experimental` opt-in gate. +- [x] (2026-05-20T00:00:00Z) Validated the opt-in fix with focused + installer tests, the required repository gates, and `coderabbit review + --agent`, which completed with 0 findings. +- [x] (2026-05-20T00:00:00Z) Verified inline review comments with Wyvern and + Scribe agents, fixed the still-valid stub documentation, design-document + ordering, users-guide subsection, and builder expectation issues, and skipped + the stale historical ExecPlan count. +- [x] (2026-05-20T00:00:00Z) Validated the inline-review follow-up with + focused tests, `make check-fmt`, `make markdownlint`, `make lint`, + `make test`, and `coderabbit review --agent`, which completed with + 0 findings. +- [x] (2026-05-21T00:00:00Z) Verified review comments about suite macro + definition and experimental lint CLI coverage. The macro redefinition concern + was stale because the two macro definitions already use complementary `cfg` + gates. The CLI success-path coverage request was valid and was implemented. + +## Surprises & discoveries + +- Observation: The installer already has an experimental lint path even though + `EXPERIMENTAL_LINT_CRATES` is currently empty. Evidence: + `installer/src/builder.rs` derives feature names as + `experimental-{lint-name-with-hyphens}` and appends them when building the + suite with `--experimental`. Impact: 8.2.1 should use that existing mechanism + instead of inventing a new feature naming scheme. + +- Observation: The roadmap prerequisites are already complete. + Evidence: `docs/roadmap.md` marks 8.1.1 and 8.1.3 as `[x]`. Impact: + implementation can use `common::rstest` detection and fingerprint APIs + directly without adding prerequisite work to this plan. + +- Observation: Current Dylint upstream docs list `dylint_linting` 6.0.0 as the + latest release, but this workspace pins `dylint_linting = "5"`. Evidence: + Firecrawl retrieved the docs.rs crate page for `dylint_linting` latest, and + `Cargo.toml` pins workspace dependency `dylint_linting = "5"`. Impact: + implementation must follow the local workspace pin and existing macro usage, + not upgrade Dylint as part of this task. + +- Observation: The first implementation slice compiles without adding a new + workspace dependency. Evidence: + `cargo check -p rstest_helper_should_be_fixture --all-targets --all-features` + and `cargo check -p whitaker_suite --all-targets --all-features` both exited + with status 0. Impact: the planned crate boundary and experimental feature + wiring fit the existing workspace structure. + +- Observation: `make fmt` can introduce Markdown reference-link breakage around + issue-style links and bracketed reference text. Evidence: the first + formatting run split `[#180][issue-180]` and a + ``[`FluentLanguageLoader`]`` reference across lines, causing Markdown lint + failures. Impact: those passages were rewritten to avoid the formatter edge + case before continuing. + +- Observation: CodeRabbit caught that normalizing `min_calls` and + `min_distinct_tests` to 1 would make a "repeated" lint meaningful for a + single occurrence. Evidence: final review finding on + `crates/rstest_helper_should_be_fixture/src/driver.rs`. Impact: thresholds + now normalize to at least 2, matching the design defaults and repeated-call + semantics. + +- Observation: Even private configuration types benefit from Rustdoc when they + are the boundary between user TOML and lint policy. Evidence: final + CodeRabbit finding on `Config`. Impact: `Config` now documents that it is + loaded from `dylint.toml` and normalized before use. + +- Observation: Final confirmation reruns of `coderabbit review --agent` were + blocked by CodeRabbit rate limiting after the Rustdoc fix. Evidence: two + retry attempts before the implementation commit and one retry after the final + push returned recoverable rate-limit errors. Impact: all reported CodeRabbit + findings were fixed and local gates were rerun, but the tool could not + provide a zero-finding confirmation after the last documentation-only code + comment change. + +- Observation: `validate_crate_names` needs option context, not just catalogue + membership. Evidence: adding `rstest_helper_should_be_fixture` to + `EXPERIMENTAL_LINT_CRATES` made explicit + `--lint rstest_helper_should_be_fixture` valid without `--experimental`. + Impact: validation now receives `CrateResolutionOptions` and rejects + experimental explicit lint requests unless the opt-in flag is set. + +- Observation: One inline review comment targeted a stale builder test name, + but the same hard-coded expectation still existed in the nearby suite + experimental feature test. Evidence: the Wyvern verification found + `experimental_features_derives_from_experimental_lint_crates` already derives + from `EXPERIMENTAL_LINT_CRATES`, while + `features_for_crate_handles_suite_experimental_mode` still used a literal + feature string. Impact: the latter test now derives its expected value from + `EXPERIMENTAL_LINT_CRATES`. + +## Decision log + +- Decision: Keep this task to scaffolding, registration, and configuration + loading, with no attempt at the full call-site detector. Rationale: Roadmap + item 8.2.1 is explicitly followed by 8.2.2 for call-site collection, 8.2.3 + for aggregation and actionable diagnostics, and 8.2.4 for UI pass/fail + coverage. Date/Author: 2026-05-18T18:36:20Z / Codex. + +- Decision: Introduce the new lint as experimental. + Rationale: `docs/developers-guide.md` states that new lints should typically + start experimental, and the lint design warns that these fixture-hygiene + lints need false-positive tuning before promotion. Date/Author: + 2026-05-18T18:36:20Z / Codex. + +- Decision: Use the existing Rust crate pattern rather than a new architectural + directory layout. Rationale: The repository already organizes lints as + independent Dylint crates under `crates/`, with pure helper code in `common/` + and driver glue inside each lint crate. This satisfies the useful part of + hexagonal architecture without transplanting a foreign package structure. + Date/Author: 2026-05-18T18:36:20Z / Codex. + +- Decision: Do not require Kani or Verus for 8.2.1. + Rationale: The bootstrap creates no substantive invariant over state spaces + or contractual business axiom. Later algorithmic milestones can revisit + property testing or bounded checking when they add grouping logic. + Date/Author: 2026-05-18T18:36:20Z / Codex. + +## Outcomes & retrospective + +Implementation has landed on branch +`8-2-1-create-the-rstest-helper-lint-crate` and draft PR + is updated for review. The +shipped behaviour is a bootstrap: the experimental Dylint crate exists, the +lint declaration and configuration defaults are wired, suite and installer +registration understand the experimental lint, and documentation states that +diagnostics follow later roadmap items. + +The plan milestone has passed local validation: + +- `make check-fmt` succeeded. +- `make markdownlint` succeeded with 0 Markdown errors. +- `make lint` succeeded. +- `make test` succeeded with 1428 tests passed and 2 skipped under the default + nextest profile. +- `coderabbit review --agent` completed with 0 findings after review fixes. + +The implementation milestone has passed local validation: + +- `cargo check -p rstest_helper_should_be_fixture --all-targets --all-features` + succeeded. +- `cargo check -p whitaker_suite --all-targets --all-features` succeeded. +- `cargo nextest run -p rstest_helper_should_be_fixture --all-targets + --all-features` passed 9 tests. +- `cargo nextest run -p whitaker_suite -p whitaker-installer --all-targets + --all-features` passed 527 tests. +- `make check-fmt` succeeded. +- `make markdownlint` succeeded with 0 Markdown errors. +- `make lint` succeeded. +- `make test` succeeded with 1428 tests passed and 2 skipped under the default + nextest profile. + +After addressing CodeRabbit's threshold, provider-fallback, and Rustdoc +findings, the final gates were rerun with the same successful outcomes. Final +CodeRabbit confirmation passes were attempted before and after the final push, +but were blocked by recoverable rate limits. + +Post-review installer policy feedback was addressed by requiring +`--experimental` for explicit experimental lint builds. The resolution unit +tests and installer BDD coverage now cover both the accepted opt-in path and +the rejected non-opt-in path. + +The opt-in fix passed: + +- `cargo nextest run -p whitaker-installer --all-targets --all-features + resolution::tests:: validate_crate_names_variants + dry_run_rejects_experimental_lint_without_opt_in` with 10 selected tests. +- `cargo nextest run -p whitaker-installer --all-targets --all-features -E + 'binary(behaviour_cli) + binary(behaviour_core)'` with 12 selected core BDD + tests. +- `cargo nextest run -p whitaker-installer --all-targets --all-features + --profile ci -E + 'test(scenario_dry_run_rejects_experimental_lint_without_opt_in)'` with the + excluded CLI BDD scenario. +- `make check-fmt`, `make markdownlint`, `make lint`, and `make test`. +- `coderabbit review --agent` with 0 findings. + +## Context and orientation + +Whitaker is a Rust Cargo workspace. The root `Cargo.toml` declares workspace +members `common`, `crates/*`, `installer`, and `suite`. Individual Dylint lint +crates live under `crates/`; shared pure helpers live in `common/`; the root +`whitaker` crate exposes shared compiler-facing glue; and the `suite/` crate +aggregates lint crates into one Dylint cdylib. + +The relevant roadmap entry is in `docs/roadmap.md`: + +```plaintext +8.2.1. Create the `rstest_helper_should_be_fixture` lint crate, register +`RSTEST_HELPER_SHOULD_BE_FIXTURE`, and wire configuration loading defaults. +``` + +The design source of truth is +`docs/lints-for-rstest-fixtures-and-test-hygiene.md`, especially "Lint A: +call-site fixture extraction". That document defines: + +- crate name: `rstest_helper_should_be_fixture`, +- lint name: `RSTEST_HELPER_SHOULD_BE_FIXTURE`, +- default configuration keys: + `min_calls`, `min_distinct_tests`, `require_identical_fixture_arg_names`, + `provider_param_attributes`, and `use_source_callee_fallback`, +- strict `rstest` detection for `rstest` and `rstest::rstest`, +- default provider parameter attributes: + `case`, `values`, `files`, `future`, and `context`, +- first-version fixture-local support for simple identifier bindings only. + +Roadmap prerequisites 8.1.1 and 8.1.3 are already complete. They provide the +shared `common::rstest` APIs in `common/src/rstest/`: + +- `is_rstest_test` and `is_rstest_test_with` for strict test detection, +- `is_rstest_fixture` and `is_rstest_fixture_with` for strict fixture + detection, +- `RstestDetectionOptions` and `fixture_local_names` for fixture-local + parameter policy, +- `ArgFingerprint` and `ArgAtom` for later helper-call fingerprinting. + +Existing lint crates show the local Dylint pattern. For example, +`crates/conditional_max_n_branches/src/driver.rs` declares its lint with +`dylint_linting::impl_late_lint!`, loads per-lint configuration through +`dylint_linting::config::(LINT_NAME)`, falls back to defaults on +missing or invalid configuration, and loads shared locale settings through +`whitaker::SharedConfig`. The new crate should match this style. + +The suite registration files are: + +- `suite/Cargo.toml`, +- `suite/src/lints.rs`, +- `suite/src/driver.rs`, +- `suite/tests/registration.rs`, +- `suite/tests/features/suite_registration.feature`. + +The installer registration files are: + +- `installer/src/resolution.rs`, +- `installer/src/builder.rs`, +- `installer/src/scanner.rs`, +- `installer/tests/behaviour_core.rs`, +- `installer/tests/features/installer.feature`. + +The user-facing documentation that may need updates is: + +- `docs/users-guide.md` for the new lint section and configuration example, +- `docs/developers-guide.md` for any new internal convention, +- `docs/lints-for-rstest-fixtures-and-test-hygiene.md` for implementation + decisions if runtime behaviour differs from the design, +- `docs/roadmap.md` when implementation is complete. + +Useful skills for implementation are `leta`, `rust-router`, `arch-crate-design`, + `rust-errors`, `hexagonal-architecture`, `en-gb-oxendict-style`, +`commit-message`, and `pr-creation`. If later algorithm work introduces +invariants, route again through `rust-router` and consider `kani` or `verus`. + +External references checked during planning: + +- Dylint `dylint_linting` docs: + . The macros `impl_late_lint!` + and configuration helpers such as `config_or_default`, `config`, and + `init_config` are the relevant public contracts. +- `rstest` fixture docs: + . Fixtures are + functions annotated with `#[fixture]` and injected as `#[rstest]` test + parameters. +- `rstest` test docs: + . `#[rstest]` supports + fixture injection, cases, values, files, `future`, and `context` parameter + attributes. + +## Plan of work + +Stage A is pre-implementation review. Read this ExecPlan, compare it with +`docs/roadmap.md` and `docs/lints-for-rstest-fixtures-and-test-hygiene.md`, and +obtain explicit approval. Do not edit production code before approval. + +Stage B creates the lint crate skeleton. Add +`crates/rstest_helper_should_be_fixture/Cargo.toml`, +`crates/rstest_helper_should_be_fixture/src/lib.rs`, and a driver module such as + `src/driver.rs`. The manifest should follow existing lint crates: +`crate-type = ["cdylib", "rlib"]`, `test = false`, a `dylint-driver` feature +that enables `dylint_linting`, `rustc_*`, `serde`, `log`, `whitaker`, and +`whitaker-common`, and a `constituent` feature that enables +`dylint_linting/constituent`. The non-driver build should compile through a +small stub module so `cargo check --workspace --all-targets --all-features` and +documentation builds stay healthy. + +Stage C implements registration and configuration policy. In the driver, +declare: + +```rust +dylint_linting::impl_late_lint! { + pub RSTEST_HELPER_SHOULD_BE_FIXTURE, + Warn, + "repeated rstest helper calls should be extracted into fixtures", + RstestHelperShouldBeFixture::default() +} +``` + +Define a `Config` with `#[serde(default, deny_unknown_fields)]` and these +defaults: + +```plaintext +min_calls = 2 +min_distinct_tests = 2 +require_identical_fixture_arg_names = false +provider_param_attributes = ["case", "values", "files", "future", "context"] +use_source_callee_fallback = false +``` + +Keep pure validation and normalization near the config type. The driver should +load configuration in `check_crate`, clamp invalid numeric thresholds to at +least `2` or reject them by falling back to defaults in the same manner as +existing lints, and build `RstestDetectionOptions` from the provider +attributes. If a value is invalid enough that clamping would hide a user +mistake, use the existing "log and default" pattern rather than panic. + +Stage D adds a minimal late-lint pass body that proves wiring without claiming +8.2.2 behaviour. The pass may inspect crate or item structure enough to prove +that it can construct configuration and detection options, but it should not +emit helper-call diagnostics until the collection and aggregation milestones +exist. Any placeholder should be named as a bootstrap path, not as completed +analysis. + +Stage E wires registration outside the crate. Add +`rstest_helper_should_be_fixture` to `installer/src/resolution.rs` +`EXPERIMENTAL_LINT_CRATES`, not to `LINT_CRATES`. Add the suite optional +dependency and a feature named `experimental-rstest-helper-should-be-fixture` to + `suite/Cargo.toml`, following the feature name that `installer/src/builder.rs` +derives from the experimental crate list. Update `suite/src/lints.rs` and +`suite/src/driver.rs` so the lint is included only when that experimental +feature is enabled. Update suite and installer tests to expect the new +experimental lint when experimental mode is on and to exclude it otherwise. + +Stage F adds tests. Unit tests should use `rstest` for `Config::default()`, +valid TOML deserialization, unknown-field rejection, provider-attribute +normalization, threshold handling, and feature-name expectations if helper +functions are added. Behaviour tests should use `rstest-bdd` where the +observable behaviour is "experimental lint resolution includes the new crate" +or "the suite metadata exposes the new lint only with experimental features". +If a crate-local behaviour file is added, keep it focused on configuration and +registration, not future diagnostic detection. + +Stage G updates documentation. Add a concise experimental lint section to +`docs/users-guide.md` that identifies the purpose, scope, and configuration +defaults. Say that this first implementation registers and configures the lint +and that diagnostic emission follows the later roadmap items if that remains +true at implementation time. Update `docs/developers-guide.md` only if a new +experimental-suite convention is introduced or clarified. Update +`docs/lints-for-rstest-fixtures-and-test-hygiene.md` with an implementation +decision note only if the implementation materially differs from the current +design. Mark `docs/roadmap.md` item 8.2.1 done only after the implementation +passes gates and review. + +Stage H validates, reviews, commits, and prepares the implementation PR. Run +targeted tests first, then the full gates. Run `coderabbit review --agent` +after the crate/configuration milestone and again before final push if the +review tool is available. Address all relevant concerns. Commit in small +logical units: crate bootstrap, registration wiring, tests, documentation, and +roadmap completion if those land separately. + +## Concrete steps + +From the repository root, confirm branch and workspace state: + +```sh +git branch --show-current +git status --short --branch +``` + +Expected output includes: + +```plaintext +8-2-1-create-the-rstest-helper-lint-crate +``` + +After approval, create the crate files and add registration wiring. Use +`apply_patch` for manual edits. Prefer existing crate files as templates: + +```sh +sed -n '1,180p' crates/conditional_max_n_branches/src/driver.rs +sed -n '1,140p' crates/conditional_max_n_branches/Cargo.toml +sed -n '1,140p' suite/src/lints.rs +sed -n '1,180p' installer/src/resolution.rs +``` + +Run targeted checks after the first code milestone: + +```sh +ACTION=check-rstest-helper +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +cargo check -p rstest_helper_should_be_fixture --all-targets --all-features 2>&1 | tee "$LOG" +``` + +Run targeted unit and behaviour tests after tests are added: + +```sh +ACTION=test-rstest-helper +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +cargo nextest run -p rstest_helper_should_be_fixture --all-targets --all-features 2>&1 | tee "$LOG" +``` + +Run suite and installer tests touched by registration: + +```sh +ACTION=test-registration +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +cargo nextest run -p whitaker_suite -p whitaker-installer --all-targets --all-features 2>&1 | tee "$LOG" +``` + +Run documentation checks after documentation changes: + +```sh +ACTION=markdownlint +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +make markdownlint 2>&1 | tee "$LOG" +``` + +If Mermaid diagrams are edited, also run: + +```sh +ACTION=nixie +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +make nixie 2>&1 | tee "$LOG" +``` + +Run final gates sequentially before committing code as complete: + +```sh +ACTION=check-fmt +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +make check-fmt 2>&1 | tee "$LOG" + +ACTION=lint +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +make lint 2>&1 | tee "$LOG" + +ACTION=test +LOG="/tmp/${ACTION}-whitaker-$(git branch --show-current).out" +make test 2>&1 | tee "$LOG" +``` + +Run CodeRabbit after each major milestone: + +```sh +coderabbit review --agent +``` + +Commit using a file-based commit message: + +```sh +git status --short +git diff --stat +git add +COMMIT_MSG_DIR=$(mktemp -d) +cat > "$COMMIT_MSG_DIR/COMMIT_MSG.md" << 'ENDOFMSG' +Create rstest helper fixture lint crate + +Add the experimental Dylint crate scaffold, registration wiring, +configuration defaults, tests, and documentation for roadmap item 8.2.1. +ENDOFMSG +git commit -F "$COMMIT_MSG_DIR/COMMIT_MSG.md" +rm -rf "$COMMIT_MSG_DIR" +``` + +Push and open a draft pull request only after validation succeeds: + +```sh +git push -u origin 8-2-1-create-the-rstest-helper-lint-crate +echo "${LODY_SESSION_ID}" +``` + +Use the Lody session URL in the PR body: + +```plaintext +https://lody.ai/leynos/sessions/${LODY_SESSION_ID} +``` + +## Validation and acceptance + +The plan is accepted for implementation when the user explicitly approves it. +Silence is not approval. + +The implementation is accepted when all of the following are true: + +- `crates/rstest_helper_should_be_fixture` exists and builds with + `--features dylint-driver`. +- The crate exposes `RSTEST_HELPER_SHOULD_BE_FIXTURE` and the pass type + `RstestHelperShouldBeFixture`. +- Configuration defaults match the lint design and are covered by `rstest` + unit tests. +- Unknown configuration fields are rejected or defaulted consistently with the + repository's existing Dylint configuration policy. +- The installer recognizes the crate as experimental and includes it only when + experimental lints are requested or when it is explicitly named. +- The suite exposes the lint only through the experimental feature + `experimental-rstest-helper-should-be-fixture`. +- Behaviour tests cover experimental registration and configuration loading in + user-observable terms. +- `docs/users-guide.md`, `docs/developers-guide.md`, and the rstest lint + design document are updated where behaviour or internal practice changed. +- `docs/roadmap.md` marks 8.2.1 done only after all implementation checks pass. +- `coderabbit review --agent` has no unresolved relevant concerns. +- `make check-fmt`, `make lint`, and `make test` all succeed. + +Expected final gate transcript shape: + +```plaintext +$ make check-fmt +... +Finished + +$ make lint +... +Finished + +$ make test +... +test result: ok +``` + +The exact test count may change as new tests are added; success means the +commands exit with status 0. + +## Idempotence and recovery + +All edit stages are ordinary file edits and can be retried from `git status`. +If a step fails part-way through, inspect `git diff` and either continue from +the incomplete file or revert only the files changed for this task. Do not +revert unrelated user changes. + +If a generated or copied lint crate scaffold is wrong, delete only +`crates/rstest_helper_should_be_fixture/` before it is committed, then recreate +it from the existing lint crate pattern. After a commit, prefer a corrective +follow-up commit over rewriting history unless the user asks for history +cleanup. + +If validation fails, inspect the matching +`/tmp/*-whitaker-8-2-1-create-the-rstest-helper-lint-crate.out` log before +changing code. Record persistent failures in `Surprises & Discoveries` or +`Decision Log` with the command and log path. + +## Artifacts and notes + +Wyvern repository-pattern findings: + +```plaintext +Existing lint crates use `dylint_linting::impl_late_lint!`, per-lint +`Config` structs, `LINT_NAME` matching the crate name, `SharedConfig` for +locale, and suite/installer registration through `suite/` and +`installer/src/resolution.rs`. +``` + +Wyvern documentation findings: + +```plaintext +8.2.1 is the bootstrap step. 8.2.2 starts call-site collection, 8.2.3 adds +aggregation and actionable diagnostics, and 8.2.4 adds UI pass/fail coverage. +``` + +Firecrawl findings: + +```plaintext +Dylint's `impl_late_lint!` macro wraps Dylint library registration, lint +declaration, lint-pass implementation, and late-pass registration for a +single-lint library. Dylint configuration is read from `dylint.toml` with +helpers such as `config`, `config_or_default`, and `init_config`. + +`rstest` fixtures are functions marked with `#[fixture]` and injected as test +arguments. `#[rstest]` parameters can also be provider-driven through +attributes such as `case`, `values`, `files`, `future`, and `context`. +``` + +## Interfaces and dependencies + +The new crate should expose these driver-facing items when `dylint-driver` is +enabled: + +```rust +pub const RSTEST_HELPER_SHOULD_BE_FIXTURE: &rustc_lint::Lint; + +pub struct RstestHelperShouldBeFixture { + config: Config, + detection_options: whitaker_common::rstest::RstestDetectionOptions, + localizer: whitaker_common::Localizer, +} +``` + +The exact field visibility may remain private. The public contract is the lint +constant and pass type. If the Dylint macro exposes a different lint constant +type than the simplified sketch above, follow the macro's actual expansion and +the pattern used by existing lint crates. + +The configuration policy should be equivalent to: + +```rust +#[derive(Clone, Debug, Deserialize, Eq, PartialEq)] +#[serde(default, deny_unknown_fields)] +struct Config { + min_calls: usize, + min_distinct_tests: usize, + require_identical_fixture_arg_names: bool, + provider_param_attributes: Vec, + use_source_callee_fallback: bool, +} +``` + +Do not export `Config` unless tests or downstream code genuinely require it. If +tests need access, prefer `pub(crate)` helpers or driver-internal tests. + +The new crate should depend on existing workspace dependencies only: + +- `dylint_linting`, +- `log`, +- `rustc_hir`, +- `rustc_lint`, +- `rustc_session`, +- `rustc_span`, +- `serde`, +- `whitaker`, +- `whitaker-common`, +- `rstest`, +- `rstest-bdd`, +- `rstest-bdd-macros`, +- `dylint_testing` if a UI harness is required for bootstrap smoke coverage. + +No new production dependency is expected for 8.2.1. + +## Revision note + +Initial draft created on 2026-05-18. It records the pre-implementation scope, +repo orientation, external references, experimental-suite decision, validation +commands, and approval gate for roadmap item 8.2.1. + +Revision on 2026-05-18 after local validation: recorded successful +`make check-fmt`, `make markdownlint`, `make lint`, and `make test` results. +This does not change the implementation scope; it only adds evidence for the +ExecPlan review milestone. + +Revision on 2026-05-18 after CodeRabbit review: wrapped one long Markdown line +reported by `coderabbit review --agent`. This is a formatting-only change with +no effect on implementation scope. + +Second revision on 2026-05-18 after CodeRabbit review: moved the `rstest` +test-documentation URL onto its own line and wrapped the following sentence. +This is a formatting-only change with no effect on implementation scope. + +Third revision on 2026-05-18 after CodeRabbit review: changed an `-isation` +spelling to the matching Oxford `-ization` spelling. This is a spelling-only +change with no effect on implementation scope. + +Fourth revision on 2026-05-18 after CodeRabbit review: recorded the clean +`coderabbit review --agent` result. This adds review evidence only and does not +change the implementation scope. + +Fifth revision on 2026-05-18 after draft PR creation: recorded the commit, +push, and draft PR URL in `Progress`. This is a process-tracking update with no +effect on implementation scope. diff --git a/docs/lints-for-rstest-fixtures-and-test-hygiene.md b/docs/lints-for-rstest-fixtures-and-test-hygiene.md index d148e218..3e3c52bd 100644 --- a/docs/lints-for-rstest-fixtures-and-test-hygiene.md +++ b/docs/lints-for-rstest-fixtures-and-test-hygiene.md @@ -429,3 +429,17 @@ standard lints. exercise the frame-selection algorithm directly, and `rstest-bdd` 0.5.x scenarios in `common/tests/rstest_span_recovery_behaviour.rs` describe the direct, recovered, first-match, and macro-only outcomes in user terms. + +## Implementation decisions (8.2.1) + +- `rstest_helper_should_be_fixture` now exists as an experimental Dylint crate. + The first implementation declares `RSTEST_HELPER_SHOULD_BE_FIXTURE`, loads + and normalizes the Lint A configuration defaults, and builds the shared + `RstestDetectionOptions` policy. +- The lint is intentionally diagnostic-silent in 8.2.1. Call-site collection, + aggregation, and user-facing suggestions remain assigned to 8.2.2 through + 8.2.4, so this bootstrap does not claim analysis behaviour it does not yet + implement. +- The suite exposes the lint only through the experimental feature + `experimental-rstest-helper-should-be-fixture`. The installer derives that + feature from `EXPERIMENTAL_LINT_CRATES` when `--experimental` is enabled. diff --git a/docs/roadmap.md b/docs/roadmap.md index e22a1d62..9aa6cf11 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -470,7 +470,7 @@ ### 8.2. `rstest_helper_should_be_fixture` lint -- [ ] 8.2.1. Create the `rstest_helper_should_be_fixture` lint crate, register +- [x] 8.2.1. Create the `rstest_helper_should_be_fixture` lint crate, register `RSTEST_HELPER_SHOULD_BE_FIXTURE`, and wire configuration loading defaults. See [rstest fixture and test hygiene lints](lints-for-rstest-fixtures-and-test-hygiene.md) diff --git a/docs/users-guide.md b/docs/users-guide.md index 73479e6e..a7752ecc 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -155,9 +155,9 @@ Whitaker lints are divided into two categories: false positives or undergo breaking changes between releases. They must be explicitly enabled. -The default `whitaker_suite` pattern includes only standard lints. At present, -all shipped Whitaker lints are standard and there are no experimental lints in -the release. +The default `whitaker_suite` pattern includes only standard lints. Whitaker +currently ships one experimental lint, `rstest_helper_should_be_fixture`, which +is available only when experimental lints are enabled. ### Enabling experimental lints @@ -167,7 +167,11 @@ the release. whitaker-installer --experimental ``` -This flag is retained for forward compatibility and currently has no effect. +This enables experimental suite features when building `whitaker_suite`. To +build experimental lints as individual libraries, combine it with +`--individual-lints`. Explicit `--lint` requests for experimental lints also +require `--experimental`; without that opt-in the installer rejects the request +before building anything. ## Lint Configuration @@ -196,6 +200,14 @@ additional_test_attributes = ["actix_rt::test", "my_framework::test"] # Allow panics in main [no_unwrap_or_else_panic] allow_in_main = true + +# Experimental rstest fixture extraction lint +[rstest_helper_should_be_fixture] +min_calls = 2 +min_distinct_tests = 2 +require_identical_fixture_arg_names = false +provider_param_attributes = ["case", "values", "files", "future", "context"] +use_source_callee_fallback = false ``` ## Localized Diagnostics @@ -503,6 +515,73 @@ not in Whitaker's default list and is not listed in ______________________________________________________________________ +### `rstest_helper_should_be_fixture` + + +#### Purpose + +Bootstraps the experimental lint that will recommend converting repeated helper +calls inside `#[rstest]` tests into injected `#[fixture]` parameters. + + +#### Scope and behaviour + +This lint is experimental. The current implementation registers the lint and +loads configuration defaults so teams can opt into the forthcoming rule without +yet receiving helper-call diagnostics. Call-site collection, cross-test +aggregation, and actionable diagnostics are tracked by the later roadmap items +8.2.2 through 8.2.4. + + +#### Configuration + +```toml +[rstest_helper_should_be_fixture] +min_calls = 2 +min_distinct_tests = 2 +require_identical_fixture_arg_names = false +provider_param_attributes = ["case", "values", "files", "future", "context"] +use_source_callee_fallback = false +``` + +`provider_param_attributes` lists `rstest` parameter attributes that should be +treated as data providers rather than fixture-local bindings. Entries may be +written either as bare names such as `case` or qualified names such as +`rstest::case`; Whitaker normalizes them to the shared detection policy. + + +#### What is allowed + +- Single-use helper calls inside `#[rstest]` tests +- Helper calls whose totals stay below `min_calls` or `min_distinct_tests` +- Parameter-provider uses covered by `provider_param_attributes`, such as + `case`, `values`, `files`, `future`, and `context` + + +#### What is denied + +When diagnostic phases are implemented, `rstest_helper_should_be_fixture` will +deny repeated non-provider helper invocations across `#[rstest]` tests when +they meet or exceed both `min_calls` and `min_distinct_tests`. The +`require_identical_fixture_arg_names` setting controls whether candidate +fixture arguments must use the same names, and `use_source_callee_fallback` +controls whether source-callsite recovery may be used for macro-expanded +callee locations. + + +#### How to fix + +- Replace repeated helper calls with a shared `#[fixture]` parameter. +- If `require_identical_fixture_arg_names` is enabled, rename helper arguments + so repeated calls use the same fixture argument names. +- Prefer provider attributes listed in `provider_param_attributes` for + parameterized data inputs rather than modelling them as fixture-local helper + calls. +- Tune `min_calls`, `min_distinct_tests`, and `use_source_callee_fallback` when + repository conventions need stricter or looser matching. + +______________________________________________________________________ + ### `test_must_not_have_example` Warns when test function documentation includes example headings (for example diff --git a/installer/src/builder.rs b/installer/src/builder.rs index d20d9681..e84e4235 100644 --- a/installer/src/builder.rs +++ b/installer/src/builder.rs @@ -281,9 +281,14 @@ mod tests { fn features_for_crate_handles_suite_experimental_mode(mut builder: Builder) { builder.config.experimental = true; let result = builder.features_for_crate(&CrateName::from("whitaker_suite")); - // At present EXPERIMENTAL_LINT_CRATES is empty, so suite features remain - // unchanged even when experimental mode is enabled. - assert_eq!(result, "dylint-driver"); + let mut expected_features = vec!["dylint-driver".to_owned()]; + expected_features.extend( + EXPERIMENTAL_LINT_CRATES + .iter() + .map(|&lint| format!("experimental-{}", lint.replace('_', "-"))), + ); + let expected = expected_features.join(","); + assert_eq!(result, expected); } #[test] diff --git a/installer/src/error.rs b/installer/src/error.rs index fd40e964..84e9a8f0 100644 --- a/installer/src/error.rs +++ b/installer/src/error.rs @@ -91,6 +91,13 @@ pub enum InstallerError { name: CrateName, }, + /// An experimental lint crate was requested without explicit opt-in. + #[error("experimental lint crate {name} requires --experimental")] + ExperimentalLintRequiresFlag { + /// Name of the experimental lint crate. + name: CrateName, + }, + /// The workspace root could not be found. #[error("workspace not found: {reason}")] WorkspaceNotFound { @@ -158,91 +165,102 @@ pub enum InstallerError { }, } -impl Clone for InstallerError { - fn clone(&self) -> Self { - // Helper to clone io::Error by preserving ErrorKind and formatted message. - // This is lossy: the original source chain is discarded. - fn clone_io_error(source: &std::io::Error) -> std::io::Error { - std::io::Error::new(source.kind(), source.to_string()) - } +// Helper to clone io::Error by preserving ErrorKind and formatted message. +// This is lossy: the original source chain is discarded. +fn clone_io_error(source: &std::io::Error) -> std::io::Error { + std::io::Error::new(source.kind(), source.to_string()) +} +impl InstallerError { + fn clone_toolchain_variant(&self) -> Option { match self { - InstallerError::ToolchainDetection { reason } => InstallerError::ToolchainDetection { + Self::ToolchainDetection { reason } => Some(Self::ToolchainDetection { reason: reason.clone(), - }, - InstallerError::ToolchainFileNotFound { path } => { - InstallerError::ToolchainFileNotFound { path: path.clone() } - } - InstallerError::InvalidToolchainFile { reason } => { - InstallerError::InvalidToolchainFile { - reason: reason.clone(), - } + }), + Self::ToolchainFileNotFound { path } => { + Some(Self::ToolchainFileNotFound { path: path.clone() }) } - InstallerError::ToolchainNotInstalled { toolchain } => { - InstallerError::ToolchainNotInstalled { - toolchain: toolchain.clone(), - } - } - InstallerError::ToolchainInstallFailed { toolchain, message } => { - InstallerError::ToolchainInstallFailed { + Self::InvalidToolchainFile { reason } => Some(Self::InvalidToolchainFile { + reason: reason.clone(), + }), + Self::ToolchainNotInstalled { toolchain } => Some(Self::ToolchainNotInstalled { + toolchain: toolchain.clone(), + }), + Self::ToolchainInstallFailed { toolchain, message } => { + Some(Self::ToolchainInstallFailed { toolchain: toolchain.clone(), message: message.clone(), - } + }) } - InstallerError::ToolchainComponentInstallFailed { + Self::ToolchainComponentInstallFailed { toolchain, components, message, - } => InstallerError::ToolchainComponentInstallFailed { + } => Some(Self::ToolchainComponentInstallFailed { toolchain: toolchain.clone(), components: components.clone(), message: message.clone(), - }, - InstallerError::BuildFailed { crate_name, reason } => InstallerError::BuildFailed { + }), + _ => None, + } + } +} + +impl Clone for InstallerError { + fn clone(&self) -> Self { + if let Some(value) = self.clone_toolchain_variant() { + return value; + } + + match self { + Self::BuildFailed { crate_name, reason } => Self::BuildFailed { crate_name: crate_name.clone(), reason: reason.clone(), }, - InstallerError::StagingFailed { reason } => InstallerError::StagingFailed { + Self::StagingFailed { reason } => Self::StagingFailed { reason: reason.clone(), }, - InstallerError::TargetNotWritable { path, reason } => { - InstallerError::TargetNotWritable { - path: path.clone(), - reason: reason.clone(), - } - } - InstallerError::LintCrateNotFound { name } => { - InstallerError::LintCrateNotFound { name: name.clone() } + Self::TargetNotWritable { path, reason } => Self::TargetNotWritable { + path: path.clone(), + reason: reason.clone(), + }, + Self::LintCrateNotFound { name } => Self::LintCrateNotFound { name: name.clone() }, + Self::ExperimentalLintRequiresFlag { name } => { + Self::ExperimentalLintRequiresFlag { name: name.clone() } } - InstallerError::WorkspaceNotFound { reason } => InstallerError::WorkspaceNotFound { + Self::WorkspaceNotFound { reason } => Self::WorkspaceNotFound { reason: reason.clone(), }, - InstallerError::InvalidCargoToml { path, reason } => InstallerError::InvalidCargoToml { + Self::InvalidCargoToml { path, reason } => Self::InvalidCargoToml { path: path.clone(), reason: reason.clone(), }, - InstallerError::Io(source) => InstallerError::Io(clone_io_error(source)), - InstallerError::Git { operation, message } => InstallerError::Git { + Self::Io(source) => Self::Io(clone_io_error(source)), + Self::Git { operation, message } => Self::Git { operation, message: message.clone(), }, - InstallerError::DependencyInstall { tool, message } => { - InstallerError::DependencyInstall { - tool, - message: message.clone(), - } - } - InstallerError::WrapperGeneration(message) => { - InstallerError::WrapperGeneration(message.clone()) - } - InstallerError::ScanFailed { source } => InstallerError::ScanFailed { + Self::DependencyInstall { tool, message } => Self::DependencyInstall { + tool, + message: message.clone(), + }, + Self::WrapperGeneration(message) => Self::WrapperGeneration(message.clone()), + Self::ScanFailed { source } => Self::ScanFailed { source: clone_io_error(source), }, - InstallerError::WriteFailed { source } => InstallerError::WriteFailed { + Self::WriteFailed { source } => Self::WriteFailed { source: clone_io_error(source), }, + Self::ToolchainDetection { .. } + | Self::ToolchainFileNotFound { .. } + | Self::InvalidToolchainFile { .. } + | Self::ToolchainNotInstalled { .. } + | Self::ToolchainInstallFailed { .. } + | Self::ToolchainComponentInstallFailed { .. } => { + unreachable!("handled by clone_toolchain_variant") + } #[cfg(any(test, feature = "test-support"))] - InstallerError::StubMismatch { message } => InstallerError::StubMismatch { + Self::StubMismatch { message } => Self::StubMismatch { message: message.clone(), }, } diff --git a/installer/src/main.rs b/installer/src/main.rs index 84f15137..d3d51250 100644 --- a/installer/src/main.rs +++ b/installer/src/main.rs @@ -333,14 +333,14 @@ fn resolve_requested_crates(args: &InstallArgs) -> Result> { .map(|name| CrateName::from(name.as_str())) .collect(); - if !lint_crates.is_empty() { - validate_crate_names(&lint_crates)?; - } - let options = CrateResolutionOptions { individual_lints: args.individual_lints, experimental: args.experimental, }; + if !lint_crates.is_empty() { + validate_crate_names(&lint_crates, &options)?; + } + Ok(resolve_crates(&lint_crates, &options)) } diff --git a/installer/src/resolution.rs b/installer/src/resolution.rs index 74a9a770..e7df03a8 100644 --- a/installer/src/resolution.rs +++ b/installer/src/resolution.rs @@ -5,6 +5,7 @@ use crate::crate_name::CrateName; use crate::error::{InstallerError, Result}; +use log::debug; /// Static list of lint crates available for building. /// @@ -25,8 +26,8 @@ pub const LINT_CRATES: &[&str] = &[ /// Static list of experimental lint crates. /// /// These lints are not included in the default suite but can be enabled via -/// the `--experimental` flag. The list is currently empty. -pub const EXPERIMENTAL_LINT_CRATES: &[&str] = &[]; +/// the `--experimental` flag. +pub const EXPERIMENTAL_LINT_CRATES: &[&str] = &["rstest_helper_should_be_fixture"]; /// The aggregated suite crate name. pub const SUITE_CRATE: &str = "whitaker_suite"; @@ -51,8 +52,9 @@ pub struct CrateResolutionOptions { /// `EXPERIMENTAL_LINT_CRATES` are included in the returned crate list. /// - In suite mode (default), the `experimental` flag is used by `BuildConfig` /// to enable experimental features when building the suite crate. -/// - When `specific_lints` are provided, experimental crates are accepted if -/// present, but the `experimental` flag does not affect the returned list. +/// - When `specific_lints` are provided, the returned list is exactly the +/// requested crate list after validation. Experimental crates still require +/// the `experimental` flag during validation. /// /// Note: This function assumes that `specific_lints` have been validated via /// `validate_crate_names()` prior to calling. Callers must validate inputs @@ -64,18 +66,36 @@ pub fn resolve_crates( ) -> Vec { if !specific_lints.is_empty() { // Assumes names have been validated via validate_crate_names(). + debug!( + target: "whitaker_installer::resolution", + "using explicit lint crate selection: {:?}", + specific_lints + ); return specific_lints.to_vec(); } if options.individual_lints { let mut crates: Vec = LINT_CRATES.iter().map(|&c| CrateName::from(c)).collect(); if options.experimental { + debug!( + target: "whitaker_installer::resolution", + "including experimental lint crates because --experimental is enabled" + ); crates.extend(EXPERIMENTAL_LINT_CRATES.iter().map(|&c| CrateName::from(c))); + } else { + debug!( + target: "whitaker_installer::resolution", + "excluding experimental lint crates because --experimental is disabled" + ); } return crates; } // Default: suite only (experimental feature is handled by BuildConfig) + debug!( + target: "whitaker_installer::resolution", + "using suite crate selection; experimental feature handling is deferred to build config" + ); vec![CrateName::from(SUITE_CRATE)] } @@ -86,16 +106,41 @@ pub fn is_known_crate(name: &CrateName) -> bool { LINT_CRATES.contains(&s) || EXPERIMENTAL_LINT_CRATES.contains(&s) || s == SUITE_CRATE } +/// Check whether a crate name identifies an experimental lint crate. +#[must_use] +pub fn is_experimental_crate(name: &CrateName) -> bool { + EXPERIMENTAL_LINT_CRATES.contains(&name.as_str()) +} + /// Validate that all specified crate names are known lint crates. /// /// # Errors /// -/// Returns an error if any crate name is not recognised. -pub fn validate_crate_names(names: &[CrateName]) -> Result<()> { +/// Returns an error if any crate name is not recognised, or if an experimental +/// crate is requested without `--experimental`. +pub fn validate_crate_names(names: &[CrateName], options: &CrateResolutionOptions) -> Result<()> { for name in names { if !is_known_crate(name) { + debug!( + target: "whitaker_installer::resolution", + "rejecting unknown lint crate `{}`", + name.as_str() + ); return Err(InstallerError::LintCrateNotFound { name: name.clone() }); } + if is_experimental_crate(name) && !options.experimental { + debug!( + target: "whitaker_installer::resolution", + "rejecting experimental lint crate `{}` because --experimental is disabled", + name.as_str() + ); + return Err(InstallerError::ExperimentalLintRequiresFlag { name: name.clone() }); + } + debug!( + target: "whitaker_installer::resolution", + "accepted lint crate `{}` for resolution", + name.as_str() + ); } Ok(()) } @@ -112,14 +157,15 @@ mod tests { expect_lint: bool, expect_suite: bool, expect_bumpy_road: bool, + expect_experimental_lint: bool, } /// Parameterised tests for resolve_crates variants. #[rstest] - #[case::default_suite_only(ResolveCratesCase { individual_lints: false, experimental: false, expect_lint: false, expect_suite: true, expect_bumpy_road: false })] - #[case::individual_lints(ResolveCratesCase { individual_lints: true, experimental: false, expect_lint: true, expect_suite: false, expect_bumpy_road: true })] - #[case::individual_with_experimental(ResolveCratesCase { individual_lints: true, experimental: true, expect_lint: true, expect_suite: false, expect_bumpy_road: true })] - #[case::suite_with_experimental(ResolveCratesCase { individual_lints: false, experimental: true, expect_lint: false, expect_suite: true, expect_bumpy_road: false })] + #[case::default_suite_only(ResolveCratesCase { individual_lints: false, experimental: false, expect_lint: false, expect_suite: true, expect_bumpy_road: false, expect_experimental_lint: false })] + #[case::individual_lints(ResolveCratesCase { individual_lints: true, experimental: false, expect_lint: true, expect_suite: false, expect_bumpy_road: true, expect_experimental_lint: false })] + #[case::individual_with_experimental(ResolveCratesCase { individual_lints: true, experimental: true, expect_lint: true, expect_suite: false, expect_bumpy_road: true, expect_experimental_lint: true })] + #[case::suite_with_experimental(ResolveCratesCase { individual_lints: false, experimental: true, expect_lint: false, expect_suite: true, expect_bumpy_road: false, expect_experimental_lint: false })] fn resolve_crates_variants(#[case] case: ResolveCratesCase) { let options = CrateResolutionOptions { individual_lints: case.individual_lints, @@ -142,6 +188,11 @@ mod tests { case.expect_bumpy_road, "bumpy_road_function inclusion mismatch" ); + assert_eq!( + crates.contains(&CrateName::from("rstest_helper_should_be_fixture")), + case.expect_experimental_lint, + "rstest_helper_should_be_fixture inclusion mismatch" + ); } #[test] @@ -152,20 +203,52 @@ mod tests { } #[rstest] - #[case::valid(&["module_max_lines", "whitaker_suite"], true)] - #[case::bumpy_road_function(&["bumpy_road_function"], true)] - #[case::unknown(&["nonexistent_lint"], false)] - fn validate_crate_names_variants(#[case] names: &[&str], #[case] expect_ok: bool) { + #[case::valid(&["module_max_lines", "whitaker_suite"], false, true)] + #[case::bumpy_road_function(&["bumpy_road_function"], false, true)] + #[case::experimental_with_flag(&["rstest_helper_should_be_fixture"], true, true)] + #[case::experimental_without_flag(&["rstest_helper_should_be_fixture"], false, false)] + #[case::unknown(&["nonexistent_lint"], false, false)] + fn validate_crate_names_variants( + #[case] names: &[&str], + #[case] experimental: bool, + #[case] expect_ok: bool, + ) { let crate_names: Vec = names.iter().map(|&s| CrateName::from(s)).collect(); - let res = validate_crate_names(&crate_names); + let options = CrateResolutionOptions { + experimental, + ..CrateResolutionOptions::default() + }; + let res = validate_crate_names(&crate_names, &options); if expect_ok { assert!(res.is_ok()); } else { let err = res.expect_err("expected validation failure"); assert!( - matches!(&err, InstallerError::LintCrateNotFound { name } if *name == crate_names[0]), + matches!( + &err, + InstallerError::LintCrateNotFound { name } + | InstallerError::ExperimentalLintRequiresFlag { name } + if *name == crate_names[0] + ), "unexpected error: {err:?}" ); } } + + #[test] + fn validate_crate_names_returns_experimental_lint_requires_flag_error() { + let name = CrateName::from("rstest_helper_should_be_fixture"); + let options = CrateResolutionOptions { + experimental: false, + ..CrateResolutionOptions::default() + }; + + let error = validate_crate_names(std::slice::from_ref(&name), &options) + .expect_err("experimental lint should require explicit opt-in"); + + assert!( + matches!(&error, InstallerError::ExperimentalLintRequiresFlag { name: error_name } if *error_name == name), + "expected ExperimentalLintRequiresFlag, got {error:?}" + ); + } } diff --git a/installer/tests/behaviour_cli.rs b/installer/tests/behaviour_cli.rs index 0445e07a..0bd1f57c 100644 --- a/installer/tests/behaviour_cli.rs +++ b/installer/tests/behaviour_cli.rs @@ -14,10 +14,12 @@ use std::process::Command; pub(crate) use support::{CliWorld, cli_world}; use support::{ assert_cli_exits_successfully, assert_cli_exits_with_error, assert_dry_run_output_is_shown, - assert_installation_succeeds_or_is_skipped, assert_suite_library_is_staged, - assert_unknown_lint_message_is_shown, configure_dry_run_unknown_lint, - configure_dry_run_with_target_dir, configure_suite_install, is_toolchain_installed, - pinned_toolchain_channel, run_installer_cli, workspace_root, + assert_experimental_lint_dry_run_output_is_shown, + assert_experimental_lint_opt_in_message_is_shown, assert_installation_succeeds_or_is_skipped, + assert_suite_library_is_staged, assert_unknown_lint_message_is_shown, + configure_dry_run_experimental_lint, configure_dry_run_experimental_lint_with_opt_in, + configure_dry_run_unknown_lint, configure_dry_run_with_target_dir, configure_suite_install, + is_toolchain_installed, pinned_toolchain_channel, run_installer_cli, workspace_root, }; #[given("the installer is invoked with dry-run and a target directory")] @@ -30,6 +32,18 @@ fn given_dry_run_unknown_lint(cli_world: &CliWorld) { configure_dry_run_unknown_lint(cli_world); } +#[given("the installer is invoked with dry-run and an experimental lint")] +fn given_dry_run_experimental_lint(cli_world: &CliWorld) { + configure_dry_run_experimental_lint(cli_world); +} + +#[given( + "the installer is invoked with dry-run, an experimental lint, and experimental lints enabled" +)] +fn given_dry_run_experimental_lint_with_opt_in(cli_world: &CliWorld) { + configure_dry_run_experimental_lint_with_opt_in(cli_world); +} + #[given("the installer is invoked to a temporary directory")] fn given_suite_install(cli_world: &CliWorld) { configure_suite_install(cli_world); @@ -60,6 +74,16 @@ fn then_unknown_lint_message_is_shown(cli_world: &CliWorld) { assert_unknown_lint_message_is_shown(cli_world); } +#[then("an experimental lint opt-in message is shown")] +fn then_experimental_lint_opt_in_message_is_shown(cli_world: &CliWorld) { + assert_experimental_lint_opt_in_message_is_shown(cli_world); +} + +#[then("experimental lint dry-run output is shown")] +fn then_experimental_lint_dry_run_output_is_shown(cli_world: &CliWorld) { + assert_experimental_lint_dry_run_output_is_shown(cli_world); +} + #[then("installation succeeds or is skipped")] fn then_installation_succeeds_or_is_skipped(cli_world: &CliWorld) { assert_installation_succeeds_or_is_skipped(cli_world); diff --git a/installer/tests/behaviour_cli/scenarios.rs b/installer/tests/behaviour_cli/scenarios.rs index e6c3a757..c95291d8 100644 --- a/installer/tests/behaviour_cli/scenarios.rs +++ b/installer/tests/behaviour_cli/scenarios.rs @@ -19,3 +19,13 @@ fn scenario_dry_run_rejects_unknown_lint(cli_world: CliWorld) { fn scenario_install_suite_to_temp_dir(cli_world: CliWorld) { let _ = cli_world; } + +#[scenario(path = "tests/features/installer.feature", index = 21)] +fn scenario_dry_run_rejects_experimental_lint_without_opt_in(cli_world: CliWorld) { + let _ = cli_world; +} + +#[scenario(path = "tests/features/installer.feature", index = 22)] +fn scenario_dry_run_accepts_experimental_lint_with_opt_in(cli_world: CliWorld) { + let _ = cli_world; +} diff --git a/installer/tests/behaviour_cli/support.rs b/installer/tests/behaviour_cli/support.rs index 5dd82f85..1bae4eac 100644 --- a/installer/tests/behaviour_cli/support.rs +++ b/installer/tests/behaviour_cli/support.rs @@ -152,6 +152,32 @@ pub(super) fn configure_dry_run_unknown_lint(cli_world: &CliWorld) { ]); } +pub(super) fn configure_dry_run_experimental_lint(cli_world: &CliWorld) { + cli_world.args.replace(vec![ + "--dry-run".to_owned(), + "--lint".to_owned(), + "rstest_helper_should_be_fixture".to_owned(), + ]); +} + +pub(super) fn configure_dry_run_experimental_lint_with_opt_in(cli_world: &CliWorld) { + let Some(channel) = ensure_required_toolchain_available(cli_world) else { + return; + }; + + let target_dir = setup_temp_dir(cli_world); + cli_world.args.replace(vec![ + "--dry-run".to_owned(), + "--experimental".to_owned(), + "--toolchain".to_owned(), + channel, + "--target-dir".to_owned(), + target_dir, + "--lint".to_owned(), + "rstest_helper_should_be_fixture".to_owned(), + ]); +} + pub(super) fn configure_suite_install(cli_world: &CliWorld) { let Some(_channel) = ensure_required_toolchain_available(cli_world) else { return; @@ -262,6 +288,49 @@ pub(super) fn assert_unknown_lint_message_is_shown(cli_world: &CliWorld) { ); } +pub(super) fn assert_experimental_lint_opt_in_message_is_shown(cli_world: &CliWorld) { + if cli_world.skip_assertions.get() { + return; + } + + let output = get_output(cli_world); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!( + !stderr.contains("Dry run - no files will be modified"), + "dry-run configuration output should not be printed on experimental-lint error, stderr: {stderr}" + ); + assert!( + !stderr.contains("Crates to build:"), + "dry-run configuration output should not be printed on experimental-lint error, stderr: {stderr}" + ); + assert!( + stderr.contains( + "experimental lint crate rstest_helper_should_be_fixture requires --experimental" + ), + "unexpected stderr: {stderr}" + ); +} + +pub(super) fn assert_experimental_lint_dry_run_output_is_shown(cli_world: &CliWorld) { + if cli_world.skip_assertions.get() { + return; + } + + let output = get_output(cli_world); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(stderr.contains("Dry run - no files will be modified")); + assert!(stderr.contains("Crates to build:")); + assert!(stderr.contains("rstest_helper_should_be_fixture")); + assert!( + !stderr.contains( + "experimental lint crate rstest_helper_should_be_fixture requires --experimental" + ), + "experimental opt-in error should not be printed when --experimental is set, stderr: {stderr}" + ); +} + pub(super) fn assert_installation_succeeds_or_is_skipped(cli_world: &CliWorld) { if cli_world.skip_assertions.get() { return; diff --git a/installer/tests/behaviour_core.rs b/installer/tests/behaviour_core.rs index 4f7daaf6..e8567d8a 100644 --- a/installer/tests/behaviour_core.rs +++ b/installer/tests/behaviour_core.rs @@ -129,7 +129,9 @@ fn then_only_requested(crate_world: &CrateResolutionWorld) { #[derive(Default)] struct ValidationWorld { names: RefCell>, + experimental: Cell, result: Cell>, + error_message: RefCell>, } #[fixture] @@ -145,6 +147,18 @@ fn given_valid_names(validation_world: &ValidationWorld) { ]); } +#[given("a list containing an experimental crate name")] +fn given_experimental_name(validation_world: &ValidationWorld) { + validation_world + .names + .replace(vec![CrateName::from("rstest_helper_should_be_fixture")]); +} + +#[given("experimental validation is enabled")] +fn given_experimental_validation_enabled(validation_world: &ValidationWorld) { + validation_world.experimental.set(true); +} + #[given("a list containing an unknown crate name")] fn given_unknown_name(validation_world: &ValidationWorld) { validation_world @@ -155,8 +169,19 @@ fn given_unknown_name(validation_world: &ValidationWorld) { #[when("the names are validated")] fn when_names_validated(validation_world: &ValidationWorld) { let names = validation_world.names.borrow(); - let result = validate_crate_names(&names).is_ok(); - validation_world.result.set(Some(result)); + let options = CrateResolutionOptions { + experimental: validation_world.experimental.get(), + ..CrateResolutionOptions::default() + }; + match validate_crate_names(&names, &options) { + Ok(()) => validation_world.result.set(Some(true)), + Err(error) => { + validation_world + .error_message + .replace(Some(error.to_string())); + validation_world.result.set(Some(false)); + } + } } #[then("validation succeeds")] @@ -169,6 +194,19 @@ fn then_validation_fails(validation_world: &ValidationWorld) { assert_eq!(validation_world.result.get(), Some(false)); } +#[then("validation fails with an experimental opt-in error")] +fn then_validation_fails_experimental_opt_in(validation_world: &ValidationWorld) { + assert_eq!(validation_world.result.get(), Some(false)); + let error_message = validation_world.error_message.borrow(); + let error_message = error_message.as_ref().expect("error message should be set"); + assert!( + error_message.contains( + "experimental lint crate rstest_helper_should_be_fixture requires --experimental" + ), + "unexpected validation error: {error_message}" + ); +} + // --------------------------------------------------------------------------- // Toolchain detection world // --------------------------------------------------------------------------- @@ -342,3 +380,13 @@ fn scenario_reject_missing_channel(toolchain_world: ToolchainWorld) { fn scenario_generate_shell_snippets(snippet_world: SnippetWorld) { let _ = snippet_world; } + +#[scenario(path = "tests/features/installer.feature", index = 19)] +fn scenario_validate_experimental_names_with_opt_in(validation_world: ValidationWorld) { + let _ = validation_world; +} + +#[scenario(path = "tests/features/installer.feature", index = 20)] +fn scenario_reject_experimental_names_without_opt_in(validation_world: ValidationWorld) { + let _ = validation_world; +} diff --git a/installer/tests/features/installer.feature b/installer/tests/features/installer.feature index 78c4107e..b0b3626a 100644 --- a/installer/tests/features/installer.feature +++ b/installer/tests/features/installer.feature @@ -111,3 +111,26 @@ Feature: Whitaker lint library installer When the installer CLI is run Then installation succeeds or is skipped And output includes DYLINT_LIBRARY_PATH instructions + + Scenario: Validate experimental crate names with opt-in + Given a list containing an experimental crate name + And experimental validation is enabled + When the names are validated + Then validation succeeds + + Scenario: Reject experimental crate names without opt-in + Given a list containing an experimental crate name + When the names are validated + Then validation fails with an experimental opt-in error + + Scenario: Dry-run rejects experimental lint without opt-in + Given the installer is invoked with dry-run and an experimental lint + When the installer CLI is run + Then the CLI exits with an error + And an experimental lint opt-in message is shown + + Scenario: Dry-run accepts experimental lint with opt-in + Given the installer is invoked with dry-run, an experimental lint, and experimental lints enabled + When the installer CLI is run + Then the CLI exits successfully + And experimental lint dry-run output is shown diff --git a/suite/Cargo.toml b/suite/Cargo.toml index 9b0b593a..8dd0238c 100644 --- a/suite/Cargo.toml +++ b/suite/Cargo.toml @@ -26,6 +26,10 @@ dylint-driver = [ "dep:rustc_session", "dep:rustc_span", ] +experimental-rstest-helper-should-be-fixture = [ + "dylint-driver", + "dep:rstest_helper_should_be_fixture", +] [dependencies] dylint_linting = { workspace = true, optional = true } @@ -42,6 +46,7 @@ module_max_lines = { path = "../crates/module_max_lines", optional = true, featu no_unwrap_or_else_panic = { path = "../crates/no_unwrap_or_else_panic", optional = true, features = ["dylint-driver", "constituent"] } no_std_fs_operations = { path = "../crates/no_std_fs_operations", optional = true, features = ["dylint-driver", "constituent"] } bumpy_road_function = { path = "../crates/bumpy_road_function", optional = true, features = ["dylint-driver", "constituent"] } +rstest_helper_should_be_fixture = { path = "../crates/rstest_helper_should_be_fixture", optional = true, features = ["dylint-driver", "constituent"] } [dev-dependencies] rstest = { workspace = true } diff --git a/suite/src/driver.rs b/suite/src/driver.rs index 50a9cd5a..daaccc52 100644 --- a/suite/src/driver.rs +++ b/suite/src/driver.rs @@ -14,12 +14,14 @@ use module_must_have_inner_docs::ModuleMustHaveInnerDocs; use no_expect_outside_tests::NoExpectOutsideTests; use no_std_fs_operations::NoStdFsOperations; use no_unwrap_or_else_panic::NoUnwrapOrElsePanic; +#[cfg(feature = "experimental-rstest-helper-should-be-fixture")] +use rstest_helper_should_be_fixture::RstestHelperShouldBeFixture; use test_must_not_have_example::TestMustNotHaveExample; dylint_library!(); macro_rules! define_suite_pass { - () => { + ($($experimental_pass:tt)*) => { rustc_lint::late_lint_methods!( declare_combined_late_lint_pass, [SuitePass, [ @@ -32,13 +34,20 @@ macro_rules! define_suite_pass { NoUnwrapOrElsePanic: no_unwrap_or_else_panic::NoUnwrapOrElsePanic::default(), NoStdFsOperations: no_std_fs_operations::NoStdFsOperations::default(), BumpyRoadFunction: bumpy_road_function::BumpyRoadFunction::default(), + $($experimental_pass)* ]] ); }; } +#[cfg(not(feature = "experimental-rstest-helper-should-be-fixture"))] define_suite_pass!(); +#[cfg(feature = "experimental-rstest-helper-should-be-fixture")] +define_suite_pass!( + RstestHelperShouldBeFixture: rstest_helper_should_be_fixture::RstestHelperShouldBeFixture::default(), +); + /// Registers the suite lints into the provided lint store. /// /// Callers should initialise configuration with diff --git a/suite/src/lints.rs b/suite/src/lints.rs index 1ba3aafe..b27c7774 100644 --- a/suite/src/lints.rs +++ b/suite/src/lints.rs @@ -47,6 +47,11 @@ pub const SUITE_LINTS: &[LintDescriptor] = &[ name: "bumpy_road_function", crate_name: "bumpy_road_function", }, + #[cfg(feature = "experimental-rstest-helper-should-be-fixture")] + LintDescriptor { + name: "rstest_helper_should_be_fixture", + crate_name: "rstest_helper_should_be_fixture", + }, ]; #[cfg(feature = "dylint-driver")] @@ -64,6 +69,8 @@ pub const SUITE_LINT_DECLS: &[&Lint] = &[ no_unwrap_or_else_panic::NO_UNWRAP_OR_ELSE_PANIC, no_std_fs_operations::NO_STD_FS_OPERATIONS, bumpy_road_function::BUMPY_ROAD_FUNCTION, + #[cfg(feature = "experimental-rstest-helper-should-be-fixture")] + rstest_helper_should_be_fixture::RSTEST_HELPER_SHOULD_BE_FIXTURE, ]; /// Returns an iterator over the canonical lint names in suite order. @@ -86,6 +93,8 @@ pub const SUITE_LINT_DECLS: &[&Lint] = &[ /// ] { /// assert!(names.contains(&expected)); /// } +/// #[cfg(feature = "experimental-rstest-helper-should-be-fixture")] +/// assert!(names.contains(&"rstest_helper_should_be_fixture")); /// ``` #[must_use = "Discarding the iterator hides suite wiring errors"] pub fn suite_lint_names() -> impl Iterator {