fix(check): ensure diagnostics caused by changes in other files get invalidated between runs (#18541)

Regression caused by the performance improvement in #18329. Figuring
this out was hard. It's luckily still fast after this change.

Closes #18516

cli/tests/integration/check_tests.rs

@@ -1,10 +1,10 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
-use std::process::Command;
 use test_util as util;
 use util::env_vars_for_npm_tests;
 use util::env_vars_for_npm_tests_no_sync_download;
 use util::TestContext;
+use util::TestContextBuilder;
 
 itest!(_095_check_with_bare_import {
   args: "check cache/095_cache_with_bare_import.ts",
@@ -214,36 +214,20 @@ fn typecheck_core() {
 
 #[test]
 fn ts_no_recheck_on_redirect() {
-  // TODO: port to test builder
+  let test_context = TestContext::default();
-  let deno_dir = util::new_deno_dir();
+  let check_command = test_context.new_command().args_vec([
-  let e = util::deno_exe_path();
+    "run",
+    "--check",
+    "run/017_import_redirect.ts",
+  ]);
 
-  let redirect_ts = util::testdata_path().join("run/017_import_redirect.ts");
+  // run once
-  assert!(redirect_ts.is_file());
+  let output = check_command.run();
-  let mut cmd = Command::new(e.clone());
+  output.assert_matches_text("[WILDCARD]Check file://[WILDCARD]");
-  cmd.env("DENO_DIR", deno_dir.path());
-  let mut initial = cmd
-    .current_dir(util::testdata_path())
-    .arg("run")
-    .arg("--check")
-    .arg(redirect_ts.clone())
-    .spawn()
-    .expect("failed to span script");
-  let status_initial =
-    initial.wait().expect("failed to wait for child process");
-  assert!(status_initial.success());
 
-  let mut cmd = Command::new(e);
+  // run again
-  cmd.env("DENO_DIR", deno_dir.path());
+  let output = check_command.run();
-  let output = cmd
+  output.assert_matches_text("Hello\n");
-    .current_dir(util::testdata_path())
-    .arg("run")
-    .arg("--check")
-    .arg(redirect_ts)
-    .output()
-    .expect("failed to spawn script");
-
-  assert!(std::str::from_utf8(&output.stderr).unwrap().is_empty());
 }
 
 itest!(check_dts {
@@ -288,3 +272,34 @@ itest!(package_json_with_deno_json {
   http_server: true,
   exit_code: 1,
 });
+
+#[test]
+fn check_error_in_dep_then_fix() {
+  let test_context = TestContextBuilder::new().use_temp_cwd().build();
+  let temp_dir = test_context.temp_dir();
+  let correct_code =
+    "export function greet(name: string) {\n  return `Hello ${name}`;\n}\n";
+  let incorrect_code =
+    "export function greet(name: number) {\n  return `Hello ${name}`;\n}\n";
+
+  temp_dir.write(
+    "main.ts",
+    "import { greet } from './greet.ts';\n\nconsole.log(greet('world'));\n",
+  );
+  temp_dir.write("greet.ts", incorrect_code);
+
+  let check_command = test_context.new_command().args_vec(["check", "main.ts"]);
+
+  let output = check_command.run();
+  output.assert_matches_text("Check [WILDCARD]main.ts\nerror: TS234[WILDCARD]");
+  output.assert_exit_code(1);
+
+  temp_dir.write("greet.ts", correct_code);
+  let output = check_command.run();
+  output.assert_matches_text("Check [WILDCARD]main.ts\n");
+
+  temp_dir.write("greet.ts", incorrect_code);
+  let output = check_command.run();
+  output.assert_matches_text("Check [WILDCARD]main.ts\nerror: TS234[WILDCARD]");
+  output.assert_exit_code(1);
+}

cli/tsc/99_main_compiler.js

@@ -824,6 +824,23 @@ delete Object.prototype.__proto__;
           return sourceFile;
         })
       : undefined;
+
+    if (checkFiles != null) {
+      // When calling program.getSemanticDiagnostics(...) with a source file, we
+      // need to call this code first in order to get it to invalidate cached
+      // diagnostics correctly. This is what program.getSemanticDiagnostics()
+      // does internally when calling without any arguments.
+      const checkFileNames = new Set(checkFiles.map((f) => f.fileName));
+      while (
+        program.getSemanticDiagnosticsOfNextAffectedFile(
+          undefined,
+          /* ignoreSourceFile */ (s) => !checkFileNames.has(s.fileName),
+        )
+      ) {
+        // keep going until there are no more affected files
+      }
+    }
+
     const diagnostics = [
       ...program.getConfigFileParsingDiagnostics(),
       ...(checkFiles == null

test_util/src/builders.rs

@@ -46,17 +46,15 @@ impl TestContextBuilder {
   }
 
   pub fn for_npm() -> Self {
-    let mut builder = Self::new();
+    Self::new().use_http_server().add_npm_env_vars()
-    builder.use_http_server().add_npm_env_vars();
-    builder
   }
 
-  pub fn use_http_server(&mut self) -> &mut Self {
+  pub fn use_http_server(mut self) -> Self {
     self.use_http_server = true;
     self
   }
 
-  pub fn use_temp_cwd(&mut self) -> &mut Self {
+  pub fn use_temp_cwd(mut self) -> Self {
     self.use_temp_cwd = true;
     self
   }
@@ -65,7 +63,7 @@ impl TestContextBuilder {
   /// In some cases, that might cause an issue though, so calling
   /// this will use a separate directory for the deno dir and the
   /// temp directory.
-  pub fn use_separate_deno_dir(&mut self) -> &mut Self {
+  pub fn use_separate_deno_dir(mut self) -> Self {
     self.use_separate_deno_dir = true;
     self
   }
@@ -73,41 +71,36 @@ impl TestContextBuilder {
   /// Copies the files at the specified directory in the "testdata" directory
   /// to the temp folder and runs the test from there. This is useful when
   /// the test creates files in the testdata directory (ex. a node_modules folder)
-  pub fn use_copy_temp_dir(&mut self, dir: impl AsRef<str>) -> &mut Self {
+  pub fn use_copy_temp_dir(mut self, dir: impl AsRef<str>) -> Self {
     self.copy_temp_dir = Some(dir.as_ref().to_string());
     self
   }
 
-  pub fn cwd(&mut self, cwd: impl AsRef<str>) -> &mut Self {
+  pub fn cwd(mut self, cwd: impl AsRef<str>) -> Self {
     self.cwd = Some(cwd.as_ref().to_string());
     self
   }
 
-  pub fn env(
+  pub fn env(mut self, key: impl AsRef<str>, value: impl AsRef<str>) -> Self {
-    &mut self,
-    key: impl AsRef<str>,
-    value: impl AsRef<str>,
-  ) -> &mut Self {
     self
       .envs
       .insert(key.as_ref().to_string(), value.as_ref().to_string());
     self
   }
 
-  pub fn add_npm_env_vars(&mut self) -> &mut Self {
+  pub fn add_npm_env_vars(mut self) -> Self {
     for (key, value) in env_vars_for_npm_tests_no_sync_download() {
-      self.env(key, value);
+      self = self.env(key, value);
     }
     self
   }
 
-  pub fn use_sync_npm_download(&mut self) -> &mut Self {
+  pub fn use_sync_npm_download(self) -> Self {
     self.env(
       // make downloads determinstic
       "DENO_UNSTABLE_NPM_SYNC_DOWNLOAD",
       "1",
-    );
+    )
-    self
   }
 
   pub fn build(&self) -> TestContext {
@@ -218,31 +211,31 @@ pub struct TestCommandBuilder {
 }
 
 impl TestCommandBuilder {
-  pub fn command_name(&mut self, name: impl AsRef<str>) -> &mut Self {
+  pub fn command_name(mut self, name: impl AsRef<str>) -> Self {
     self.command_name = name.as_ref().to_string();
     self
   }
 
-  pub fn args(&mut self, text: impl AsRef<str>) -> &mut Self {
+  pub fn args(mut self, text: impl AsRef<str>) -> Self {
     self.args = text.as_ref().to_string();
     self
   }
 
   pub fn args_vec<T: AsRef<str>, I: IntoIterator<Item = T>>(
-    &mut self,
+    mut self,
     args: I,
-  ) -> &mut Self {
+  ) -> Self {
     self.args_vec = args.into_iter().map(|a| a.as_ref().to_string()).collect();
     self
   }
 
-  pub fn stdin(&mut self, text: impl AsRef<str>) -> &mut Self {
+  pub fn stdin(mut self, text: impl AsRef<str>) -> Self {
     self.stdin = Some(text.as_ref().to_string());
     self
   }
 
   /// Splits the output into stdout and stderr rather than having them combined.
-  pub fn split_output(&mut self) -> &mut Self {
+  pub fn split_output(mut self) -> Self {
     // Note: it was previously attempted to capture stdout & stderr separately
     // then forward the output to a combined pipe, but this was found to be
     // too racy compared to providing the same combined pipe to both.
@@ -250,23 +243,19 @@ impl TestCommandBuilder {
     self
   }
 
-  pub fn env(
+  pub fn env(mut self, key: impl AsRef<str>, value: impl AsRef<str>) -> Self {
-    &mut self,
-    key: impl AsRef<str>,
-    value: impl AsRef<str>,
-  ) -> &mut Self {
     self
       .envs
       .insert(key.as_ref().to_string(), value.as_ref().to_string());
     self
   }
 
-  pub fn env_clear(&mut self) -> &mut Self {
+  pub fn env_clear(mut self) -> Self {
     self.env_clear = true;
     self
   }
 
-  pub fn cwd(&mut self, cwd: impl AsRef<str>) -> &mut Self {
+  pub fn cwd(mut self, cwd: impl AsRef<str>) -> Self {
     self.cwd = Some(cwd.as_ref().to_string());
     self
   }

test_util/src/lib.rs

@@ -2076,13 +2076,13 @@ impl<'a> CheckOutputIntegrationTest<'a> {
   pub fn output(&self) -> TestCommandOutput {
     let mut context_builder = TestContextBuilder::default();
     if self.temp_cwd {
-      context_builder.use_temp_cwd();
+      context_builder = context_builder.use_temp_cwd();
     }
     if let Some(dir) = &self.copy_temp_dir {
-      context_builder.use_copy_temp_dir(dir);
+      context_builder = context_builder.use_copy_temp_dir(dir);
     }
     if self.http_server {
-      context_builder.use_http_server();
+      context_builder = context_builder.use_http_server();
     }
 
     let context = context_builder.build();
@@ -2090,22 +2090,22 @@ impl<'a> CheckOutputIntegrationTest<'a> {
     let mut command_builder = context.new_command();
 
     if !self.args.is_empty() {
-      command_builder.args(self.args);
+      command_builder = command_builder.args(self.args);
     }
     if !self.args_vec.is_empty() {
-      command_builder.args_vec(self.args_vec.clone());
+      command_builder = command_builder.args_vec(self.args_vec.clone());
     }
     if let Some(input) = &self.input {
-      command_builder.stdin(input);
+      command_builder = command_builder.stdin(input);
     }
     for (key, value) in &self.envs {
-      command_builder.env(key, value);
+      command_builder = command_builder.env(key, value);
     }
     if self.env_clear {
-      command_builder.env_clear();
+      command_builder = command_builder.env_clear();
     }
     if let Some(cwd) = &self.cwd {
-      command_builder.cwd(cwd);
+      command_builder = command_builder.cwd(cwd);
     }
 
     command_builder.run()