Skip to content

Commit

Permalink
fix: error on conflicting assertions
Browse files Browse the repository at this point in the history
Previously, if a specifier was imported more than once, with conflicting
assertions, we'd only really consider the final import. This is wrong:
conflicting assertions are an error in-themselves already. I think this
behaviour is still wrong, because if a module is imported with incorrect
assertions later in the graph after the module has already been loaded by
an import with a correct assertion, no error will be raised.
  • Loading branch information
lucacasonato committed May 20, 2022
1 parent 89affe4 commit cbf7134
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 28 deletions.
70 changes: 42 additions & 28 deletions src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ pub enum ModuleGraphError {
actual_media_type: MediaType,
expected_media_type: MediaType,
},
ConflictingAssertions(ModuleSpecifier),
LoadingErr(ModuleSpecifier, Arc<anyhow::Error>),
Missing(ModuleSpecifier),
ParseErr(ModuleSpecifier, deno_ast::Diagnostic),
Expand Down Expand Up @@ -163,6 +164,9 @@ impl Clone for ModuleGraphError {
actual_media_type: *actual_media_type,
expected_media_type: *expected_media_type,
},
Self::ConflictingAssertions(specifier) => {
Self::ConflictingAssertions(specifier.clone())
}
Self::UnsupportedImportAssertionType(specifier, kind) => {
Self::UnsupportedImportAssertionType(specifier.clone(), kind.clone())
}
Expand All @@ -184,7 +188,8 @@ impl ModuleGraphError {
| Self::InvalidSource(s, _)
| Self::UnsupportedMediaType(s, _)
| Self::UnsupportedImportAssertionType(s, _)
| Self::Missing(s) => s,
| Self::Missing(s)
| Self::ConflictingAssertions(s) => s,
Self::InvalidTypeAssertion { specifier, .. } => specifier,
}
}
Expand All @@ -201,6 +206,7 @@ impl fmt::Display for ModuleGraphError {
Self::InvalidSource(specifier, Some(filename)) => write!(f, "The source code is invalid, as it does not match the expected hash in the lock file.\n Specifier: {}\n Lock file: {}", specifier, filename),
Self::InvalidSource(specifier, None) => write!(f, "The source code is invalid, as it does not match the expected hash in the lock file.\n Specifier: {}", specifier),
Self::InvalidTypeAssertion { specifier, actual_media_type, expected_media_type } => write!(f, "Expected a {} module, but identified a {} module.\n Specifier: {}", expected_media_type, actual_media_type, specifier),
Self::ConflictingAssertions(specifier) => write!(f, "Module \"{specifier}\" was imported with conflicting assertions."),
Self::UnsupportedMediaType(specifier, MediaType::Json) => write!(f, "Expected a JavaScript or TypeScript module, but identified a Json module. Consider importing Json modules with an import assertion with the type of \"json\".\n Specifier: {}", specifier),
Self::UnsupportedMediaType(specifier, media_type) => write!(f, "Expected a JavaScript or TypeScript module, but identified a {} module. Importing these types of modules is currently not supported.\n Specifier: {}", media_type, specifier),
Self::UnsupportedImportAssertionType(_, kind) => write!(f, "The import assertion type of \"{}\" is unsupported.", kind),
Expand Down Expand Up @@ -1559,15 +1565,7 @@ impl<'a> Builder<'a> {
Some((specifier, kind, Ok(Some(response)))) => {
let assert_types =
self.pending_assert_types.remove(&specifier).unwrap();
for maybe_assert_type in assert_types {
self.visit(
&specifier,
&kind,
&response,
&build_kind,
maybe_assert_type,
)
}
self.visit(&specifier, &kind, &response, &build_kind, assert_types);
Some(specifier)
}
Some((specifier, _, Ok(None))) => {
Expand Down Expand Up @@ -1700,42 +1698,58 @@ impl<'a> Builder<'a> {
kind: &ModuleKind,
response: &LoadResponse,
build_kind: &BuildKind,
maybe_assert_type: Option<String>,
assert_types: HashSet<Option<String>>,
) {
let (specifier, module_slot) = match response {
LoadResponse::BuiltIn { specifier } => {
self.check_specifier(requested_specifier, specifier);
let module_slot = ModuleSlot::Module(Module::new_without_source(
specifier.clone(),
ModuleKind::BuiltIn,
));
self.check_specifier(requested_specifier, &specifier);
let module_slot = if assert_types.len() != 1 {
ModuleSlot::Err(ModuleGraphError::ConflictingAssertions(
specifier.clone(),
))
} else {
ModuleSlot::Module(Module::new_without_source(
specifier.clone(),
ModuleKind::BuiltIn,
))
};
(specifier, module_slot)
}
LoadResponse::External { specifier } => {
self.check_specifier(requested_specifier, specifier);
let module_slot = ModuleSlot::Module(Module::new_without_source(
specifier.clone(),
ModuleKind::External,
));
self.check_specifier(requested_specifier, &specifier);
let module_slot = if assert_types.len() != 1 {
ModuleSlot::Err(ModuleGraphError::ConflictingAssertions(
specifier.clone(),
))
} else {
ModuleSlot::Module(Module::new_without_source(
specifier.clone(),
ModuleKind::External,
))
};
(specifier, module_slot)
}
LoadResponse::Module {
specifier,
content,
maybe_headers,
} => {
self.check_specifier(requested_specifier, specifier);
(
specifier,
self.check_specifier(requested_specifier, &specifier);
let module_slot = if assert_types.len() != 1 {
ModuleSlot::Err(ModuleGraphError::ConflictingAssertions(
specifier.clone(),
))
} else {
self.visit_module(
specifier,
&specifier,
kind,
maybe_headers.as_ref(),
content.clone(),
build_kind,
maybe_assert_type,
),
)
assert_types.into_iter().next().unwrap(),
)
};
(specifier, module_slot)
}
};
self
Expand Down
3 changes: 3 additions & 0 deletions src/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,9 @@ impl ModuleGraphError {
Self::InvalidTypeAssertion { .. } => {
fmt_error_msg(f, prefix, last, specifier, "(invalid import assertion)")
}
Self::ConflictingAssertions { .. } => {
fmt_error_msg(f, prefix, last, specifier, "(conflicting assertions)")
}
Self::LoadingErr(_, _) => {
fmt_error_msg(f, prefix, last, specifier, "(loading error)")
}
Expand Down

0 comments on commit cbf7134

Please sign in to comment.