diff --git a/Cargo.lock b/Cargo.lock index a83b513870..4c3440e8c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1037,6 +1037,7 @@ dependencies = [ "trust-dns-server", "twox-hash", "typed-arena", + "unicode-width", "uuid", "walkdir", "winapi", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 6fdf7c51a2..1a74469eab 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -131,6 +131,7 @@ tokio-util.workspace = true tower-lsp.workspace = true twox-hash = "=1.6.3" typed-arena = "=2.0.1" +unicode-width = "0.1" uuid = { workspace = true, features = ["serde"] } walkdir = "=2.3.2" zeromq = { version = "=0.3.4", default-features = false, features = ["tcp-transport", "tokio-runtime"] } diff --git a/cli/diagnostics.rs b/cli/diagnostics.rs new file mode 100644 index 0000000000..eb8a0de60a --- /dev/null +++ b/cli/diagnostics.rs @@ -0,0 +1,641 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +use std::borrow::Cow; +use std::fmt; +use std::fmt::Display; +use std::fmt::Write as _; + +use deno_ast::ModuleSpecifier; +use deno_ast::SourcePos; +use deno_ast::SourceRange; +use deno_ast::SourceRanged; +use deno_ast::SourceTextInfo; +use deno_graph::ParsedSourceStore; +use deno_runtime::colors; +use unicode_width::UnicodeWidthStr; + +pub trait SourceTextStore { + fn get_source_text<'a>( + &'a self, + specifier: &ModuleSpecifier, + ) -> Option>; +} + +pub struct SourceTextParsedSourceStore<'a>(pub &'a dyn ParsedSourceStore); + +impl SourceTextStore for SourceTextParsedSourceStore<'_> { + fn get_source_text<'a>( + &'a self, + specifier: &ModuleSpecifier, + ) -> Option> { + let parsed_source = self.0.get_parsed_source(specifier)?; + Some(Cow::Owned(parsed_source.text_info().clone())) + } +} + +pub enum DiagnosticLevel { + Error, + Warning, +} + +#[derive(Clone, Copy, Debug)] +pub struct DiagnosticSourceRange { + pub start: DiagnosticSourcePos, + pub end: DiagnosticSourcePos, +} + +#[derive(Clone, Copy, Debug)] +pub enum DiagnosticSourcePos { + SourcePos(SourcePos), + ByteIndex(usize), +} + +impl DiagnosticSourcePos { + fn pos(&self, source: &SourceTextInfo) -> SourcePos { + match self { + DiagnosticSourcePos::SourcePos(pos) => *pos, + DiagnosticSourcePos::ByteIndex(index) => source.range().start() + *index, + } + } +} + +#[derive(Clone, Debug)] +pub enum DiagnosticLocation<'a> { + /// The diagnostic is relevant to an entire file. + File { + /// The specifier of the module that contains the diagnostic. + specifier: Cow<'a, ModuleSpecifier>, + }, + /// The diagnostic is relevant to a specific position in a file. + /// + /// This variant will get the relevant `SouceTextInfo` from the cache using + /// the given specifier, and will then calculate the line and column numbers + /// from the given `SourcePos`. + PositionInFile { + /// The specifier of the module that contains the diagnostic. + specifier: Cow<'a, ModuleSpecifier>, + /// The source position of the diagnostic. + source_pos: DiagnosticSourcePos, + }, +} + +impl<'a> DiagnosticLocation<'a> { + fn specifier(&self) -> &ModuleSpecifier { + match self { + DiagnosticLocation::File { specifier } => specifier, + DiagnosticLocation::PositionInFile { specifier, .. } => specifier, + } + } + + /// Return the line and column number of the diagnostic. + /// + /// The line number is 1-indexed. + /// + /// The column number is 1-indexed. This is the number of UTF-16 code units + /// from the start of the line to the diagnostic. + /// Why UTF-16 code units? Because that's what VS Code understands, and + /// everyone uses VS Code. :) + fn position(&self, sources: &dyn SourceTextStore) -> Option<(usize, usize)> { + match self { + DiagnosticLocation::File { .. } => None, + DiagnosticLocation::PositionInFile { + specifier, + source_pos, + } => { + let source = sources.get_source_text(specifier).expect( + "source text should be in the cache if the location is in a file", + ); + let pos = source_pos.pos(&source); + let line_index = source.line_index(pos); + let line_start_pos = source.line_start(line_index); + let content = source.range_text(&SourceRange::new(line_start_pos, pos)); + let line = line_index + 1; + let column = content.encode_utf16().count() + 1; + Some((line, column)) + } + } + } +} + +pub struct DiagnosticSnippet<'a> { + /// The source text for this snippet. The + pub source: DiagnosticSnippetSource<'a>, + /// The piece of the snippet that should be highlighted. + pub highlight: DiagnosticSnippetHighlight<'a>, +} + +pub struct DiagnosticSnippetHighlight<'a> { + /// The range of the snippet that should be highlighted. + pub range: DiagnosticSourceRange, + /// The style of the highlight. + pub style: DiagnosticSnippetHighlightStyle, + /// An optional inline description of the highlight. + pub description: Option>, +} + +pub enum DiagnosticSnippetHighlightStyle { + /// The highlight is an error. This will place red carets under the highlight. + Error, + #[allow(dead_code)] + /// The highlight is a warning. This will place yellow carets under the + /// highlight. + Warning, + #[allow(dead_code)] + /// The highlight shows code additions. This will place green + signs under + /// the highlight and will highlight the code in green. + Addition, + /// The highlight shows a hint. This will place blue dashes under the + /// highlight. + Hint, +} + +impl DiagnosticSnippetHighlightStyle { + fn style_underline( + &self, + s: impl std::fmt::Display, + ) -> impl std::fmt::Display { + match self { + DiagnosticSnippetHighlightStyle::Error => colors::red_bold(s), + DiagnosticSnippetHighlightStyle::Warning => colors::yellow_bold(s), + DiagnosticSnippetHighlightStyle::Addition => colors::green_bold(s), + DiagnosticSnippetHighlightStyle::Hint => colors::intense_blue(s), + } + } + + fn underline_char(&self) -> char { + match self { + DiagnosticSnippetHighlightStyle::Error => '^', + DiagnosticSnippetHighlightStyle::Warning => '^', + DiagnosticSnippetHighlightStyle::Addition => '+', + DiagnosticSnippetHighlightStyle::Hint => '-', + } + } +} + +pub enum DiagnosticSnippetSource<'a> { + /// The specifier of the module that should be displayed in this snippet. The + /// contents of the file will be retrieved from the `SourceTextStore`. + Specifier(Cow<'a, ModuleSpecifier>), + #[allow(dead_code)] + /// The source text that should be displayed in this snippet. + /// + /// This should be used if the text of the snippet is not available in the + /// `SourceTextStore`. + SourceTextInfo(Cow<'a, deno_ast::SourceTextInfo>), +} + +impl<'a> DiagnosticSnippetSource<'a> { + fn to_source_text_info( + &self, + sources: &'a dyn SourceTextStore, + ) -> Cow<'a, SourceTextInfo> { + match self { + DiagnosticSnippetSource::Specifier(specifier) => { + sources.get_source_text(specifier).expect( + "source text should be in the cache if snippet source is a specifier", + ) + } + DiagnosticSnippetSource::SourceTextInfo(info) => info.clone(), + } + } +} + +/// Returns the text of the line with the given number. +fn line_text(source: &SourceTextInfo, line_number: usize) -> &str { + source.line_text(line_number - 1) +} + +/// Returns the text of the line that contains the given position, split at the +/// given position. +fn line_text_split( + source: &SourceTextInfo, + pos: DiagnosticSourcePos, +) -> (&str, &str) { + let pos = pos.pos(source); + let line_index = source.line_index(pos); + let line_start_pos = source.line_start(line_index); + let line_end_pos = source.line_end(line_index); + let before = source.range_text(&SourceRange::new(line_start_pos, pos)); + let after = source.range_text(&SourceRange::new(pos, line_end_pos)); + (before, after) +} + +/// Returns the text of the line that contains the given positions, split at the +/// given positions. +/// +/// If the positions are on different lines, this will panic. +fn line_text_split3( + source: &SourceTextInfo, + start_pos: DiagnosticSourcePos, + end_pos: DiagnosticSourcePos, +) -> (&str, &str, &str) { + let start_pos = start_pos.pos(source); + let end_pos = end_pos.pos(source); + let line_index = source.line_index(start_pos); + assert_eq!( + line_index, + source.line_index(end_pos), + "start and end must be on the same line" + ); + let line_start_pos = source.line_start(line_index); + let line_end_pos = source.line_end(line_index); + let before = source.range_text(&SourceRange::new(line_start_pos, start_pos)); + let between = source.range_text(&SourceRange::new(start_pos, end_pos)); + let after = source.range_text(&SourceRange::new(end_pos, line_end_pos)); + (before, between, after) +} + +/// Returns the line number (1 indexed) of the line that contains the given +/// position. +fn line_number(source: &SourceTextInfo, pos: DiagnosticSourcePos) -> usize { + source.line_index(pos.pos(source)) + 1 +} + +pub trait Diagnostic { + /// The level of the diagnostic. + fn level(&self) -> DiagnosticLevel; + + /// The diagnostic code, like `no-explicit-any` or `ban-untagged-ignore`. + fn code(&self) -> impl fmt::Display + '_; + + /// The human-readable diagnostic message. + fn message(&self) -> impl fmt::Display + '_; + + /// The location this diagnostic is associated with. + fn location(&self) -> DiagnosticLocation; + + /// A snippet showing the source code associated with the diagnostic. + fn snippet(&self) -> Option>; + + /// A hint for fixing the diagnostic. + fn hint(&self) -> Option; + + /// A snippet showing how the diagnostic can be fixed. + fn snippet_fixed(&self) -> Option>; + + fn info(&self) -> Cow<'_, [Cow<'_, str>]>; + + /// An optional URL to the documentation for the diagnostic. + fn docs_url(&self) -> Option; + + fn display<'a>( + &'a self, + sources: &'a dyn SourceTextStore, + ) -> DiagnosticDisplay<'a, Self> { + DiagnosticDisplay { + diagnostic: self, + sources, + } + } +} + +struct RepeatingCharFmt(char, usize); +impl fmt::Display for RepeatingCharFmt { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for _ in 0..self.1 { + f.write_char(self.0)?; + } + Ok(()) + } +} + +/// How many spaces a tab should be displayed as. 2 is the default used for +/// `deno fmt`, so we'll use that here. +const TAB_WIDTH: usize = 2; + +struct ReplaceTab<'a>(&'a str); +impl fmt::Display for ReplaceTab<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut written = 0; + for (i, c) in self.0.char_indices() { + if c == '\t' { + self.0[written..i].fmt(f)?; + RepeatingCharFmt(' ', TAB_WIDTH).fmt(f)?; + written = i + 1; + } + } + self.0[written..].fmt(f)?; + Ok(()) + } +} + +/// The width of the string as displayed, assuming tabs are 2 spaces wide. +/// +/// This display width assumes that zero-width-joined characters are the width +/// of their consituent characters. This means that "Person: Red Hair" (which is +/// represented as "Person" + "ZWJ" + "Red Hair") will have a width of 4. +/// +/// Whether this is correct is unfortunately dependent on the font / terminal +/// being used. Here is a list of what terminals consider the length of +/// "Person: Red Hair" to be: +/// +/// | Terminal | Rendered Width | +/// | ---------------- | -------------- | +/// | Windows Terminal | 5 chars | +/// | iTerm (macOS) | 2 chars | +/// | Terminal (macOS) | 2 chars | +/// | VS Code terminal | 4 chars | +/// | GNOME Terminal | 4 chars | +/// +/// If we really wanted to, we could try and detect the terminal being used and +/// adjust the width accordingly. However, this is probably not worth the +/// effort. +fn display_width(str: &str) -> usize { + str.width_cjk() + (str.chars().filter(|c| *c == '\t').count() * TAB_WIDTH) +} + +pub struct DiagnosticDisplay<'a, T: Diagnostic + ?Sized> { + diagnostic: &'a T, + sources: &'a dyn SourceTextStore, +} + +impl Display for DiagnosticDisplay<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + print_diagnostic(f, self.sources, self.diagnostic) + } +} + +// error[missing-return-type]: missing explicit return type on public function +// at /mnt/artemis/Projects/github.com/denoland/deno/test.ts:1:16 +// | +// 1 | export function test() { +// | ^^^^ +// = hint: add an explicit return type to the function +// | +// 1 | export function test(): string { +// | ^^^^^^^^ +// +// info: all functions that are exported from a module must have an explicit return type to support fast check and documentation generation. +// docs: https://jsr.io/d/missing-return-type +fn print_diagnostic( + io: &mut dyn std::fmt::Write, + sources: &dyn SourceTextStore, + diagnostic: &(impl Diagnostic + ?Sized), +) -> Result<(), std::fmt::Error> { + match diagnostic.level() { + DiagnosticLevel::Error => { + write!( + io, + "{}", + colors::red_bold(format_args!("error[{}]", diagnostic.code())) + )?; + } + DiagnosticLevel::Warning => { + write!( + io, + "{}", + colors::yellow(format_args!("warning[{}]", diagnostic.code())) + )?; + } + } + + writeln!(io, ": {}", colors::bold(diagnostic.message()))?; + + let mut max_line_number_digits = 1; + if let Some(snippet) = diagnostic.snippet() { + let source = snippet.source.to_source_text_info(sources); + let last_line = line_number(&source, snippet.highlight.range.end); + max_line_number_digits = max_line_number_digits.max(last_line.ilog10() + 1); + } + if let Some(snippet) = diagnostic.snippet_fixed() { + let source = snippet.source.to_source_text_info(sources); + let last_line = line_number(&source, snippet.highlight.range.end); + max_line_number_digits = max_line_number_digits.max(last_line.ilog10() + 1); + } + + let location = diagnostic.location(); + write!( + io, + "{}{}", + RepeatingCharFmt(' ', max_line_number_digits as usize), + colors::intense_blue("-->"), + )?; + let location_specifier = location.specifier(); + if let Ok(path) = location_specifier.to_file_path() { + write!(io, " {}", colors::cyan(path.display()))?; + } else { + write!(io, " {}", colors::cyan(location_specifier.as_str()))?; + } + if let Some((line, column)) = location.position(sources) { + write!( + io, + "{}", + colors::yellow(format_args!(":{}:{}", line, column)) + )?; + } + writeln!(io)?; + + if let Some(snippet) = diagnostic.snippet() { + print_snippet(io, sources, &snippet, max_line_number_digits)?; + }; + + if let Some(hint) = diagnostic.hint() { + write!( + io, + "{} {} ", + RepeatingCharFmt(' ', max_line_number_digits as usize), + colors::intense_blue("=") + )?; + writeln!(io, "{}: {}", colors::bold("hint"), hint)?; + } + + if let Some(snippet) = diagnostic.snippet_fixed() { + print_snippet(io, sources, &snippet, max_line_number_digits)?; + } + + writeln!(io)?; + + let mut needs_final_newline = false; + for info in diagnostic.info().iter() { + needs_final_newline = true; + writeln!(io, " {}: {}", colors::intense_blue("info"), info)?; + } + if let Some(docs_url) = diagnostic.docs_url() { + needs_final_newline = true; + writeln!(io, " {}: {}", colors::intense_blue("docs"), docs_url)?; + } + + if needs_final_newline { + writeln!(io)?; + } + + Ok(()) +} + +/// Prints a snippet to the given writer and returns the line number indent. +fn print_snippet( + io: &mut dyn std::fmt::Write, + sources: &dyn SourceTextStore, + snippet: &DiagnosticSnippet<'_>, + max_line_number_digits: u32, +) -> Result<(), std::fmt::Error> { + let DiagnosticSnippet { source, highlight } = snippet; + + fn print_padded( + io: &mut dyn std::fmt::Write, + text: impl std::fmt::Display, + padding: u32, + ) -> Result<(), std::fmt::Error> { + for _ in 0..padding { + write!(io, " ")?; + } + write!(io, "{}", text)?; + Ok(()) + } + + let source = source.to_source_text_info(sources); + + let start_line_number = line_number(&source, highlight.range.start); + let end_line_number = line_number(&source, highlight.range.end); + + print_padded(io, colors::intense_blue(" | "), max_line_number_digits)?; + writeln!(io)?; + for line_number in start_line_number..=end_line_number { + print_padded( + io, + colors::intense_blue(format_args!("{} | ", line_number)), + max_line_number_digits - line_number.ilog10() - 1, + )?; + + let padding_width; + let highlight_width; + if line_number == start_line_number && start_line_number == end_line_number + { + let (before, between, after) = + line_text_split3(&source, highlight.range.start, highlight.range.end); + write!(io, "{}", ReplaceTab(before))?; + match highlight.style { + DiagnosticSnippetHighlightStyle::Addition => { + write!(io, "{}", colors::green(ReplaceTab(between)))?; + } + _ => { + write!(io, "{}", ReplaceTab(between))?; + } + } + writeln!(io, "{}", ReplaceTab(after))?; + padding_width = display_width(before); + highlight_width = display_width(between); + } else if line_number == start_line_number { + let (before, after) = line_text_split(&source, highlight.range.start); + write!(io, "{}", ReplaceTab(before))?; + match highlight.style { + DiagnosticSnippetHighlightStyle::Addition => { + write!(io, "{}", colors::green(ReplaceTab(after)))?; + } + _ => { + write!(io, "{}", ReplaceTab(after))?; + } + } + writeln!(io)?; + padding_width = display_width(before); + highlight_width = display_width(after); + } else if line_number == end_line_number { + let (before, after) = line_text_split(&source, highlight.range.end); + match highlight.style { + DiagnosticSnippetHighlightStyle::Addition => { + write!(io, "{}", colors::green(ReplaceTab(before)))?; + } + _ => { + write!(io, "{}", ReplaceTab(before))?; + } + } + write!(io, "{}", ReplaceTab(after))?; + writeln!(io)?; + padding_width = 0; + highlight_width = display_width(before); + } else { + let line = line_text(&source, line_number); + writeln!(io, "{}", ReplaceTab(line))?; + padding_width = 0; + highlight_width = display_width(line); + } + + print_padded(io, colors::intense_blue(" | "), max_line_number_digits)?; + write!(io, "{}", RepeatingCharFmt(' ', padding_width))?; + let underline = + RepeatingCharFmt(highlight.style.underline_char(), highlight_width); + write!(io, "{}", highlight.style.style_underline(underline))?; + + if line_number == end_line_number { + if let Some(description) = &highlight.description { + write!(io, " {}", highlight.style.style_underline(description))?; + } + } + + writeln!(io)?; + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::borrow::Cow; + + use deno_ast::ModuleSpecifier; + use deno_ast::SourceTextInfo; + + use super::SourceTextStore; + + struct TestSource { + specifier: ModuleSpecifier, + text_info: SourceTextInfo, + } + + impl SourceTextStore for TestSource { + fn get_source_text<'a>( + &'a self, + specifier: &ModuleSpecifier, + ) -> Option> { + if specifier == &self.specifier { + Some(Cow::Borrowed(&self.text_info)) + } else { + None + } + } + } + + #[test] + fn test_display_width() { + assert_eq!(super::display_width("abc"), 3); + assert_eq!(super::display_width("\t"), 2); + assert_eq!(super::display_width("\t\t123"), 7); + assert_eq!(super::display_width("πŸŽ„"), 2); + assert_eq!(super::display_width("πŸŽ„πŸŽ„"), 4); + assert_eq!(super::display_width("πŸ§‘β€πŸ¦°"), 4); + } + + #[test] + fn test_position_in_file_from_text_info_simple() { + let specifier: ModuleSpecifier = "file:///dev/test.ts".parse().unwrap(); + let text_info = SourceTextInfo::new("foo\nbar\nbaz".into()); + let pos = text_info.line_start(1); + let sources = TestSource { + specifier: specifier.clone(), + text_info, + }; + let location = super::DiagnosticLocation::PositionInFile { + specifier: Cow::Borrowed(&specifier), + source_pos: super::DiagnosticSourcePos::SourcePos(pos), + }; + let position = location.position(&sources).unwrap(); + assert_eq!(position, (2, 1)) + } + + #[test] + fn test_position_in_file_from_text_info_emoji() { + let specifier: ModuleSpecifier = "file:///dev/test.ts".parse().unwrap(); + let text_info = SourceTextInfo::new("πŸ§‘β€πŸ¦°text".into()); + let pos = text_info.line_start(0) + 11; // the end of the emoji + let sources = TestSource { + specifier: specifier.clone(), + text_info, + }; + let location = super::DiagnosticLocation::PositionInFile { + specifier: Cow::Borrowed(&specifier), + source_pos: super::DiagnosticSourcePos::SourcePos(pos), + }; + let position = location.position(&sources).unwrap(); + assert_eq!(position, (1, 6)) + } +} diff --git a/cli/main.rs b/cli/main.rs index ae89d4c946..3f49677d8d 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -5,6 +5,7 @@ mod auth_tokens; mod cache; mod cdp; mod deno_std; +mod diagnostics; mod emit; mod errors; mod factory; diff --git a/cli/tests/integration/watcher_tests.rs b/cli/tests/integration/watcher_tests.rs index 008df33859..6a2cab08a5 100644 --- a/cli/tests/integration/watcher_tests.rs +++ b/cli/tests/integration/watcher_tests.rs @@ -62,7 +62,7 @@ where if t.starts_with("Watcher") { break; } - if t.starts_with('(') { + if t.starts_with("error[") { str.push_str(&t); str.push('\n'); } diff --git a/cli/tests/testdata/doc/referenced_private_types_lint.out b/cli/tests/testdata/doc/referenced_private_types_lint.out index 54f2250595..328435cd79 100644 --- a/cli/tests/testdata/doc/referenced_private_types_lint.out +++ b/cli/tests/testdata/doc/referenced_private_types_lint.out @@ -1,8 +1,28 @@ -Missing JSDoc comment. - at file:///[WILDCARD]/referenced_private_types.ts:5:1 +error[missing-jsdoc]: exported symbol is missing JSDoc documentation + --> [WILDCARD]:5:1 + | +5 | export class MyClass { + | ^ + + +error[private-type-ref]: public type 'MyClass.prototype.prop' references private type 'MyInterface' + --> [WILDCARD]:6:3 + | +6 | prop: MyInterface = {}; + | ^ + = hint: make the referenced type public or remove the reference + | +1 | interface MyInterface { + | - this is the referenced type + + info: to ensure documentation is complete all types that are exposed in the public API must be public + + +error[missing-jsdoc]: exported symbol is missing JSDoc documentation + --> [WILDCARD]:6:3 + | +6 | prop: MyInterface = {}; + | ^ -Type 'MyClass.prototype.prop' references type 'MyInterface' which is not exported from a root module. -Missing JSDoc comment. - at file:///[WILDCARD]/referenced_private_types.ts:6:3 error: Found 3 documentation lint errors. diff --git a/cli/tests/testdata/lint/expected_from_stdin.out b/cli/tests/testdata/lint/expected_from_stdin.out index 90f455fdc6..59f32166fc 100644 --- a/cli/tests/testdata/lint/expected_from_stdin.out +++ b/cli/tests/testdata/lint/expected_from_stdin.out @@ -1,3 +1,12 @@ -[WILDCARD] +error[no-explicit-any]: `any` type is not allowed + --> [WILDCARD]$deno$stdin.ts:1:9 + | +1 | let _a: any; + | ^^^ + = hint: Use a specific type other than `any` + + docs: https://lint.deno.land/#no-explicit-any + + Found 1 problem Checked 1 file diff --git a/cli/tests/testdata/lint/expected_from_stdin_json.out b/cli/tests/testdata/lint/expected_from_stdin_json.out index 7ea40e9573..26bf7ddc70 100644 --- a/cli/tests/testdata/lint/expected_from_stdin_json.out +++ b/cli/tests/testdata/lint/expected_from_stdin_json.out @@ -13,7 +13,7 @@ "bytePos": 11 } }, - "filename": "_stdin.ts", + "filename": "[WILDCARD]$deno$stdin.ts", "message": "`any` type is not allowed", "code": "no-explicit-any", "hint": [WILDCARD] diff --git a/cli/tests/testdata/lint/expected_quiet.out b/cli/tests/testdata/lint/expected_quiet.out index 7a2ef48e6e..e46a94a2d1 100644 --- a/cli/tests/testdata/lint/expected_quiet.out +++ b/cli/tests/testdata/lint/expected_quiet.out @@ -1,14 +1,20 @@ -(ban-untagged-ignore) Ignore directive requires lint rule name(s) -// deno-lint-ignore -^^^^^^^^^^^^^^^^^^^ - at [WILDCARD]file1.js:1:1 +error[ban-untagged-ignore]: Ignore directive requires lint rule name(s) + --> [WILDCARD]file1.js:1:1 + | +1 | // deno-lint-ignore + | ^^^^^^^^^^^^^^^^^^^ + = hint: Add one or more lint rule names. E.g. // deno-lint-ignore adjacent-overload-signatures - hint: [WILDCARD] + docs: https://lint.deno.land/#ban-untagged-ignore -(no-empty) Empty block statement -while (false) {} - ^^ - at [WILDCARD]file1.js:2:15 - hint: [WILDCARD] +error[no-empty]: Empty block statement + --> [WILDCARD]file1.js:2:15 + | +2 | while (false) {} + | ^^ + = hint: Add code or comment to the empty block + + docs: https://lint.deno.land/#no-empty + diff --git a/cli/tests/testdata/lint/watch/badly_linted.js.out b/cli/tests/testdata/lint/watch/badly_linted.js.out index 5c715695aa..07ae031c33 100644 --- a/cli/tests/testdata/lint/watch/badly_linted.js.out +++ b/cli/tests/testdata/lint/watch/badly_linted.js.out @@ -1,2 +1,2 @@ -(no-unused-vars) `a` is never used -(prefer-const) `a` is never reassigned +error[no-unused-vars]: `a` is never used +error[prefer-const]: `a` is never reassigned diff --git a/cli/tests/testdata/lint/watch/badly_linted_fixed1.js.out b/cli/tests/testdata/lint/watch/badly_linted_fixed1.js.out index fe74a7a2ca..67e3c9dd84 100644 --- a/cli/tests/testdata/lint/watch/badly_linted_fixed1.js.out +++ b/cli/tests/testdata/lint/watch/badly_linted_fixed1.js.out @@ -1 +1 @@ -(prefer-const) `_a` is never reassigned +error[prefer-const]: `_a` is never reassigned diff --git a/cli/tests/testdata/lint/with_config.out b/cli/tests/testdata/lint/with_config.out index 2ea821c05b..cffd6b9c73 100644 --- a/cli/tests/testdata/lint/with_config.out +++ b/cli/tests/testdata/lint/with_config.out @@ -1,18 +1,22 @@ -(ban-untagged-todo) TODO should be tagged with (@username) or (#issue) -// TODO: foo -^^^^^^^^^^^^ - at [WILDCARD]a.ts:1:1 +error[ban-untagged-todo]: TODO should be tagged with (@username) or (#issue) + --> [WILDCARD]a.ts:1:1 + | +1 | // TODO: foo + | ^^^^^^^^^^^^ + = hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - help: for further information visit https://lint.deno.land/#ban-untagged-todo + docs: https://lint.deno.land/#ban-untagged-todo -(no-unused-vars) `add` is never used -function add(a: number, b: number): number { - ^^^ - at [WILDCARD]a.ts:2:10 - hint: If this is intentional, prefix it with an underscore like `_add` - help: for further information visit https://lint.deno.land/#no-unused-vars +error[no-unused-vars]: `add` is never used + --> [WILDCARD]a.ts:2:10 + | +2 | function add(a: number, b: number): number { + | ^^^ + = hint: If this is intentional, prefix it with an underscore like `_add` + + docs: https://lint.deno.land/#no-unused-vars + Found 2 problems Checked 1 file diff --git a/cli/tests/testdata/lint/with_config_and_flags.out b/cli/tests/testdata/lint/with_config_and_flags.out index 41432df9dd..f3ad3cafb0 100644 --- a/cli/tests/testdata/lint/with_config_and_flags.out +++ b/cli/tests/testdata/lint/with_config_and_flags.out @@ -1,18 +1,22 @@ -(ban-untagged-todo) TODO should be tagged with (@username) or (#issue) -// TODO: this file should be ignored -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - at [WILDCARD]b.ts:1:1 +error[ban-untagged-todo]: TODO should be tagged with (@username) or (#issue) + --> [WILDCARD]b.ts:1:1 + | +1 | // TODO: this file should be ignored + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - help: for further information visit https://lint.deno.land/#ban-untagged-todo + docs: https://lint.deno.land/#ban-untagged-todo -(no-unused-vars) `subtract` is never used -function subtract(a: number, b: number): number { - ^^^^^^^^ - at [WILDCARD]b.ts:2:10 - hint: If this is intentional, prefix it with an underscore like `_subtract` - help: for further information visit https://lint.deno.land/#no-unused-vars +error[no-unused-vars]: `subtract` is never used + --> [WILDCARD]b.ts:2:10 + | +2 | function subtract(a: number, b: number): number { + | ^^^^^^^^ + = hint: If this is intentional, prefix it with an underscore like `_subtract` + + docs: https://lint.deno.land/#no-unused-vars + Found 2 problems Checked 1 file diff --git a/cli/tests/testdata/lint/with_config_without_tags.out b/cli/tests/testdata/lint/with_config_without_tags.out index 2ea821c05b..cffd6b9c73 100644 --- a/cli/tests/testdata/lint/with_config_without_tags.out +++ b/cli/tests/testdata/lint/with_config_without_tags.out @@ -1,18 +1,22 @@ -(ban-untagged-todo) TODO should be tagged with (@username) or (#issue) -// TODO: foo -^^^^^^^^^^^^ - at [WILDCARD]a.ts:1:1 +error[ban-untagged-todo]: TODO should be tagged with (@username) or (#issue) + --> [WILDCARD]a.ts:1:1 + | +1 | // TODO: foo + | ^^^^^^^^^^^^ + = hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - hint: Add a user tag or issue reference to the TODO comment, e.g. TODO(@djones), TODO(djones), TODO(#123) - help: for further information visit https://lint.deno.land/#ban-untagged-todo + docs: https://lint.deno.land/#ban-untagged-todo -(no-unused-vars) `add` is never used -function add(a: number, b: number): number { - ^^^ - at [WILDCARD]a.ts:2:10 - hint: If this is intentional, prefix it with an underscore like `_add` - help: for further information visit https://lint.deno.land/#no-unused-vars +error[no-unused-vars]: `add` is never used + --> [WILDCARD]a.ts:2:10 + | +2 | function add(a: number, b: number): number { + | ^^^ + = hint: If this is intentional, prefix it with an underscore like `_add` + + docs: https://lint.deno.land/#no-unused-vars + Found 2 problems Checked 1 file diff --git a/cli/tests/testdata/publish/invalid_fast_check.out b/cli/tests/testdata/publish/invalid_fast_check.out index e34cfecdf6..37e25e269d 100644 --- a/cli/tests/testdata/publish/invalid_fast_check.out +++ b/cli/tests/testdata/publish/invalid_fast_check.out @@ -1,8 +1,13 @@ Checking fast check type graph for errors... +error[zap-missing-explicit-return-type]: missing explicit return type in the public API + --> [WILDCARD]mod.ts:2:17 + | +2 | export function getRandom() { + | ^^^^^^^^^ this function is missing an explicit return type + = hint: add an explit return type to the function -missing explicit return type in the public API - at file:///[WILDCARD]/publish/invalid_fast_check/mod.ts@68 + info: all functions in the public API must have an explicit return type + docs: https://jsr.io/go/zap-missing-explicit-return-type -Fixing these fast check errors is required to make the code fast check compatible which enables type checking your package's TypeScript code with the same performance as if you had distributed declaration files. Do any of these errors seem too restrictive or incorrect? Please open an issue if so to help us improve: https://github.com/denoland/deno/issues -error: Had 1 fast check error. +error: Found 1 problem diff --git a/cli/tests/testdata/publish/javascript_missing_decl_file.out b/cli/tests/testdata/publish/javascript_missing_decl_file.out index 02478a5b5f..bf7797c09d 100644 --- a/cli/tests/testdata/publish/javascript_missing_decl_file.out +++ b/cli/tests/testdata/publish/javascript_missing_decl_file.out @@ -1,6 +1,13 @@ Checking fast check type graph for errors... -Warning Package '@foo/bar' is a JavaScript package without a corresponding declaration file. This may lead to a non-optimal experience for users of your package. For performance reasons, it's recommended to ship a corresponding TypeScript declaration file or to convert to TypeScript. -Ensuring type checks... +warning[zap-unsupported-javascript-entrypoint]: used a JavaScript module without type declarations as an entrypoints + --> [WILDCARD]mod.js + = hint: add a type declaration (d.ts) for the JavaScript module, or rewrite it to TypeScript + + info: JavaScript files with no corresponding declaration require type inference to be type checked + info: fast check avoids type inference, so JavaScript entrypoints should be avoided + docs: https://jsr.io/go/zap-unsupported-javascript-entrypoint + + Publishing @foo/bar@1.0.0 ... Successfully published @foo/bar@1.0.0 Visit http://127.0.0.1:4250/@foo/bar@1.0.0 for details diff --git a/cli/tools/doc.rs b/cli/tools/doc.rs index 4a59ec986b..2cb9ddfbae 100644 --- a/cli/tools/doc.rs +++ b/cli/tools/doc.rs @@ -5,6 +5,16 @@ use crate::args::DocHtmlFlag; use crate::args::DocSourceFileFlag; use crate::args::Flags; use crate::colors; +use crate::diagnostics::Diagnostic; +use crate::diagnostics::DiagnosticLevel; +use crate::diagnostics::DiagnosticLocation; +use crate::diagnostics::DiagnosticSnippet; +use crate::diagnostics::DiagnosticSnippetHighlight; +use crate::diagnostics::DiagnosticSnippetHighlightStyle; +use crate::diagnostics::DiagnosticSnippetSource; +use crate::diagnostics::DiagnosticSourcePos; +use crate::diagnostics::DiagnosticSourceRange; +use crate::diagnostics::SourceTextParsedSourceStore; use crate::display::write_json_to_stdout; use crate::display::write_to_stdout_ignore_sigpipe; use crate::factory::CliFactory; @@ -23,7 +33,10 @@ use deno_graph::ModuleAnalyzer; use deno_graph::ModuleParser; use deno_graph::ModuleSpecifier; use doc::DocDiagnostic; +use doc::DocDiagnosticKind; use indexmap::IndexMap; +use lsp_types::Url; +use std::borrow::Cow; use std::collections::BTreeMap; use std::rc::Rc; @@ -129,7 +142,7 @@ pub async fn doc(flags: Flags, doc_flags: DocFlags) -> Result<(), AnyError> { if doc_flags.lint { let diagnostics = doc_parser.take_diagnostics(); - check_diagnostics(&diagnostics)?; + check_diagnostics(&**parsed_source_cache, &diagnostics)?; } doc_nodes_by_url @@ -291,7 +304,118 @@ fn print_docs_to_stdout( write_to_stdout_ignore_sigpipe(details.as_bytes()).map_err(AnyError::from) } -fn check_diagnostics(diagnostics: &[DocDiagnostic]) -> Result<(), AnyError> { +impl Diagnostic for DocDiagnostic { + fn level(&self) -> DiagnosticLevel { + DiagnosticLevel::Error + } + + fn code(&self) -> impl std::fmt::Display + '_ { + match self.kind { + DocDiagnosticKind::MissingJsDoc => "missing-jsdoc", + DocDiagnosticKind::MissingExplicitType => "missing-explicit-type", + DocDiagnosticKind::MissingReturnType => "missing-return-type", + DocDiagnosticKind::PrivateTypeRef { .. } => "private-type-ref", + } + } + + fn message(&self) -> impl std::fmt::Display + '_ { + match &self.kind { + DocDiagnosticKind::MissingJsDoc => { + Cow::Borrowed("exported symbol is missing JSDoc documentation") + } + DocDiagnosticKind::MissingExplicitType => { + Cow::Borrowed("exported symbol is missing an explicit type annotation") + } + DocDiagnosticKind::MissingReturnType => Cow::Borrowed( + "exported function is missing an explicit return type annotation", + ), + DocDiagnosticKind::PrivateTypeRef { + reference, name, .. + } => Cow::Owned(format!( + "public type '{name}' references private type '{reference}'", + )), + } + } + + fn location(&self) -> DiagnosticLocation { + let specifier = Url::parse(&self.location.filename).unwrap(); + DiagnosticLocation::PositionInFile { + specifier: Cow::Owned(specifier), + source_pos: DiagnosticSourcePos::ByteIndex(self.location.byte_index), + } + } + + fn snippet(&self) -> Option> { + let specifier = Url::parse(&self.location.filename).unwrap(); + Some(DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Owned(specifier)), + highlight: DiagnosticSnippetHighlight { + style: DiagnosticSnippetHighlightStyle::Error, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::ByteIndex(self.location.byte_index), + end: DiagnosticSourcePos::ByteIndex(self.location.byte_index + 1), + }, + description: None, + }, + }) + } + + fn hint(&self) -> Option { + match &self.kind { + DocDiagnosticKind::PrivateTypeRef { .. } => { + Some("make the referenced type public or remove the reference") + } + _ => None, + } + } + fn snippet_fixed(&self) -> Option> { + match &self.kind { + DocDiagnosticKind::PrivateTypeRef { + reference_location, .. + } => { + let specifier = Url::parse(&reference_location.filename).unwrap(); + Some(DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Owned(specifier)), + highlight: DiagnosticSnippetHighlight { + style: DiagnosticSnippetHighlightStyle::Hint, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::ByteIndex( + reference_location.byte_index, + ), + end: DiagnosticSourcePos::ByteIndex( + reference_location.byte_index + 1, + ), + }, + description: Some(Cow::Borrowed("this is the referenced type")), + }, + }) + } + _ => None, + } + } + + fn info(&self) -> std::borrow::Cow<'_, [std::borrow::Cow<'_, str>]> { + match &self.kind { + DocDiagnosticKind::MissingJsDoc => Cow::Borrowed(&[]), + DocDiagnosticKind::MissingExplicitType => Cow::Borrowed(&[]), + DocDiagnosticKind::MissingReturnType => Cow::Borrowed(&[]), + DocDiagnosticKind::PrivateTypeRef { .. } => { + Cow::Borrowed(&[Cow::Borrowed( + "to ensure documentation is complete all types that are exposed in the public API must be public", + )]) + } + } + } + + fn docs_url(&self) -> Option { + None::<&str> + } +} + +fn check_diagnostics( + parsed_source_cache: &dyn deno_graph::ParsedSourceStore, + diagnostics: &[DocDiagnostic], +) -> Result<(), AnyError> { if diagnostics.is_empty() { return Ok(()); } @@ -309,18 +433,13 @@ fn check_diagnostics(diagnostics: &[DocDiagnostic]) -> Result<(), AnyError> { .push(diagnostic); } - for (filename, diagnostics_by_lc) in diagnostic_groups { - for (line, diagnostics_by_col) in diagnostics_by_lc { - for (col, diagnostics) in diagnostics_by_col { + for (_, diagnostics_by_lc) in diagnostic_groups { + for (_, diagnostics_by_col) in diagnostics_by_lc { + for (_, diagnostics) in diagnostics_by_col { for diagnostic in diagnostics { - log::warn!("{}", diagnostic.message()); + let sources = SourceTextParsedSourceStore(parsed_source_cache); + eprintln!("{}", diagnostic.display(&sources)); } - log::warn!( - " at {}:{}:{}\n", - colors::cyan(filename.as_str()), - colors::yellow(&line.to_string()), - colors::yellow(&(col + 1).to_string()) - ) } } } diff --git a/cli/tools/lint.rs b/cli/tools/lint.rs index 20fd12ce2e..8de8160de8 100644 --- a/cli/tools/lint.rs +++ b/cli/tools/lint.rs @@ -8,6 +8,16 @@ use crate::args::LintOptions; use crate::args::LintReporterKind; use crate::args::LintRulesConfig; use crate::colors; +use crate::diagnostics::Diagnostic; +use crate::diagnostics::DiagnosticLevel; +use crate::diagnostics::DiagnosticLocation; +use crate::diagnostics::DiagnosticSnippet; +use crate::diagnostics::DiagnosticSnippetHighlight; +use crate::diagnostics::DiagnosticSnippetHighlightStyle; +use crate::diagnostics::DiagnosticSnippetSource; +use crate::diagnostics::DiagnosticSourcePos; +use crate::diagnostics::DiagnosticSourceRange; +use crate::diagnostics::SourceTextStore; use crate::factory::CliFactory; use crate::tools::fmt::run_parallelized; use crate::util::file_watcher; @@ -16,22 +26,25 @@ use crate::util::fs::FileCollector; use crate::util::path::is_script_ext; use crate::util::sync::AtomicFlag; use deno_ast::MediaType; +use deno_ast::ModuleSpecifier; +use deno_ast::ParsedSource; +use deno_ast::SourceTextInfo; use deno_config::glob::FilePatterns; use deno_core::anyhow::bail; use deno_core::error::generic_error; use deno_core::error::AnyError; -use deno_core::error::JsStackFrame; use deno_core::serde_json; +use deno_core::url; use deno_lint::diagnostic::LintDiagnostic; use deno_lint::linter::LintFileOptions; use deno_lint::linter::Linter; use deno_lint::linter::LinterBuilder; use deno_lint::rules; use deno_lint::rules::LintRule; -use deno_runtime::fmt_errors::format_location; use log::debug; use log::info; use serde::Serialize; +use std::borrow::Cow; use std::fs; use std::io::stdin; use std::io::Read; @@ -42,7 +55,7 @@ use std::sync::Mutex; use crate::cache::IncrementalCache; -static STDIN_FILE_NAME: &str = "_stdin.ts"; +static STDIN_FILE_NAME: &str = "$deno$stdin.ts"; fn create_reporter(kind: LintReporterKind) -> Box { match kind { @@ -110,9 +123,10 @@ pub async fn lint(flags: Flags, lint_flags: LintFlags) -> Result<(), AnyError> { let reporter_kind = lint_options.reporter_kind; let reporter_lock = Arc::new(Mutex::new(create_reporter(reporter_kind))); let lint_rules = get_config_rules_err_empty(lint_options.rules)?; - let r = lint_stdin(lint_rules); - let success = - handle_lint_result(STDIN_FILE_NAME, r, reporter_lock.clone()); + let file_path = cli_options.initial_cwd().join(STDIN_FILE_NAME); + let file_path = file_path.to_string_lossy(); + let r = lint_stdin(&file_path, lint_rules); + let success = handle_lint_result(&file_path, r, reporter_lock.clone()); reporter_lock.lock().unwrap().close(1); success } else { @@ -173,10 +187,11 @@ async fn lint_files( } let r = lint_file(&file_path, file_text, lint_rules); - if let Ok((file_diagnostics, file_text)) = &r { + if let Ok((file_diagnostics, file_source)) = &r { if file_diagnostics.is_empty() { // update the incremental cache if there were no diagnostics - incremental_cache.update_file(&file_path, file_text) + incremental_cache + .update_file(&file_path, file_source.text_info().text_str()) } } @@ -262,27 +277,28 @@ fn lint_file( file_path: &Path, source_code: String, lint_rules: Vec<&'static dyn LintRule>, -) -> Result<(Vec, String), AnyError> { +) -> Result<(Vec, ParsedSource), AnyError> { let filename = file_path.to_string_lossy().to_string(); let media_type = MediaType::from_path(file_path); let linter = create_linter(lint_rules); - let (_, file_diagnostics) = linter.lint_file(LintFileOptions { + let (source, file_diagnostics) = linter.lint_file(LintFileOptions { filename, media_type, source_code: source_code.clone(), })?; - Ok((file_diagnostics, source_code)) + Ok((file_diagnostics, source)) } /// Lint stdin and write result to stdout. /// Treats input as TypeScript. /// Compatible with `--json` flag. fn lint_stdin( + file_path: &str, lint_rules: Vec<&'static dyn LintRule>, -) -> Result<(Vec, String), AnyError> { +) -> Result<(Vec, ParsedSource), AnyError> { let mut source_code = String::new(); if stdin().read_to_string(&mut source_code).is_err() { return Err(generic_error("Failed to read from stdin")); @@ -290,18 +306,18 @@ fn lint_stdin( let linter = create_linter(lint_rules); - let (_, file_diagnostics) = linter.lint_file(LintFileOptions { - filename: STDIN_FILE_NAME.to_string(), + let (source, file_diagnostics) = linter.lint_file(LintFileOptions { + filename: file_path.to_string(), source_code: source_code.clone(), media_type: MediaType::TypeScript, })?; - Ok((file_diagnostics, source_code)) + Ok((file_diagnostics, source)) } fn handle_lint_result( file_path: &str, - result: Result<(Vec, String), AnyError>, + result: Result<(Vec, ParsedSource), AnyError>, reporter_lock: Arc>>, ) -> bool { let mut reporter = reporter_lock.lock().unwrap(); @@ -310,7 +326,7 @@ fn handle_lint_result( Ok((mut file_diagnostics, source)) => { sort_diagnostics(&mut file_diagnostics); for d in file_diagnostics.iter() { - reporter.visit_diagnostic(d, source.split('\n').collect()); + reporter.visit_diagnostic(d, &source); } file_diagnostics.is_empty() } @@ -322,7 +338,7 @@ fn handle_lint_result( } trait LintReporter { - fn visit_diagnostic(&mut self, d: &LintDiagnostic, source_lines: Vec<&str>); + fn visit_diagnostic(&mut self, d: &LintDiagnostic, source: &ParsedSource); fn visit_error(&mut self, file_path: &str, err: &AnyError); fn close(&mut self, check_count: usize); } @@ -343,29 +359,77 @@ impl PrettyLintReporter { } } +impl Diagnostic for LintDiagnostic { + fn level(&self) -> DiagnosticLevel { + DiagnosticLevel::Error + } + + fn code(&self) -> impl std::fmt::Display + '_ { + &self.code + } + + fn message(&self) -> impl std::fmt::Display + '_ { + &self.message + } + + fn location(&self) -> DiagnosticLocation { + let specifier = url::Url::from_file_path(&self.filename).unwrap(); + DiagnosticLocation::PositionInFile { + specifier: Cow::Owned(specifier), + source_pos: DiagnosticSourcePos::ByteIndex(self.range.start.byte_index), + } + } + + fn snippet(&self) -> Option> { + let specifier = url::Url::from_file_path(&self.filename).unwrap(); + let range = DiagnosticSourceRange { + start: DiagnosticSourcePos::ByteIndex(self.range.start.byte_index), + end: DiagnosticSourcePos::ByteIndex(self.range.end.byte_index), + }; + Some(DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Owned(specifier)), + highlight: DiagnosticSnippetHighlight { + range, + style: DiagnosticSnippetHighlightStyle::Error, + description: None, + }, + }) + } + + fn hint(&self) -> Option { + self.hint.as_ref().map(|h| h as &dyn std::fmt::Display) + } + + fn snippet_fixed(&self) -> Option> { + None // todo + } + + fn info(&self) -> Cow<'_, [std::borrow::Cow<'_, str>]> { + Cow::Borrowed(&[]) + } + + fn docs_url(&self) -> Option { + Some(format!("https://lint.deno.land/#{}", &self.code)) + } +} + +struct OneSource<'a>(&'a ParsedSource); + +impl SourceTextStore for OneSource<'_> { + fn get_source_text<'a>( + &'a self, + _specifier: &ModuleSpecifier, + ) -> Option> { + Some(Cow::Borrowed(self.0.text_info())) + } +} + impl LintReporter for PrettyLintReporter { - fn visit_diagnostic(&mut self, d: &LintDiagnostic, source_lines: Vec<&str>) { + fn visit_diagnostic(&mut self, d: &LintDiagnostic, source: &ParsedSource) { self.lint_count += 1; - let pretty_message = format!("({}) {}", colors::red(&d.code), &d.message); - - let message = format_diagnostic( - &d.code, - &pretty_message, - &source_lines, - &d.range, - d.hint.as_ref(), - &format_location(&JsStackFrame::from_location( - Some(d.filename.clone()), - // todo(dsherret): these should use "display positions" - // which take into account the added column index of tab - // indentation - Some(d.range.start.line_index as i64 + 1), - Some(d.range.start.column_index as i64 + 1), - )), - ); - - eprintln!("{message}\n"); + let sources = OneSource(source); + eprintln!("{}", d.display(&sources)); } fn visit_error(&mut self, file_path: &str, err: &AnyError) { @@ -399,7 +463,7 @@ impl CompactLintReporter { } impl LintReporter for CompactLintReporter { - fn visit_diagnostic(&mut self, d: &LintDiagnostic, _source_lines: Vec<&str>) { + fn visit_diagnostic(&mut self, d: &LintDiagnostic, _source: &ParsedSource) { self.lint_count += 1; eprintln!( @@ -432,69 +496,6 @@ impl LintReporter for CompactLintReporter { } } -pub fn format_diagnostic( - diagnostic_code: &str, - message_line: &str, - source_lines: &[&str], - range: &deno_lint::diagnostic::Range, - maybe_hint: Option<&String>, - formatted_location: &str, -) -> String { - let mut lines = vec![]; - - for (i, line) in source_lines - .iter() - .enumerate() - .take(range.end.line_index + 1) - .skip(range.start.line_index) - { - lines.push(line.to_string()); - if range.start.line_index == range.end.line_index { - lines.push(format!( - "{}{}", - " ".repeat(range.start.column_index), - colors::red( - &"^".repeat(range.end.column_index - range.start.column_index) - ) - )); - } else { - let line_len = line.len(); - if range.start.line_index == i { - lines.push(format!( - "{}{}", - " ".repeat(range.start.column_index), - colors::red(&"^".repeat(line_len - range.start.column_index)) - )); - } else if range.end.line_index == i { - lines - .push(colors::red(&"^".repeat(range.end.column_index)).to_string()); - } else if line_len != 0 { - lines.push(colors::red(&"^".repeat(line_len)).to_string()); - } - } - } - - let hint = if let Some(hint) = maybe_hint { - format!(" {} {}\n", colors::cyan("hint:"), hint) - } else { - "".to_string() - }; - let help = format!( - " {} for further information visit https://lint.deno.land/#{}", - colors::cyan("help:"), - diagnostic_code - ); - - format!( - "{message_line}\n{snippets}\n at {formatted_location}\n\n{hint}{help}", - message_line = message_line, - snippets = lines.join("\n"), - formatted_location = formatted_location, - hint = hint, - help = help - ) -} - #[derive(Serialize)] struct JsonLintReporter { diagnostics: Vec, @@ -511,7 +512,7 @@ impl JsonLintReporter { } impl LintReporter for JsonLintReporter { - fn visit_diagnostic(&mut self, d: &LintDiagnostic, _source_lines: Vec<&str>) { + fn visit_diagnostic(&mut self, d: &LintDiagnostic, _source: &ParsedSource) { self.diagnostics.push(d.clone()); } diff --git a/cli/tools/registry/diagnostics.rs b/cli/tools/registry/diagnostics.rs new file mode 100644 index 0000000000..f6d81f5a85 --- /dev/null +++ b/cli/tools/registry/diagnostics.rs @@ -0,0 +1,152 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +use std::borrow::Cow; +use std::fmt::Display; +use std::sync::Arc; +use std::sync::Mutex; + +use deno_ast::swc::common::util::take::Take; +use deno_core::anyhow::anyhow; +use deno_core::error::AnyError; +use deno_graph::FastCheckDiagnostic; +use deno_graph::ParsedSourceStore; + +use crate::diagnostics::Diagnostic; +use crate::diagnostics::DiagnosticLevel; +use crate::diagnostics::DiagnosticLocation; +use crate::diagnostics::DiagnosticSnippet; +use crate::diagnostics::DiagnosticSnippetHighlight; +use crate::diagnostics::DiagnosticSnippetHighlightStyle; +use crate::diagnostics::DiagnosticSnippetSource; +use crate::diagnostics::DiagnosticSourcePos; +use crate::diagnostics::DiagnosticSourceRange; +use crate::diagnostics::SourceTextParsedSourceStore; + +#[derive(Clone, Default)] +pub struct PublishDiagnosticsCollector { + diagnostics: Arc>>, +} + +impl PublishDiagnosticsCollector { + pub fn print_and_error( + &self, + sources: &dyn ParsedSourceStore, + ) -> Result<(), AnyError> { + let mut errors = 0; + let diagnostics = self.diagnostics.lock().unwrap().take(); + let sources = SourceTextParsedSourceStore(sources); + for diagnostic in diagnostics { + eprintln!("{}", diagnostic.display(&sources)); + if matches!(diagnostic.level(), DiagnosticLevel::Error) { + errors += 1; + } + } + if errors > 0 { + Err(anyhow!( + "Found {} problem{}", + errors, + if errors == 1 { "" } else { "s" } + )) + } else { + Ok(()) + } + } + + pub fn push(&self, diagnostic: PublishDiagnostic) { + self.diagnostics.lock().unwrap().push(diagnostic); + } +} + +pub enum PublishDiagnostic { + FastCheck { diagnostic: FastCheckDiagnostic }, +} + +impl Diagnostic for PublishDiagnostic { + fn level(&self) -> DiagnosticLevel { + match self { + PublishDiagnostic::FastCheck { + diagnostic: FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. }, + } => DiagnosticLevel::Warning, + PublishDiagnostic::FastCheck { .. } => DiagnosticLevel::Error, + } + } + + fn code(&self) -> impl Display + '_ { + match &self { + PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.code(), + } + } + + fn message(&self) -> impl Display + '_ { + match &self { + PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.to_string(), // todo + } + } + + fn location(&self) -> DiagnosticLocation { + match &self { + PublishDiagnostic::FastCheck { diagnostic } => match diagnostic.range() { + Some(range) => DiagnosticLocation::PositionInFile { + specifier: Cow::Borrowed(diagnostic.specifier()), + source_pos: DiagnosticSourcePos::SourcePos(range.range.start), + }, + None => DiagnosticLocation::File { + specifier: Cow::Borrowed(diagnostic.specifier()), + }, + }, + } + } + + fn snippet(&self) -> Option> { + match &self { + PublishDiagnostic::FastCheck { diagnostic } => { + diagnostic.range().map(|range| DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Borrowed( + diagnostic.specifier(), + )), + highlight: DiagnosticSnippetHighlight { + style: DiagnosticSnippetHighlightStyle::Error, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::SourcePos(range.range.start), + end: DiagnosticSourcePos::SourcePos(range.range.end), + }, + description: diagnostic.range_description().map(Cow::Borrowed), + }, + }) + } + } + } + + fn hint(&self) -> Option { + match &self { + PublishDiagnostic::FastCheck { diagnostic } => { + Some(diagnostic.fix_hint()) + } + } + } + + fn snippet_fixed(&self) -> Option> { + None + } + + fn info(&self) -> Cow<'_, [Cow<'_, str>]> { + match &self { + PublishDiagnostic::FastCheck { diagnostic } => { + let infos = diagnostic + .additional_info() + .iter() + .map(|s| Cow::Borrowed(*s)) + .collect(); + Cow::Owned(infos) + } + } + } + + fn docs_url(&self) -> Option { + match &self { + PublishDiagnostic::FastCheck { diagnostic } => { + Some(format!("https://jsr.io/go/{}", diagnostic.code())) + } + } + } +} diff --git a/cli/tools/registry/graph.rs b/cli/tools/registry/graph.rs index b64350887c..29c825242c 100644 --- a/cli/tools/registry/graph.rs +++ b/cli/tools/registry/graph.rs @@ -12,6 +12,9 @@ use deno_core::error::AnyError; use deno_graph::FastCheckDiagnostic; use deno_graph::ModuleGraph; +use super::diagnostics::PublishDiagnostic; +use super::diagnostics::PublishDiagnosticsCollector; + #[derive(Debug)] pub struct MemberRoots { pub name: String, @@ -61,11 +64,13 @@ pub fn resolve_config_file_roots_from_exports( Ok(exports) } -pub fn surface_fast_check_type_graph_errors( +/// Collects diagnostics from the module graph for the given packages. +/// Returns true if any diagnostics were collected. +pub fn collect_fast_check_type_graph_diagnostics( graph: &ModuleGraph, packages: &[MemberRoots], -) -> Result<(), AnyError> { - let mut diagnostic_count = 0; + diagnostics_collector: &PublishDiagnosticsCollector, +) -> bool { let mut seen_diagnostics = HashSet::new(); let mut seen_modules = HashSet::with_capacity(graph.specifiers_count()); for package in packages { @@ -85,31 +90,18 @@ pub fn surface_fast_check_type_graph_errors( }; if let Some(diagnostic) = esm_module.fast_check_diagnostic() { for diagnostic in diagnostic.flatten_multiple() { + if !seen_diagnostics.insert(diagnostic.message_with_range_for_test()) + { + continue; + } + diagnostics_collector.push(PublishDiagnostic::FastCheck { + diagnostic: diagnostic.clone(), + }); if matches!( diagnostic, FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. } ) { - // ignore JS packages for fast check - log::warn!( - concat!( - "{} Package '{}' is a JavaScript package without a corresponding ", - "declaration file. This may lead to a non-optimal experience for ", - "users of your package. For performance reasons, it's recommended ", - "to ship a corresponding TypeScript declaration file or to ", - "convert to TypeScript.", - ), - deno_runtime::colors::yellow("Warning"), - package.name, - ); break 'analyze_package; // no need to keep analyzing this package - } else { - let message = diagnostic.message_with_range_for_test(); - if !seen_diagnostics.insert(message.clone()) { - continue; - } - - log::error!("\n{}", message); - diagnostic_count += 1; } } } @@ -133,22 +125,5 @@ pub fn surface_fast_check_type_graph_errors( } } - if diagnostic_count > 0 { - // for the time being, tell the user why we have these errors and the benefit they bring - log::error!( - concat!( - "\nFixing these fast check errors is required to make the code fast check compatible ", - "which enables type checking your package's TypeScript code with the same ", - "performance as if you had distributed declaration files. Do any of these ", - "errors seem too restrictive or incorrect? Please open an issue if so to ", - "help us improve: https://github.com/denoland/deno/issues\n", - ) - ); - bail!( - "Had {} fast check error{}.", - diagnostic_count, - if diagnostic_count == 1 { "" } else { "s" } - ) - } - Ok(()) + !seen_diagnostics.is_empty() } diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index 3b8ffcd043..8eaf612583 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -32,15 +32,17 @@ use crate::factory::CliFactory; use crate::graph_util::ModuleGraphBuilder; use crate::http_util::HttpClient; use crate::tools::check::CheckOptions; +use crate::tools::registry::diagnostics::PublishDiagnosticsCollector; +use crate::tools::registry::graph::collect_fast_check_type_graph_diagnostics; use crate::tools::registry::graph::get_workspace_member_roots; use crate::tools::registry::graph::resolve_config_file_roots_from_exports; -use crate::tools::registry::graph::surface_fast_check_type_graph_errors; use crate::tools::registry::graph::MemberRoots; use crate::util::display::human_size; use crate::util::import_map::ImportMapUnfurler; mod api; mod auth; +mod diagnostics; mod graph; mod publish_order; mod tar; @@ -166,47 +168,6 @@ pub enum Permission<'s> { }, } -/// Prints diagnostics like so: -/// ``` -/// -/// Warning -/// β”œβ•Œ Dynamic import was not analyzable... -/// β”œβ•Œβ•Œ at file:///dev/foo/bar/foo.ts:4:5 -/// | -/// β”œβ•Œ Dynamic import was not analyzable... -/// β”œβ•Œβ•Œ at file:///dev/foo/bar/foo.ts:4:5 -/// | -/// β”œβ•Œ Dynamic import was not analyzable... -/// β””β•Œβ•Œ at file:///dev/foo/bar/foo.ts:4:5 -/// -/// ``` -fn print_diagnostics(diagnostics: &[String]) { - if !diagnostics.is_empty() { - let len = diagnostics.len(); - log::warn!(""); - log::warn!("{}", crate::colors::yellow("Warning")); - for (i, diagnostic) in diagnostics.iter().enumerate() { - let last_diagnostic = i == len - 1; - let lines = diagnostic.split('\n').collect::>(); - let lines_len = lines.len(); - if i != 0 { - log::warn!("|"); - } - for (j, line) in lines.iter().enumerate() { - let last_line = j == lines_len - 1; - if j == 0 { - log::warn!("β”œβ•Œ {}", line); - } else if last_line && last_diagnostic { - log::warn!("β””β•Œβ•Œ {}", line); - } else { - log::warn!("β”œβ•Œβ•Œ {}", line); - } - } - } - log::warn!(""); - } -} - async fn get_auth_headers( client: &reqwest::Client, registry_url: String, @@ -484,11 +445,6 @@ async fn perform_publish( .values() .cloned() .collect::>(); - let diagnostics = packages - .iter() - .flat_map(|p| p.tarball.diagnostics.iter().cloned()) - .collect::>(); - print_diagnostics(&diagnostics); ensure_scopes_and_packages_exist( client, @@ -679,6 +635,7 @@ async fn publish_package( async fn prepare_packages_for_publishing( cli_factory: &CliFactory, + diagnostics_collector: &PublishDiagnosticsCollector, deno_json: ConfigFile, import_map: Arc, ) -> Result< @@ -700,6 +657,7 @@ async fn prepare_packages_for_publishing( module_graph_builder, type_checker, cli_options, + diagnostics_collector, &[MemberRoots { name: get_deno_json_package_name(&deno_json)?, dir_url: deno_json.specifier.join("./").unwrap().clone(), @@ -724,6 +682,7 @@ async fn prepare_packages_for_publishing( module_graph_builder, type_checker, cli_options, + diagnostics_collector, &roots, ) .await?; @@ -765,6 +724,7 @@ async fn build_and_check_graph_for_publish( module_graph_builder: &ModuleGraphBuilder, type_checker: &TypeChecker, cli_options: &CliOptions, + diagnostics_collector: &PublishDiagnosticsCollector, packages: &[MemberRoots], ) -> Result, deno_core::anyhow::Error> { let graph = Arc::new( @@ -784,28 +744,34 @@ async fn build_and_check_graph_for_publish( ); graph.valid()?; log::info!("Checking fast check type graph for errors..."); - surface_fast_check_type_graph_errors(&graph, packages)?; - log::info!("Ensuring type checks..."); - let diagnostics = type_checker - .check_diagnostics( - graph.clone(), - CheckOptions { - lib: cli_options.ts_type_lib_window(), - log_ignored_options: false, - reload: cli_options.reload_flag(), - }, - ) - .await?; - if !diagnostics.is_empty() { - bail!( - concat!( - "{:#}\n\n", - "You may have discovered a bug in Deno's fast check implementation. ", - "Fast check is still early days and we would appreciate if you log a ", - "bug if you believe this is one: https://github.com/denoland/deno/issues/" - ), - diagnostics - ); + let has_fast_check_diagnostics = collect_fast_check_type_graph_diagnostics( + &graph, + packages, + diagnostics_collector, + ); + if !has_fast_check_diagnostics { + log::info!("Ensuring type checks..."); + let diagnostics = type_checker + .check_diagnostics( + graph.clone(), + CheckOptions { + lib: cli_options.ts_type_lib_window(), + log_ignored_options: false, + reload: cli_options.reload_flag(), + }, + ) + .await?; + if !diagnostics.is_empty() { + bail!( + concat!( + "{:#}\n\n", + "You may have discovered a bug in Deno's fast check implementation. ", + "Fast check is still early days and we would appreciate if you log a ", + "bug if you believe this is one: https://github.com/denoland/deno/issues/" + ), + diagnostics + ); + } } Ok(graph) } @@ -836,14 +802,20 @@ pub async fn publish( ); }; + let diagnostics_collector = PublishDiagnosticsCollector::default(); + let (publish_order_graph, prepared_package_by_name) = prepare_packages_for_publishing( &cli_factory, + &diagnostics_collector, config_file.clone(), import_map, ) .await?; + diagnostics_collector + .print_and_error(&**cli_factory.parsed_source_cache())?; + if prepared_package_by_name.is_empty() { bail!("No packages to publish"); } diff --git a/runtime/colors.rs b/runtime/colors.rs index f8b18b9771..ff9ee24dbc 100644 --- a/runtime/colors.rs +++ b/runtime/colors.rs @@ -80,7 +80,7 @@ impl fmt::Write for StdIoStdFmtWriter<'_> { } } -struct Style { +pub struct Style { colorspec: ColorSpec, inner: I, } @@ -101,65 +101,68 @@ impl fmt::Display for Style { } #[inline] -fn style<'a>( - s: impl fmt::Display + 'a, - colorspec: ColorSpec, -) -> impl fmt::Display + 'a { +fn style<'a, S: fmt::Display + 'a>(s: S, colorspec: ColorSpec) -> Style { Style { colorspec, inner: s, } } -pub fn red_bold<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn red_bold<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Red)).set_bold(true); style(s, style_spec) } -pub fn green_bold<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn green_bold<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Green)).set_bold(true); style(s, style_spec) } -pub fn italic<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn yellow_bold<'a, S: fmt::Display + 'a>(s: S) -> Style { + let mut style_spec = ColorSpec::new(); + style_spec.set_fg(Some(Yellow)).set_bold(true); + style(s, style_spec) +} + +pub fn italic<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_italic(true); style(s, style_spec) } -pub fn italic_gray<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn italic_gray<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Ansi256(8))).set_italic(true); style(s, style_spec) } -pub fn italic_bold<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn italic_bold<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_bold(true).set_italic(true); style(s, style_spec) } -pub fn white_on_red<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn white_on_red<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_bg(Some(Red)).set_fg(Some(White)); style(s, style_spec) } -pub fn black_on_green<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn black_on_green<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_bg(Some(Green)).set_fg(Some(Black)); style(s, style_spec) } -pub fn yellow<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn yellow<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Yellow)); style(s, style_spec) } -pub fn cyan<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn cyan<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Cyan)); style(s, style_spec) @@ -173,43 +176,43 @@ pub fn cyan_with_underline<'a>( style(s, style_spec) } -pub fn cyan_bold<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn cyan_bold<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Cyan)).set_bold(true); style(s, style_spec) } -pub fn magenta<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn magenta<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Magenta)); style(s, style_spec) } -pub fn red<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn red<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Red)); style(s, style_spec) } -pub fn green<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn green<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Green)); style(s, style_spec) } -pub fn bold<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn bold<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_bold(true); style(s, style_spec) } -pub fn gray<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn gray<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Ansi256(245))); style(s, style_spec) } -pub fn intense_blue<'a>(s: impl fmt::Display + 'a) -> impl fmt::Display + 'a { +pub fn intense_blue<'a, S: fmt::Display + 'a>(s: S) -> Style { let mut style_spec = ColorSpec::new(); style_spec.set_fg(Some(Blue)).set_intense(true); style(s, style_spec)