From d1ef561dbfef8de0ffd39d1548ce9b14a3c7f540 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 31 Oct 2023 18:19:42 -0400 Subject: [PATCH] feat: `deno doc --lint` (#21032) Adds a new `--lint` flag to `deno doc` that surfaces three kinds of diagnostics: 1. Diagnostic for non-exported type referenced in an exported type. * Why? People often forget to export types from a module in TypeScript. To supress this diagnostic, add an `@internal` jsdoc tag to the internal type. 1. Diagnostic for missing return type or missing property type on a **public** type. * Why? Otherwise `deno doc` will not display good documentation. Adding explicit types also helps with type checking performance. 1. Diagnostic for missing jsdoc on a **public** type. * Why? Everything should be documented. This diagnostic can be supressed by adding a jsdoc comment description. If the lint passes, `deno doc` generates documentation as usual. For example, checking for deno doc diagnostics on the CI: ```shellsession $ deno doc --lint mod.ts second_entrypoint.ts > /dev/null ``` This feature is incredibly useful for library authors. ## Why not include this in `deno lint`? 1. The command needs the documenation output in order to figure out the diagnostics. 1. `deno lint` doesn't understand where the entrypoints are. That's critical for the diagnostics to be useful. 1. It's much more performant to do this while generating documentation. 1. There is precedence in rustdoc (ex. `#![warn(missing_docs)]`). ## Why not `--check`? It is confusing with `deno run --check`, since that means to run type checking (and confusing with `deno check --docs`). ## Output Future Improvement The output is not ideal atm, but it's fine for a first pass. We will improve it in the future. Closes https://github.com/denoland/deno_lint/pull/972 Closes https://github.com/denoland/deno_lint/issues/970 Closes https://github.com/denoland/deno/issues/19356 --- cli/args/flags.rs | 48 ++++++++++++++++- cli/tests/integration/doc_tests.rs | 11 ++++ .../doc/referenced_private_types_fixed.out | 16 ++++++ .../doc/referenced_private_types_fixed.ts | 11 ++++ .../doc/referenced_private_types_lint.out | 10 ++++ cli/tools/doc.rs | 51 ++++++++++++++++++- 6 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 cli/tests/testdata/doc/referenced_private_types_fixed.out create mode 100644 cli/tests/testdata/doc/referenced_private_types_fixed.ts create mode 100644 cli/tests/testdata/doc/referenced_private_types_lint.out diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 271a56ac36..0e4621d674 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -109,6 +109,7 @@ impl Default for DocSourceFileFlag { pub struct DocFlags { pub private: bool, pub json: bool, + pub lint: bool, pub source_files: DocSourceFileFlag, pub filter: Option, } @@ -1329,6 +1330,10 @@ Output documentation in JSON format: deno doc --json ./path/to/module.ts +Lint a module for documentation diagnostics: + + deno doc --lint ./path/to/module.ts + Target a specific symbol: deno doc ./path/to/module.ts MyClass.someField @@ -1363,7 +1368,14 @@ Show documentation for runtime built-ins: .long("filter") .help("Dot separated path to symbol") .required(false) - .conflicts_with("json"), + .conflicts_with("json") + .conflicts_with("lint"), + ) + .arg( + Arg::new("lint") + .long("lint") + .help("Output documentation diagnostics.") + .action(ArgAction::SetTrue), ) // TODO(nayeemrmn): Make `--builtin` a proper option. Blocked by // https://github.com/clap-rs/clap/issues/1794. Currently `--builtin` is @@ -3145,11 +3157,13 @@ fn doc_parse(flags: &mut Flags, matches: &mut ArgMatches) { DocSourceFileFlag::Builtin }; let private = matches.get_flag("private"); + let lint = matches.get_flag("lint"); let json = matches.get_flag("json"); let filter = matches.remove_one::("filter"); flags.subcommand = DenoSubcommand::Doc(DocFlags { source_files, json, + lint, filter, private, }); @@ -6044,6 +6058,7 @@ mod tests { source_files: DocSourceFileFlag::Paths(vec!["script.ts".to_owned()]), private: false, json: false, + lint: false, filter: None, }), import_map_path: Some("import_map.json".to_owned()), @@ -7298,6 +7313,7 @@ mod tests { subcommand: DenoSubcommand::Doc(DocFlags { private: false, json: true, + lint: false, source_files: DocSourceFileFlag::Paths(vec![ "path/to/module.ts".to_string() ]), @@ -7320,6 +7336,7 @@ mod tests { subcommand: DenoSubcommand::Doc(DocFlags { private: false, json: false, + lint: false, source_files: DocSourceFileFlag::Paths(vec![ "path/to/module.ts".to_string() ]), @@ -7336,6 +7353,7 @@ mod tests { subcommand: DenoSubcommand::Doc(DocFlags { private: false, json: false, + lint: false, source_files: Default::default(), filter: None, }), @@ -7355,6 +7373,7 @@ mod tests { Flags { subcommand: DenoSubcommand::Doc(DocFlags { private: false, + lint: false, json: false, source_files: DocSourceFileFlag::Builtin, filter: Some("Deno.Listener".to_string()), @@ -7376,6 +7395,7 @@ mod tests { Flags { subcommand: DenoSubcommand::Doc(DocFlags { private: true, + lint: false, json: false, source_files: DocSourceFileFlag::Paths(vec![ "path/to/module.js".to_string() @@ -7399,6 +7419,7 @@ mod tests { Flags { subcommand: DenoSubcommand::Doc(DocFlags { private: false, + lint: false, json: false, source_files: DocSourceFileFlag::Paths(vec![ "path/to/module.js".to_string(), @@ -7423,6 +7444,31 @@ mod tests { subcommand: DenoSubcommand::Doc(DocFlags { private: false, json: false, + lint: false, + source_files: DocSourceFileFlag::Paths(vec![ + "path/to/module.js".to_string(), + "path/to/module2.js".to_string() + ]), + filter: None, + }), + ..Flags::default() + } + ); + + let r = flags_from_vec(svec![ + "deno", + "doc", + "--lint", + "path/to/module.js", + "path/to/module2.js" + ]); + assert_eq!( + r.unwrap(), + Flags { + subcommand: DenoSubcommand::Doc(DocFlags { + private: false, + lint: true, + json: false, source_files: DocSourceFileFlag::Paths(vec![ "path/to/module.js".to_string(), "path/to/module2.js".to_string() diff --git a/cli/tests/integration/doc_tests.rs b/cli/tests/integration/doc_tests.rs index 2afa8ca92a..a16f99dd9a 100644 --- a/cli/tests/integration/doc_tests.rs +++ b/cli/tests/integration/doc_tests.rs @@ -53,6 +53,17 @@ itest!(deno_doc_referenced_private_types { output: "doc/referenced_private_types.out", }); +itest!(deno_doc_lint_referenced_private_types_error { + args: "doc --lint doc/referenced_private_types.ts", + exit_code: 1, + output: "doc/referenced_private_types_lint.out", +}); + +itest!(deno_doc_lint_referenced_private_types_fixed { + args: "doc --lint doc/referenced_private_types_fixed.ts", + output: "doc/referenced_private_types_fixed.out", +}); + itest!(_060_deno_doc_displays_all_overloads_in_details_view { args: "doc --filter NS.test doc/060_deno_doc_displays_all_overloads_in_details_view.ts", diff --git a/cli/tests/testdata/doc/referenced_private_types_fixed.out b/cli/tests/testdata/doc/referenced_private_types_fixed.out new file mode 100644 index 0000000000..4621c6371e --- /dev/null +++ b/cli/tests/testdata/doc/referenced_private_types_fixed.out @@ -0,0 +1,16 @@ +Defined in file:///[WILDCARD]/referenced_private_types_fixed.ts:8:1 + +class MyClass + Doc comment + + prop: MyInterface + Doc comment + +Defined in file:///[WILDCARD]/referenced_private_types_fixed.ts:2:1 + +interface MyInterface + Doc comment + + prop?: string + Doc comment + diff --git a/cli/tests/testdata/doc/referenced_private_types_fixed.ts b/cli/tests/testdata/doc/referenced_private_types_fixed.ts new file mode 100644 index 0000000000..cd99bc76ea --- /dev/null +++ b/cli/tests/testdata/doc/referenced_private_types_fixed.ts @@ -0,0 +1,11 @@ +/** Doc comment */ +export interface MyInterface { + /** Doc comment */ + prop?: string; +} + +/** Doc comment */ +export class MyClass { + /** Doc comment */ + prop: MyInterface = {}; +} diff --git a/cli/tests/testdata/doc/referenced_private_types_lint.out b/cli/tests/testdata/doc/referenced_private_types_lint.out new file mode 100644 index 0000000000..bb8c599f49 --- /dev/null +++ b/cli/tests/testdata/doc/referenced_private_types_lint.out @@ -0,0 +1,10 @@ +Type is not exported, but referenced by an exported type. + at file:///[WILDCARD]/referenced_private_types.ts:1:1 + +Missing JS documentation comment. + at file:///[WILDCARD]/referenced_private_types.ts:5:1 + +Missing JS documentation comment. + at file:///[WILDCARD]/referenced_private_types.ts:6:3 + +error: Found 3 documentation diagnostics. diff --git a/cli/tools/doc.rs b/cli/tools/doc.rs index 193362267a..4028c412fc 100644 --- a/cli/tools/doc.rs +++ b/cli/tools/doc.rs @@ -1,5 +1,7 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use std::collections::BTreeMap; + use crate::args::DocFlags; use crate::args::DocSourceFileFlag; use crate::args::Flags; @@ -18,6 +20,8 @@ use deno_graph::CapturingModuleParser; use deno_graph::DefaultParsedSourceStore; use deno_graph::GraphKind; use deno_graph::ModuleSpecifier; +use doc::DocDiagnostic; +use indexmap::IndexMap; pub async fn print_docs( flags: Flags, @@ -101,7 +105,7 @@ pub async fn print_docs( capturing_parser, doc::DocParserOptions { private: doc_flags.private, - diagnostics: false, + diagnostics: doc_flags.lint, }, )?; @@ -112,6 +116,11 @@ pub async fn print_docs( doc_nodes.extend_from_slice(&nodes); } + if doc_flags.lint { + let diagnostics = doc_parser.take_diagnostics(); + check_diagnostics(&diagnostics)?; + } + doc_nodes } }; @@ -144,3 +153,43 @@ pub async fn print_docs( write_to_stdout_ignore_sigpipe(details.as_bytes()).map_err(AnyError::from) } } + +fn check_diagnostics(diagnostics: &[DocDiagnostic]) -> Result<(), AnyError> { + if diagnostics.is_empty() { + return Ok(()); + } + + // group by location then by line (sorted) then column (sorted) + let mut diagnostic_groups = IndexMap::new(); + for diagnostic in diagnostics { + diagnostic_groups + .entry(diagnostic.location.filename.clone()) + .or_insert_with(BTreeMap::new) + .entry(diagnostic.location.line) + .or_insert_with(BTreeMap::new) + .entry(diagnostic.location.col) + .or_insert_with(Vec::new) + .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 diagnostic in diagnostics { + log::warn!("{}", diagnostic.kind); + } + log::warn!( + " at {}:{}:{}\n", + colors::cyan(filename.as_str()), + colors::yellow(&line.to_string()), + colors::yellow(&(col + 1).to_string()) + ) + } + } + } + bail!( + "Found {} documentation diagnostic{}.", + colors::bold(diagnostics.len().to_string()), + if diagnostics.len() == 1 { "" } else { "s" } + ); +}