fix: several regressions in TS compiler (#6177)

This commit fixes several regressions in TS compiler: * double compilation of same module during same process run * compilation of JavaScript entry point with non-JS imports * unexpected skip of emit during compilation Additional checks were added to ensure "allowJs" setting is used in TS compiler if JavaScript has non-JS dependencies.
denoland · Jun 10, 2020 · 4b7d3b0 · 4b7d3b0
1 parent f364a4c
commit 4b7d3b0
Show file tree

Hide file tree

Showing 10 changed files with 206 additions and 32 deletions.
diff --git a/cli/global_state.rs b/cli/global_state.rs
@@ -143,11 +143,13 @@ impl GlobalState {
       .expect("Source file not found");
 
     // Check if we need to compile files.
+    let module_graph_files = module_graph.values().collect::<Vec<_>>();
     let should_compile = needs_compilation(
       self.ts_compiler.compile_js,
       out.media_type,
-      module_graph.values().collect::<Vec<_>>(),
+      &module_graph_files,
     );
+    let allow_js = should_allow_js(&module_graph_files);
 
     if should_compile {
       self
@@ -158,6 +160,7 @@ impl GlobalState {
           target_lib,
           permissions,
           module_graph,
+          allow_js,
         )
         .await?;
     }
@@ -241,14 +244,37 @@ impl GlobalState {
   }
 }
 
+/// Determine if TS compiler should be run with `allowJs` setting on. This
+/// is the case when there's a JavaScript file with non-JavaScript import.
+fn should_allow_js(module_graph_files: &[&ModuleGraphFile]) -> bool {
+  module_graph_files.iter().any(|module_file| {
+    if module_file.media_type != (MediaType::JavaScript as i32) {
+      false
+    } else {
+      module_file.imports.iter().any(|import_desc| {
+        let import_file = module_graph_files
+          .iter()
+          .find(|f| {
+            f.specifier == import_desc.resolved_specifier.to_string().as_str()
+          })
+          .expect("Failed to find imported file");
+        let media_type = import_file.media_type;
+        media_type == (MediaType::TypeScript as i32)
+          || media_type == (MediaType::TSX as i32)
+          || media_type == (MediaType::JSX as i32)
+      })
+    }
+  })
+}
+
 // Compilation happens if either:
 // - `checkJs` is set to true in TS config
 // - entry point is a TS file
 // - any dependency in module graph is a TS file
 fn needs_compilation(
   compile_js: bool,
   media_type: MediaType,
-  module_graph_files: Vec<&ModuleGraphFile>,
+  module_graph_files: &[&ModuleGraphFile],
 ) -> bool {
   let mut needs_compilation = match media_type {
     msg::MediaType::TypeScript | msg::MediaType::TSX | msg::MediaType::JSX => {
@@ -276,12 +302,49 @@ fn thread_safe() {
 }
 
 #[test]
-fn test_needs_compilation() {
-  assert!(!needs_compilation(
-    false,
-    MediaType::JavaScript,
-    vec![&ModuleGraphFile {
-      specifier: "some/file.js".to_string(),
+fn test_should_allow_js() {
+  use crate::module_graph::ImportDescriptor;
+
+  assert!(should_allow_js(&[
+    &ModuleGraphFile {
+      specifier: "file:///some/file.ts".to_string(),
+      url: "file:///some/file.ts".to_string(),
+      redirect: None,
+      filename: "some/file.ts".to_string(),
+      imports: vec![],
+      referenced_files: vec![],
+      lib_directives: vec![],
+      types_directives: vec![],
+      type_headers: vec![],
+      media_type: MediaType::TypeScript as i32,
+      source_code: "function foo() {}".to_string(),
+    },
+    &ModuleGraphFile {
+      specifier: "file:///some/file1.js".to_string(),
+      url: "file:///some/file1.js".to_string(),
+      redirect: None,
+      filename: "some/file1.js".to_string(),
+      imports: vec![ImportDescriptor {
+        specifier: "./file.ts".to_string(),
+        resolved_specifier: ModuleSpecifier::resolve_url(
+          "file:///some/file.ts",
+        )
+        .unwrap(),
+        type_directive: None,
+        resolved_type_directive: None,
+      }],
+      referenced_files: vec![],
+      lib_directives: vec![],
+      types_directives: vec![],
+      type_headers: vec![],
+      media_type: MediaType::JavaScript as i32,
+      source_code: "function foo() {}".to_string(),
+    },
+  ],));
+
+  assert!(!should_allow_js(&[
+    &ModuleGraphFile {
+      specifier: "file:///some/file.js".to_string(),
       url: "file:///some/file.js".to_string(),
       redirect: None,
       filename: "some/file.js".to_string(),
@@ -292,28 +355,86 @@ fn test_needs_compilation() {
       type_headers: vec![],
       media_type: MediaType::JavaScript as i32,
       source_code: "function foo() {}".to_string(),
-    }]
-  ));
-  assert!(!needs_compilation(false, MediaType::JavaScript, vec![]));
-  assert!(needs_compilation(true, MediaType::JavaScript, vec![]));
-  assert!(needs_compilation(false, MediaType::TypeScript, vec![]));
-  assert!(needs_compilation(false, MediaType::JSX, vec![]));
-  assert!(needs_compilation(false, MediaType::TSX, vec![]));
-  assert!(needs_compilation(
+    },
+    &ModuleGraphFile {
+      specifier: "file:///some/file1.js".to_string(),
+      url: "file:///some/file1.js".to_string(),
+      redirect: None,
+      filename: "some/file1.js".to_string(),
+      imports: vec![ImportDescriptor {
+        specifier: "./file.js".to_string(),
+        resolved_specifier: ModuleSpecifier::resolve_url(
+          "file:///some/file.js",
+        )
+        .unwrap(),
+        type_directive: None,
+        resolved_type_directive: None,
+      }],
+      referenced_files: vec![],
+      lib_directives: vec![],
+      types_directives: vec![],
+      type_headers: vec![],
+      media_type: MediaType::JavaScript as i32,
+      source_code: "function foo() {}".to_string(),
+    },
+  ],));
+}
+
+#[test]
+fn test_needs_compilation() {
+  assert!(!needs_compilation(
     false,
     MediaType::JavaScript,
-    vec![&ModuleGraphFile {
-      specifier: "some/file.ts".to_string(),
-      url: "file:///some/file.ts".to_string(),
+    &[&ModuleGraphFile {
+      specifier: "some/file.js".to_string(),
+      url: "file:///some/file.js".to_string(),
       redirect: None,
-      filename: "some/file.ts".to_string(),
+      filename: "some/file.js".to_string(),
       imports: vec![],
       referenced_files: vec![],
       lib_directives: vec![],
       types_directives: vec![],
       type_headers: vec![],
-      media_type: MediaType::TypeScript as i32,
+      media_type: MediaType::JavaScript as i32,
       source_code: "function foo() {}".to_string(),
-    }]
+    }],
+  ));
+
+  assert!(!needs_compilation(false, MediaType::JavaScript, &[]));
+  assert!(needs_compilation(true, MediaType::JavaScript, &[]));
+  assert!(needs_compilation(false, MediaType::TypeScript, &[]));
+  assert!(needs_compilation(false, MediaType::JSX, &[]));
+  assert!(needs_compilation(false, MediaType::TSX, &[]));
+  assert!(needs_compilation(
+    false,
+    MediaType::JavaScript,
+    &[
+      &ModuleGraphFile {
+        specifier: "file:///some/file.ts".to_string(),
+        url: "file:///some/file.ts".to_string(),
+        redirect: None,
+        filename: "some/file.ts".to_string(),
+        imports: vec![],
+        referenced_files: vec![],
+        lib_directives: vec![],
+        types_directives: vec![],
+        type_headers: vec![],
+        media_type: MediaType::TypeScript as i32,
+        source_code: "function foo() {}".to_string(),
+      },
+      &ModuleGraphFile {
+        specifier: "file:///some/file1.js".to_string(),
+        url: "file:///some/file1.js".to_string(),
+        redirect: None,
+        filename: "some/file1.js".to_string(),
+        imports: vec![],
+        referenced_files: vec![],
+        lib_directives: vec![],
+        types_directives: vec![],
+        type_headers: vec![],
+        media_type: MediaType::JavaScript as i32,
+        source_code: "function foo() {}".to_string(),
+      },
+    ],
   ));
 }
diff --git a/cli/js/compiler.ts b/cli/js/compiler.ts
@@ -1049,6 +1049,7 @@ interface SourceFileMapEntry {
 
 interface CompilerRequestCompile {
   type: CompilerRequestType.Compile;
+  allowJs: boolean;
   target: CompilerHostTarget;
   rootNames: string[];
   configPath?: string;
@@ -1099,6 +1100,7 @@ interface RuntimeBundleResult {
 
 function compile(request: CompilerRequestCompile): CompileResult {
   const {
+    allowJs,
     bundle,
     config,
     configPath,
@@ -1138,6 +1140,10 @@ function compile(request: CompilerRequestCompile): CompileResult {
   }));
   let diagnostics: readonly ts.Diagnostic[] = [];
 
+  if (!bundle) {
+    host.mergeOptions({ allowJs });
+  }
+
   // if there is a configuration supplied, we need to parse that
   if (config && config.length && configPath) {
     const configResult = host.configure(cwd, configPath, config);
@@ -1167,7 +1173,22 @@ function compile(request: CompilerRequestCompile): CompileResult {
         setRootExports(program, rootNames[0]);
       }
       const emitResult = program.emit();
-      assert(emitResult.emitSkipped === false, "Unexpected skip of the emit.");
+      // If `checkJs` is off we still might be compiling entry point JavaScript file
+      // (if it has `.ts` imports), but it won't be emitted. In that case we skip
+      // assertion.
+      if (!bundle) {
+        if (options.checkJs) {
+          assert(
+            emitResult.emitSkipped === false,
+            "Unexpected skip of the emit."
+          );
+        }
+      } else {
+        assert(
+          emitResult.emitSkipped === false,
+          "Unexpected skip of the emit."
+        );
+      }
       // emitResult.diagnostics is `readonly` in TS3.5+ and can't be assigned
       // without casting.
       diagnostics = emitResult.diagnostics;

diff --git a/cli/module_graph.rs b/cli/module_graph.rs
@@ -74,22 +74,22 @@ pub struct ModuleGraph(HashMap<String, ModuleGraphFile>);
 #[derive(Debug, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct ImportDescriptor {
-  specifier: String,
+  pub specifier: String,
   #[serde(serialize_with = "serialize_module_specifier")]
-  resolved_specifier: ModuleSpecifier,
+  pub resolved_specifier: ModuleSpecifier,
   // These two fields are for support of @deno-types directive
   // directly prepending import statement
-  type_directive: Option<String>,
+  pub type_directive: Option<String>,
   #[serde(serialize_with = "serialize_option_module_specifier")]
-  resolved_type_directive: Option<ModuleSpecifier>,
+  pub resolved_type_directive: Option<ModuleSpecifier>,
 }
 
 #[derive(Debug, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct ReferenceDescriptor {
-  specifier: String,
+  pub specifier: String,
   #[serde(serialize_with = "serialize_module_specifier")]
-  resolved_specifier: ModuleSpecifier,
+  pub resolved_specifier: ModuleSpecifier,
 }
 
 #[derive(Debug, Serialize)]

diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs
@@ -1938,6 +1938,12 @@ itest!(cjs_imports {
 itest!(ts_import_from_js {
   args: "run --quiet --reload ts_import_from_js.js",
   output: "ts_import_from_js.js.out",
+  http_server: true,
+});
+
+itest!(single_compile_with_reload {
+  args: "run --reload --allow-read single_compile_with_reload.ts",
+  output: "single_compile_with_reload.ts.out",
 });
 
 itest!(proto_exploit {

diff --git a/cli/tests/single_compile_with_reload.ts b/cli/tests/single_compile_with_reload.ts
@@ -0,0 +1,4 @@
+await import("./005_more_imports.ts");
+console.log("1");
+await import("./005_more_imports.ts");
+console.log("2");
diff --git a/cli/tests/single_compile_with_reload.ts.out b/cli/tests/single_compile_with_reload.ts.out
@@ -0,0 +1,5 @@
+Compile [WILDCARD]single_compile_with_reload.ts
+Compile [WILDCARD]005_more_imports.ts
+Hello
+1
+2
diff --git a/cli/tests/ts_import_from_js.deps.js b/cli/tests/ts_import_from_js.deps.js
@@ -0,0 +1,2 @@
+import "./005_more_imports.ts";
+export { printHello } from "http://localhost:4545/cli/tests/subdir/mod2.ts";
diff --git a/cli/tests/ts_import_from_js.js b/cli/tests/ts_import_from_js.js
@@ -1 +1,3 @@
-import "./005_more_imports.ts";
+import { printHello } from "./ts_import_from_js.deps.js";
+printHello();
+console.log("success");
diff --git a/cli/tests/ts_import_from_js.js.out b/cli/tests/ts_import_from_js.js.out
@@ -1 +1,3 @@
 Hello
+Hello
+success
diff --git a/cli/tsc.rs b/cli/tsc.rs
@@ -405,14 +405,18 @@ impl TsCompiler {
     })))
   }
 
-  // TODO(bartlomieju): this method is no longer needed
   /// Mark given module URL as compiled to avoid multiple compilations of same
   /// module in single run.
   fn mark_compiled(&self, url: &Url) {
     let mut c = self.compiled.lock().unwrap();
     c.insert(url.clone());
   }
 
+  fn has_compiled(&self, url: &Url) -> bool {
+    let c = self.compiled.lock().unwrap();
+    c.contains(url)
+  }
+
   /// Check if there is compiled source in cache that is valid
   /// and can be used again.
   // TODO(bartlomieju): there should be check that cached file actually exists
@@ -459,10 +463,14 @@ impl TsCompiler {
     target: TargetLib,
     permissions: Permissions,
     module_graph: HashMap<String, ModuleGraphFile>,
+    allow_js: bool,
   ) -> Result<(), ErrBox> {
     let mut has_cached_version = false;
 
-    if self.use_disk_cache {
+    // Only use disk cache if `--reload` flag was not used or
+    // this file has already been compiled during current process
+    // lifetime.
+    if self.use_disk_cache || self.has_compiled(&source_file.url) {
       if let Some(metadata) = self.get_graph_metadata(&source_file.url) {
         has_cached_version = true;
 
@@ -503,6 +511,7 @@ impl TsCompiler {
     let j = match (compiler_config.path, compiler_config.content) {
       (Some(config_path), Some(config_data)) => json!({
         "type": msg::CompilerRequestType::Compile as i32,
+        "allowJs": allow_js,
         "target": target,
         "rootNames": root_names,
         "bundle": bundle,
@@ -514,6 +523,7 @@ impl TsCompiler {
       }),
       _ => json!({
         "type": msg::CompilerRequestType::Compile as i32,
+        "allowJs": allow_js,
         "target": target,
         "rootNames": root_names,
         "bundle": bundle,
@@ -1151,6 +1161,7 @@ mod tests {
         TargetLib::Main,
         Permissions::allow_all(),
         module_graph,
+        false,
       )
       .await;
     assert!(result.is_ok());
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		import "./005_more_imports.ts";
		export { printHello } from "http://localhost:4545/cli/tests/subdir/mod2.ts";
-Original file line number
+Diff line change
@@ -1 +1,3 @@
     Hello
+    Hello
+    success