From 23b9be7b37c40f8c29f9ce50439ad0e25b85282c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 1 Apr 2023 10:04:56 -0400 Subject: [PATCH] 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 | 73 +++++++++++++++++----------- cli/tsc/99_main_compiler.js | 17 +++++++ test_util/src/builders.rs | 51 ++++++++----------- test_util/src/lib.rs | 18 +++---- 4 files changed, 90 insertions(+), 69 deletions(-) diff --git a/cli/tests/integration/check_tests.rs b/cli/tests/integration/check_tests.rs index 7e6fbd8363..84ddd53be8 100644 --- a/cli/tests/integration/check_tests.rs +++ b/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 deno_dir = util::new_deno_dir(); - let e = util::deno_exe_path(); + let test_context = TestContext::default(); + let check_command = test_context.new_command().args_vec([ + "run", + "--check", + "run/017_import_redirect.ts", + ]); - let redirect_ts = util::testdata_path().join("run/017_import_redirect.ts"); - assert!(redirect_ts.is_file()); - let mut cmd = Command::new(e.clone()); - 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()); + // run once + let output = check_command.run(); + output.assert_matches_text("[WILDCARD]Check file://[WILDCARD]"); - let mut cmd = Command::new(e); - cmd.env("DENO_DIR", deno_dir.path()); - let output = cmd - .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()); + // run again + let output = check_command.run(); + output.assert_matches_text("Hello\n"); } 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); +} diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index b8189278c0..2f565770c4 100644 --- a/cli/tsc/99_main_compiler.js +++ b/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 diff --git a/test_util/src/builders.rs b/test_util/src/builders.rs index 0a0f2244fd..254490073a 100644 --- a/test_util/src/builders.rs +++ b/test_util/src/builders.rs @@ -46,17 +46,15 @@ impl TestContextBuilder { } pub fn for_npm() -> Self { - let mut builder = Self::new(); - builder.use_http_server().add_npm_env_vars(); - builder + Self::new().use_http_server().add_npm_env_vars() } - 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) -> &mut Self { + pub fn use_copy_temp_dir(mut self, dir: impl AsRef) -> Self { self.copy_temp_dir = Some(dir.as_ref().to_string()); self } - pub fn cwd(&mut self, cwd: impl AsRef) -> &mut Self { + pub fn cwd(mut self, cwd: impl AsRef) -> Self { self.cwd = Some(cwd.as_ref().to_string()); self } - pub fn env( - &mut self, - key: impl AsRef, - value: impl AsRef, - ) -> &mut Self { + pub fn env(mut self, key: impl AsRef, value: impl AsRef) -> 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) -> &mut Self { + pub fn command_name(mut self, name: impl AsRef) -> Self { self.command_name = name.as_ref().to_string(); self } - pub fn args(&mut self, text: impl AsRef) -> &mut Self { + pub fn args(mut self, text: impl AsRef) -> Self { self.args = text.as_ref().to_string(); self } pub fn args_vec, I: IntoIterator>( - &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) -> &mut Self { + pub fn stdin(mut self, text: impl AsRef) -> 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( - &mut self, - key: impl AsRef, - value: impl AsRef, - ) -> &mut Self { + pub fn env(mut self, key: impl AsRef, value: impl AsRef) -> 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) -> &mut Self { + pub fn cwd(mut self, cwd: impl AsRef) -> Self { self.cwd = Some(cwd.as_ref().to_string()); self } diff --git a/test_util/src/lib.rs b/test_util/src/lib.rs index 5c6bc97f7a..0b6ce16156 100644 --- a/test_util/src/lib.rs +++ b/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()