fix(publish): properly display graph validation errors (#22775)

The graph validation errors were displaying cryptically during publish.
This fixes that.

cli/factory.rs

@@ -651,7 +651,6 @@ impl CliFactory {
       .get_or_try_init_async(async {
         Ok(Arc::new(ModuleGraphCreator::new(
           self.options.clone(),
-          self.fs().clone(),
           self.npm_resolver().await?.clone(),
           self.module_graph_builder().await?.clone(),
           self.maybe_lockfile().clone(),
@@ -689,7 +688,6 @@ impl CliFactory {
       .get_or_try_init_async(async {
         Ok(Arc::new(ModuleLoadPreparer::new(
           self.options.clone(),
-          self.fs().clone(),
           self.graph_container().clone(),
           self.maybe_lockfile().clone(),
           self.module_graph_builder().await?.clone(),

cli/graph_util.rs

@@ -58,27 +58,6 @@ pub struct GraphValidOptions {
   pub is_vendoring: bool,
 }
 
-/// Check if `roots` and their deps are available. Returns `Ok(())` if
-/// so. Returns `Err(_)` if there is a known module graph or resolution
-/// error statically reachable from `roots` and not a dynamic import.
-pub fn graph_valid_with_cli_options(
-  graph: &ModuleGraph,
-  fs: &dyn FileSystem,
-  roots: &[ModuleSpecifier],
-  options: &CliOptions,
-) -> Result<(), AnyError> {
-  graph_valid(
-    graph,
-    fs,
-    roots,
-    GraphValidOptions {
-      is_vendoring: false,
-      follow_type_only: options.type_check_mode().is_true(),
-      check_js: options.check_js(),
-    },
-  )
-}
-
 /// Check if `roots` and their deps are available. Returns `Ok(())` if
 /// so. Returns `Err(_)` if there is a known module graph or resolution
 /// error statically reachable from `roots`.
@@ -214,7 +193,6 @@ pub struct CreateGraphOptions<'a> {
 
 pub struct ModuleGraphCreator {
   options: Arc<CliOptions>,
-  fs: Arc<dyn FileSystem>,
   npm_resolver: Arc<dyn CliNpmResolver>,
   module_graph_builder: Arc<ModuleGraphBuilder>,
   lockfile: Option<Arc<Mutex<Lockfile>>>,
@@ -224,7 +202,6 @@ pub struct ModuleGraphCreator {
 impl ModuleGraphCreator {
   pub fn new(
     options: Arc<CliOptions>,
-    fs: Arc<dyn FileSystem>,
     npm_resolver: Arc<dyn CliNpmResolver>,
     module_graph_builder: Arc<ModuleGraphBuilder>,
     lockfile: Option<Arc<Mutex<Lockfile>>>,
@@ -232,7 +209,6 @@ impl ModuleGraphCreator {
   ) -> Self {
     Self {
       options,
-      fs,
       npm_resolver,
       lockfile,
       module_graph_builder,
@@ -267,9 +243,10 @@ impl ModuleGraphCreator {
       .await
   }
 
-  pub async fn create_publish_graph(
+  pub async fn create_and_validate_publish_graph(
     &self,
     packages: &[WorkspaceMemberConfig],
+    build_fast_check_graph: bool,
   ) -> Result<ModuleGraph, AnyError> {
     let mut roots = Vec::new();
     for package in packages {
@@ -283,15 +260,18 @@ impl ModuleGraphCreator {
         loader: None,
       })
       .await?;
+    self.graph_valid(&graph)?;
     if self.options.type_check_mode().is_true() {
       self.type_check_graph(graph.clone()).await?;
     }
+    if build_fast_check_graph {
       self.module_graph_builder.build_fast_check_graph(
         &mut graph,
         BuildFastCheckGraphOptions {
           workspace_fast_check: true,
         },
       )?;
+    }
     Ok(graph)
   }
 
@@ -330,12 +310,7 @@ impl ModuleGraphCreator {
       })
       .await?;
 
-    graph_valid_with_cli_options(
-      &graph,
-      self.fs.as_ref(),
-      &graph.roots,
-      &self.options,
-    )?;
+    self.graph_valid(&graph)?;
     if let Some(lockfile) = &self.lockfile {
       graph_lock_or_exit(&graph, &mut lockfile.lock());
     }
@@ -349,6 +324,10 @@ impl ModuleGraphCreator {
     }
   }
 
+  pub fn graph_valid(&self, graph: &ModuleGraph) -> Result<(), AnyError> {
+    self.module_graph_builder.graph_valid(graph)
+  }
+
   async fn type_check_graph(
     &self,
     graph: ModuleGraph,
@@ -658,6 +637,30 @@ impl ModuleGraphBuilder {
       permissions,
     )
   }
+
+  /// Check if `roots` and their deps are available. Returns `Ok(())` if
+  /// so. Returns `Err(_)` if there is a known module graph or resolution
+  /// error statically reachable from `roots` and not a dynamic import.
+  pub fn graph_valid(&self, graph: &ModuleGraph) -> Result<(), AnyError> {
+    self.graph_roots_valid(graph, &graph.roots)
+  }
+
+  pub fn graph_roots_valid(
+    &self,
+    graph: &ModuleGraph,
+    roots: &[ModuleSpecifier],
+  ) -> Result<(), AnyError> {
+    graph_valid(
+      graph,
+      self.fs.as_ref(),
+      roots,
+      GraphValidOptions {
+        is_vendoring: false,
+        follow_type_only: self.options.type_check_mode().is_true(),
+        check_js: self.options.check_js(),
+      },
+    )
+  }
 }
 
 pub fn error_for_any_npm_specifier(
@@ -697,7 +700,8 @@ pub fn enhanced_module_error_message(
   error: &ModuleError,
 ) -> String {
   let additional_message = match error {
-    ModuleError::Missing(specifier, _) => {
+    ModuleError::LoadingErr(specifier, _, _) // ex. "Is a directory" error
+    | ModuleError::Missing(specifier, _) => {
       SloppyImportsResolver::resolve_with_fs(
         fs,
         specifier,

cli/module_loader.rs

@@ -7,7 +7,6 @@ use crate::args::TsTypeLib;
 use crate::cache::ParsedSourceCache;
 use crate::emit::Emitter;
 use crate::graph_util::graph_lock_or_exit;
-use crate::graph_util::graph_valid_with_cli_options;
 use crate::graph_util::CreateGraphOptions;
 use crate::graph_util::ModuleGraphBuilder;
 use crate::graph_util::ModuleGraphContainer;
@@ -51,7 +50,6 @@ use deno_graph::JsonModule;
 use deno_graph::Module;
 use deno_graph::Resolution;
 use deno_lockfile::Lockfile;
-use deno_runtime::deno_fs;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::permissions::PermissionsContainer;
 use deno_semver::npm::NpmPackageReqReference;
@@ -65,7 +63,6 @@ use std::sync::Arc;
 
 pub struct ModuleLoadPreparer {
   options: Arc<CliOptions>,
-  fs: Arc<dyn deno_fs::FileSystem>,
   graph_container: Arc<ModuleGraphContainer>,
   lockfile: Option<Arc<Mutex<Lockfile>>>,
   module_graph_builder: Arc<ModuleGraphBuilder>,
@@ -77,7 +74,6 @@ impl ModuleLoadPreparer {
   #[allow(clippy::too_many_arguments)]
   pub fn new(
     options: Arc<CliOptions>,
-    fs: Arc<dyn deno_fs::FileSystem>,
     graph_container: Arc<ModuleGraphContainer>,
     lockfile: Option<Arc<Mutex<Lockfile>>>,
     module_graph_builder: Arc<ModuleGraphBuilder>,
@@ -86,7 +82,6 @@ impl ModuleLoadPreparer {
   ) -> Self {
     Self {
       options,
-      fs,
       graph_container,
       lockfile,
       module_graph_builder,
@@ -134,12 +129,7 @@ impl ModuleLoadPreparer {
       )
       .await?;
 
-    graph_valid_with_cli_options(
-      graph,
-      self.fs.as_ref(),
-      &roots,
-      &self.options,
-    )?;
+    self.module_graph_builder.graph_roots_valid(graph, &roots)?;
 
     // If there is a lockfile...
     if let Some(lockfile) = &self.lockfile {

cli/tools/bench/mod.rs

@@ -7,7 +7,6 @@ use crate::colors;
 use crate::display::write_json_to_stdout;
 use crate::factory::CliFactory;
 use crate::factory::CliFactoryBuilder;
-use crate::graph_util::graph_valid_with_cli_options;
 use crate::graph_util::has_graph_root_local_dependent_changed;
 use crate::module_loader::ModuleLoadPreparer;
 use crate::ops;
@@ -526,14 +525,10 @@ pub async fn run_benchmarks_with_watch(
           Permissions::from_options(&cli_options.permissions_options())?;
 
         let graph = module_graph_creator
-          .create_graph(graph_kind, bench_modules.clone())
+          .create_graph(graph_kind, bench_modules)
           .await?;
-        graph_valid_with_cli_options(
-          &graph,
-          factory.fs().as_ref(),
-          &bench_modules,
-          cli_options,
-        )?;
+        module_graph_creator.graph_valid(&graph)?;
+        let bench_modules = &graph.roots;
 
         let bench_modules_to_reload = if let Some(changed_paths) = changed_paths
         {
@@ -542,7 +537,7 @@ pub async fn run_benchmarks_with_watch(
           for bench_module_specifier in bench_modules {
             if has_graph_root_local_dependent_changed(
               &graph,
-              &bench_module_specifier,
+              bench_module_specifier,
               &changed_paths,
             ) {
               result.push(bench_module_specifier.clone());
@@ -556,7 +551,7 @@ pub async fn run_benchmarks_with_watch(
         let worker_factory =
           Arc::new(factory.create_cli_main_worker_factory().await?);
 
-        // todo(THIS PR): why are we collecting specifiers twice in a row?
+        // todo(dsherret): why are we collecting specifiers twice in a row?
         // Seems like a perf bug.
         let specifiers =
           collect_specifiers(bench_options.files, is_supported_bench_path)?

cli/tools/lint/mod.rs

@@ -184,7 +184,9 @@ async fn lint_files(
         .filter_map(|p| ModuleSpecifier::from_file_path(p).ok())
         .collect::<HashSet<_>>();
       futures.push(deno_core::unsync::spawn(async move {
-        let graph = module_graph_creator.create_publish_graph(&members).await?;
+        let graph = module_graph_creator
+          .create_and_validate_publish_graph(&members, true)
+          .await?;
         // todo(dsherret): this isn't exactly correct as linting isn't properly
         // setup to handle workspaces. Iterating over the workspace members
         // should be done at a higher level because it also needs to take into

cli/tools/registry/mod.rs

@@ -812,8 +812,10 @@ async fn build_and_check_graph_for_publish(
   diagnostics_collector: &PublishDiagnosticsCollector,
   packages: &[WorkspaceMemberConfig],
 ) -> Result<Arc<deno_graph::ModuleGraph>, deno_core::anyhow::Error> {
-  let graph = module_graph_creator.create_publish_graph(packages).await?;
-  graph.valid()?;
+  let build_fast_check_graph = !allow_slow_types;
+  let graph = module_graph_creator
+    .create_and_validate_publish_graph(packages, build_fast_check_graph)
+    .await?;
 
   // todo(dsherret): move to lint rule
   collect_invalid_external_imports(&graph, diagnostics_collector);

cli/tools/test/mod.rs

@@ -10,7 +10,6 @@ use crate::factory::CliFactory;
 use crate::factory::CliFactoryBuilder;
 use crate::file_fetcher::File;
 use crate::file_fetcher::FileFetcher;
-use crate::graph_util::graph_valid_with_cli_options;
 use crate::graph_util::has_graph_root_local_dependent_changed;
 use crate::module_loader::ModuleLoadPreparer;
 use crate::ops;
@@ -1612,14 +1611,10 @@ pub async fn run_tests_with_watch(
         let permissions =
           Permissions::from_options(&cli_options.permissions_options())?;
         let graph = module_graph_creator
-          .create_graph(graph_kind, test_modules.clone())
+          .create_graph(graph_kind, test_modules)
           .await?;
-        graph_valid_with_cli_options(
-          &graph,
-          factory.fs().as_ref(),
-          &test_modules,
-          &cli_options,
-        )?;
+        module_graph_creator.graph_valid(&graph)?;
+        let test_modules = &graph.roots;
 
         let test_modules_to_reload = if let Some(changed_paths) = changed_paths
         {
@@ -1628,7 +1623,7 @@ pub async fn run_tests_with_watch(
           for test_module_specifier in test_modules {
             if has_graph_root_local_dependent_changed(
               &graph,
-              &test_module_specifier,
+              test_module_specifier,
               &changed_paths,
             ) {
               result.push(test_module_specifier.clone());

tests/integration/publish_tests.rs

@@ -275,6 +275,15 @@ itest!(sloppy_imports {
   http_server: true,
 });
 
+itest!(sloppy_imports_not_enabled {
+  args: "publish --token 'sadfasdf' --dry-run",
+  output: "publish/sloppy_imports_not_enabled.out",
+  cwd: Some("publish/sloppy_imports"),
+  envs: env_vars_for_jsr_tests(),
+  http_server: true,
+  exit_code: 1,
+});
+
 itest!(sloppy_imports_no_warnings {
   args: "publish --token 'sadfasdf' --dry-run  --unstable-sloppy-imports",
   output: "publish/sloppy_imports_no_warnings.out",

tests/testdata/publish/sloppy_imports_not_enabled.out

@@ -0,0 +1,2 @@
+error: [WILDCARD] Maybe specify path to 'index.ts' file in directory instead or run with --unstable-sloppy-imports
+    at file:///[WILDCARD]/sloppy_imports/mod.ts:1:20