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
This commit is contained in:
David Sherret 2024-03-13 16:38:01 -04:00 committed by GitHub
parent c9a9d040a3
commit 43d066cb70
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 15 additions and 72 deletions

View file

@ -3,7 +3,6 @@
use crate::args::jsr_url; use crate::args::jsr_url;
use crate::args::CliOptions; use crate::args::CliOptions;
use crate::args::Lockfile; use crate::args::Lockfile;
use crate::args::TsTypeLib;
use crate::args::DENO_DISABLE_PEDANTIC_NODE_WARNINGS; use crate::args::DENO_DISABLE_PEDANTIC_NODE_WARNINGS;
use crate::cache; use crate::cache;
use crate::cache::GlobalHttpCache; use crate::cache::GlobalHttpCache;
@ -46,7 +45,6 @@ use deno_runtime::permissions::PermissionsContainer;
use deno_semver::package::PackageNv; use deno_semver::package::PackageNv;
use deno_semver::package::PackageReq; use deno_semver::package::PackageReq;
use import_map::ImportMapError; use import_map::ImportMapError;
use std::collections::HashMap;
use std::collections::HashSet; use std::collections::HashSet;
use std::path::PathBuf; use std::path::PathBuf;
use std::sync::Arc; use std::sync::Arc;
@ -754,29 +752,20 @@ fn get_resolution_error_bare_specifier(
} }
} }
#[derive(Debug)]
struct GraphData {
graph: Arc<ModuleGraph>,
checked_libs: HashMap<TsTypeLib, HashSet<ModuleSpecifier>>,
}
/// Holds the `ModuleGraph` and what parts of it are type checked. /// Holds the `ModuleGraph` and what parts of it are type checked.
pub struct ModuleGraphContainer { pub struct ModuleGraphContainer {
// Allow only one request to update the graph data at a time, // Allow only one request to update the graph data at a time,
// but allow other requests to read from it at any time even // but allow other requests to read from it at any time even
// while another request is updating the data. // while another request is updating the data.
update_queue: Arc<TaskQueue>, update_queue: Arc<TaskQueue>,
graph_data: Arc<RwLock<GraphData>>, inner: Arc<RwLock<Arc<ModuleGraph>>>,
} }
impl ModuleGraphContainer { impl ModuleGraphContainer {
pub fn new(graph_kind: GraphKind) -> Self { pub fn new(graph_kind: GraphKind) -> Self {
Self { Self {
update_queue: Default::default(), update_queue: Default::default(),
graph_data: Arc::new(RwLock::new(GraphData { inner: Arc::new(RwLock::new(Arc::new(ModuleGraph::new(graph_kind)))),
graph: Arc::new(ModuleGraph::new(graph_kind)),
checked_libs: Default::default(),
})),
} }
} }
@ -787,53 +776,13 @@ impl ModuleGraphContainer {
let permit = self.update_queue.acquire().await; let permit = self.update_queue.acquire().await;
ModuleGraphUpdatePermit { ModuleGraphUpdatePermit {
permit, permit,
graph_data: self.graph_data.clone(), inner: self.inner.clone(),
graph: (*self.graph_data.read().graph).clone(), graph: (**self.inner.read()).clone(),
} }
} }
pub fn graph(&self) -> Arc<ModuleGraph> { pub fn graph(&self) -> Arc<ModuleGraph> {
self.graph_data.read().graph.clone() self.inner.read().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,
}
} }
} }
@ -873,7 +822,7 @@ pub fn has_graph_root_local_dependent_changed(
/// new graph in the ModuleGraphContainer. /// new graph in the ModuleGraphContainer.
pub struct ModuleGraphUpdatePermit<'a> { pub struct ModuleGraphUpdatePermit<'a> {
permit: TaskQueuePermit<'a>, permit: TaskQueuePermit<'a>,
graph_data: Arc<RwLock<GraphData>>, inner: Arc<RwLock<Arc<ModuleGraph>>>,
graph: ModuleGraph, graph: ModuleGraph,
} }
@ -887,7 +836,7 @@ impl<'a> ModuleGraphUpdatePermit<'a> {
/// and returns an Arc to the new module graph. /// and returns an Arc to the new module graph.
pub fn commit(self) -> Arc<ModuleGraph> { pub fn commit(self) -> Arc<ModuleGraph> {
let graph = Arc::new(self.graph); 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 drop(self.permit); // explicit drop for clarity
graph graph
} }

View file

@ -55,7 +55,6 @@ use deno_runtime::permissions::PermissionsContainer;
use deno_semver::npm::NpmPackageReqReference; use deno_semver::npm::NpmPackageReqReference;
use deno_terminal::colors; use deno_terminal::colors;
use std::borrow::Cow; use std::borrow::Cow;
use std::collections::HashSet;
use std::pin::Pin; use std::pin::Pin;
use std::rc::Rc; use std::rc::Rc;
use std::str; use std::str;
@ -110,11 +109,7 @@ impl ModuleLoadPreparer {
let mut graph_update_permit = let mut graph_update_permit =
self.graph_container.acquire_update_permit().await; self.graph_container.acquire_update_permit().await;
let graph = graph_update_permit.graph_mut(); let graph = graph_update_permit.graph_mut();
let has_type_checked = !graph.roots.is_empty();
// Determine any modules that have already been emitted this session and
// should be skipped.
let reload_exclusions: HashSet<ModuleSpecifier> =
graph.specifiers().map(|(s, _)| s.clone()).collect();
self self
.module_graph_builder .module_graph_builder
@ -146,25 +141,24 @@ impl ModuleLoadPreparer {
drop(_pb_clear_guard); drop(_pb_clear_guard);
// type check if necessary // type check if necessary
if self.options.type_check_mode().is_true() if self.options.type_check_mode().is_true() && !has_type_checked {
&& !self.graph_container.is_type_checked(&roots, lib)
{
let graph = graph.segment(&roots);
self self
.type_checker .type_checker
.check( .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<ModuleGraph>
// back from the return value.
(*graph).clone(),
check::CheckOptions { check::CheckOptions {
build_fast_check_graph: true, build_fast_check_graph: true,
lib, lib,
log_ignored_options: false, log_ignored_options: false,
reload: self.options.reload_flag() reload: self.options.reload_flag(),
&& !roots.iter().all(|r| reload_exclusions.contains(r)),
type_check_mode: self.options.type_check_mode(), type_check_mode: self.options.type_check_mode(),
}, },
) )
.await?; .await?;
self.graph_container.set_type_checked(&roots, lib);
} }
log::debug!("Prepared module load."); log::debug!("Prepared module load.");