fix(doc): surface graph errors as warnings (#25888)

5 files changed, 75 insertions, 65 deletions

diff --git a/cli/graph_util.rs b/cli/graph_util.rs
index 8a6839919..17f7d6739 100644
--- a/cli/graph_util.rs
+++ b/cli/graph_util.rs
@@ -52,14 +52,14 @@ use std::ops::Deref;
 use std::path::PathBuf;
 use std::sync::Arc;
 
-#[derive(Clone, Copy)]
+#[derive(Clone)]
 pub struct GraphValidOptions {
   pub check_js: bool,
   pub kind: GraphKind,
-  pub is_vendoring: bool,
-  /// Whether to exit the process for lockfile errors.
-  /// Otherwise, surfaces lockfile errors as errors.
-  pub exit_lockfile_errors: bool,
+  /// Whether to exit the process for integrity check errors such as
+  /// lockfile checksum mismatches and JSR integrity failures.
+  /// Otherwise, surfaces integrity errors as errors.
+  pub exit_integrity_errors: bool,
 }
 
 /// Check if `roots` and their deps are available. Returns `Ok(())` if
@@ -75,17 +75,54 @@ pub fn graph_valid(
   roots: &[ModuleSpecifier],
   options: GraphValidOptions,
 ) -> Result<(), AnyError> {
-  if options.exit_lockfile_errors {
-    graph_exit_lock_errors(graph);
+  if options.exit_integrity_errors {
+    graph_exit_integrity_errors(graph);
   }
 
-  let mut errors = graph
+  let mut errors = graph_walk_errors(
+    graph,
+    fs,
+    roots,
+    GraphWalkErrorsOptions {
+      check_js: options.check_js,
+      kind: options.kind,
+    },
+  );
+  if let Some(error) = errors.next() {
+    Err(error)
+  } else {
+    // finally surface the npm resolution result
+    if let Err(err) = &graph.npm_dep_graph_result {
+      return Err(custom_error(
+        get_error_class_name(err),
+        format_deno_graph_error(err.as_ref().deref()),
+      ));
+    }
+    Ok(())
+  }
+}
+
+#[derive(Clone)]
+pub struct GraphWalkErrorsOptions {
+  pub check_js: bool,
+  pub kind: GraphKind,
+}
+
+/// Walks the errors found in the module graph that should be surfaced to users
+/// and enhances them with CLI information.
+pub fn graph_walk_errors<'a>(
+  graph: &'a ModuleGraph,
+  fs: &'a Arc<dyn FileSystem>,
+  roots: &'a [ModuleSpecifier],
+  options: GraphWalkErrorsOptions,
+) -> impl Iterator<Item = AnyError> + 'a {
+  graph
     .walk(
       roots.iter(),
       deno_graph::WalkOptions {
         check_js: options.check_js,
         kind: options.kind,
-        follow_dynamic: options.is_vendoring,
+        follow_dynamic: false,
         prefer_fast_check_graph: false,
       },
     )
@@ -109,7 +146,7 @@ pub fn graph_valid(
           )
         }
         ModuleGraphError::ModuleError(error) => {
-          enhanced_lockfile_error_message(error)
+          enhanced_integrity_error_message(error)
             .or_else(|| enhanced_sloppy_imports_error_message(fs, error))
             .unwrap_or_else(|| format_deno_graph_error(error))
         }
@@ -132,56 +169,18 @@ pub fn graph_valid(
         return None;
       }
 
-      if options.is_vendoring {
-        // warn about failing dynamic imports when vendoring, but don't fail completely
-        if matches!(
-          error,
-          ModuleGraphError::ModuleError(ModuleError::MissingDynamic(_, _))
-        ) {
-          log::warn!("Ignoring: {}", message);
-          return None;
-        }
-
-        // ignore invalid downgrades and invalid local imports when vendoring
-        match &error {
-          ModuleGraphError::ResolutionError(err)
-          | ModuleGraphError::TypesResolutionError(err) => {
-            if matches!(
-              err,
-              ResolutionError::InvalidDowngrade { .. }
-                | ResolutionError::InvalidLocalImport { .. }
-            ) {
-              return None;
-            }
-          }
-          ModuleGraphError::ModuleError(_) => {}
-        }
-      }
-
       Some(custom_error(get_error_class_name(&error.into()), message))
-    });
-  if let Some(error) = errors.next() {
-    Err(error)
-  } else {
-    // finally surface the npm resolution result
-    if let Err(err) = &graph.npm_dep_graph_result {
-      return Err(custom_error(
-        get_error_class_name(err),
-        format_deno_graph_error(err.as_ref().deref()),
-      ));
-    }
-    Ok(())
-  }
+    })
 }
 
-pub fn graph_exit_lock_errors(graph: &ModuleGraph) {
+pub fn graph_exit_integrity_errors(graph: &ModuleGraph) {
   for error in graph.module_errors() {
-    exit_for_lockfile_error(error);
+    exit_for_integrity_error(error);
   }
 }
 
-fn exit_for_lockfile_error(err: &ModuleError) {
-  if let Some(err_message) = enhanced_lockfile_error_message(err) {
+fn exit_for_integrity_error(err: &ModuleError) {
+  if let Some(err_message) = enhanced_integrity_error_message(err) {
     log::error!("{} {}", colors::red("error:"), err_message);
     std::process::exit(10);
   }
@@ -719,14 +718,13 @@ impl ModuleGraphBuilder {
       &self.fs,
       roots,
       GraphValidOptions {
-        is_vendoring: false,
         kind: if self.options.type_check_mode().is_true() {
           GraphKind::All
         } else {
           GraphKind::CodeOnly
         },
         check_js: self.options.check_js(),
-        exit_lockfile_errors: true,
+        exit_integrity_errors: true,
       },
     )
   }
@@ -780,7 +778,7 @@ fn enhanced_sloppy_imports_error_message(
   }
 }
 
-fn enhanced_lockfile_error_message(err: &ModuleError) -> Option<String> {
+fn enhanced_integrity_error_message(err: &ModuleError) -> Option<String> {
   match err {
     ModuleError::LoadingErr(
       specifier,
diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index aed4b47ef..216b14de6 100644
--- a/cli/lsp/language_server.rs
+++ b/cli/lsp/language_server.rs
@@ -274,10 +274,9 @@ impl LanguageServer {
         factory.fs(),
         &roots,
         graph_util::GraphValidOptions {
-          is_vendoring: false,
           kind: GraphKind::All,
           check_js: false,
-          exit_lockfile_errors: false,
+          exit_integrity_errors: false,
         },
       )?;
 
diff --git a/cli/tools/doc.rs b/cli/tools/doc.rs
index 0ba3b84fb..8f37632c8 100644
--- a/cli/tools/doc.rs
+++ b/cli/tools/doc.rs
@@ -7,7 +7,9 @@ use crate::args::Flags;
 use crate::colors;
 use crate::display;
 use crate::factory::CliFactory;
-use crate::graph_util::graph_exit_lock_errors;
+use crate::graph_util::graph_exit_integrity_errors;
+use crate::graph_util::graph_walk_errors;
+use crate::graph_util::GraphWalkErrorsOptions;
 use crate::tsc::get_types_declaration_file_text;
 use crate::util::fs::collect_specifiers;
 use deno_ast::diagnostics::Diagnostic;
@@ -107,7 +109,7 @@ pub async fn doc(
     }
     DocSourceFileFlag::Paths(ref source_files) => {
       let module_graph_creator = factory.module_graph_creator().await?;
-      let maybe_lockfile = cli_options.maybe_lockfile();
+      let fs = factory.fs();
 
       let module_specifiers = collect_specifiers(
         FilePatterns {
@@ -127,8 +129,18 @@ pub async fn doc(
         .create_graph(GraphKind::TypesOnly, module_specifiers.clone())
         .await?;
 
-      if maybe_lockfile.is_some() {
-        graph_exit_lock_errors(&graph);
+      graph_exit_integrity_errors(&graph);
+      let errors = graph_walk_errors(
+        &graph,
+        fs,
+        &module_specifiers,
+        GraphWalkErrorsOptions {
+          check_js: false,
+          kind: GraphKind::TypesOnly,
+        },
+      );
+      for error in errors {
+        log::warn!("{} {}", colors::yellow("Warning"), error);
       }
 
       let doc_parser = doc::DocParser::new(
diff --git a/cli/tools/info.rs b/cli/tools/info.rs
index 174785631..b92887d53 100644
--- a/cli/tools/info.rs
+++ b/cli/tools/info.rs
@@ -29,7 +29,7 @@ use crate::args::Flags;
 use crate::args::InfoFlags;
 use crate::display;
 use crate::factory::CliFactory;
-use crate::graph_util::graph_exit_lock_errors;
+use crate::graph_util::graph_exit_integrity_errors;
 use crate::npm::CliNpmResolver;
 use crate::npm::ManagedCliNpmResolver;
 use crate::util::checksum;
@@ -75,7 +75,7 @@ pub async fn info(
 
     // write out the lockfile if there is one
     if let Some(lockfile) = &maybe_lockfile {
-      graph_exit_lock_errors(&graph);
+      graph_exit_integrity_errors(&graph);
       lockfile.write_if_changed()?;
     }
 
diff --git a/tests/specs/permission/allow_import/doc.out b/tests/specs/permission/allow_import/doc.out
index bc748d726..6a57eafe6 100644
--- a/tests/specs/permission/allow_import/doc.out
+++ b/tests/specs/permission/allow_import/doc.out
@@ -1,4 +1,5 @@
-[# todo(dsherret): we should probably at least show a warning here]
+Warning Requires import access to "localhost:4545", run again with the --allow-import flag
+    at file:///[WILDLINE]/doc.ts:1:15
 Defined in file:///[WILDLINE]/doc.ts:3:1
 
 class Test