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

feat(search-output-formatter): initialize search output formatter #3258

Merged
Merged
Show file tree
Hide file tree
Changes from 11 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

## Unreleased

- Implement [the search command output formatter](https://github.com/biomejs/biome/issues/2462). Contributed by @BackupMiles

### CLI

#### Bug fixes
Expand Down
4 changes: 4 additions & 0 deletions crates/biome_cli/src/execute/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ pub(crate) struct UnhandledDiagnostic;
#[diagnostic(category = "parse", message = "Skipped file with syntax errors")]
pub(crate) struct SkippedDiagnostic;

#[derive(Debug, Diagnostic)]
#[diagnostic(category = "search", tags(SEARCH))]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, you think the category should be enough to right, and no tags would be needed here? If so, I guess that makes sense :)

Copy link
Contributor Author

@BackupMiles BackupMiles Jun 24, 2024

Choose a reason for hiding this comment

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

Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Yes, this is the reason why I went with it:

  • I think a SearchDiagnostic should have a Hint severity
    • The DiagnosticPrinter will deem a diagnostic skippable if its severity is below its diagnostic_level -- in our case, anything below Information is deemed skippable
  • For fn should_skip_diagnostic to print a search diagnostic despite its low severity, I figured I needed to have a SEARCH tag attached to the diagnostic_tags (see traverse.rs::should_skip_diagnostic)

With your previous comments, I realized I am able to get this info out of the printer execution if needed: so, if there are no better ways, I could change my code to query the execution if you'd want me to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Yes, this is the reason why I went with it:

  • I think a SearchDiagnostic should have a Hint severity

    • The DiagnosticPrinter will deem a diagnostic skippable if its severity is below its diagnostic_level -- in our case, anything below Information is deemed skippable
  • For fn should_skip_diagnostic to print a search diagnostic despite its low severity, I figured I needed to have a SEARCH tag attached to the diagnostic_tags (see traverse.rs::should_skip_diagnostic)

With your previous comments, I realized I am able to get this info out of the printer execution if needed: so, if there are no better ways, I could change my code to query the execution if you'd want me to

I apologize for this confusion: after checking thoroughly, I realized this behavior was not happening the way I was imagining it.
I have now proceeded to remove the SEARCH tag and its associated checks altogether 👍

pub(crate) struct SearchDiagnostic;

/// Extension trait for turning [Display]-able error types into [TraversalError]
pub(crate) trait ResultExt {
type Result;
Expand Down
4 changes: 4 additions & 0 deletions crates/biome_cli/src/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,10 @@ impl Execution {
matches!(self.traversal_mode, TraversalMode::CI { .. })
}

pub(crate) const fn is_search(&self) -> bool {
matches!(self.traversal_mode, TraversalMode::Search { .. })
}

pub(crate) const fn is_check(&self) -> bool {
matches!(self.traversal_mode, TraversalMode::Check { .. })
}
Expand Down
27 changes: 22 additions & 5 deletions crates/biome_cli/src/execute/process_file/search.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::execute::diagnostics::ResultExt;
use crate::execute::diagnostics::{ResultExt, SearchDiagnostic};
use crate::execute::process_file::workspace_file::WorkspaceFile;
use crate::execute::process_file::{FileResult, FileStatus, SharedTraversalOptions};
use biome_diagnostics::category;
use crate::execute::process_file::{FileResult, FileStatus, Message, SharedTraversalOptions};
use biome_diagnostics::{category, DiagnosticExt, Severity};
use biome_service::workspace::PatternId;
use std::path::Path;

Expand All @@ -21,16 +21,33 @@ pub(crate) fn search_with_guard<'ctx>(
) -> FileResult {
tracing::info_span!("Processes searching", path =? workspace_file.path.display()).in_scope(
move || {
let _result = workspace_file
let result = workspace_file
.guard()
.search_pattern(pattern)
.with_file_path_and_code(
workspace_file.path.display().to_string(),
category!("search"),
)?;

let input = workspace_file.input()?;
let file_name = workspace_file.path.display().to_string();

// FIXME: We need to report some real results here...
Copy link
Member

Choose a reason for hiding this comment

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

Is the FIXME still relevant after this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess so, since the formatting is ready for the engine that provides real results is :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so, since the formatting is ready for the engine that provides real results is :)

Sorry for my last out-of-sync reply: I actually went ahead and removed this one, in favor of keeping the other TODO inside the function which is actually supposed to return the matches

Ok(FileStatus::Unchanged)
let search_results = Message::Diagnostics {
name: file_name,
content: input,
diagnostics: result
.matches
.into_iter()
.map(|mat| {
SearchDiagnostic
.with_file_span(mat)
.with_severity(Severity::Hint)
Copy link
Member

Choose a reason for hiding this comment

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

If the severity of SearchDiagnostic doesn't depend on external factors (for example, the configuration), then the severity can be set directly in the definition of SearchDiagnostic:

#[derive(Debug, Diagnostic)]
#[diagnostic(category = "search", tags(SEARCH), severity = Hint)]
struct SearchDiagnostic;

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Severity::Information is also more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

I think so too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I will go ahead and change the severity to Information then

})
.collect(),
skipped_diagnostics: 0,
};
Ok(FileStatus::Message(search_results))
},
)
}
4 changes: 4 additions & 0 deletions crates/biome_cli/src/execute/traverse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ impl<'ctx> DiagnosticsPrinter<'ctx> {
/// - it should not be considered if its severity level is lower than the one provided via CLI;
/// - it should not be considered if it's a verbose diagnostic and the CLI **didn't** request a `--verbose` option.
fn should_skip_diagnostic(&self, severity: Severity, diagnostic_tags: DiagnosticTags) -> bool {
if diagnostic_tags.is_search() {
return false;
}

if severity < self.diagnostic_level {
return true;
}
Expand Down
21 changes: 15 additions & 6 deletions crates/biome_cli/src/reporter/terminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,15 @@ impl<'a> ReporterVisitor for ConsoleReporterVisitor<'a> {

fn report_diagnostics(
&mut self,
_execution: &Execution,
execution: &Execution,
diagnostics_payload: DiagnosticsPayload,
) -> io::Result<()> {
for diagnostic in &diagnostics_payload.diagnostics {
if execution.is_search() {
self.0.log(markup! {{PrintDiagnostic::search(diagnostic)}});
continue;
}
ematipico marked this conversation as resolved.
Show resolved Hide resolved

if diagnostic.severity() >= diagnostics_payload.diagnostic_level {
if diagnostic.tags().is_verbose() && diagnostics_payload.verbose {
self.0
Expand Down Expand Up @@ -83,13 +88,17 @@ impl fmt::Display for Files {
}
}

struct SummaryDetail(usize);
struct SummaryDetail<'a>(pub(crate) &'a TraversalMode, usize);

impl fmt::Display for SummaryDetail {
impl<'a> fmt::Display for SummaryDetail<'a> {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
if self.0 > 0 {
if let TraversalMode::Search { .. } = self.0 {
return Ok(());
}

if self.1 > 0 {
fmt.write_markup(markup! {
" Fixed "{Files(self.0)}"."
" Fixed "{Files(self.1)}"."
})
} else {
fmt.write_markup(markup! {
Expand Down Expand Up @@ -147,7 +156,7 @@ pub(crate) struct ConsoleTraversalSummary<'a>(
impl<'a> fmt::Display for ConsoleTraversalSummary<'a> {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
let summary = SummaryTotal(self.0, self.1.changed + self.1.unchanged, &self.1.duration);
let detail = SummaryDetail(self.1.changed);
let detail = SummaryDetail(self.0, self.1.changed);
fmt.write_markup(markup!(<Info>{summary}{detail}</Info>))?;

if self.1.errors > 0 {
Expand Down
6 changes: 6 additions & 0 deletions crates/biome_diagnostics/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ pub(super) enum DiagnosticTag {
UnnecessaryCode,
DeprecatedCode,
Verbose,
Search,
}

bitflags! {
Expand All @@ -190,13 +191,18 @@ bitflags! {
const DEPRECATED_CODE = 1 << DiagnosticTag::DeprecatedCode as u8;
/// This diagnostic is verbose and should be printed only if the `--verbose` option is provided
const VERBOSE = 1 << DiagnosticTag::Verbose as u8;
// This diagnostic is a match result from a search operation
const SEARCH = 1 << DiagnosticTag::Search as u8;
}
}

impl DiagnosticTags {
pub const fn is_verbose(&self) -> bool {
self.contains(Self::VERBOSE)
}
pub const fn is_search(&self) -> bool {
self.contains(Self::SEARCH)
}
}

// Implement the `Diagnostic` on the `Infallible` error type from the standard
Expand Down
30 changes: 27 additions & 3 deletions crates/biome_diagnostics/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,31 @@ impl<D: AsDiagnostic + ?Sized> std::fmt::Display for PrintDescription<'_, D> {
pub struct PrintDiagnostic<'fmt, D: ?Sized> {
diag: &'fmt D,
verbose: bool,
search: bool,
}

impl<'fmt, D: AsDiagnostic + ?Sized> PrintDiagnostic<'fmt, D> {
pub fn simple(diag: &'fmt D) -> Self {
Self {
diag,
verbose: false,
search: false,
}
}

pub fn verbose(diag: &'fmt D) -> Self {
Self {
diag,
verbose: true,
search: false,
}
}

pub fn search(diag: &'fmt D) -> Self {
Self {
diag,
verbose: false,
search: true,
}
}
}
Expand All @@ -67,14 +78,19 @@ impl<D: AsDiagnostic + ?Sized> fmt::Display for PrintDiagnostic<'_, D> {
// Wrap the formatter with an indentation level and print the advices
let mut slot = None;
let mut fmt = IndentWriter::wrap(fmt, &mut slot, true, " ");
let mut visitor = PrintAdvices(&mut fmt);

print_advices(&mut visitor, diagnostic, self.verbose)
if self.search {
let mut visitor = PrintSearch(&mut fmt);
print_advices(&mut visitor, diagnostic, self.verbose)
} else {
let mut visitor = PrintAdvices(&mut fmt);
print_advices(&mut visitor, diagnostic, self.verbose)
}
}
}

/// Display struct implementing the formatting of a diagnostic header.
struct PrintHeader<'fmt, D: ?Sized>(&'fmt D);
pub(crate) struct PrintHeader<'fmt, D: ?Sized>(pub(crate) &'fmt D);

impl<D: Diagnostic + ?Sized> fmt::Display for PrintHeader<'_, D> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> io::Result<()> {
Expand Down Expand Up @@ -347,6 +363,14 @@ impl<D: Diagnostic + ?Sized> fmt::Display for PrintCauseChain<'_, D> {
}
}

struct PrintSearch<'a, 'b>(&'a mut fmt::Formatter<'b>);

impl Visit for PrintSearch<'_, '_> {
fn record_frame(&mut self, location: Location<'_>) -> io::Result<()> {
frame::print_highlighted_frame(self.0, location)
}
}

/// Implementation of [Visitor] that prints the advices for a diagnostic.
struct PrintAdvices<'a, 'b>(&'a mut fmt::Formatter<'b>);

Expand Down
69 changes: 69 additions & 0 deletions crates/biome_diagnostics/src/display/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,75 @@ pub(super) fn print_frame(fmt: &mut fmt::Formatter<'_>, location: Location<'_>)
fmt.write_str("\n")
}

pub(super) fn print_highlighted_frame(
fmt: &mut fmt::Formatter<'_>,
location: Location<'_>,
) -> io::Result<()> {
let Some(span) = location.span else {
return Ok(());
};
let Some(source_code) = location.source_code else {
return Ok(());
};

// TODO: instead of calculating lines for every match,
// check if the Grit engine is able to pull them out
let source = SourceFile::new(source_code);

let start = source.location(span.start())?;
let end = source.location(span.end())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this makes me realize we need to calculate line numbers for every match, which can potentially be a bit expensive. No big deal either way, but a good reminder that I may want to see if the Grit engine can already report the line numbers for us. At least it tracks them for some purposes already (though not everywhere), so I may be able to pull those out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like me to add a TODO to keep track of this possibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be appreciated!


let match_line_start = start.line_number;
let match_line_end = end.line_number.saturating_add(1);

for line_index in IntoIter::new(match_line_start..match_line_end) {
let current_range = source.line_range(line_index.to_zero_indexed());
let current_range = match current_range {
Ok(v) => v,
Err(_) => continue,
};

let current_text = source_code.text[current_range].trim_end_matches(['\r', '\n']);

let is_first_line = line_index == start.line_number;
let is_last_line = line_index == end.line_number;

let start_index_relative_to_line =
span.start().max(current_range.start()) - current_range.start();
let end_index_relative_to_line =
span.end().min(current_range.end()) - current_range.start();

let marker = if is_first_line && is_last_line {
TextRange::new(start_index_relative_to_line, end_index_relative_to_line)
} else if is_last_line {
let start_index: u32 = current_text.text_len().into();

let safe_start_index =
start_index.saturating_sub(current_text.trim_start().text_len().into());

TextRange::new(TextSize::from(safe_start_index), end_index_relative_to_line)
} else {
TextRange::new(start_index_relative_to_line, current_text.text_len())
};

fmt.write_markup(markup! {
<Emphasis>{format_args!("{line_index} \u{2502} ")}</Emphasis>
})?;

let start_range = &current_text[0..marker.start().into()];
let highlighted_range = &current_text[marker.start().into()..marker.end().into()];
let end_range = &current_text[marker.end().into()..current_text.text_len().into()];

write!(fmt, "{start_range}")?;
fmt.write_markup(markup! { <Emphasis><Info>{highlighted_range}</Info></Emphasis> })?;
write!(fmt, "{end_range}")?;

writeln!(fmt)?;
}

Ok(())
}

/// Calculate the length of the string representation of `value`
pub(super) fn calculate_print_width(mut value: OneIndexed) -> NonZeroUsize {
// SAFETY: Constant is being initialized with a non-zero value
Expand Down
1 change: 1 addition & 0 deletions crates/biome_diagnostics/src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ impl From<DiagnosticTag> for DiagnosticTags {
DiagnosticTag::UnnecessaryCode => DiagnosticTags::UNNECESSARY_CODE,
DiagnosticTag::DeprecatedCode => DiagnosticTags::DEPRECATED_CODE,
DiagnosticTag::Verbose => DiagnosticTags::VERBOSE,
DiagnosticTag::Search => DiagnosticTags::SEARCH,
}
}
}
Expand Down
36 changes: 33 additions & 3 deletions crates/biome_service/src/workspace/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use biome_json_parser::{parse_json_with_cache, JsonParserOptions};
use biome_json_syntax::JsonFileSource;
use biome_parser::AnyParse;
use biome_project::NodeJsProject;
use biome_rowan::NodeCache;
use biome_rowan::{NodeCache, TextLen, TextRange, TextSize};
use dashmap::{mapref::entry::Entry, DashMap};
use indexmap::IndexSet;
use std::ffi::OsStr;
Expand Down Expand Up @@ -75,6 +75,24 @@ pub(crate) struct Document {
node_cache: NodeCache,
}

// TODO: remove once an actual implementation for the matches is present
struct DummySearchMatchesProvider;

impl DummySearchMatchesProvider {
// a match that goes from the first to the last character of the first line, if present
fn get_range(input: &str) -> Vec<TextRange> {
let mut lines = input.lines();
let first_line = lines.next();

let max_size = match first_line {
Some(v) => v.text_len(),
None => return vec![],
};

vec![TextRange::new(TextSize::from(0), max_size)]
}
}

impl WorkspaceServer {
/// Create a new [Workspace]
///
Expand Down Expand Up @@ -783,10 +801,22 @@ impl Workspace for WorkspaceServer {
}

fn search_pattern(&self, params: SearchPatternParams) -> Result<SearchResults, WorkspaceError> {
let SearchPatternParams { path, .. } = params;

// FIXME: Let's implement some real matching here...
Copy link
Member

Choose a reason for hiding this comment

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

Is the FIXME still applicable? I see there's some new code, maybe the FIXME should become a TODO or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A TODO definitely sounds better, thanks! 👍


let document = self
.documents
.get_mut(&path)
.ok_or_else(WorkspaceError::not_found)?;

let content = document.content.as_str();

let match_ranges = DummySearchMatchesProvider::get_range(content);

Ok(SearchResults {
file: params.path,
matches: Vec::new(),
file: path,
matches: match_ranges,
})
}

Expand Down