From c54852f30dd49e79c584ff8ff41813d7f7bbfb73 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Wed, 18 Dec 2024 14:37:18 -0800 Subject: [PATCH] refactor: make `Analyzer` accept the diagnostics configuration. This commit refactors the `new` and `new_with_validator` methods of `Analyzer` to directly accept the `DiagnosticsConfig` rather than an iterator of rules which it immediately uses to create a `DiagnosticsConfig` internally. By having the `DiagnosticsConfig` (which is `Copy`) passed in directly, we don't have to borrow a rule exception list that might also be used as part of the validator closure passed to `new_with_validator`, sparing us a clone. --- gauntlet/src/lib.rs | 3 ++- wdl-analysis/CHANGELOG.md | 2 ++ wdl-analysis/src/analyzer.rs | 30 +++++++++++++----------------- wdl-analysis/tests/analysis.rs | 5 +++-- wdl-doc/src/lib.rs | 3 ++- wdl-engine/tests/inputs.rs | 3 ++- wdl-engine/tests/tasks.rs | 3 ++- wdl-lsp/src/server.rs | 3 ++- wdl/src/bin/wdl.rs | 3 ++- 9 files changed, 30 insertions(+), 25 deletions(-) diff --git a/gauntlet/src/lib.rs b/gauntlet/src/lib.rs index 4c19b0638..964ed4f57 100644 --- a/gauntlet/src/lib.rs +++ b/gauntlet/src/lib.rs @@ -40,6 +40,7 @@ use report::Status; use report::UnmatchedStatus; pub use repository::Repository; use wdl::analysis::Analyzer; +use wdl::analysis::DiagnosticsConfig; use wdl::analysis::rules; use wdl::ast::Diagnostic; use wdl::lint::LintVisitor; @@ -174,7 +175,7 @@ pub async fn gauntlet(args: Args) -> Result<()> { let analyzer = Analyzer::new_with_validator( // Don't bother duplicating analysis warnings for arena mode - if args.arena { Vec::new() } else { rules() }, + DiagnosticsConfig::new(if args.arena { Vec::new() } else { rules() }), move |_: (), _, _, _| async move {}, move || { let mut validator = if !args.arena { diff --git a/wdl-analysis/CHANGELOG.md b/wdl-analysis/CHANGELOG.md index 5bbae1554..daa867d5d 100644 --- a/wdl-analysis/CHANGELOG.md +++ b/wdl-analysis/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +* Changed the `new` and `new_with_validator` methods of `Analyzer` to take the + diagnostics configuration rather than a rule iterator ([#274](https://github.com/stjude-rust-labs/wdl/pull/274)). * Refactored the `AnalysisResult` and `Document` types to move properties of the former into the latter; this will assist in evaluation of documents in that the `Document` alone can be passed into evaluation ([#265](https://github.com/stjude-rust-labs/wdl/pull/265)). diff --git a/wdl-analysis/src/analyzer.rs b/wdl-analysis/src/analyzer.rs index 6f12c1bb4..1a6197313 100644 --- a/wdl-analysis/src/analyzer.rs +++ b/wdl-analysis/src/analyzer.rs @@ -309,7 +309,7 @@ pub struct DiagnosticsConfig { impl DiagnosticsConfig { /// Creates a new diagnostics configuration from a rule set. - pub fn new>(rules: impl Iterator) -> Self { + pub fn new>(rules: impl IntoIterator) -> Self { let mut unused_import = None; let mut unused_input = None; let mut unused_declaration = None; @@ -402,26 +402,23 @@ impl Analyzer where Context: Send + Clone + 'static, { - /// Constructs a new analyzer with the given analysis rules. + /// Constructs a new analyzer with the given diagnostics config. /// /// The provided progress callback will be invoked during analysis. /// /// The analyzer will use a default validator for validation. /// /// The analyzer must be constructed from the context of a Tokio runtime. - pub fn new, Progress, Return>( - rules: impl IntoIterator, - progress: Progress, - ) -> Self + pub fn new(config: DiagnosticsConfig, progress: Progress) -> Self where Progress: Fn(Context, ProgressKind, usize, usize) -> Return + Send + 'static, Return: Future, { - Self::new_with_validator(rules, progress, Validator::default) + Self::new_with_validator(config, progress, Validator::default) } - /// Constructs a new analyzer with the given analysis rules and validator - /// function. + /// Constructs a new analyzer with the given diagnostics config and + /// validator function. /// /// The provided progress callback will be invoked during analysis. /// @@ -429,8 +426,8 @@ where /// initialize a thread-local validator. /// /// The analyzer must be constructed from the context of a Tokio runtime. - pub fn new_with_validator, Progress, Return, Validator>( - rules: impl IntoIterator, + pub fn new_with_validator( + config: DiagnosticsConfig, progress: Progress, validator: Validator, ) -> Self @@ -441,7 +438,6 @@ where { let (tx, rx) = mpsc::unbounded_channel(); let tokio = Handle::current(); - let config = DiagnosticsConfig::new(rules.into_iter()); let handle = std::thread::spawn(move || { let queue = AnalysisQueue::new(config, tokio, progress, validator); queue.run(rx); @@ -706,7 +702,7 @@ mod test { #[tokio::test] async fn it_returns_empty_results() { - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); let results = analyzer.analyze(()).await.unwrap(); assert!(results.is_empty()); } @@ -730,7 +726,7 @@ workflow test { .expect("failed to create test file"); // Analyze the file and check the resulting diagnostic - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); analyzer .add_document(path_to_uri(&path).expect("should convert to URI")) .await @@ -785,7 +781,7 @@ workflow test { .expect("failed to create test file"); // Analyze the file and check the resulting diagnostic - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); analyzer .add_document(path_to_uri(&path).expect("should convert to URI")) .await @@ -857,7 +853,7 @@ workflow test { .expect("failed to create test file"); // Analyze the file and check the resulting diagnostic - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); analyzer .add_document(path_to_uri(&path).expect("should convert to URI")) .await @@ -933,7 +929,7 @@ workflow test { .expect("failed to create test file"); // Add all three documents to the analyzer - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); analyzer .add_directory(dir.path().to_path_buf()) .await diff --git a/wdl-analysis/tests/analysis.rs b/wdl-analysis/tests/analysis.rs index 9f42dd49f..e2841488e 100644 --- a/wdl-analysis/tests/analysis.rs +++ b/wdl-analysis/tests/analysis.rs @@ -33,6 +33,7 @@ use path_clean::clean; use pretty_assertions::StrComparison; use wdl_analysis::AnalysisResult; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::path_to_uri; use wdl_analysis::rules; use wdl_ast::Diagnostic; @@ -164,7 +165,7 @@ async fn main() { println!("\nrunning {} tests\n", tests.len()); // Start with a single analysis pass over all the test files - let analyzer = Analyzer::new(rules(), |_, _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_, _, _, _| async {}); for test in &tests { analyzer .add_directory(test.clone()) @@ -215,7 +216,7 @@ async fn main() { // Some tests are sensitive to the order in which files are parsed (e.g. // detecting cycles) For those, use a new analyzer and analyze the // `source.wdl` directly - let analyzer = Analyzer::new(rules(), |_, _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_, _, _, _| async {}); for test_name in single_file { let test = Path::new("tests/analysis").join(test_name); let document = test.join("source.wdl"); diff --git a/wdl-doc/src/lib.rs b/wdl-doc/src/lib.rs index e17e89ce5..77178109e 100644 --- a/wdl-doc/src/lib.rs +++ b/wdl-doc/src/lib.rs @@ -25,6 +25,7 @@ use pulldown_cmark::Options; use pulldown_cmark::Parser; use tokio::io::AsyncWriteExt; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::rules; use wdl_ast::AstToken; use wdl_ast::Document as AstDocument; @@ -141,7 +142,7 @@ pub async fn document_workspace(path: PathBuf) -> Result<()> { std::fs::create_dir(&docs_dir)?; } - let analyzer = Analyzer::new(rules(), |_: (), _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_: (), _, _, _| async {}); analyzer.add_directory(abs_path.clone()).await?; let results = analyzer.analyze(()).await?; diff --git a/wdl-engine/tests/inputs.rs b/wdl-engine/tests/inputs.rs index 1fb3c2ed0..4534698aa 100644 --- a/wdl-engine/tests/inputs.rs +++ b/wdl-engine/tests/inputs.rs @@ -37,6 +37,7 @@ use pretty_assertions::StrComparison; use rayon::prelude::*; use wdl_analysis::AnalysisResult; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::rules; use wdl_analysis::types::Types; use wdl_ast::Diagnostic; @@ -197,7 +198,7 @@ async fn main() { println!("\nrunning {} tests\n", tests.len()); // Start with a single analysis pass over all the test files - let analyzer = Analyzer::new(rules(), |_, _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_, _, _, _| async {}); for test in &tests { analyzer .add_directory(test.clone()) diff --git a/wdl-engine/tests/tasks.rs b/wdl-engine/tests/tasks.rs index f5807b33c..10e8cad01 100644 --- a/wdl-engine/tests/tasks.rs +++ b/wdl-engine/tests/tasks.rs @@ -47,6 +47,7 @@ use tempfile::TempDir; use walkdir::WalkDir; use wdl_analysis::AnalysisResult; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::document::Document; use wdl_analysis::rules; use wdl_ast::Diagnostic; @@ -411,7 +412,7 @@ async fn main() { println!("\nrunning {} tests\n", tests.len()); // Start with a single analysis pass over all the test files - let analyzer = Analyzer::new(rules(), |_, _, _, _| async {}); + let analyzer = Analyzer::new(DiagnosticsConfig::new(rules()), |_, _, _, _| async {}); for test in &tests { analyzer .add_directory(test.clone()) diff --git a/wdl-lsp/src/server.rs b/wdl-lsp/src/server.rs index 3216809bb..26d56564d 100644 --- a/wdl-lsp/src/server.rs +++ b/wdl-lsp/src/server.rs @@ -24,6 +24,7 @@ use tracing::error; use tracing::info; use uuid::Uuid; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::IncrementalChange; use wdl_analysis::SourceEdit; use wdl_analysis::SourcePosition; @@ -252,7 +253,7 @@ impl Server { client, options, analyzer: Analyzer::::new_with_validator( - rules(), + DiagnosticsConfig::new(rules()), move |token, kind, current, total| { let client = analyzer_client.clone(); async move { diff --git a/wdl/src/bin/wdl.rs b/wdl/src/bin/wdl.rs index 8de375cf0..55eaee523 100644 --- a/wdl/src/bin/wdl.rs +++ b/wdl/src/bin/wdl.rs @@ -37,6 +37,7 @@ use wdl::ast::Validator; use wdl::lint::LintVisitor; use wdl_analysis::AnalysisResult; use wdl_analysis::Analyzer; +use wdl_analysis::DiagnosticsConfig; use wdl_analysis::Rule; use wdl_analysis::path_to_uri; use wdl_analysis::rules; @@ -95,7 +96,7 @@ async fn analyze>( ); let analyzer = Analyzer::new_with_validator( - rules, + DiagnosticsConfig::new(rules), move |bar: ProgressBar, kind, completed, total| async move { bar.set_position(completed.try_into().unwrap()); if completed == 0 {