From a9a6bb800c1a88e0851d0ea00507fe26e3b379d9 Mon Sep 17 00:00:00 2001 From: Boshen Date: Sun, 20 Aug 2023 15:12:08 +0800 Subject: [PATCH] refactor(cli,linter): move path processing logic from cli to linter (#766) --- Cargo.lock | 4 +- crates/oxc_cli/Cargo.toml | 3 -- crates/oxc_cli/src/lint/mod.rs | 37 +++----------- crates/oxc_diagnostics/src/lib.rs | 2 +- crates/oxc_linter/Cargo.toml | 1 + crates/oxc_linter/src/lib.rs | 31 +++--------- crates/oxc_linter/src/options.rs | 22 +++++++++ crates/oxc_linter/src/service.rs | 81 ++++++++++++++++++++++--------- crates/oxc_linter/src/tester.rs | 29 ++++------- justfile | 2 +- 10 files changed, 106 insertions(+), 106 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 327600572..602b45443 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1525,7 +1525,6 @@ version = "0.0.0" dependencies = [ "bpaf", "codespan-reporting", - "dashmap", "ignore", "jemallocator", "miette", @@ -1535,11 +1534,9 @@ dependencies = [ "oxc_index", "oxc_linter", "oxc_parser", - "oxc_semantic", "oxc_span", "oxc_type_synthesis", "rayon", - "rustc-hash", ] [[package]] @@ -1641,6 +1638,7 @@ dependencies = [ "oxc_span", "oxc_syntax", "phf", + "rayon", "regex", "rust-lapper", "rustc-hash", diff --git a/crates/oxc_cli/Cargo.toml b/crates/oxc_cli/Cargo.toml index 0cef843a6..e05ff1ce3 100644 --- a/crates/oxc_cli/Cargo.toml +++ b/crates/oxc_cli/Cargo.toml @@ -28,17 +28,14 @@ oxc_diagnostics = { workspace = true } oxc_index = { workspace = true } oxc_linter = { workspace = true } oxc_parser = { workspace = true } -oxc_semantic = { workspace = true } oxc_span = { workspace = true } oxc_type_synthesis = { workspace = true } # TODO temp, for type check output, replace with Miette codespan-reporting = "0.11.1" -dashmap = { workspace = true } ignore = { workspace = true, features = ["simd-accel"] } miette = { workspace = true, features = ["fancy-no-backtrace"] } rayon = { workspace = true } -rustc-hash = { workspace = true } bpaf = { workspace = true, features = ["derive", "autocomplete", "bright-color"] } # git2 = { version = "0.16.1", default_features = false } diff --git a/crates/oxc_cli/src/lint/mod.rs b/crates/oxc_cli/src/lint/mod.rs index d1ce61f12..6cab899b2 100644 --- a/crates/oxc_cli/src/lint/mod.rs +++ b/crates/oxc_cli/src/lint/mod.rs @@ -1,7 +1,6 @@ mod error; use std::{ - fs, io::BufWriter, path::Path, sync::{ @@ -48,10 +47,10 @@ impl Runner for LintRunner { let now = std::time::Instant::now(); - let filter = if filter.is_empty() { LintOptions::default().filter } else { filter }; - - let lint_options = - LintOptions { filter, fix: fix_options.fix, timing: misc_options.timing }; + let lint_options = LintOptions::default() + .with_filter(filter) + .with_fix(fix_options.fix) + .with_timing(misc_options.timing); let linter = Arc::new(Linter::from_options(lint_options)); @@ -60,6 +59,7 @@ impl Runner for LintRunner { .with_max_warnings(warning_options.max_warnings); let number_of_files = Arc::new(AtomicUsize::new(0)); + let (tx_path, rx_path) = mpsc::channel::>(); rayon::spawn({ @@ -75,35 +75,12 @@ impl Runner for LintRunner { } }); - let processing = Arc::new(AtomicUsize::new(0)); rayon::spawn({ - let linter = Arc::clone(&linter); + let lint_service = oxc_linter::LintService::new(Arc::clone(&linter)); let tx_error = diagnostic_service.sender().clone(); - let processing = Arc::clone(&processing); move || { while let Ok(path) = rx_path.recv() { - processing.fetch_add(1, Ordering::Relaxed); - let tx_error = tx_error.clone(); - let linter = Arc::clone(&linter); - let processing = Arc::clone(&processing); - rayon::spawn(move || { - let source_text = fs::read_to_string(&path) - .unwrap_or_else(|_| panic!("Failed to read {path:?}")); - - let diagnostics = oxc_linter::LintService::new(linter) - .run(&path, &source_text) - .map(|errors| { - DiagnosticService::wrap_diagnostics(&path, &source_text, errors) - }); - - if let Some(diagnostics) = diagnostics { - tx_error.send(Some(diagnostics)).unwrap(); - } - processing.fetch_sub(1, Ordering::Relaxed); - if processing.load(Ordering::Relaxed) == 0 { - tx_error.send(None).unwrap(); - } - }); + lint_service.run_path(path, &tx_error); } } }); diff --git a/crates/oxc_diagnostics/src/lib.rs b/crates/oxc_diagnostics/src/lib.rs index 75ebe5dfb..60add3abd 100644 --- a/crates/oxc_diagnostics/src/lib.rs +++ b/crates/oxc_diagnostics/src/lib.rs @@ -7,7 +7,7 @@ mod service; use std::path::PathBuf; -pub use crate::service::DiagnosticService; +pub use crate::service::{DiagnosticSender, DiagnosticService, DiagnosticTuple}; pub use graphic_reporter::{GraphicalReportHandler, GraphicalTheme}; pub use miette; pub use thiserror; diff --git a/crates/oxc_linter/Cargo.toml b/crates/oxc_linter/Cargo.toml index d15b63ffc..b097c7d77 100644 --- a/crates/oxc_linter/Cargo.toml +++ b/crates/oxc_linter/Cargo.toml @@ -23,6 +23,7 @@ oxc_semantic = { workspace = true } oxc_syntax = { workspace = true } oxc_formatter = { workspace = true } +rayon = { workspace = true } lazy_static = { workspace = true } # used in oxc_macros serde_json = { workspace = true } regex = { workspace = true } diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 286a8580e..9c70c8cf9 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -42,10 +42,6 @@ impl Linter { .cloned() .filter(|rule| rule.category() == RuleCategory::Correctness) .collect::>(); - Self::from_rules(rules) - } - - pub fn from_rules(rules: Vec) -> Self { Self { rules, options: LintOptions::default() } } @@ -54,6 +50,12 @@ impl Linter { Self { rules, options } } + #[must_use] + pub fn with_rules(mut self, rules: Vec) -> Self { + self.rules = rules; + self + } + pub fn rules(&self) -> &Vec { &self.rules } @@ -78,27 +80,6 @@ impl Linter { self } - pub fn from_json_str(s: &str) -> Self { - let rules = serde_json::from_str(s) - .ok() - .and_then(|v: serde_json::Value| v.get("rules").cloned()) - .and_then(|v| v.as_object().cloned()) - .map_or_else( - || RULES.to_vec(), - |rules_config| { - RULES - .iter() - .map(|rule| { - let value = rules_config.get(rule.name()); - rule.read_json(value.cloned()) - }) - .collect() - }, - ); - - Self::from_rules(rules) - } - pub fn run<'a>(&self, ctx: LintContext<'a>) -> Vec> { let timing = self.options.timing; let semantic = Rc::clone(ctx.semantic()); diff --git a/crates/oxc_linter/src/options.rs b/crates/oxc_linter/src/options.rs index d72341635..072d4d8fd 100644 --- a/crates/oxc_linter/src/options.rs +++ b/crates/oxc_linter/src/options.rs @@ -21,6 +21,28 @@ impl Default for LintOptions { } } +impl LintOptions { + #[must_use] + pub fn with_filter(mut self, filter: Vec<(AllowWarnDeny, String)>) -> Self { + if !filter.is_empty() { + self.filter = filter; + } + self + } + + #[must_use] + pub fn with_fix(mut self, yes: bool) -> Self { + self.fix = yes; + self + } + + #[must_use] + pub fn with_timing(mut self, yes: bool) -> Self { + self.timing = yes; + self + } +} + #[derive(Debug, Clone, Copy, Eq, PartialEq)] pub enum AllowWarnDeny { Allow, diff --git a/crates/oxc_linter/src/service.rs b/crates/oxc_linter/src/service.rs index 998fc00cb..4ea227408 100644 --- a/crates/oxc_linter/src/service.rs +++ b/crates/oxc_linter/src/service.rs @@ -1,60 +1,93 @@ -use std::{fs, path::Path, rc::Rc, sync::Arc}; +use std::{ + fs, + path::Path, + rc::Rc, + sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }, +}; use oxc_allocator::Allocator; -use oxc_diagnostics::Error; +use oxc_diagnostics::{DiagnosticSender, DiagnosticService}; use oxc_parser::Parser; use oxc_semantic::SemanticBuilder; use oxc_span::SourceType; -use crate::{Fixer, LintContext, Linter}; +use crate::{Fixer, LintContext, Linter, Message}; pub struct LintService { linter: Arc, + processing: Arc, } impl LintService { pub fn new(linter: Arc) -> Self { - Self { linter } + Self { linter, processing: Arc::new(AtomicUsize::new(0)) } } /// # Panics - pub fn run(&self, path: &Path, source_text: &str) -> Option> { - let allocator = Allocator::default(); + pub fn run_path(&self, path: Box, tx_error: &DiagnosticSender) { + self.processing.fetch_add(1, Ordering::Relaxed); + let linter = Arc::clone(&self.linter); + let tx_error = tx_error.clone(); + let processing = Arc::clone(&self.processing); + rayon::spawn(move || { + let allocator = Allocator::default(); + let source_text = + fs::read_to_string(&path).unwrap_or_else(|_| panic!("Failed to read {path:?}")); + + let mut messages = Self::run_source(&linter, &path, &allocator, &source_text, true); + + if linter.options().fix { + let fix_result = Fixer::new(&source_text, messages).fix(); + fs::write(&path, fix_result.fixed_code.as_bytes()).unwrap(); + messages = fix_result.messages; + } + + let errors = messages.into_iter().map(|m| m.error).collect(); + let diagnostics = DiagnosticService::wrap_diagnostics(&path, &source_text, errors); + + if !diagnostics.1.is_empty() { + tx_error.send(Some(diagnostics)).unwrap(); + } + + processing.fetch_sub(1, Ordering::Relaxed); + if processing.load(Ordering::Relaxed) == 0 { + tx_error.send(None).unwrap(); + } + }); + } + + pub(crate) fn run_source<'a>( + linter: &Linter, + path: &Path, + allocator: &'a Allocator, + source_text: &'a str, + check_syntax_errors: bool, + ) -> Vec> { let source_type = SourceType::from_path(path).unwrap_or_else(|_| panic!("Incorrect {path:?}")); - let ret = Parser::new(&allocator, source_text, source_type) + let ret = Parser::new(allocator, source_text, source_type) .allow_return_outside_function(true) .parse(); if !ret.errors.is_empty() { - return Some(ret.errors); + return ret.errors.into_iter().map(|err| Message::new(err, None)).collect(); }; let program = allocator.alloc(ret.program); let semantic_ret = SemanticBuilder::new(source_text, source_type) .with_trivias(ret.trivias) - .with_check_syntax_error(true) + .with_check_syntax_error(check_syntax_errors) .with_module_record_builder(true) .build(program); if !semantic_ret.errors.is_empty() { - return Some(semantic_ret.errors); + return semantic_ret.errors.into_iter().map(|err| Message::new(err, None)).collect(); }; let lint_ctx = LintContext::new(&Rc::new(semantic_ret.semantic)); - let result = self.linter.run(lint_ctx); - - if result.is_empty() { - return None; - } - - if self.linter.options().fix { - let fix_result = Fixer::new(source_text, result).fix(); - fs::write(path, fix_result.fixed_code.as_bytes()).unwrap(); - let errors = fix_result.messages.into_iter().map(|m| m.error).collect(); - return Some(errors); - } - - Some(result.into_iter().map(|diagnostic| diagnostic.error).collect()) + linter.run(lint_ctx) } } diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 172ee867c..38618752d 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -1,13 +1,14 @@ -use std::{borrow::Cow, path::PathBuf, rc::Rc}; +use std::{ + borrow::Cow, + path::{Path, PathBuf}, + sync::Arc, +}; use oxc_allocator::Allocator; use oxc_diagnostics::miette::{GraphicalReportHandler, GraphicalTheme, NamedSource}; -use oxc_parser::Parser; -use oxc_semantic::SemanticBuilder; -use oxc_span::SourceType; use serde_json::Value; -use crate::{rules::RULES, Fixer, LintContext, Linter, Message}; +use crate::{rules::RULES, Fixer, LintOptions, LintService, Linter, Message}; pub struct Tester { rule_name: &'static str, @@ -123,28 +124,18 @@ impl Tester { fn run_rules<'a>( &mut self, allocator: &'a Allocator, - path: &PathBuf, + path: &Path, source_text: &'a str, config: Option, is_fix: bool, ) -> Vec> { - let source_type = SourceType::from_path(path).expect("incorrect {path:?}"); - let ret = Parser::new(allocator, source_text, source_type) - .allow_return_outside_function(true) - .parse(); - assert!(ret.errors.is_empty(), "{:?}", &ret.errors); - let program = allocator.alloc(ret.program); - let semantic_ret = SemanticBuilder::new(source_text, source_type) - .with_trivias(ret.trivias) - .with_module_record_builder(true) - .build(program); - assert!(semantic_ret.errors.is_empty(), "{:?}", &semantic_ret.errors); let rule = RULES .iter() .find(|rule| rule.name() == self.rule_name) .unwrap_or_else(|| panic!("Rule not found: {}", &self.rule_name)); let rule = rule.read_json(config); - let lint_context = LintContext::new(&Rc::new(semantic_ret.semantic)); - Linter::from_rules(vec![rule]).with_fix(is_fix).run(lint_context) + let options = LintOptions::default().with_fix(is_fix); + let linter = Arc::new(Linter::from_options(options).with_rules(vec![rule])); + LintService::run_source(&linter, path, allocator, source_text, false) } } diff --git a/justfile b/justfile index ee0bfedfb..ff4b84550 100755 --- a/justfile +++ b/justfile @@ -32,7 +32,7 @@ update: # --no-vcs-ignores: cargo-watch has a bug loading all .gitignores, including the ones listed in .gitignore # use .ignore file getting the ignore list watch command: - cargo watch --no-vcs-ignores -x '{{command}}' + cargo watch --no-vcs-ignores -i '*snap*' -x '{{command}}' # Format all files fmt: