diff --git a/AGENTS.md b/AGENTS.md index c360a0f..d22a1c6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,14 +56,14 @@ Every Rust source file must start with: - **Newtypes** for domain types (e.g., `ApiIdent`, `VcsRevision`, `GitCommitHash`, `ApiSpecFileName`) - **Builder patterns** for complex construction (e.g., `ManagedApi` with `with_extra_validation`, `with_git_stub_storage`) - **Type states** encoded in generics when state transitions matter -- **Lifetimes** used extensively to avoid cloning (e.g., `Problem<'a>`, `Resolution<'a>`, `Fix<'a>`) +- **Lifetimes** used extensively to avoid cloning (e.g., `VersionProblem<'a>`, `Resolution<'a>`, `Fix<'a>`) - **Restricted visibility**: Use `pub(crate)` and `pub(super)` liberally - **Non-exhaustive**: Consider forward compatibility for public error types ### Error handling - Use `thiserror` for error types with `#[derive(Error)]`. -- Group errors by category with specific enums (e.g., `Problem`, `Note`). +- Group errors by category with specific enums (e.g., `VersionProblem`, `NonVersionProblem`, `Note`). - Provide rich error context using `anyhow::Context`. - Two-tier model: - **Problems**: User-visible issues that may be fixable or unfixable. @@ -179,17 +179,27 @@ pub enum ResolutionKind { NewLocally, // Versioned, only in current branch. } -pub enum Problem<'a> { - LocalSpecFileOrphaned { ... }, +// Per-(api, version) problems: anything tied to a specific supported +// version. The bulk of the variants live here. +pub enum VersionProblem<'a> { BlessedVersionMissingLocal { ... }, BlessedVersionBroken { ... }, LockstepStale { ... }, LocalVersionMissingLocal { ... }, // ... more variants } + +// Problems not tied to a specific version: orphaned files, unparseable +// files, and the "latest" symlink for versioned APIs. +pub enum NonVersionProblem<'a> { + LocalSpecFileOrphaned { ... }, + UnparseableLocalFile { ... }, + LatestLinkMissing { ... }, + LatestLinkStale { ... }, +} ``` -Problems are either **fixable** (tool can auto-correct) or **unfixable** (require manual intervention). +Both problem enums are either **fixable** (tool can auto-correct) or **unfixable** (require manual intervention). Splitting per-version problems out lets rendering code that needs per-(api, version) context (e.g., compatibility-issue dedup) require it at the type level. ### Key design principles @@ -213,7 +223,7 @@ crates/ │ │ ├── apis.rs # ManagedApiConfig, ManagedApi, ManagedApis │ │ ├── cmd/ # CLI commands (dispatch, check, generate, list, debug) │ │ ├── environment.rs # Environment configuration and resolution -│ │ ├── resolved.rs # Resolution logic, Problem enum, Fix enum +│ │ ├── resolved.rs # Resolution logic, VersionProblem / NonVersionProblem / Fix enums │ │ ├── compatibility.rs # Wire compatibility checking via drift │ │ ├── validation.rs # OpenAPI document validation │ │ ├── vcs/ # VCS abstraction (RepoVcs: Git/Jujutsu dispatch) diff --git a/crates/dropshot-api-manager/src/cmd/debug.rs b/crates/dropshot-api-manager/src/cmd/debug.rs index 1195997..054b980 100644 --- a/crates/dropshot-api-manager/src/cmd/debug.rs +++ b/crates/dropshot-api-manager/src/cmd/debug.rs @@ -59,7 +59,7 @@ pub(crate) fn debug_impl( for note in resolved.notes() { println!("NOTE: {}", note); } - for problem in resolved.general_problems() { + for problem in resolved.orphaned_and_unparseable() { println!("PROBLEM: {}", problem); } diff --git a/crates/dropshot-api-manager/src/cmd/generate.rs b/crates/dropshot-api-manager/src/cmd/generate.rs index e209db6..1c0b2e4 100644 --- a/crates/dropshot-api-manager/src/cmd/generate.rs +++ b/crates/dropshot-api-manager/src/cmd/generate.rs @@ -6,11 +6,12 @@ use crate::{ environment::{BlessedSource, GeneratedSource, ResolvedEnv}, output::{ CheckResult, OutputOpts, Styles, display_api_spec_version, - display_load_problems, display_resolution, display_resolution_problems, + display_load_problems, display_non_version_problems, + display_resolution, display_version_problems, headers::{self, *}, plural, }, - resolved::{Problem, Resolved}, + resolved::{Fix, Resolved}, }; use anyhow::{Result, anyhow, bail}; use owo_colors::OwoColorize; @@ -118,8 +119,7 @@ fn generate_impl_inner( checked above" ); - let problems: Vec<_> = resolution.problems().collect(); - if problems.is_empty() { + if !resolution.has_problems() { writeln!( writer, "{:>HEADER_WIDTH$} {}", @@ -135,10 +135,10 @@ fn generate_impl_inner( display_api_spec_version(api, version, styles, resolution), )?; - fix_problems( + apply_fixes( writer, env, - problems, + resolution.problems().map(|p| expect_fix(p.fix())), styles, &mut num_updated, &mut num_errors, @@ -154,10 +154,10 @@ fn generate_impl_inner( ident.style(styles.filename), )?; - fix_problems( + apply_fixes( writer, env, - std::iter::once(symlink_problem), + std::iter::once(expect_fix(symlink_problem.fix())), styles, &mut num_updated, &mut num_errors, @@ -173,11 +173,10 @@ fn generate_impl_inner( } // Fix problems not associated with any supported version, if any. - let general_problems: Vec<_> = resolved.general_problems().collect(); - fix_problems( + apply_fixes( writer, env, - general_problems, + resolved.orphaned_and_unparseable().map(|p| expect_fix(p.fix())), styles, &mut num_updated, &mut num_errors, @@ -217,10 +216,11 @@ fn generate_impl_inner( display_load_problems(writer, &errors, styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files_recheck); - let general_problems: Vec<_> = resolved.general_problems().collect(); - nproblems += general_problems.len(); - if !general_problems.is_empty() { - display_resolution_problems(writer, env, general_problems, styles)?; + let orphaned_and_unparseable: Vec<_> = + resolved.orphaned_and_unparseable().collect(); + nproblems += orphaned_and_unparseable.len(); + if !orphaned_and_unparseable.is_empty() { + display_non_version_problems(writer, orphaned_and_unparseable, styles)?; } for api in apis.iter_apis() { let ident = api.ident(); @@ -237,7 +237,7 @@ fn generate_impl_inner( (this is a bug)", ident, version )?; - display_resolution_problems(writer, env, problems, styles)?; + display_version_problems(writer, env, problems, styles)?; } } @@ -248,9 +248,8 @@ fn generate_impl_inner( "found unexpected problem with API {} symlink (this is a bug)", ident )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -314,21 +313,23 @@ fn print_final_status( Ok(()) } -fn fix_problems<'a, T>( +/// Unwrap a `fix()` result at an `apply_fixes` call site. +/// +/// Precondition: [`Resolved::has_unfixable_problems`] returned false, so +/// every reachable problem must produce a fix. +fn expect_fix<'a>(fix: Option>) -> Fix<'a> { + fix.expect("problem is fixable per has_unfixable_problems guard") +} + +fn apply_fixes<'a>( writer: &mut dyn io::Write, env: &ResolvedEnv, - problems: T, + fixes: impl IntoIterator>, styles: &Styles, num_updated: &mut usize, num_errors: &mut usize, -) -> io::Result<()> -where - T: IntoIterator>, -{ - for p in problems { - // We should have already bailed out if there were any unfixable - // problems. - let fix = p.fix().expect("attempting to fix unfixable problem"); +) -> io::Result<()> { + for fix in fixes { match fix.execute(env) { Ok(steps) => { *num_updated += 1; diff --git a/crates/dropshot-api-manager/src/output.rs b/crates/dropshot-api-manager/src/output.rs index 00084c3..b87d04d 100644 --- a/crates/dropshot-api-manager/src/output.rs +++ b/crates/dropshot-api-manager/src/output.rs @@ -4,7 +4,10 @@ use crate::{ FAILURE_EXIT_CODE, NEEDS_UPDATE_EXIT_CODE, apis::{ManagedApi, ManagedApis}, environment::{ErrorAccumulator, ResolvedEnv}, - resolved::{Problem, Resolution, ResolutionKind, Resolved}, + resolved::{ + Fix, NonVersionProblem, Resolution, ResolutionKind, Resolved, + VersionProblem, + }, validation::CheckStale, }; use anyhow::bail; @@ -294,7 +297,7 @@ pub fn display_resolution( let mut num_fresh = 0; let mut num_stale = 0; let mut num_failed = 0; - let mut num_general_problems = 0; + let mut num_non_version_problems = 0; // Print problems associated with a supported API version // (i.e., one of the expected OpenAPI documents). @@ -321,16 +324,15 @@ pub fn display_resolution( if let Some(symlink_problem) = resolved.symlink_problem(ident) { if symlink_problem.is_fixable() { - num_general_problems += 1; + num_non_version_problems += 1; writeln!( writer, "{:>HEADER_WIDTH$} {} \"latest\" symlink", STALE.style(styles.warning_header), ident.style(styles.filename), )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -342,9 +344,8 @@ pub fn display_resolution( FAILURE.style(styles.failure_header), ident.style(styles.filename), )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -361,8 +362,9 @@ pub fn display_resolution( } // Print problems not associated with any supported version, if any. - let general_problems: Vec<_> = resolved.general_problems().collect(); - num_general_problems += if !general_problems.is_empty() { + let orphaned_and_unparseable: Vec<_> = + resolved.orphaned_and_unparseable().collect(); + num_non_version_problems += if !orphaned_and_unparseable.is_empty() { writeln!( writer, "\n{:>HEADER_WIDTH$} problems not associated with a specific \ @@ -370,10 +372,12 @@ pub fn display_resolution( "Other".style(styles.warning_header), )?; - let (fixable, unfixable): (Vec<&Problem>, Vec<&Problem>) = - general_problems.iter().partition(|p| p.is_fixable()); + let (fixable, unfixable): ( + Vec<&NonVersionProblem>, + Vec<&NonVersionProblem>, + ) = orphaned_and_unparseable.iter().partition(|p| p.is_fixable()); num_failed += unfixable.len(); - display_resolution_problems(writer, env, general_problems, styles)?; + display_non_version_problems(writer, orphaned_and_unparseable, styles)?; fixable.len() } else { 0 @@ -399,7 +403,7 @@ pub fn display_resolution( // Print a summary line. let status_header = if num_failed > 0 { FAILURE.style(styles.failure_header) - } else if num_stale > 0 || num_general_problems > 0 { + } else if num_stale > 0 || num_non_version_problems > 0 { STALE.style(styles.warning_header) } else { SUCCESS.style(styles.success_header) @@ -416,8 +420,8 @@ pub fn display_resolution( num_fresh.style(styles.bold), num_stale.style(styles.bold), num_failed.style(styles.bold), - num_general_problems.style(styles.bold), - plural::problems(num_general_problems), + num_non_version_problems.style(styles.bold), + plural::problems(num_non_version_problems), )?; if num_failed > 0 { writeln!( @@ -427,7 +431,7 @@ pub fn display_resolution( format!("{} generate", env.command).style(styles.bold) )?; Ok(CheckResult::Failures) - } else if num_stale > 0 || num_general_problems > 0 { + } else if num_stale > 0 || num_non_version_problems > 0 { writeln!( writer, "{:>HEADER_WIDTH$} (run {} to update)", @@ -496,46 +500,35 @@ fn summarize_one( display_api_spec_version(api, version, styles, resolution), )?; - display_resolution_problems(writer, env, problems, styles)?; + display_version_problems(writer, env, problems, styles)?; } Ok(()) } -/// Print a formatted list of Problems to `writer`. -pub fn display_resolution_problems<'a, T>( +/// Print a formatted list of per-(api, version) [`VersionProblem`]s to +/// `writer`, including any compatibility-issue diffs and fix descriptions. +pub fn display_version_problems<'a, T>( writer: &mut dyn io::Write, env: &ResolvedEnv, problems: T, styles: &Styles, ) -> io::Result<()> where - T: IntoIterator>, + T: IntoIterator>, { for p in problems.into_iter() { - let subheader_width = HEADER_WIDTH + 4; - let first_indent = format!( - "{:>subheader_width$}: ", - if p.is_fixable() { - "problem".style(styles.warning_header) - } else { - "error".style(styles.failure_header) - } - ); - let more_indent = " ".repeat(subheader_width + 2); - writeln!( - writer, - "{}", - textwrap::fill( - &InlineErrorChain::new(&p).to_string(), - textwrap::Options::with_termwidth() - .initial_indent(&first_indent) - .subsequent_indent(&more_indent) - ) - )?; + write_problem_header(writer, p, p.is_fixable(), styles)?; + + // Body indent used by the nested BlessedVersionBroken issue list. + // Mirrors the continuation indent inside the problem header so + // wrapped issue lines line up under the header text. + let more_indent = " ".repeat(HEADER_WIDTH + 4 + 2); // For BlessedVersionBroken, print each item separately, along with a // diff between blessed and generated versions. - if let Problem::BlessedVersionBroken { compatibility_issues } = &p { + if let VersionProblem::BlessedVersionBroken { compatibility_issues } = + &p + { for issue in compatibility_issues { // Print each compatibility issue on a new line, prefixed with // "- ". @@ -578,7 +571,7 @@ where // For BlessedLatestVersionBytewiseMismatch, show a diff between blessed // and generated versions even though there's no fix. - if let Problem::BlessedLatestVersionBytewiseMismatch { + if let VersionProblem::BlessedLatestVersionBytewiseMismatch { blessed, generated, } = p @@ -606,28 +599,11 @@ where continue; }; - let first_indent = format!( - "{:>subheader_width$}: ", - "fix".style(styles.warning_header) - ); - let fix_str = fix.to_string(); - let steps = fix_str.trim_end().split("\n"); - for s in steps { - writeln!( - writer, - "{}", - textwrap::fill( - &format!("will {}", s), - textwrap::Options::with_termwidth() - .initial_indent(&first_indent) - .subsequent_indent(&more_indent) - ) - )?; - } + write_fix_summary(writer, &fix, styles)?; // When possible, print a useful diff of changes. let do_diff = match p { - Problem::LockstepStale { found, generated } => { + VersionProblem::LockstepStale { found, generated } => { let diff = TextDiff::from_lines( found.contents(), generated.contents(), @@ -639,7 +615,7 @@ where .join(generated.spec_file_name().path()); Some((diff, path1, path2)) } - Problem::ExtraFileStale { + VersionProblem::ExtraFileStale { check_stale: CheckStale::Modified { full_path, actual, expected }, .. @@ -647,7 +623,7 @@ where let diff = TextDiff::from_lines(actual, expected); Some((diff, full_path.clone(), full_path.clone())) } - Problem::LocalVersionStale { spec_files, generated } + VersionProblem::LocalVersionStale { spec_files, generated } if spec_files.len() == 1 => { let diff = TextDiff::from_lines( @@ -685,6 +661,85 @@ where Ok(()) } +/// Print a formatted list of [`NonVersionProblem`]s to `writer`. +/// +/// None of these variants have associated diffs, so this is just a header + +/// fix. +pub fn display_non_version_problems<'a, T>( + writer: &mut dyn io::Write, + problems: T, + styles: &Styles, +) -> io::Result<()> +where + T: IntoIterator>, +{ + for p in problems.into_iter() { + write_problem_header(writer, p, p.is_fixable(), styles)?; + if let Some(fix) = p.fix() { + write_fix_summary(writer, &fix, styles)?; + } + } + Ok(()) +} + +/// Write the `problem:` / `error:` header line for a single problem, wrapping +/// the error chain to the terminal width. +fn write_problem_header( + writer: &mut dyn io::Write, + error: &dyn std::error::Error, + is_fixable: bool, + styles: &Styles, +) -> io::Result<()> { + let subheader_width = HEADER_WIDTH + 4; + let first_indent = format!( + "{:>subheader_width$}: ", + if is_fixable { + "problem".style(styles.warning_header) + } else { + "error".style(styles.failure_header) + } + ); + let more_indent = " ".repeat(subheader_width + 2); + writeln!( + writer, + "{}", + textwrap::fill( + &InlineErrorChain::new(error).to_string(), + textwrap::Options::with_termwidth() + .initial_indent(&first_indent) + .subsequent_indent(&more_indent) + ) + ) +} + +/// Write the `fix:` line(s) for a single fix, splitting multi-step fixes into +/// separate `will ...` lines that share the column structure of +/// [`write_problem_header`]. +fn write_fix_summary( + writer: &mut dyn io::Write, + fix: &Fix<'_>, + styles: &Styles, +) -> io::Result<()> { + let subheader_width = HEADER_WIDTH + 4; + let first_indent = + format!("{:>subheader_width$}: ", "fix".style(styles.warning_header)); + let more_indent = " ".repeat(subheader_width + 2); + let fix_str = fix.to_string(); + for s in fix_str.trim_end().split("\n") { + writeln!( + writer, + "{}", + textwrap::fill( + &format!("will {}", s), + textwrap::Options::with_termwidth() + .initial_indent(&first_indent) + .subsequent_indent(&more_indent) + ) + )?; + } + Ok(()) +} + /// Adapter for [`Error`]s that provides a [`std::fmt::Display`] implementation /// that print the full chain of error sources, separated by `: `. pub struct InlineErrorChain<'a>(&'a dyn std::error::Error); diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 60b4137..6039f5d 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -73,19 +73,21 @@ pub enum Note { /// and local spec files for a particular API pub struct Resolution<'a> { kind: ResolutionKind, - problems: Vec>, + problems: Vec>, } impl<'a> Resolution<'a> { - pub fn new_lockstep(problems: Vec>) -> Resolution<'a> { + pub fn new_lockstep(problems: Vec>) -> Resolution<'a> { Resolution { kind: ResolutionKind::Lockstep, problems } } - pub fn new_blessed(problems: Vec>) -> Resolution<'a> { + pub fn new_blessed(problems: Vec>) -> Resolution<'a> { Resolution { kind: ResolutionKind::Blessed, problems } } - pub fn new_new_locally(problems: Vec>) -> Resolution<'a> { + pub fn new_new_locally( + problems: Vec>, + ) -> Resolution<'a> { Resolution { kind: ResolutionKind::NewLocally, problems } } @@ -94,7 +96,7 @@ impl<'a> Resolution<'a> { } /// Add a problem to this resolution. - pub fn add_problem(&mut self, problem: Problem<'a>) { + pub fn add_problem(&mut self, problem: VersionProblem<'a>) { self.problems.push(problem); } @@ -102,7 +104,9 @@ impl<'a> Resolution<'a> { self.problems().any(|p| !p.is_fixable()) } - pub fn problems(&self) -> impl Iterator> + '_ { + pub fn problems( + &self, + ) -> impl Iterator> + '_ { self.problems.iter() } @@ -132,10 +136,12 @@ impl Display for ResolutionKind { } } -/// Identifies the kind of a `Problem` without carrying borrowed data. +/// Identifies the kind of a `VersionProblem` or `NonVersionProblem` without +/// carrying borrowed data. /// -/// Each variant corresponds 1:1 to a `Problem` variant. The exhaustive -/// match in `Problem::kind` ensures that adding a new `Problem` variant +/// Each variant corresponds 1:1 to a variant of one of the two problem +/// enums. Exhaustive matches in `VersionProblem::kind` and +/// `NonVersionProblem::kind` ensure that adding a new problem variant /// without updating this enum causes a compile error. #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] #[expect(missing_docs)] @@ -165,7 +171,8 @@ pub enum ProblemKind { GitStubFirstCommitUnknown, } -/// Owned summary of a `Problem` for test assertions. +/// Owned summary of a `VersionProblem` or `NonVersionProblem` for test +/// assertions. /// /// Contains just enough information to identify a problem: which API it /// belongs to, which version (if any), and its [`ProblemKind`]. Because all @@ -204,10 +211,15 @@ impl ProblemSummary { } /// Describes a problem resolving the blessed spec(s), generated spec(s), and -/// local spec files for a particular API. +/// local spec files for an API that is *not* tied to a specific supported +/// version. +/// +/// Per-(api, version) problems live in [`VersionProblem`] instead. Splitting +/// the two means that we can establish at compile time that, e.g., symlinks can +/// never have `BlessedVersionBroken`, and rendering code that needs +/// version-specific context can require it unconditionally. #[derive(Debug, Error)] -pub enum Problem<'a> { - // These problems are not associated with any *supported* version of an API. +pub enum NonVersionProblem<'a> { #[error( "A local OpenAPI document was found that does not correspond to a \ supported version of this API: {spec_file_name}. This is unusual, \ @@ -227,8 +239,31 @@ pub enum Problem<'a> { )] UnparseableLocalFile { unparseable_file: UnparseableFile }, - // All other problems are associated with specific supported versions of an - // API. + #[error("\"Latest\" symlink for versioned API {api_ident:?} is missing")] + LatestLinkMissing { + api_ident: ApiIdent, + link: &'a VersionedApiSpecFileName, + }, + + #[error( + "\"Latest\" symlink for versioned API {api_ident:?} is stale: points \ + to {}, but should be {}", + found.basename(), + link.basename(), + )] + LatestLinkStale { + api_ident: ApiIdent, + found: &'a VersionedApiSpecFileName, + link: &'a VersionedApiSpecFileName, + }, +} + +/// Describes a problem with a specific supported (api, version) pair. +/// +/// Companion to [`NonVersionProblem`], which holds problems that aren't tied to +/// a specific version. +#[derive(Debug, Error)] +pub enum VersionProblem<'a> { #[error( "This version is blessed, and it's a supported version, but it's \ missing a local OpenAPI document. This can happen with dependent \ @@ -357,24 +392,6 @@ pub enum Problem<'a> { check_stale: CheckStale, }, - #[error("\"Latest\" symlink for versioned API {api_ident:?} is missing")] - LatestLinkMissing { - api_ident: ApiIdent, - link: &'a VersionedApiSpecFileName, - }, - - #[error( - "\"Latest\" symlink for versioned API {api_ident:?} is stale: points \ - to {}, but should be {}", - found.basename(), - link.basename(), - )] - LatestLinkStale { - api_ident: ApiIdent, - found: &'a VersionedApiSpecFileName, - link: &'a VersionedApiSpecFileName, - }, - #[error( "Blessed non-latest version is stored as a full JSON file. This can \ be converted to a Git stub. This tool can perform the conversion for \ @@ -433,68 +450,113 @@ pub enum Problem<'a> { }, } -impl<'a> Problem<'a> { +impl<'a> NonVersionProblem<'a> { /// Returns the discriminant of this problem as a [`ProblemKind`]. /// - /// The match is exhaustive (no wildcard), so adding a new `Problem` + /// The match is exhaustive (no wildcard), so adding a new `NonVersionProblem` /// variant without updating this method causes a compile error. pub fn kind(&self) -> ProblemKind { match self { - Problem::LocalSpecFileOrphaned { .. } => { + NonVersionProblem::LocalSpecFileOrphaned { .. } => { ProblemKind::LocalSpecFileOrphaned } - Problem::UnparseableLocalFile { .. } => { + NonVersionProblem::UnparseableLocalFile { .. } => { ProblemKind::UnparseableLocalFile } - Problem::BlessedVersionMissingLocal { .. } => { + NonVersionProblem::LatestLinkMissing { .. } => { + ProblemKind::LatestLinkMissing + } + NonVersionProblem::LatestLinkStale { .. } => { + ProblemKind::LatestLinkStale + } + } + } + + pub fn is_fixable(&self) -> bool { + self.fix().is_some() + } + + pub fn fix(&'a self) -> Option> { + match self { + NonVersionProblem::LocalSpecFileOrphaned { spec_file_name } => { + Some(Fix::DeleteFiles { + files: DisplayableVec(vec![spec_file_name.clone().into()]), + }) + } + NonVersionProblem::UnparseableLocalFile { unparseable_file } => { + Some(Fix::DeleteUnparseableFile { + path: unparseable_file.path.clone(), + }) + } + NonVersionProblem::LatestLinkStale { api_ident, link, .. } + | NonVersionProblem::LatestLinkMissing { api_ident, link } => { + Some(Fix::UpdateSymlink { api_ident, link }) + } + } + } +} + +impl<'a> VersionProblem<'a> { + /// Returns the discriminant of this problem as a [`ProblemKind`]. + /// + /// The match is exhaustive (no wildcard), so adding a new + /// `VersionProblem` variant without updating this method causes a compile + /// error. + pub fn kind(&self) -> ProblemKind { + match self { + VersionProblem::BlessedVersionMissingLocal { .. } => { ProblemKind::BlessedVersionMissingLocal } - Problem::BlessedVersionExtraLocalSpec { .. } => { + VersionProblem::BlessedVersionExtraLocalSpec { .. } => { ProblemKind::BlessedVersionExtraLocalSpec } - Problem::BlessedVersionCompareError { .. } => { + VersionProblem::BlessedVersionCompareError { .. } => { ProblemKind::BlessedVersionCompareError } - Problem::BlessedVersionBroken { .. } => { + VersionProblem::BlessedVersionBroken { .. } => { ProblemKind::BlessedVersionBroken } - Problem::BlessedLatestVersionBytewiseMismatch { .. } => { + VersionProblem::BlessedLatestVersionBytewiseMismatch { .. } => { ProblemKind::BlessedLatestVersionBytewiseMismatch } - Problem::LockstepMissingLocal { .. } => { + VersionProblem::LockstepMissingLocal { .. } => { ProblemKind::LockstepMissingLocal } - Problem::LockstepStale { .. } => ProblemKind::LockstepStale, - Problem::LocalVersionMissingLocal { .. } => { + VersionProblem::LockstepStale { .. } => ProblemKind::LockstepStale, + VersionProblem::LocalVersionMissingLocal { .. } => { ProblemKind::LocalVersionMissingLocal } - Problem::LocalVersionExtra { .. } => ProblemKind::LocalVersionExtra, - Problem::LocalVersionStale { .. } => ProblemKind::LocalVersionStale, - Problem::GeneratedSourceMissing { .. } => { + VersionProblem::LocalVersionExtra { .. } => { + ProblemKind::LocalVersionExtra + } + VersionProblem::LocalVersionStale { .. } => { + ProblemKind::LocalVersionStale + } + VersionProblem::GeneratedSourceMissing { .. } => { ProblemKind::GeneratedSourceMissing } - Problem::GeneratedValidationError { .. } => { + VersionProblem::GeneratedValidationError { .. } => { ProblemKind::GeneratedValidationError } - Problem::ExtraFileStale { .. } => ProblemKind::ExtraFileStale, - Problem::LatestLinkMissing { .. } => ProblemKind::LatestLinkMissing, - Problem::LatestLinkStale { .. } => ProblemKind::LatestLinkStale, - Problem::BlessedVersionShouldBeGitStub { .. } => { + VersionProblem::ExtraFileStale { .. } => { + ProblemKind::ExtraFileStale + } + VersionProblem::BlessedVersionShouldBeGitStub { .. } => { ProblemKind::BlessedVersionShouldBeGitStub } - Problem::GitStubShouldBeJson { .. } => { + VersionProblem::GitStubShouldBeJson { .. } => { ProblemKind::GitStubShouldBeJson } - Problem::BlessedVersionCorruptedLocal { .. } => { + VersionProblem::BlessedVersionCorruptedLocal { .. } => { ProblemKind::BlessedVersionCorruptedLocal } - Problem::DuplicateLocalFile { .. } => { + VersionProblem::DuplicateLocalFile { .. } => { ProblemKind::DuplicateLocalFile } - Problem::GitStubCommitStale { .. } => { + VersionProblem::GitStubCommitStale { .. } => { ProblemKind::GitStubCommitStale } - Problem::GitStubFirstCommitUnknown { .. } => { + VersionProblem::GitStubFirstCommitUnknown { .. } => { ProblemKind::GitStubFirstCommitUnknown } } @@ -506,36 +568,32 @@ impl<'a> Problem<'a> { pub fn fix(&'a self) -> Option> { match self { - Problem::LocalSpecFileOrphaned { spec_file_name } => { - Some(Fix::DeleteFiles { - files: DisplayableVec(vec![spec_file_name.clone().into()]), - }) - } - Problem::BlessedVersionMissingLocal { blessed, git_stub } => { - Some(Fix::RestoreFromBlessed { - blessed, - git_stub: git_stub.as_ref(), - }) - } - Problem::BlessedVersionExtraLocalSpec { spec_file_name } => { + VersionProblem::BlessedVersionMissingLocal { + blessed, + git_stub, + } => Some(Fix::RestoreFromBlessed { + blessed, + git_stub: git_stub.as_ref(), + }), + VersionProblem::BlessedVersionExtraLocalSpec { spec_file_name } => { Some(Fix::DeleteFiles { files: DisplayableVec(vec![spec_file_name.clone().into()]), }) } - Problem::BlessedVersionCompareError { .. } => None, - Problem::BlessedVersionBroken { .. } => None, - Problem::BlessedLatestVersionBytewiseMismatch { .. } => None, - Problem::LockstepMissingLocal { generated } - | Problem::LockstepStale { generated, .. } => { + VersionProblem::BlessedVersionCompareError { .. } => None, + VersionProblem::BlessedVersionBroken { .. } => None, + VersionProblem::BlessedLatestVersionBytewiseMismatch { .. } => None, + VersionProblem::LockstepMissingLocal { generated } + | VersionProblem::LockstepStale { generated, .. } => { Some(Fix::UpdateLockstepFile { generated }) } - Problem::LocalVersionMissingLocal { generated } => { + VersionProblem::LocalVersionMissingLocal { generated } => { Some(Fix::UpdateVersionedFiles { old: DisplayableVec(Vec::new()), generated, }) } - Problem::LocalVersionExtra { spec_file_names } => { + VersionProblem::LocalVersionExtra { spec_file_names } => { Some(Fix::DeleteFiles { files: DisplayableVec( spec_file_names @@ -547,7 +605,7 @@ impl<'a> Problem<'a> { ), }) } - Problem::LocalVersionStale { spec_files, generated } => { + VersionProblem::LocalVersionStale { spec_files, generated } => { Some(Fix::UpdateVersionedFiles { old: DisplayableVec( spec_files.iter().map(|s| s.spec_file_name()).collect(), @@ -555,22 +613,19 @@ impl<'a> Problem<'a> { generated, }) } - Problem::GeneratedSourceMissing { .. } => None, - Problem::GeneratedValidationError { .. } => None, - Problem::ExtraFileStale { path, check_stale, .. } => { + VersionProblem::GeneratedSourceMissing { .. } => None, + VersionProblem::GeneratedValidationError { .. } => None, + VersionProblem::ExtraFileStale { path, check_stale, .. } => { Some(Fix::UpdateExtraFile { path, check_stale }) } - Problem::LatestLinkStale { api_ident, link, .. } - | Problem::LatestLinkMissing { api_ident, link } => { - Some(Fix::UpdateSymlink { api_ident, link }) - } - Problem::BlessedVersionShouldBeGitStub { local_file, git_stub } => { - Some(Fix::ConvertToGitStub { local_file, git_stub }) - } - Problem::GitStubShouldBeJson { local_file, blessed } => { + VersionProblem::BlessedVersionShouldBeGitStub { + local_file, + git_stub, + } => Some(Fix::ConvertToGitStub { local_file, git_stub }), + VersionProblem::GitStubShouldBeJson { local_file, blessed } => { Some(Fix::ConvertToJson { local_file, blessed }) } - Problem::BlessedVersionCorruptedLocal { + VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub, @@ -579,22 +634,17 @@ impl<'a> Problem<'a> { blessed, git_stub: git_stub.as_ref(), }), - Problem::DuplicateLocalFile { local_file } => { + VersionProblem::DuplicateLocalFile { local_file } => { Some(Fix::DeleteFiles { files: DisplayableVec(vec![ local_file.spec_file_name().clone(), ]), }) } - Problem::GitStubCommitStale { local_file, git_stub } => { + VersionProblem::GitStubCommitStale { local_file, git_stub } => { Some(Fix::UpdateGitStub { local_file, git_stub }) } - Problem::GitStubFirstCommitUnknown { .. } => None, - Problem::UnparseableLocalFile { unparseable_file } => { - Some(Fix::DeleteUnparseableFile { - path: unparseable_file.path.clone(), - }) - } + VersionProblem::GitStubFirstCommitUnknown { .. } => None, } } } @@ -1051,7 +1101,13 @@ fn symlink_file(target: &str, path: &Utf8Path) -> std::io::Result<()> { /// local spec files for a given API pub struct Resolved<'a> { notes: Vec, - non_version_problems: Vec<(ApiIdent, Option, Problem<'a>)>, + /// Non-version problems that aren't attached to a supported (api, version) + /// pair: local files for unsupported versions and unparseable local + /// files. The "latest" symlink problems are *also* non-version problems, + /// but they live on [`ApiResolved::symlink`] and are reached via + /// [`Resolved::symlink_problem`]. + orphaned_and_unparseable: + Vec<(ApiIdent, Option, NonVersionProblem<'a>)>, api_results: BTreeMap>, nexpected_documents: usize, } @@ -1098,10 +1154,10 @@ impl<'a> Resolved<'a> { // Get the other easy case out of the way: if there are any local spec // files for APIs or API versions that aren't supported any more, that's // a (fixable) problem. - let mut non_version_problems: Vec<( + let mut orphaned_and_unparseable: Vec<( ApiIdent, Option, - Problem<'_>, + NonVersionProblem<'_>, )> = resolve_orphaned_local_specs(&supported_versions_by_api, local) .map(|spec_file_name| { let ident = spec_file_name.ident().clone(); @@ -1109,7 +1165,7 @@ impl<'a> Resolved<'a> { ( ident, version, - Problem::LocalSpecFileOrphaned { + NonVersionProblem::LocalSpecFileOrphaned { spec_file_name: spec_file_name.clone(), }, ) @@ -1143,7 +1199,7 @@ impl<'a> Resolved<'a> { Resolution { kind, problems: vec![ - Problem::GeneratedSourceMissing { + VersionProblem::GeneratedSourceMissing { api_ident: ident.clone(), }, ], @@ -1191,10 +1247,10 @@ impl<'a> Resolved<'a> { for unparseable in api_files.unparseable_files() { // Only report if no fix will overwrite this path. if !paths_written.contains(&unparseable.path) { - non_version_problems.push(( + orphaned_and_unparseable.push(( ident.clone(), None, - Problem::UnparseableLocalFile { + NonVersionProblem::UnparseableLocalFile { unparseable_file: unparseable.clone(), }, )); @@ -1204,7 +1260,7 @@ impl<'a> Resolved<'a> { Resolved { notes, - non_version_problems, + orphaned_and_unparseable, api_results, nexpected_documents, } @@ -1218,8 +1274,17 @@ impl<'a> Resolved<'a> { self.notes.iter() } - pub fn general_problems(&self) -> impl Iterator> + '_ { - self.non_version_problems.iter().map(|(_, _, problem)| problem) + /// Iterate over non-version problems that aren't attached to a specific + /// supported (api, version) pair: local files for unsupported versions + /// and unparseable local files. + /// + /// Does *not* include "latest" symlink problems, which are also + /// [`NonVersionProblem`]s but are reached via + /// [`Resolved::symlink_problem`]. + pub fn orphaned_and_unparseable( + &self, + ) -> impl Iterator> + '_ { + self.orphaned_and_unparseable.iter().map(|(_, _, problem)| problem) } pub fn resolution_for_api_version( @@ -1230,25 +1295,32 @@ impl<'a> Resolved<'a> { self.api_results.get(ident).and_then(|v| v.by_version.get(version)) } - pub fn symlink_problem(&self, ident: &ApiIdent) -> Option<&Problem<'_>> { + /// Returns the "latest" symlink problem for an API, if any. + /// + /// Symlink problems are [`NonVersionProblem`]s but live separately from + /// [`Resolved::orphaned_and_unparseable`] because they're scoped to an + /// API rather than a (file, version) pair. + pub fn symlink_problem( + &self, + ident: &ApiIdent, + ) -> Option<&NonVersionProblem<'_>> { self.api_results.get(ident).and_then(|v| v.symlink.as_ref()) } pub fn has_unfixable_problems(&self) -> bool { - self.general_problems().any(|p| !p.is_fixable()) + self.orphaned_and_unparseable().any(|p| !p.is_fixable()) || self.api_results.values().any(|a| a.has_unfixable_problems()) } /// Returns an owned, ordered list of all problems as summaries. /// - /// Order: general (non-version-specific) problems first, then per-API - /// (sorted by ident), per-version (sorted by semver), then symlink - /// problems. + /// Order: orphaned/unparseable problems first, then per-API (sorted by + /// ident), per-version (sorted by semver), then symlink problems. pub fn problem_summaries(&self) -> Vec { let mut summaries = Vec::new(); - // General problems. - for (ident, version, problem) in &self.non_version_problems { + // Orphaned and unparseable problems. + for (ident, version, problem) in &self.orphaned_and_unparseable { summaries.push(ProblemSummary { api_ident: ident.clone(), version: version.clone(), @@ -1282,7 +1354,7 @@ impl<'a> Resolved<'a> { struct ApiResolved<'a> { by_version: BTreeMap>, - symlink: Option>, + symlink: Option>, } impl ApiResolved<'_> { @@ -1424,9 +1496,11 @@ fn resolve_api<'a>( version, Resolution { kind, - problems: vec![Problem::GeneratedSourceMissing { - api_ident: api.ident().clone(), - }], + problems: vec![ + VersionProblem::GeneratedSourceMissing { + api_ident: api.ident().clone(), + }, + ], }, ); }; @@ -1461,7 +1535,7 @@ fn resolve_api<'a>( if let Some((Some(spec_file_name), error)) = latest_first_commit_error && let Some(resolution) = by_version.get_mut(latest_version) { - resolution.add_problem(Problem::GitStubFirstCommitUnknown { + resolution.add_problem(VersionProblem::GitStubFirstCommitUnknown { spec_file_name, source: error, }); @@ -1559,7 +1633,7 @@ fn resolve_api<'a>( generated_version ); }); - Some(Problem::LatestLinkStale { + Some(NonVersionProblem::LatestLinkStale { api_ident: api.ident().clone(), link: blessed.versioned_spec_file_name(), found: latest_local, @@ -1568,7 +1642,7 @@ fn resolve_api<'a>( ResolutionKind::NewLocally => { // latest_generated is not blessed, so update // the symlink. - Some(Problem::LatestLinkStale { + Some(NonVersionProblem::LatestLinkStale { api_ident: api.ident().clone(), link: latest_generated, found: latest_local, @@ -1603,7 +1677,7 @@ fn resolve_api<'a>( generated_version ); }); - Some(Problem::LatestLinkMissing { + Some(NonVersionProblem::LatestLinkMissing { api_ident: api.ident().clone(), link: blessed.versioned_spec_file_name(), }) @@ -1611,7 +1685,7 @@ fn resolve_api<'a>( ResolutionKind::NewLocally => { // latest_generated is not blessed, so update // the symlink to the generated version. - Some(Problem::LatestLinkMissing { + Some(NonVersionProblem::LatestLinkMissing { api_ident: api.ident().clone(), link: latest_generated, }) @@ -1647,9 +1721,11 @@ fn resolve_api_lockstep<'a>( // didn't include this API's document). return BTreeMap::from([( version.clone(), - Resolution::new_lockstep(vec![Problem::GeneratedSourceMissing { - api_ident: api.ident().clone(), - }]), + Resolution::new_lockstep(vec![ + VersionProblem::GeneratedSourceMissing { + api_ident: api.ident().clone(), + }, + ]), )]); }; @@ -1691,9 +1767,11 @@ fn resolve_api_lockstep<'a>( match local { Some(local_file) if local_file.contents() == generated.contents() => (), Some(found) => { - problems.push(Problem::LockstepStale { found, generated }) + problems.push(VersionProblem::LockstepStale { found, generated }) + } + None => { + problems.push(VersionProblem::LockstepMissingLocal { generated }) } - None => problems.push(Problem::LockstepMissingLocal { generated }), }; BTreeMap::from([(version.clone(), Resolution::new_lockstep(problems))]) @@ -1773,13 +1851,13 @@ fn resolve_api_version_blessed<'a>( match api_compatible(blessed.value(), generated.value()) { Ok(issues) => { if !issues.is_empty() { - problems.push(Problem::BlessedVersionBroken { + problems.push(VersionProblem::BlessedVersionBroken { compatibility_issues: issues, }); } } Err(error) => { - problems.push(Problem::BlessedVersionCompareError { error }) + problems.push(VersionProblem::BlessedVersionCompareError { error }) } }; @@ -1793,7 +1871,7 @@ fn resolve_api_version_blessed<'a>( && problems.is_empty() && generated.contents() != blessed.contents() { - problems.push(Problem::BlessedLatestVersionBytewiseMismatch { + problems.push(VersionProblem::BlessedLatestVersionBytewiseMismatch { blessed, generated, }); @@ -1852,7 +1930,7 @@ fn resolve_api_version_blessed<'a>( // expensive because it may need to resolve a git revision to a commit // hash. let compute_storage_format = - |problems: &mut Vec>| -> VersionStorageFormat { + |problems: &mut Vec>| -> VersionStorageFormat { match git_stub { Some(r) => { match r.to_git_stub(&env.repo_root, merge_base, &env.vcs) { @@ -1861,12 +1939,14 @@ fn resolve_api_version_blessed<'a>( current, ), Err(error) => { - problems.push(Problem::GitStubFirstCommitUnknown { - spec_file_name: blessed - .versioned_spec_file_name() - .clone(), - source: error, - }); + problems.push( + VersionProblem::GitStubFirstCommitUnknown { + spec_file_name: blessed + .versioned_spec_file_name() + .clone(), + source: error, + }, + ); VersionStorageFormat::Error } } @@ -1880,13 +1960,13 @@ fn resolve_api_version_blessed<'a>( if use_git_stub_storage && !is_latest { match compute_storage_format(&mut problems) { VersionStorageFormat::GitStub(g) => { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: Some(g), }); } VersionStorageFormat::Json => { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: None, }); @@ -1899,7 +1979,7 @@ fn resolve_api_version_blessed<'a>( } } } else { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: None, }); @@ -1911,7 +1991,7 @@ fn resolve_api_version_blessed<'a>( // Report corrupted local files that need regeneration from blessed. for local_file in &corrupted { - problems.push(Problem::BlessedVersionCorruptedLocal { + problems.push(VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub: None, @@ -1927,14 +2007,18 @@ fn resolve_api_version_blessed<'a>( // case) for deletion. for local_file in matching { if local_file.spec_file_name().is_git_stub() { - problems.push(Problem::DuplicateLocalFile { local_file }); + problems.push(VersionProblem::DuplicateLocalFile { + local_file, + }); } } } else { let local_file = matching[0]; if local_file.spec_file_name().is_git_stub() { - problems - .push(Problem::GitStubShouldBeJson { local_file, blessed }); + problems.push(VersionProblem::GitStubShouldBeJson { + local_file, + blessed, + }); } } } else { @@ -1951,7 +2035,7 @@ fn resolve_api_version_blessed<'a>( None } }; - problems.push(Problem::BlessedVersionCorruptedLocal { + problems.push(VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub, @@ -1964,13 +2048,13 @@ fn resolve_api_version_blessed<'a>( let check_git_stub_staleness = |local_file: &'a LocalApiSpecFile, expected_git_stub: &GitStub, - problems: &mut Vec>| { + problems: &mut Vec>| { // Non-gitstub files (JSON) don't have a commit to check. let Some(local_commit) = local_file.git_stub_commit() else { return; }; if *local_commit != expected_git_stub.commit() { - problems.push(Problem::GitStubCommitStale { + problems.push(VersionProblem::GitStubCommitStale { local_file, git_stub: expected_git_stub.clone(), }); @@ -1993,8 +2077,9 @@ fn resolve_api_version_blessed<'a>( // have Git stub: this file is redundant. (VersionStorageFormat::GitStub(_), false) | (VersionStorageFormat::Json, true) => { - problems - .push(Problem::DuplicateLocalFile { local_file }); + problems.push(VersionProblem::DuplicateLocalFile { + local_file, + }); } // Format matches and is a Git stub: check for staleness. ( @@ -2018,14 +2103,16 @@ fn resolve_api_version_blessed<'a>( match (&storage_format, local_file.spec_file_name().is_git_stub()) { (VersionStorageFormat::GitStub(git_stub), false) => { // Should be Git stub but is JSON: convert to Git stub. - problems.push(Problem::BlessedVersionShouldBeGitStub { - local_file, - git_stub: git_stub.clone(), - }); + problems.push( + VersionProblem::BlessedVersionShouldBeGitStub { + local_file, + git_stub: git_stub.clone(), + }, + ); } (VersionStorageFormat::Json, true) => { // Should be JSON but is Git stub: convert to JSON. - problems.push(Problem::GitStubShouldBeJson { + problems.push(VersionProblem::GitStubShouldBeJson { local_file, blessed, }); @@ -2051,7 +2138,7 @@ fn resolve_api_version_blessed<'a>( // Report non-matching local files as extra. problems.extend(non_matching.into_iter().map(|s| { - Problem::BlessedVersionExtraLocalSpec { + VersionProblem::BlessedVersionExtraLocalSpec { spec_file_name: s .spec_file_name() .as_versioned() @@ -2084,10 +2171,11 @@ fn resolve_api_version_local<'a>( // There was no matching spec. if non_matching.is_empty() { // There were no non-matching specs, either. - problems.push(Problem::LocalVersionMissingLocal { generated }); + problems + .push(VersionProblem::LocalVersionMissingLocal { generated }); } else { // There were non-matching specs. This is your basic "stale" case. - problems.push(Problem::LocalVersionStale { + problems.push(VersionProblem::LocalVersionStale { spec_files: non_matching, generated, }); @@ -2106,7 +2194,7 @@ fn resolve_api_version_local<'a>( }) .collect(), ); - problems.push(Problem::LocalVersionExtra { spec_file_names }); + problems.push(VersionProblem::LocalVersionExtra { spec_file_names }); } Resolution::new_new_locally(problems) @@ -2118,7 +2206,7 @@ fn validate_generated( validation: Option<&DynValidationFn>, version: ApiVersion<'_>, generated: &GeneratedApiSpecFile, - problems: &mut Vec>, + problems: &mut Vec>, ) { match validate( env, @@ -2129,7 +2217,7 @@ fn validate_generated( generated, ) { Err(source) => { - problems.push(Problem::GeneratedValidationError { + problems.push(VersionProblem::GeneratedValidationError { api_ident: api.ident().clone(), version: version.version.clone(), source, @@ -2140,7 +2228,7 @@ fn validate_generated( match status { CheckStatus::Fresh => (), CheckStatus::Stale(check_stale) => { - problems.push(Problem::ExtraFileStale { + problems.push(VersionProblem::ExtraFileStale { api_ident: api.ident().clone(), path, check_stale,