From 43d066cb7069c8c55aa555d7959a8612a52facc7 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 13 Mar 2024 16:38:01 -0400 Subject: [PATCH] fix: stop type checking during runtime (#22854) In addition to the reasons for this outlined by @nayeemrmn in #14877 (which I think are reasons alone to not do this), this simplifies things a lot because then we don't need to implement the following: 1. Need to handle a JSR module dynamically importing a module within it. 2. Need to handle importing an export of a JSR dep then another export dynamically loaded later. Additionally, people should be running `deno check dynamic_import.ts` instead of relying on this behaviour. Landing this as a fix because it's blocking people in some scenarios and the current behaviour is broken (I didn't even have to change any tests to remove this, which is bad). Closes #22852 Closes #14877 Closes #22580 --- cli/graph_util.rs | 65 +++++--------------------------------------- cli/module_loader.rs | 22 ++++++--------- 2 files changed, 15 insertions(+), 72 deletions(-) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 316eb7a21f..b729a1b618 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -3,7 +3,6 @@ use crate::args::jsr_url; use crate::args::CliOptions; use crate::args::Lockfile; -use crate::args::TsTypeLib; use crate::args::DENO_DISABLE_PEDANTIC_NODE_WARNINGS; use crate::cache; use crate::cache::GlobalHttpCache; @@ -46,7 +45,6 @@ use deno_runtime::permissions::PermissionsContainer; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; use import_map::ImportMapError; -use std::collections::HashMap; use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; @@ -754,29 +752,20 @@ fn get_resolution_error_bare_specifier( } } -#[derive(Debug)] -struct GraphData { - graph: Arc, - checked_libs: HashMap>, -} - /// Holds the `ModuleGraph` and what parts of it are type checked. pub struct ModuleGraphContainer { // Allow only one request to update the graph data at a time, // but allow other requests to read from it at any time even // while another request is updating the data. update_queue: Arc, - graph_data: Arc>, + inner: Arc>>, } impl ModuleGraphContainer { pub fn new(graph_kind: GraphKind) -> Self { Self { update_queue: Default::default(), - graph_data: Arc::new(RwLock::new(GraphData { - graph: Arc::new(ModuleGraph::new(graph_kind)), - checked_libs: Default::default(), - })), + inner: Arc::new(RwLock::new(Arc::new(ModuleGraph::new(graph_kind)))), } } @@ -787,53 +776,13 @@ impl ModuleGraphContainer { let permit = self.update_queue.acquire().await; ModuleGraphUpdatePermit { permit, - graph_data: self.graph_data.clone(), - graph: (*self.graph_data.read().graph).clone(), + inner: self.inner.clone(), + graph: (**self.inner.read()).clone(), } } pub fn graph(&self) -> Arc { - self.graph_data.read().graph.clone() - } - - /// Mark `roots` and all of their dependencies as type checked under `lib`. - /// Assumes that all of those modules are known. - pub fn set_type_checked(&self, roots: &[ModuleSpecifier], lib: TsTypeLib) { - // It's ok to analyze and update this while the module graph itself is - // being updated in a permit because the module graph update is always - // additive and this will be a subset of the original graph - let graph = self.graph(); - let entries = graph.walk( - roots, - deno_graph::WalkOptions { - check_js: true, - follow_dynamic: true, - follow_type_only: true, - }, - ); - - // now update - let mut data = self.graph_data.write(); - let checked_lib_set = data.checked_libs.entry(lib).or_default(); - for (specifier, _) in entries { - checked_lib_set.insert(specifier.clone()); - } - } - - /// Check if `roots` are all marked as type checked under `lib`. - pub fn is_type_checked( - &self, - roots: &[ModuleSpecifier], - lib: TsTypeLib, - ) -> bool { - let data = self.graph_data.read(); - match data.checked_libs.get(&lib) { - Some(checked_lib_set) => roots.iter().all(|r| { - let found = data.graph.resolve(r); - checked_lib_set.contains(&found) - }), - None => false, - } + self.inner.read().clone() } } @@ -873,7 +822,7 @@ pub fn has_graph_root_local_dependent_changed( /// new graph in the ModuleGraphContainer. pub struct ModuleGraphUpdatePermit<'a> { permit: TaskQueuePermit<'a>, - graph_data: Arc>, + inner: Arc>>, graph: ModuleGraph, } @@ -887,7 +836,7 @@ impl<'a> ModuleGraphUpdatePermit<'a> { /// and returns an Arc to the new module graph. pub fn commit(self) -> Arc { let graph = Arc::new(self.graph); - self.graph_data.write().graph = graph.clone(); + *self.inner.write() = graph.clone(); drop(self.permit); // explicit drop for clarity graph } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 0ff7ef4edd..5149c4afbc 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -55,7 +55,6 @@ use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use deno_terminal::colors; use std::borrow::Cow; -use std::collections::HashSet; use std::pin::Pin; use std::rc::Rc; use std::str; @@ -110,11 +109,7 @@ impl ModuleLoadPreparer { let mut graph_update_permit = self.graph_container.acquire_update_permit().await; let graph = graph_update_permit.graph_mut(); - - // Determine any modules that have already been emitted this session and - // should be skipped. - let reload_exclusions: HashSet = - graph.specifiers().map(|(s, _)| s.clone()).collect(); + let has_type_checked = !graph.roots.is_empty(); self .module_graph_builder @@ -146,25 +141,24 @@ impl ModuleLoadPreparer { drop(_pb_clear_guard); // type check if necessary - if self.options.type_check_mode().is_true() - && !self.graph_container.is_type_checked(&roots, lib) - { - let graph = graph.segment(&roots); + if self.options.type_check_mode().is_true() && !has_type_checked { self .type_checker .check( - graph, + // todo(perf): since this is only done the first time the graph is + // created, we could avoid the clone of the graph here by providing + // the actual graph on the first run and then getting the Arc + // back from the return value. + (*graph).clone(), check::CheckOptions { build_fast_check_graph: true, lib, log_ignored_options: false, - reload: self.options.reload_flag() - && !roots.iter().all(|r| reload_exclusions.contains(r)), + reload: self.options.reload_flag(), type_check_mode: self.options.type_check_mode(), }, ) .await?; - self.graph_container.set_type_checked(&roots, lib); } log::debug!("Prepared module load.");