Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(cli): traversal #3563

Merged
merged 7 commits into from
Aug 4, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,28 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

- `biome init` now generates a new config file with more options set.
This change intends to improve discoverability of the options and to set the more commonly used options to their default values.
Contributed by Conaclos
Contributed by @Conaclos
- The `--verbose` flag how reports the list of files that were evaluated, and the list of files that were fixed.
The **evaluated** files are the those files that can be handled by Biome, files that are ignored, don't have an extension or have an extension that Biome can't evaluate are excluded by this list.
The **fixed** files are those files that were handled by Biome and *changed*. Files that stays the same after the process are excluded from this list.

```shell
VERBOSE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ List of files that were evaluated
ematipico marked this conversation as resolved.
Show resolved Hide resolved

- biome/biome.json
- biome/packages/@biomejs/cli-win32-arm64/package.json
- biome/packages/tailwindcss-config-analyzer/package.json

VERBOSE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ List of files that were fixed

- biome/biome/packages/tailwindcss-config-analyzer/src/generate-tailwind-preset.ts
```

Contributed by @ematipico

#### Bug fixes

Expand Down
27 changes: 16 additions & 11 deletions crates/biome_cli/src/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::cli_options::{CliOptions, CliReporter};
use crate::commands::MigrateSubCommand;
use crate::diagnostics::ReportDiagnostic;
use crate::execute::migrate::MigratePayload;
use crate::execute::traverse::traverse;
use crate::execute::traverse::{traverse, TraverseResult};
use crate::reporter::github::{GithubReporter, GithubReporterVisitor};
use crate::reporter::json::{JsonReporter, JsonReporterVisitor};
use crate::reporter::junit::{JunitReporter, JunitReporterVisitor};
Expand Down Expand Up @@ -431,18 +431,22 @@ pub fn execute_mode(
};
migrate::run(payload)
} else {
let (summary_result, diagnostics) = traverse(&execution, &mut session, cli_options, paths)?;
let TraverseResult {
summary,
evaluated_paths,
diagnostics,
} = traverse(&execution, &mut session, cli_options, paths)?;
let console = session.app.console;
let errors = summary_result.errors;
let skipped = summary_result.skipped;
let processed = summary_result.changed + summary_result.unchanged;
let should_exit_on_warnings = summary_result.warnings > 0 && cli_options.error_on_warnings;
let errors = summary.errors;
let skipped = summary.skipped;
let processed = summary.changed + summary.unchanged;
let should_exit_on_warnings = summary.warnings > 0 && cli_options.error_on_warnings;

match execution.report_mode {
ReportMode::Terminal { with_summary } => {
if with_summary {
let reporter = SummaryReporter {
summary: summary_result,
summary,
diagnostics_payload: DiagnosticsPayload {
verbose: cli_options.verbose,
diagnostic_level: cli_options.diagnostic_level,
Expand All @@ -453,13 +457,14 @@ pub fn execute_mode(
reporter.write(&mut SummaryReporterVisitor(console))?;
} else {
let reporter = ConsoleReporter {
summary: summary_result,
summary,
diagnostics_payload: DiagnosticsPayload {
verbose: cli_options.verbose,
diagnostic_level: cli_options.diagnostic_level,
diagnostics,
},
execution: execution.clone(),
evaluated_paths,
};
reporter.write(&mut ConsoleReporterVisitor(console))?;
}
Expand All @@ -469,15 +474,15 @@ pub fn execute_mode(
<Warn>"The "<Emphasis>"--json"</Emphasis>" option is "<Underline>"unstable/experimental"</Underline>" and its output might change between patches/minor releases."</Warn>
});
let reporter = JsonReporter {
summary: summary_result,
summary,
diagnostics: DiagnosticsPayload {
verbose: cli_options.verbose,
diagnostic_level: cli_options.diagnostic_level,
diagnostics,
},
execution: execution.clone(),
};
let mut buffer = JsonReporterVisitor::new(summary_result);
let mut buffer = JsonReporterVisitor::new(summary);
reporter.write(&mut buffer)?;
if pretty {
let content = serde_json::to_string(&buffer).map_err(|error| {
Expand Down Expand Up @@ -517,7 +522,7 @@ pub fn execute_mode(
}
ReportMode::Junit => {
let reporter = JunitReporter {
summary: summary_result,
summary,
diagnostics_payload: DiagnosticsPayload {
verbose: cli_options.verbose,
diagnostic_level: cli_options.diagnostic_level,
Expand Down
34 changes: 1 addition & 33 deletions crates/biome_cli/src/execute/process_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use search::search;
use std::marker::PhantomData;
use std::ops::Deref;
use std::path::Path;

#[derive(Debug)]
pub(crate) enum FileStatus {
/// File changed and it was a success
Expand Down Expand Up @@ -150,38 +149,7 @@ pub(crate) fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult {

// then we pick the specific features for this file
let unsupported_reason = match ctx.execution.traversal_mode() {
TraversalMode::Check { .. } => file_features
.support_kind_for(&FeatureKind::Lint)
.and_then(|support_kind| {
if support_kind.is_not_enabled() {
Some(support_kind)
} else {
None
}
})
.and(
file_features
.support_kind_for(&FeatureKind::Format)
.and_then(|support_kind| {
if support_kind.is_not_enabled() {
Some(support_kind)
} else {
None
}
}),
)
.and(
file_features
.support_kind_for(&FeatureKind::OrganizeImports)
.and_then(|support_kind| {
if support_kind.is_not_enabled() {
Some(support_kind)
} else {
None
}
}),
),
TraversalMode::CI { .. } => file_features
TraversalMode::Check { .. } | TraversalMode::CI { .. } => file_features
.support_kind_for(&FeatureKind::Lint)
.and_then(|support_kind| {
if support_kind.is_not_enabled() {
Expand Down
74 changes: 56 additions & 18 deletions crates/biome_cli/src/execute/traverse.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::process_file::{process_file, DiffKind, FileStatus, Message};
use super::process_file::{process_file, DiffKind, FileResult, FileStatus, Message};
use super::{Execution, TraversalMode};
use crate::cli_options::CliOptions;
use crate::execute::diagnostics::{
Expand All @@ -9,13 +9,15 @@ use crate::reporter::TraversalSummary;
use crate::{CliDiagnostic, CliSession};
use biome_diagnostics::DiagnosticTags;
use biome_diagnostics::{category, DiagnosticExt, Error, Resource, Severity};
use biome_fs::{BiomePath, FileSystem, PathInterner};
use biome_fs::{BiomePath, EvaluatedPath, FileSystem, PathInterner};
use biome_fs::{TraversalContext, TraversalScope};
use biome_service::workspace::{DropPatternParams, IsPathIgnoredParams};
use biome_service::{extension_error, workspace::SupportsFeatureParams, Workspace, WorkspaceError};
use crossbeam::channel::{unbounded, Receiver, Sender};
use rustc_hash::FxHashSet;
use std::panic::UnwindSafe;
use std::sync::atomic::AtomicU32;
use std::sync::RwLock;
use std::{
env::current_dir,
ffi::OsString,
Expand All @@ -29,12 +31,18 @@ use std::{
time::{Duration, Instant},
};

pub(crate) struct TraverseResult {
pub(crate) summary: TraversalSummary,
pub(crate) evaluated_paths: FxHashSet<EvaluatedPath>,
pub(crate) diagnostics: Vec<Error>,
}

pub(crate) fn traverse(
execution: &Execution,
session: &mut CliSession,
cli_options: &CliOptions,
mut inputs: Vec<OsString>,
) -> Result<(TraversalSummary, Vec<Error>), CliDiagnostic> {
) -> Result<TraverseResult, CliDiagnostic> {
init_thread_pool();

if inputs.is_empty() {
Expand Down Expand Up @@ -81,15 +89,15 @@ pub(crate) fn traverse(
.with_diagnostic_level(cli_options.diagnostic_level)
.with_max_diagnostics(max_diagnostics);

let (duration, diagnostics) = thread::scope(|s| {
let (duration, evaluated_paths, diagnostics) = thread::scope(|s| {
let handler = thread::Builder::new()
.name(String::from("biome::console"))
.spawn_scoped(s, || printer.run(receiver, recv_files))
.expect("failed to spawn console thread");

// The traversal context is scoped to ensure all the channels it
// contains are properly closed once the traversal finishes
let elapsed = traverse_inputs(
let (elapsed, evaluated_paths) = traverse_inputs(
fs,
inputs,
&TraversalOptions {
Expand All @@ -102,12 +110,13 @@ pub(crate) fn traverse(
skipped: &skipped,
messages: sender,
remaining_diagnostics: &remaining_diagnostics,
evaluated_paths: RwLock::default(),
},
);
// wait for the main thread to finish
let diagnostics = handler.join().unwrap();

(elapsed, diagnostics)
(elapsed, evaluated_paths, diagnostics)
});

// Make sure patterns are always cleaned up at the end of traversal.
Expand All @@ -124,8 +133,8 @@ pub(crate) fn traverse(
let skipped = skipped.load(Ordering::Relaxed);
let suggested_fixes_skipped = printer.skipped_fixes();
let diagnostics_not_printed = printer.not_printed_diagnostics();
Ok((
TraversalSummary {
Ok(TraverseResult {
summary: TraversalSummary {
changed,
unchanged,
duration,
Expand All @@ -135,8 +144,9 @@ pub(crate) fn traverse(
suggested_fixes_skipped,
diagnostics_not_printed,
},
evaluated_paths,
diagnostics,
))
})
}

/// This function will setup the global Rayon thread pool the first time it's called
Expand All @@ -154,15 +164,27 @@ fn init_thread_pool() {

/// Initiate the filesystem traversal tasks with the provided input paths and
/// run it to completion, returning the duration of the process
fn traverse_inputs(fs: &dyn FileSystem, inputs: Vec<OsString>, ctx: &TraversalOptions) -> Duration {
fn traverse_inputs(
fs: &dyn FileSystem,
inputs: Vec<OsString>,
ctx: &TraversalOptions,
) -> (Duration, FxHashSet<EvaluatedPath>) {
let start = Instant::now();
fs.traversal(Box::new(move |scope: &dyn TraversalScope| {
for input in inputs {
scope.spawn(ctx, PathBuf::from(input));
scope.evaluate(ctx, PathBuf::from(input));
}
}));

let paths = ctx.evaluated_paths();

fs.traversal(Box::new(|scope: &dyn TraversalScope| {
for path in paths.clone() {
scope.handle(ctx, path.to_bath_buf());
}
}));

start.elapsed()
(start.elapsed(), paths)
}

// struct DiagnosticsReporter<'ctx> {}
Expand Down Expand Up @@ -483,11 +505,16 @@ pub(crate) struct TraversalOptions<'ctx, 'app> {
/// The approximate number of diagnostics the console will print before
/// folding the rest into the "skipped diagnostics" counter
pub(crate) remaining_diagnostics: &'ctx AtomicU32,

/// List of paths that should be processed
pub(crate) evaluated_paths: RwLock<FxHashSet<EvaluatedPath>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure: why do we need a RWLock here? Have we several processes that “evaluate” files? Or it is just for toggling if a file was fixed? In this last case, we could use an atomic bool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TraversalOptions is sent as a reference across multiple threads. So we need RwLock for two reasons:

  • enable interior mutability when having a &self
  • lock the resource when multiple threads want to update the list

}

impl<'ctx, 'app> TraversalOptions<'ctx, 'app> {
pub(crate) fn increment_changed(&self) {
pub(crate) fn increment_changed(&self, path: &Path) {
self.changed.fetch_add(1, Ordering::Relaxed);
let mut evaluated_paths = self.evaluated_paths.write().unwrap();
evaluated_paths.replace(EvaluatedPath::new_evaluated(path));
}
pub(crate) fn increment_unchanged(&self) {
self.unchanged.fetch_add(1, Ordering::Relaxed);
Expand Down Expand Up @@ -518,6 +545,10 @@ impl<'ctx, 'app> TraversalContext for TraversalOptions<'ctx, 'app> {
&self.interner
}

fn evaluated_paths(&self) -> FxHashSet<EvaluatedPath> {
self.evaluated_paths.read().unwrap().clone()
}

fn push_diagnostic(&self, error: Error) {
self.push_message(error);
}
Expand Down Expand Up @@ -588,18 +619,25 @@ impl<'ctx, 'app> TraversalContext for TraversalOptions<'ctx, 'app> {
}
}

fn handle_file(&self, path: &Path) {
handle_file(self, path)
fn handle_path(&self, path: &Path) {
handle_file(self, path, process_file)
}

fn store_path(&self, path: &Path) {
self.evaluated_paths.write().unwrap().insert(path.into());
}
}

/// This function wraps the [process_file] function implementing the traversal
/// in a [catch_unwind] block and emit diagnostics in case of error (either the
/// traversal function returns Err or panics)
ematipico marked this conversation as resolved.
Show resolved Hide resolved
fn handle_file(ctx: &TraversalOptions, path: &Path) {
match catch_unwind(move || process_file(ctx, path)) {
fn handle_file<F>(ctx: &TraversalOptions, path: &Path, func: F)
where
F: Fn(&TraversalOptions, &Path) -> FileResult + UnwindSafe,
{
match catch_unwind(move || func(ctx, path)) {
Ok(Ok(FileStatus::Changed)) => {
ctx.increment_changed();
ctx.increment_changed(path);
}
Ok(Ok(FileStatus::Unchanged)) => {
ctx.increment_unchanged();
Expand Down
12 changes: 12 additions & 0 deletions crates/biome_cli/src/reporter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ pub(crate) mod terminal;

use crate::execute::Execution;
use biome_diagnostics::{Error, Severity};
use biome_fs::EvaluatedPath;
use rustc_hash::FxHashSet;
use serde::Serialize;
use std::io;
use std::time::Duration;
Expand Down Expand Up @@ -47,6 +49,16 @@ pub trait ReporterVisitor {
summary: TraversalSummary,
) -> io::Result<()>;

/// Writes the paths that were handled during a run.
/// The fist list represents the paths that were evaluated, the second list represents the paths that were fixed
ematipico marked this conversation as resolved.
Show resolved Hide resolved
fn report_handled_paths(
&mut self,
evaluated_paths: FxHashSet<EvaluatedPath>,
) -> io::Result<()> {
let _ = evaluated_paths;
Ok(())
}

/// Writes a diagnostics
fn report_diagnostics(
&mut self,
Expand Down
Loading