fix(check): should bust check cache when json module or npm resolution changes (#19941)

A small part of #19928.

Cargo.lock

@@ -1035,9 +1035,9 @@ dependencies = [
 
 [[package]]
 name = "deno_doc"
-version = "0.63.1"
+version = "0.64.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "499936300106c3c67caae87e29def3df5ea9385db6ed7428f154972f70ed39fa"
+checksum = "c25d38dc94e4c2190bc7a1851790dd142fd0068888b3a26a1acb958db5113829"
 dependencies = [
  "cfg-if",
  "deno_ast",
@@ -1053,9 +1053,9 @@ dependencies = [
 
 [[package]]
 name = "deno_emit"
-version = "0.24.0"
+version = "0.25.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6bdc024d2c1e5ec56ef6f923be2c2fea4662d596b0a68074ccf89991b38a05e7"
+checksum = "0ef00ad78bc75e7f35a01bd4b5c32a97018eebc55c311e4b287e158a29021348"
 dependencies = [
  "anyhow",
  "base64 0.13.1",
@@ -1121,9 +1121,9 @@ dependencies = [
 
 [[package]]
 name = "deno_graph"
-version = "0.49.0"
+version = "0.50.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a7e07fdff6c7dc1a9a7c03ce69435fda4b53641d2d6d3d3ed6d29cf67fefd3ea"
+checksum = "73502c4d93a17f259b6edee6d5a5ba063e2fcdcdaeb6ca1c6953129cc14be6a7"
 dependencies = [
  "anyhow",
  "data-url",
@@ -1330,9 +1330,9 @@ dependencies = [
 
 [[package]]
 name = "deno_npm"
-version = "0.9.1"
+version = "0.10.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "371ef0398b5b5460d66b78a958d5015658e198ad3a29fb9ce329459272fd29aa"
+checksum = "fa5d1097de53e8ce3316d3e44095e253719ae367cf7478263f83082f44dddabf"
 dependencies = [
  "anyhow",
  "async-trait",
@@ -1340,7 +1340,6 @@ dependencies = [
  "futures",
  "log",
  "monch",
- "once_cell",
  "serde",
  "thiserror",
 ]
@@ -1424,11 +1423,12 @@ dependencies = [
 
 [[package]]
 name = "deno_semver"
-version = "0.2.2"
+version = "0.3.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "242c8ad9f4ce614ec0fa2e6b3d834f2662ce024ca78e9ed4c58d812cbfc3e41d"
+checksum = "96f99990457915af1f444900003ffd5a9d3ab2e5337b06d681e56ca371b3e11f"
 dependencies = [
  "monch",
+ "once_cell",
  "serde",
  "thiserror",
  "url",
@@ -1944,9 +1944,9 @@ dependencies = [
 
 [[package]]
 name = "eszip"
-version = "0.45.0"
+version = "0.48.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bbf5a0f47c2e73cb7631accc282ebda9bc9ed9b2034abfddec983dc9c8f78e7a"
+checksum = "58d2382a70396ccffd260adb879ed9f954c45457e8075830c7d4ec30c6dc8ad2"
 dependencies = [
  "anyhow",
  "base64 0.21.0",

Cargo.toml

@@ -51,8 +51,8 @@ deno_bench_util = { version = "0.107.0", path = "./bench_util" }
 test_util = { path = "./test_util" }
 deno_lockfile = "0.14.1"
 deno_media_type = { version = "0.1.0", features = ["module_specifier"] }
-deno_npm = "0.9.1"
+deno_npm = "0.10.1"
-deno_semver = "0.2.2"
+deno_semver = "0.3.0"
 
 # exts
 deno_broadcast_channel = { version = "0.107.0", path = "./ext/broadcast_channel" }

cli/Cargo.toml

@@ -42,16 +42,16 @@ winres.workspace = true
 [dependencies]
 deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_graph", "module_specifier", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] }
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
-deno_doc = "=0.63.1"
+deno_doc = "=0.64.0"
-deno_emit = "=0.24.0"
+deno_emit = "=0.25.0"
-deno_graph = "=0.49.0"
+deno_graph = "=0.50.0"
 deno_lint = { version = "=0.49.0", features = ["docs"] }
 deno_lockfile.workspace = true
 deno_npm.workspace = true
 deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "include_js_files_for_snapshotting"] }
 deno_semver.workspace = true
 deno_task_shell = "=0.13.1"
-eszip = "=0.45.0"
+eszip = "=0.48.0"
 napi_sym.workspace = true
 
 async-trait.workspace = true

cli/args/package_json.rs

@@ -85,7 +85,7 @@ pub fn get_local_package_json_version_reqs(
     match result {
       Ok(version_req) => Ok(NpmPackageReq {
         name: name.to_string(),
-        version_req: Some(version_req),
+        version_req,
       }),
       Err(err) => Err(PackageJsonDepValueParseError::Specifier(err)),
     }

cli/npm/resolution.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use std::collections::HashMap;
 use std::collections::HashSet;
 use std::sync::Arc;
 
@@ -249,6 +250,10 @@ impl NpmResolution {
     Ok(nv)
   }
 
+  pub fn package_reqs(&self) -> HashMap<NpmPackageReq, NpmPackageNv> {
+    self.snapshot.read().package_reqs().clone()
+  }
+
   pub fn all_system_packages(
     &self,
     system_info: &NpmSystemInfo,
@@ -303,45 +308,44 @@ async fn add_package_reqs_to_snapshot(
   get_new_snapshot: impl Fn() -> NpmResolutionSnapshot,
 ) -> Result<NpmResolutionSnapshot, AnyError> {
   let snapshot = get_new_snapshot();
-  if !snapshot.has_pending()
+  let snapshot = if !snapshot.has_pending()
     && package_reqs
       .iter()
       .all(|req| snapshot.package_reqs().contains_key(req))
   {
     log::debug!("Snapshot already up to date. Skipping pending resolution.");
-    return Ok(snapshot);
+    snapshot
-  }
+  } else {
+    let pending_resolver = get_npm_pending_resolver(api);
+    let result = pending_resolver
+      .resolve_pending(snapshot, package_reqs)
+      .await;
+    api.clear_memory_cache();
+    match result {
+      Ok(snapshot) => snapshot,
+      Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => {
+        log::debug!("{err:#}");
+        log::debug!("npm resolution failed. Trying again...");
 
-  let pending_resolver = get_npm_pending_resolver(api);
+        // try again
-  let result = pending_resolver
+        let snapshot = get_new_snapshot();
-    .resolve_pending(snapshot, package_reqs)
+        let result = pending_resolver
-    .await;
+          .resolve_pending(snapshot, package_reqs)
-  api.clear_memory_cache();
+          .await;
-  let snapshot = match result {
+        api.clear_memory_cache();
-    Ok(snapshot) => snapshot,
+        // now surface the result after clearing the cache
-    Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => {
+        result?
-      log::debug!("{err:#}");
+      }
-      log::debug!("npm resolution failed. Trying again...");
+      Err(err) => return Err(err.into()),
-
-      // try again
-      let snapshot = get_new_snapshot();
-      let result = pending_resolver
-        .resolve_pending(snapshot, package_reqs)
-        .await;
-      api.clear_memory_cache();
-      // now surface the result after clearing the cache
-      result?
     }
-    Err(err) => return Err(err.into()),
   };
 
   if let Some(lockfile_mutex) = maybe_lockfile {
     let mut lockfile = lockfile_mutex.lock();
     populate_lockfile_from_snapshot(&mut lockfile, &snapshot)?;
-    Ok(snapshot)
-  } else {
-    Ok(snapshot)
   }
+
+  Ok(snapshot)
 }
 
 fn get_npm_pending_resolver(

cli/npm/resolvers/mod.rs

@@ -4,6 +4,7 @@ mod common;
 mod global;
 mod local;
 
+use std::collections::HashMap;
 use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -153,7 +154,7 @@ impl CliNpmResolver {
     let Some(cache_folder_id) = self
       .fs_resolver
       .resolve_package_cache_folder_id_from_specifier(specifier)? else {
-return Ok(None);
+        return Ok(None);
       };
     Ok(Some(
       self
@@ -229,6 +230,10 @@ return Ok(None);
     .unwrap()
   }
 
+  pub fn package_reqs(&self) -> HashMap<NpmPackageReq, NpmPackageNv> {
+    self.resolution.package_reqs()
+  }
+
   pub fn snapshot(&self) -> NpmResolutionSnapshot {
     self.resolution.snapshot()
   }

cli/standalone/mod.rs

@@ -191,7 +191,7 @@ impl ModuleLoader for EmbeddedModuleLoader {
       }
 
       let module = module?;
-      let code = module.source().await.unwrap_or_default();
+      let code = module.source().await.unwrap_or_else(|| Arc::new([]));
       let code = std::str::from_utf8(&code)
         .map_err(|_| type_error("Module source is not utf-8"))?
         .to_owned()
@@ -204,6 +204,9 @@ impl ModuleLoader for EmbeddedModuleLoader {
           eszip::ModuleKind::Jsonc => {
             return Err(type_error("jsonc modules not supported"))
           }
+          eszip::ModuleKind::OpaqueData => {
+            unreachable!();
+          }
         },
         code,
         &module_specifier,

cli/tests/integration/check_tests.rs

@@ -307,3 +307,80 @@ fn check_error_in_dep_then_fix() {
   output.assert_matches_text("Check [WILDCARD]main.ts\nerror: TS234[WILDCARD]");
   output.assert_exit_code(1);
 }
+
+#[test]
+fn json_module_check_then_error() {
+  let test_context = TestContextBuilder::new().use_temp_cwd().build();
+  let temp_dir = test_context.temp_dir();
+  let correct_code = "{ \"foo\": \"bar\" }";
+  let incorrect_code = "{ \"foo2\": \"bar\" }";
+
+  temp_dir.write(
+    "main.ts",
+    "import test from './test.json' assert { type: 'json' }; console.log(test.foo);\n",
+  );
+  temp_dir.write("test.json", correct_code);
+
+  let check_command = test_context.new_command().args_vec(["check", "main.ts"]);
+
+  check_command.run().assert_exit_code(0).skip_output_check();
+
+  temp_dir.write("test.json", incorrect_code);
+  check_command
+    .run()
+    .assert_matches_text("Check [WILDCARD]main.ts\nerror: TS2551[WILDCARD]")
+    .assert_exit_code(1);
+}
+
+#[test]
+fn npm_module_check_then_error() {
+  let test_context = TestContextBuilder::new()
+    .use_temp_cwd()
+    .add_npm_env_vars()
+    .use_http_server()
+    .build();
+  let temp_dir = test_context.temp_dir();
+  temp_dir.write("deno.json", "{}"); // so the lockfile gets loaded
+
+  // get the lockfiles values first (this is necessary because the test
+  // server generates different tarballs based on the operating system)
+  test_context
+    .new_command()
+    .args_vec([
+      "cache",
+      "npm:@denotest/breaking-change-between-versions@1.0.0",
+      "npm:@denotest/breaking-change-between-versions@2.0.0",
+    ])
+    .run()
+    .skip_output_check();
+  let lockfile = temp_dir.path().join("deno.lock");
+  let mut lockfile_content =
+    lockfile.read_json::<deno_lockfile::LockfileContent>();
+
+  // make the specifier resolve to version 1
+  lockfile_content.npm.specifiers.insert(
+    "@denotest/breaking-change-between-versions".to_string(),
+    "@denotest/breaking-change-between-versions@1.0.0".to_string(),
+  );
+  lockfile.write_json(&lockfile_content);
+  temp_dir.write(
+    "main.ts",
+    "import { oldName } from 'npm:@denotest/breaking-change-between-versions'; console.log(oldName());\n",
+  );
+
+  let check_command = test_context.new_command().args_vec(["check", "main.ts"]);
+  check_command.run().assert_exit_code(0).skip_output_check();
+
+  // now update the lockfile to use version 2 instead, which should cause a
+  // type checking error because the oldName no longer exists
+  lockfile_content.npm.specifiers.insert(
+    "@denotest/breaking-change-between-versions".to_string(),
+    "@denotest/breaking-change-between-versions@2.0.0".to_string(),
+  );
+  lockfile.write_json(&lockfile_content);
+
+  check_command
+    .run()
+    .assert_matches_text("Check [WILDCARD]main.ts\nerror: TS2305[WILDCARD]has no exported member 'oldName'[WILDCARD]")
+    .assert_exit_code(1);
+}

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/1.0.0/index.d.ts

@@ -0,0 +1 @@
+export function oldName(): 1;

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/1.0.0/index.js

@@ -0,0 +1,3 @@
+export function newName() {
+  return 1;
+}

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/1.0.0/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "@denotest/breaking-change-between-versions",
+  "version": "1.0.0",
+  "type": "module",
+  "main": "index.js"
+}

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/2.0.0/index.d.ts

@@ -0,0 +1 @@
+export function newName(): 2;

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/2.0.0/index.js

@@ -0,0 +1,3 @@
+export function newName() {
+  return 2;
+}

cli/tests/testdata/npm/registry/@denotest/breaking-change-between-versions/2.0.0/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "@denotest/breaking-change-between-versions",
+  "version": "2.0.0",
+  "type": "module",
+  "main": "index.js"
+}

cli/tools/check.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use std::collections::HashMap;
 use std::collections::HashSet;
 use std::sync::Arc;
 
@@ -10,6 +11,8 @@ use deno_graph::Module;
 use deno_graph::ModuleGraph;
 use deno_runtime::colors;
 use deno_runtime::deno_node::NodeResolver;
+use deno_semver::npm::NpmPackageNv;
+use deno_semver::npm::NpmPackageReq;
 use once_cell::sync::Lazy;
 use regex::Regex;
 
@@ -93,7 +96,12 @@ impl TypeChecker {
     let debug = self.cli_options.log_level() == Some(log::Level::Debug);
     let cache = TypeCheckCache::new(self.caches.type_checking_cache_db());
     let check_js = ts_config.get_check_js();
-    let check_hash = match get_check_hash(&graph, type_check_mode, &ts_config) {
+    let check_hash = match get_check_hash(
+      &graph,
+      self.npm_resolver.package_reqs(),
+      type_check_mode,
+      &ts_config,
+    ) {
       CheckHashResult::NoFiles => return Ok(()),
       CheckHashResult::Hash(hash) => hash,
     };
@@ -186,6 +194,7 @@ enum CheckHashResult {
 /// be used to tell
 fn get_check_hash(
   graph: &ModuleGraph,
+  package_reqs: HashMap<NpmPackageReq, NpmPackageNv>,
   type_check_mode: TypeCheckMode,
   ts_config: &TsConfig,
 ) -> CheckHashResult {
@@ -198,11 +207,10 @@ fn get_check_hash(
   hasher.write(&ts_config.as_bytes());
 
   let check_js = ts_config.get_check_js();
-  let mut sorted_modules = graph.modules().collect::<Vec<_>>();
-  sorted_modules.sort_by_key(|m| m.specifier().as_str()); // make it deterministic
   let mut has_file = false;
   let mut has_file_to_type_check = false;
-  for module in sorted_modules {
+  // this iterator of modules is already deterministic, so no need to sort it
+  for module in graph.modules() {
     match module {
       Module::Esm(module) => {
         let ts_check = has_ts_check(module.media_type, &module.source);
@@ -240,15 +248,36 @@ fn get_check_hash(
         hasher.write_str(module.specifier.as_str());
         hasher.write_str(&module.source);
       }
-      Module::Json(_)
+      Module::Node(_) => {
-      | Module::External(_)
+        // the @types/node package will be in the resolved
-      | Module::Node(_)
+        // snapshot below so don't bother including it here
-      | Module::Npm(_) => {
+      }
-        // ignore
+      Module::Npm(_) => {
+        // don't bother adding this specifier to the hash
+        // because what matters is the resolved npm snapshot,
+        // which is hashed below
+      }
+      Module::Json(module) => {
+        has_file_to_type_check = true;
+        hasher.write_str(module.specifier.as_str());
+        hasher.write_str(&module.source);
+      }
+      Module::External(module) => {
+        hasher.write_str(module.specifier.as_str());
       }
     }
   }
 
+  // Check if any of the top level npm pckages have changed. We could go
+  // further and check all the individual npm packages, but that's
+  // probably overkill.
+  let mut package_reqs = package_reqs.into_iter().collect::<Vec<_>>();
+  package_reqs.sort_by(|a, b| a.0.cmp(&b.0)); // determinism
+  for (pkg_req, pkg_nv) in package_reqs {
+    hasher.write_hashable(&pkg_req);
+    hasher.write_hashable(&pkg_nv);
+  }
+
   if !has_file || !check_js && !has_file_to_type_check {
     // no files to type check
     CheckHashResult::NoFiles

cli/tsc/mod.rs

@@ -28,6 +28,7 @@ use deno_core::ModuleSpecifier;
 use deno_core::OpState;
 use deno_core::RuntimeOptions;
 use deno_core::Snapshot;
+use deno_graph::GraphKind;
 use deno_graph::Module;
 use deno_graph::ModuleGraph;
 use deno_graph::ResolutionResolved;
@@ -319,7 +320,7 @@ pub struct Response {
   pub stats: Stats,
 }
 
-#[derive(Debug, Default)]
+#[derive(Debug)]
 struct State {
   hash_data: u64,
   graph: Arc<ModuleGraph>,
@@ -331,6 +332,21 @@ struct State {
   current_dir: PathBuf,
 }
 
+impl Default for State {
+  fn default() -> Self {
+    Self {
+      hash_data: Default::default(),
+      graph: Arc::new(ModuleGraph::new(GraphKind::All)),
+      maybe_tsbuildinfo: Default::default(),
+      maybe_response: Default::default(),
+      maybe_node_resolver: Default::default(),
+      remapped_specifiers: Default::default(),
+      root_map: Default::default(),
+      current_dir: Default::default(),
+    }
+  }
+}
+
 impl State {
   pub fn new(
     graph: Arc<ModuleGraph>,

test_util/src/fs.rs

@@ -10,6 +10,8 @@ use std::sync::Arc;
 
 use anyhow::Context;
 use lsp_types::Url;
+use serde::de::DeserializeOwned;
+use serde::Serialize;
 
 use crate::assertions::assert_wildcard_match;
 
@@ -110,6 +112,10 @@ impl PathRef {
       .with_context(|| format!("Could not read file: {}", self))
   }
 
+  pub fn read_json<TValue: DeserializeOwned>(&self) -> TValue {
+    serde_json::from_str(&self.read_to_string()).unwrap()
+  }
+
   pub fn rename(&self, to: impl AsRef<Path>) {
     fs::rename(self, self.join(to)).unwrap();
   }
@@ -118,6 +124,11 @@ impl PathRef {
     fs::write(self, text.as_ref()).unwrap();
   }
 
+  pub fn write_json<TValue: Serialize>(&self, value: &TValue) {
+    let text = serde_json::to_string_pretty(value).unwrap();
+    self.write(text);
+  }
+
   pub fn symlink_dir(
     &self,
     oldpath: impl AsRef<Path>,