From fc21439d650f83ec8551d1ae0633bd19446a9dee Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 24 Jul 2024 17:51:09 -0400 Subject: [PATCH] feat: support diagnostics without ranges (#1299) --- examples/dlint/diagnostics.rs | 33 +++++--- src/context.rs | 140 ++++++++++++++++++++++------------ src/diagnostic.rs | 67 ++++++++++------ src/linter.rs | 12 ++- src/test_util.rs | 29 +++---- 5 files changed, 185 insertions(+), 96 deletions(-) diff --git a/examples/dlint/diagnostics.rs b/examples/dlint/diagnostics.rs index 21527d70..d010d83d 100644 --- a/examples/dlint/diagnostics.rs +++ b/examples/dlint/diagnostics.rs @@ -16,17 +16,28 @@ pub fn display_diagnostics( fn print_compact(diagnostics: &[LintDiagnostic]) { for diagnostic in diagnostics { - let display_index = diagnostic - .text_info - .line_and_column_display(diagnostic.range.start); - eprintln!( - "{}: line {}, col {}, Error - {} ({})", - diagnostic.specifier, - display_index.line_number, - display_index.column_number, - diagnostic.message, - diagnostic.code - ) + match &diagnostic.range { + Some(range) => { + let display_index = + range.text_info.line_and_column_display(range.range.start); + eprintln!( + "{}: line {}, col {}, Error - {} ({})", + diagnostic.specifier, + display_index.line_number, + display_index.column_number, + diagnostic.details.message, + diagnostic.details.code + ) + } + None => { + eprintln!( + "{}: {} ({})", + diagnostic.specifier, + diagnostic.message(), + diagnostic.code() + ) + } + } } } diff --git a/src/context.rs b/src/context.rs index 17c8f5ed..23812967 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,7 +1,9 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::control_flow::ControlFlow; -use crate::diagnostic::{LintDiagnostic, LintFix}; +use crate::diagnostic::{ + LintDiagnostic, LintDiagnosticDetails, LintDiagnosticRange, LintFix, +}; use crate::ignore_directives::{ parse_line_ignore_directives, CodeStatus, FileIgnoreDirective, LineIgnoreDirective, @@ -234,18 +236,20 @@ impl<'a> Context<'a> { for diagnostic in self.diagnostics.iter().cloned() { if let Some(f) = self.file_ignore_directive.as_mut() { - if f.check_used(&diagnostic.code) { + if f.check_used(&diagnostic.details.code) { continue; } } + let Some(range) = diagnostic.range.as_ref() else { + continue; + }; - let diagnostic_line = - diagnostic.text_info.line_index(diagnostic.range.start); + let diagnostic_line = range.text_info.line_index(range.range.start); if diagnostic_line > 0 { if let Some(l) = self.line_ignore_directives.get_mut(&(diagnostic_line - 1)) { - if l.check_used(&diagnostic.code) { + if l.check_used(&diagnostic.details.code) { continue; } } @@ -290,11 +294,13 @@ impl<'a> Context<'a> { file_ignore.codes().iter().filter(is_unused_code) { let d = self.create_diagnostic( - file_ignore.range(), - CODE, - format!("Ignore for code \"{}\" was not used.", unused_code), - None, - Vec::new(), + Some(self.create_diagnostic_range(file_ignore.range())), + self.create_diagnostic_details( + CODE, + format!("Ignore for code \"{}\" was not used.", unused_code), + None, + Vec::new(), + ), ); diagnostics.push(d); } @@ -309,11 +315,13 @@ impl<'a> Context<'a> { line_ignore.codes().iter().filter(is_unused_code) { let d = self.create_diagnostic( - line_ignore.range(), - CODE, - format!("Ignore for code \"{}\" was not used.", unused_code), - None, - Vec::new(), + Some(self.create_diagnostic_range(line_ignore.range())), + self.create_diagnostic_details( + CODE, + format!("Ignore for code \"{}\" was not used.", unused_code), + None, + Vec::new(), + ), ); diagnostics.push(d); } @@ -337,11 +345,13 @@ impl<'a> Context<'a> { file_ignore.codes().keys().filter(is_unknown_rule) { let d = self.create_diagnostic( - file_ignore.range(), - rules::ban_unknown_rule_code::CODE, - format!("Unknown rule for code \"{}\"", unknown_rule_code), - None, - Vec::new(), + Some(self.create_diagnostic_range(file_ignore.range())), + self.create_diagnostic_details( + rules::ban_unknown_rule_code::CODE, + format!("Unknown rule for code \"{}\"", unknown_rule_code), + None, + Vec::new(), + ), ); diagnostics.push(d); } @@ -352,11 +362,13 @@ impl<'a> Context<'a> { line_ignore.codes().keys().filter(is_unknown_rule) { let d = self.create_diagnostic( - line_ignore.range(), - rules::ban_unknown_rule_code::CODE, - format!("Unknown rule for code \"{}\"", unknown_rule_code), - None, - Vec::new(), + Some(self.create_diagnostic_range(line_ignore.range())), + self.create_diagnostic_details( + rules::ban_unknown_rule_code::CODE, + format!("Unknown rule for code \"{}\"", unknown_rule_code), + None, + Vec::new(), + ), ); diagnostics.push(d); } @@ -386,14 +398,15 @@ impl<'a> Context<'a> { code: impl ToString, message: impl ToString, ) { - let diagnostic = self.create_diagnostic( - range, - code, - message.to_string(), - None, - Vec::new(), + self.add_diagnostic_details( + Some(self.create_diagnostic_range(range)), + self.create_diagnostic_details( + code, + message.to_string(), + None, + Vec::new(), + ), ); - self.diagnostics.push(diagnostic); } pub fn add_diagnostic_with_hint( @@ -403,14 +416,15 @@ impl<'a> Context<'a> { message: impl ToString, hint: impl ToString, ) { - let diagnostic = self.create_diagnostic( - range, - code, - message, - Some(hint.to_string()), - Vec::new(), + self.add_diagnostic_details( + Some(self.create_diagnostic_range(range)), + self.create_diagnostic_details( + code, + message, + Some(hint.to_string()), + Vec::new(), + ), ); - self.diagnostics.push(diagnostic); } pub fn add_diagnostic_with_fixes( @@ -421,31 +435,61 @@ impl<'a> Context<'a> { hint: Option, fixes: Vec, ) { - let diagnostic = self.create_diagnostic(range, code, message, hint, fixes); - self.diagnostics.push(diagnostic); + self.add_diagnostic_details( + Some(self.create_diagnostic_range(range)), + self.create_diagnostic_details(code, message, hint, fixes), + ); + } + + pub fn add_diagnostic_details( + &mut self, + maybe_range: Option, + details: LintDiagnosticDetails, + ) { + self + .diagnostics + .push(self.create_diagnostic(maybe_range, details)); } pub(crate) fn create_diagnostic( &self, - range: SourceRange, + maybe_range: Option, + details: LintDiagnosticDetails, + ) -> LintDiagnostic { + LintDiagnostic { + specifier: self.specifier().clone(), + range: maybe_range, + details, + } + } + + pub(crate) fn create_diagnostic_details( + &self, code: impl ToString, message: impl ToString, maybe_hint: Option, fixes: Vec, - ) -> LintDiagnostic { - LintDiagnostic { - specifier: self.specifier().clone(), - range, - text_info: self.text_info().clone(), + ) -> LintDiagnosticDetails { + LintDiagnosticDetails { message: message.to_string(), code: code.to_string(), hint: maybe_hint, fixes, custom_docs_url: None, - range_description: None, info: vec![], } } + + pub(crate) fn create_diagnostic_range( + &self, + range: SourceRange, + ) -> LintDiagnosticRange { + LintDiagnosticRange { + range, + text_info: self.text_info().clone(), + description: None, + } + } } /// A struct containing a boolean value to control whether a node's children diff --git a/src/diagnostic.rs b/src/diagnostic.rs index f664b2e1..830f5874 100644 --- a/src/diagnostic.rs +++ b/src/diagnostic.rs @@ -27,10 +27,15 @@ pub struct LintFix { } #[derive(Clone)] -pub struct LintDiagnostic { - pub specifier: ModuleSpecifier, - pub range: SourceRange, +pub struct LintDiagnosticRange { pub text_info: SourceTextInfo, + pub range: SourceRange, + /// Additional information displayed beside the highlighted range. + pub description: Option, +} + +#[derive(Clone)] +pub struct LintDiagnosticDetails { pub message: String, pub code: String, pub hint: Option, @@ -44,50 +49,68 @@ pub struct LintDiagnostic { /// URL to the lint rule documentation. By default, the url uses the /// code to link to lint.deno.land pub custom_docs_url: Option, - /// Additional information displayed beside the highlighted range. - pub range_description: Option, /// Displays additional information at the end of a diagnostic. pub info: Vec>, } +#[derive(Clone)] +pub struct LintDiagnostic { + pub specifier: ModuleSpecifier, + /// Optional range within the file. + /// + /// Diagnostics that don't have a range mean there's something wrong with + /// the whole file. + pub range: Option, + pub details: LintDiagnosticDetails, +} + impl Diagnostic for LintDiagnostic { fn level(&self) -> DiagnosticLevel { DiagnosticLevel::Error } fn code(&self) -> Cow<'_, str> { - Cow::Borrowed(&self.code) + Cow::Borrowed(&self.details.code) } fn message(&self) -> Cow<'_, str> { - Cow::Borrowed(&self.message) + Cow::Borrowed(&self.details.message) } fn location(&self) -> DiagnosticLocation { - DiagnosticLocation::ModulePosition { - specifier: Cow::Borrowed(&self.specifier), - text_info: Cow::Borrowed(&self.text_info), - source_pos: DiagnosticSourcePos::SourcePos(self.range.start), + match &self.range { + Some(range) => DiagnosticLocation::ModulePosition { + specifier: Cow::Borrowed(&self.specifier), + text_info: Cow::Borrowed(&range.text_info), + source_pos: DiagnosticSourcePos::SourcePos(range.range.start), + }, + None => DiagnosticLocation::Module { + specifier: Cow::Borrowed(&self.specifier), + }, } } fn snippet(&self) -> Option> { - let range = DiagnosticSourceRange { - start: DiagnosticSourcePos::SourcePos(self.range.start), - end: DiagnosticSourcePos::SourcePos(self.range.end), - }; + let range = self.range.as_ref()?; Some(DiagnosticSnippet { - source: Cow::Borrowed(&self.text_info), + source: Cow::Borrowed(&range.text_info), highlights: vec![DiagnosticSnippetHighlight { - range, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::SourcePos(range.range.start), + end: DiagnosticSourcePos::SourcePos(range.range.end), + }, style: DiagnosticSnippetHighlightStyle::Error, - description: self.range_description.as_deref().map(Cow::Borrowed), + description: range.description.as_deref().map(Cow::Borrowed), }], }) } fn hint(&self) -> Option> { - self.hint.as_ref().map(|s| Cow::Borrowed(s.as_str())) + self + .details + .hint + .as_ref() + .map(|s| Cow::Borrowed(s.as_str())) } fn snippet_fixed(&self) -> Option> { @@ -95,16 +118,16 @@ impl Diagnostic for LintDiagnostic { } fn info(&self) -> Cow<'_, [std::borrow::Cow<'_, str>]> { - Cow::Borrowed(&self.info) + Cow::Borrowed(&self.details.info) } fn docs_url(&self) -> Option> { - if let Some(custom_docs_url) = &self.custom_docs_url { + if let Some(custom_docs_url) = &self.details.custom_docs_url { Some(Cow::Borrowed(custom_docs_url)) } else { Some(Cow::Owned(format!( "https://lint.deno.land/rules/{}", - &self.code + &self.details.code ))) } } diff --git a/src/linter.rs b/src/linter.rs index 4ed4ff21..2c5ebb79 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -6,6 +6,7 @@ use crate::diagnostic::LintDiagnostic; use crate::ignore_directives::parse_file_ignore_directives; use crate::performance_mark::PerformanceMark; use crate::rules::{ban_unknown_rule_code::BanUnknownRuleCode, LintRule}; +use deno_ast::diagnostics::Diagnostic; use deno_ast::MediaType; use deno_ast::ParsedSource; use deno_ast::{ModuleSpecifier, ParseDiagnostic}; @@ -140,8 +141,15 @@ impl Linter { // Run `ban-unused-ignore` diagnostics.extend(context.ban_unused_ignore(&self.ctx.rules)); - // Finally sort by position the diagnostics originates on - diagnostics.sort_by_key(|d| d.range.start); + // Finally sort by position the diagnostics originates on then by code + diagnostics.sort_by(|a, b| { + let a_range = a.range.as_ref().map(|r| r.range.start); + let b_range = b.range.as_ref().map(|r| r.range.start); + match a_range.cmp(&b_range) { + std::cmp::Ordering::Equal => a.code().cmp(&b.code()), + cmp => cmp, + } + }); diagnostics } diff --git a/src/test_util.rs b/src/test_util.rs index b685911c..9fed7ef7 100644 --- a/src/test_util.rs +++ b/src/test_util.rs @@ -209,7 +209,7 @@ impl LintErrTester { "Actual diagnostics:\n{:#?}", diagnostics .iter() - .map(|d| d.message.to_string()) + .map(|d| d.details.message.to_string()) .collect::>() ); assert_eq!( @@ -358,10 +358,11 @@ pub fn assert_diagnostic( col: usize, source: &str, ) { - let line_and_column = diagnostic + let diagnostic_range = diagnostic.range.as_ref().unwrap(); + let line_and_column = diagnostic_range .text_info - .line_and_column_index(diagnostic.range.start); - if diagnostic.code == code + .line_and_column_index(diagnostic_range.range.start); + if diagnostic.details.code == code // todo(dsherret): we should change these to be consistent (ex. both 1-indexed) && line_and_column.line_index + 1 == line && line_and_column.column_index == col @@ -370,7 +371,7 @@ pub fn assert_diagnostic( } panic!( "expect diagnostics {} at {}:{} to be {} at {}:{}\n\nsource:\n{}\n", - diagnostic.code, + diagnostic.details.code, line_and_column.line_index + 1, line_and_column.column_index, code, @@ -393,13 +394,14 @@ fn assert_diagnostic_2( fixes: &[LintErrFix], text_info: &SourceTextInfo, ) { - let line_and_column = diagnostic + let diagnostic_range = diagnostic.range.as_ref().unwrap(); + let line_and_column = diagnostic_range .text_info - .line_and_column_index(diagnostic.range.start); + .line_and_column_index(diagnostic_range.range.start); assert_eq!( - code, diagnostic.code, + code, diagnostic.details.code, "Rule code is expected to be \"{}\", but got \"{}\"\n\nsource:\n{}\n", - code, diagnostic.code, source + code, diagnostic.details.code, source ); assert_eq!( line, @@ -415,19 +417,20 @@ fn assert_diagnostic_2( col, line_and_column.column_index, source ); assert_eq!( - message, &diagnostic.message, + message, &diagnostic.details.message, "Diagnostic message is expected to be \"{}\", but got \"{}\"\n\nsource:\n{}\n", - message, &diagnostic.message, source + message, &diagnostic.details.message, source ); assert_eq!( hint, - diagnostic.hint.as_deref(), + diagnostic.details.hint.as_deref(), "Diagnostic hint is expected to be \"{:?}\", but got \"{:?}\"\n\nsource:\n{}\n", hint, - diagnostic.hint.as_deref(), + diagnostic.details.hint.as_deref(), source ); let actual_fixes = diagnostic + .details .fixes .iter() .map(|fix| LintErrFix {