Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_diagnostics): allow diagnostic locations to be created without a resource #3834

Merged
merged 2 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
17 changes: 8 additions & 9 deletions crates/rome_analyze/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl Diagnostic for AnalyzerDiagnostic {
}
}

fn location(&self) -> Option<Location<'_>> {
fn location(&self) -> Location<'_> {
match &self.kind {
DiagnosticKind::Rule {
rule_diagnostic,
Expand Down Expand Up @@ -111,13 +111,12 @@ impl Diagnostic for AnalyzerDiagnostic {
detail.log_category,
&markup! { {detail.message} }.to_owned(),
)?;
if let Some(location) = Location::builder()
.span(&detail.range)
.resource(file_id)
.build()
{
visitor.record_frame(location)?;
}
visitor.record_frame(
Location::builder()
.span(&detail.range)
.resource(file_id)
.build(),
)?;
}
// we then print notes
for (log_category, note) in &rule_advices.notes {
Expand Down Expand Up @@ -169,7 +168,7 @@ impl AnalyzerDiagnostic {
DiagnosticKind::Rule {
rule_diagnostic, ..
} => rule_diagnostic.span,
DiagnosticKind::Raw(error) => error.location().and_then(|location| location.span),
DiagnosticKind::Raw(error) => error.location().span,
}
}

Expand Down
4 changes: 1 addition & 3 deletions crates/rome_analyze/src/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,7 @@ impl Advices for RuleAdvice {
detail.log_category,
&markup! { {detail.message} }.to_owned(),
)?;
if let Some(location) = Location::builder().span(&detail.range).build() {
visitor.record_frame(location)?;
}
visitor.record_frame(Location::builder().span(&detail.range).build())?;
}
// we then print notes
for (log_category, note) in &self.notes {
Expand Down
66 changes: 30 additions & 36 deletions crates/rome_cli/src/traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,34 +368,33 @@ fn process_messages(options: ProcessMessagesOptions) {
}

Message::Error(mut err) => {
if let Some(location) = err.location() {
if let Resource::File(FilePath::FileId(file_id)) = location.resource {
// Retrieves the file name from the file ID cache, if it's a miss
// flush entries from the interner channel until it's found
let file_name = match paths.get(&file_id) {
Some(path) => Some(path),
None => loop {
match recv_files.recv() {
Ok((id, path)) => {
paths.insert(id, path.display().to_string());
if id == file_id {
break Some(&paths[&file_id]);
}
let location = err.location();
if let Some(Resource::File(FilePath::FileId(file_id))) = &location.resource {
// Retrieves the file name from the file ID cache, if it's a miss
// flush entries from the interner channel until it's found
let file_name = match paths.get(file_id) {
Some(path) => Some(path),
None => loop {
match recv_files.recv() {
Ok((id, path)) => {
paths.insert(id, path.display().to_string());
if id == *file_id {
break Some(&paths[file_id]);
}
// In case the channel disconnected without sending
// the path we need, print the error without a file
// name (normally this should never happen)
Err(_) => break None,
}
},
};
// In case the channel disconnected without sending
// the path we need, print the error without a file
// name (normally this should never happen)
Err(_) => break None,
}
},
};

if let Some(path) = file_name {
err = err.with_file_path(FilePath::PathAndId {
path: path.as_str(),
file_id,
});
}
if let Some(path) = file_name {
err = err.with_file_path(FilePath::PathAndId {
path: path.as_str(),
file_id: *file_id,
});
}
}

Expand All @@ -417,18 +416,13 @@ fn process_messages(options: ProcessMessagesOptions) {
});
}
} else {
let file_name = err
.location()
.and_then(|location| {
let path = match &location.resource {
Resource::File(file) => file,
_ => return None,
};

path.path()
})
.unwrap_or("<unknown>");
let location = err.location();
let path = match &location.resource {
Some(Resource::File(file)) => file.path(),
_ => None,
};

let file_name = path.unwrap_or("<unknown>");
let title = PrintDescription(&err).to_string();
let code = err.category().and_then(|code| code.name().parse().ok());

Expand Down
2 changes: 1 addition & 1 deletion crates/rome_diagnostics/examples/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ impl Advices for NotFoundAdvices {

visitor.record_log(LogCategory::Info, &"Ignore patterns were defined here")?;
visitor.record_frame(Location {
resource: Resource::File(FilePath::Path(&self.configuration_path)),
resource: Some(Resource::File(FilePath::Path(&self.configuration_path))),
span: Some(self.configuration_span),
source_code: Some(SourceCode {
text: &self.configuration_source_code,
Expand Down
2 changes: 1 addition & 1 deletion crates/rome_diagnostics/examples/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl Advices for LintAdvices {

visitor.record_log(LogCategory::Info, &"This constant is declared here")?;
visitor.record_frame(Location {
resource: Resource::File(FilePath::Path(&self.path)),
resource: Some(Resource::File(FilePath::Path(&self.path))),
span: Some(self.declaration_span),
source_code: Some(SourceCode {
text: &self.source_code,
Expand Down
4 changes: 1 addition & 3 deletions crates/rome_diagnostics/src/advice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,7 @@ where
.source_code(&self.source_code)
.build();

if let Some(location) = location {
visitor.record_frame(location)?;
}
visitor.record_frame(location)?;

Ok(())
}
Expand Down
56 changes: 24 additions & 32 deletions crates/rome_diagnostics/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ mod internal {
self.source.as_diagnostic().verbose_advices(visitor)
}

fn location(&self) -> Option<Location<'_>> {
fn location(&self) -> Location<'_> {
self.source.as_diagnostic().location()
}

Expand Down Expand Up @@ -321,7 +321,7 @@ mod internal {
self.source.as_diagnostic().verbose_advices(visitor)
}

fn location(&self) -> Option<Location<'_>> {
fn location(&self) -> Location<'_> {
self.source.as_diagnostic().location()
}

Expand Down Expand Up @@ -371,32 +371,24 @@ mod internal {
self.source.as_diagnostic().verbose_advices(visitor)
}

fn location(&self) -> Option<Location<'_>> {
self.source
.as_diagnostic()
.location()
.map(|loc| Location {
resource: match loc.resource {
Resource::Argv => Resource::Argv,
Resource::Memory => Resource::Memory,
Resource::File(file) => {
if let Some(Resource::File(path)) = &self.path {
Resource::File(file.or(path.as_deref()))
} else {
Resource::File(file)
}
fn location(&self) -> Location<'_> {
let loc = self.source.as_diagnostic().location();
Location {
resource: match loc.resource {
Some(Resource::Argv) => Some(Resource::Argv),
Some(Resource::Memory) => Some(Resource::Memory),
Some(Resource::File(file)) => {
if let Some(Resource::File(path)) = &self.path {
Some(Resource::File(file.or(path.as_deref())))
} else {
Some(Resource::File(file))
}
},
span: loc.span,
source_code: loc.source_code,
})
.or_else(|| {
Some(Location {
resource: self.path.as_ref()?.as_deref(),
span: None,
source_code: None,
})
})
}
None => self.path.as_ref().map(Resource::as_deref),
},
span: loc.span,
source_code: loc.source_code,
}
}

fn tags(&self) -> DiagnosticTags {
Expand Down Expand Up @@ -464,14 +456,14 @@ mod internal {
}
}

fn location(&self) -> Option<Location<'_>> {
let location = self.source.as_diagnostic().location()?;
Some(Location {
fn location(&self) -> Location<'_> {
let location = self.source.as_diagnostic().location();
Location {
source_code: location
.source_code
.or_else(|| Some(self.source_code.as_ref()?.as_deref())),
..location
})
}
}

fn tags(&self) -> DiagnosticTags {
Expand Down Expand Up @@ -568,7 +560,7 @@ mod internal {
self.source.as_diagnostic().verbose_advices(visitor)
}

fn location(&self) -> Option<Location<'_>> {
fn location(&self) -> Location<'_> {
self.source.as_diagnostic().location()
}

Expand Down
4 changes: 2 additions & 2 deletions crates/rome_diagnostics/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ pub trait Diagnostic: Debug {
/// specific text range within the content of that location. Finally, it
/// may also provide the source string for that location (this is required
/// in order to display a code frame advice for the diagnostic).
fn location(&self) -> Option<Location<'_>> {
None
fn location(&self) -> Location<'_> {
Location::builder().build()
}

/// Tags convey additional boolean metadata about the nature of a diagnostic:
Expand Down
65 changes: 30 additions & 35 deletions crates/rome_diagnostics/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,30 +64,28 @@ impl<'fmt, D: Diagnostic + ?Sized> fmt::Display for PrintHeader<'fmt, D> {
let mut slot = None;
let mut fmt = CountWidth::wrap(f, &mut slot);

// Print the diagnostic location if it has one
if let Some(location) = diagnostic.location() {
// Print the path if it's a file
let file_name = match &location.resource {
Resource::File(file) => file.path(),
_ => None,
};

if let Some(name) = file_name {
fmt.write_str(name)?;

// Print the line and column position if the location has a span and source code
// (the source code is necessary to convert a byte offset into a line + column)
if let (Some(span), Some(source_code)) = (location.span, location.source_code) {
let file = SourceFile::new(source_code);
if let Ok(location) = file.location(span.start()) {
fmt.write_markup(markup! {
":"{location.line_number.get()}":"{location.column_number.get()}
})?;
}
}
// Print the diagnostic location if it has a file path
let location = diagnostic.location();
let file_name = match &location.resource {
Some(Resource::File(file)) => file.path(),
_ => None,
};

fmt.write_str(" ")?;
if let Some(name) = file_name {
fmt.write_str(name)?;

// Print the line and column position if the location has a span and source code
// (the source code is necessary to convert a byte offset into a line + column)
if let (Some(span), Some(source_code)) = (location.span, location.source_code) {
let file = SourceFile::new(source_code);
if let Ok(location) = file.location(span.start()) {
fmt.write_markup(markup! {
":"{location.line_number.get()}":"{location.column_number.get()}
})?;
}
}

fmt.write_str(" ")?;
}

// Print the category of the diagnostic, with a hyperlink if
Expand Down Expand Up @@ -182,18 +180,14 @@ where
{
// Visit the advices of the diagnostic with a lightweight visitor that
// detects if the diagnostic has any frame or backtrace advice
let skip_frame = if let Some(location) = diagnostic.location() {
let mut frame_visitor = FrameVisitor {
location,
skip_frame: false,
};
let mut frame_visitor = FrameVisitor {
location: diagnostic.location(),
skip_frame: false,
};

diagnostic.advices(&mut frame_visitor)?;
diagnostic.advices(&mut frame_visitor)?;

frame_visitor.skip_frame
} else {
false
};
let skip_frame = frame_visitor.skip_frame;

// Print the message for the diagnostic as a log advice
print_message_advice(visitor, diagnostic, skip_frame)?;
Expand Down Expand Up @@ -276,7 +270,8 @@ where
// If the diagnostic has no explicit code frame or backtrace advice, print
// a code frame advice with the location of the diagnostic
if !skip_frame {
if let Some(location) = diagnostic.location().filter(|loc| loc.span.is_some()) {
let location = diagnostic.location();
if location.span.is_some() {
visitor.record_frame(location)?;
}
}
Expand Down Expand Up @@ -624,7 +619,7 @@ mod tests {
Ok(())
}

fn location(&self) -> Option<Location<'_>> {
fn location(&self) -> Location<'_> {
Location::builder()
.resource(&self.path)
.span(&self.span)
Expand Down Expand Up @@ -668,7 +663,7 @@ mod tests {
impl Advices for FrameAdvice {
fn record(&self, visitor: &mut dyn Visit) -> io::Result<()> {
visitor.record_frame(Location {
resource: Resource::File(FilePath::Path("other_path")),
resource: Some(Resource::File(FilePath::Path("other_path"))),
span: Some(TextRange::new(TextSize::from(8), TextSize::from(16))),
source_code: Some(SourceCode {
text: "context location context",
Expand Down
2 changes: 1 addition & 1 deletion crates/rome_diagnostics/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl Error {
}

/// Calls [Diagnostic::location] on the [Diagnostic] wrapped by this [Error].
pub fn location(&self) -> Option<Location<'_>> {
pub fn location(&self) -> Location<'_> {
self.as_diagnostic().location()
}

Expand Down
Loading