From 745333f073aba4c97e7d06c731063105493cde5a Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Wed, 24 Jan 2024 14:49:33 +0100 Subject: [PATCH] chore: improve unanalyzable dynamic import diagnostic (#22051) --- cli/tests/integration/publish_tests.rs | 11 +++ .../testdata/publish/invalid_fast_check.out | 1 - .../publish/javascript_missing_decl_file.out | 1 - .../publish/unanalyzable_dynamic_import.out | 16 ++++ .../unanalyzable_dynamic_import/deno.json | 7 ++ .../unanalyzable_dynamic_import/mod.ts | 1 + cli/tools/registry/diagnostics.rs | 73 ++++++++++++++---- cli/tools/registry/graph.rs | 5 +- cli/tools/registry/mod.rs | 58 +++++++++------ cli/tools/registry/tar.rs | 12 ++- cli/util/import_map.rs | 74 +++++++++++++++---- 11 files changed, 196 insertions(+), 63 deletions(-) create mode 100644 cli/tests/testdata/publish/unanalyzable_dynamic_import.out create mode 100644 cli/tests/testdata/publish/unanalyzable_dynamic_import/deno.json create mode 100644 cli/tests/testdata/publish/unanalyzable_dynamic_import/mod.ts diff --git a/cli/tests/integration/publish_tests.rs b/cli/tests/integration/publish_tests.rs index e185065aa5..f04f9c6824 100644 --- a/cli/tests/integration/publish_tests.rs +++ b/cli/tests/integration/publish_tests.rs @@ -50,6 +50,17 @@ itest!(javascript_missing_decl_file { temp_cwd: true, }); +itest!(unanalyzable_dynamic_import { + args: "publish --token 'sadfasdf'", + output: "publish/unanalyzable_dynamic_import.out", + cwd: Some("publish/unanalyzable_dynamic_import"), + copy_temp_dir: Some("publish/unanalyzable_dynamic_import"), + envs: env_vars_for_registry(), + exit_code: 0, + http_server: true, + temp_cwd: true, +}); + itest!(javascript_decl_file { args: "publish --token 'sadfasdf'", output: "publish/javascript_decl_file.out", diff --git a/cli/tests/testdata/publish/invalid_fast_check.out b/cli/tests/testdata/publish/invalid_fast_check.out index 37e25e269d..f37638b9fd 100644 --- a/cli/tests/testdata/publish/invalid_fast_check.out +++ b/cli/tests/testdata/publish/invalid_fast_check.out @@ -9,5 +9,4 @@ error[zap-missing-explicit-return-type]: missing explicit return type in the pub info: all functions in the public API must have an explicit return type docs: https://jsr.io/go/zap-missing-explicit-return-type - error: Found 1 problem diff --git a/cli/tests/testdata/publish/javascript_missing_decl_file.out b/cli/tests/testdata/publish/javascript_missing_decl_file.out index bf7797c09d..557451b29e 100644 --- a/cli/tests/testdata/publish/javascript_missing_decl_file.out +++ b/cli/tests/testdata/publish/javascript_missing_decl_file.out @@ -7,7 +7,6 @@ warning[zap-unsupported-javascript-entrypoint]: used a JavaScript module without info: fast check avoids type inference, so JavaScript entrypoints should be avoided docs: https://jsr.io/go/zap-unsupported-javascript-entrypoint - Publishing @foo/bar@1.0.0 ... Successfully published @foo/bar@1.0.0 Visit http://127.0.0.1:4250/@foo/bar@1.0.0 for details diff --git a/cli/tests/testdata/publish/unanalyzable_dynamic_import.out b/cli/tests/testdata/publish/unanalyzable_dynamic_import.out new file mode 100644 index 0000000000..3be7ece875 --- /dev/null +++ b/cli/tests/testdata/publish/unanalyzable_dynamic_import.out @@ -0,0 +1,16 @@ +Checking fast check type graph for errors... +Ensuring type checks... +Check file://[WILDCARD]/mod.ts +warning[unanalyzable-dynamic-import]: unable to analyze dynamic import + --> [WILDCARD]mod.ts:1:7 + | +1 | await import("asd " + asd); + | ^^^^^^^^^^^^^^^^^^^^ the unanalyzable dynamic import + + info: after publishing this package, imports from the local import map do not work + info: dynamic imports that can not be analyzed at publish time will not be rewritten automatically + info: make sure the dynamic import is resolvable at runtime without an import map + +Publishing @foo/bar@1.0.0 ... +Successfully published @foo/bar@1.0.0 +Visit http://127.0.0.1:4250/@foo/bar@1.0.0 for details diff --git a/cli/tests/testdata/publish/unanalyzable_dynamic_import/deno.json b/cli/tests/testdata/publish/unanalyzable_dynamic_import/deno.json new file mode 100644 index 0000000000..213a7cec63 --- /dev/null +++ b/cli/tests/testdata/publish/unanalyzable_dynamic_import/deno.json @@ -0,0 +1,7 @@ +{ + "name": "@foo/bar", + "version": "1.0.0", + "exports": { + ".": "./mod.ts" + } +} diff --git a/cli/tests/testdata/publish/unanalyzable_dynamic_import/mod.ts b/cli/tests/testdata/publish/unanalyzable_dynamic_import/mod.ts new file mode 100644 index 0000000000..fd53cb2c89 --- /dev/null +++ b/cli/tests/testdata/publish/unanalyzable_dynamic_import/mod.ts @@ -0,0 +1 @@ +await import("asd " + asd); diff --git a/cli/tools/registry/diagnostics.rs b/cli/tools/registry/diagnostics.rs index f6d81f5a85..0a847c46b1 100644 --- a/cli/tools/registry/diagnostics.rs +++ b/cli/tools/registry/diagnostics.rs @@ -21,6 +21,7 @@ use crate::diagnostics::DiagnosticSnippetSource; use crate::diagnostics::DiagnosticSourcePos; use crate::diagnostics::DiagnosticSourceRange; use crate::diagnostics::SourceTextParsedSourceStore; +use crate::util::import_map::ImportMapUnfurlDiagnostic; #[derive(Clone, Default)] pub struct PublishDiagnosticsCollector { @@ -36,7 +37,7 @@ impl PublishDiagnosticsCollector { let diagnostics = self.diagnostics.lock().unwrap().take(); let sources = SourceTextParsedSourceStore(sources); for diagnostic in diagnostics { - eprintln!("{}", diagnostic.display(&sources)); + eprint!("{}", diagnostic.display(&sources)); if matches!(diagnostic.level(), DiagnosticLevel::Error) { errors += 1; } @@ -58,34 +59,42 @@ impl PublishDiagnosticsCollector { } pub enum PublishDiagnostic { - FastCheck { diagnostic: FastCheckDiagnostic }, + FastCheck(FastCheckDiagnostic), + ImportMapUnfurl(ImportMapUnfurlDiagnostic), } impl Diagnostic for PublishDiagnostic { fn level(&self) -> DiagnosticLevel { match self { - PublishDiagnostic::FastCheck { - diagnostic: FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. }, - } => DiagnosticLevel::Warning, - PublishDiagnostic::FastCheck { .. } => DiagnosticLevel::Error, + PublishDiagnostic::FastCheck( + FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. }, + ) => DiagnosticLevel::Warning, + PublishDiagnostic::FastCheck(_) => DiagnosticLevel::Error, + PublishDiagnostic::ImportMapUnfurl(_) => DiagnosticLevel::Warning, } } fn code(&self) -> impl Display + '_ { match &self { - PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.code(), + PublishDiagnostic::FastCheck(diagnostic) => diagnostic.code(), + PublishDiagnostic::ImportMapUnfurl(diagnostic) => diagnostic.code(), } } fn message(&self) -> impl Display + '_ { match &self { - PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.to_string(), // todo + PublishDiagnostic::FastCheck(diagnostic) => { + Cow::Owned(diagnostic.to_string()) + } + PublishDiagnostic::ImportMapUnfurl(diagnostic) => { + Cow::Borrowed(diagnostic.message()) + } } } fn location(&self) -> DiagnosticLocation { match &self { - PublishDiagnostic::FastCheck { diagnostic } => match diagnostic.range() { + PublishDiagnostic::FastCheck(diagnostic) => match diagnostic.range() { Some(range) => DiagnosticLocation::PositionInFile { specifier: Cow::Borrowed(diagnostic.specifier()), source_pos: DiagnosticSourcePos::SourcePos(range.range.start), @@ -94,12 +103,21 @@ impl Diagnostic for PublishDiagnostic { specifier: Cow::Borrowed(diagnostic.specifier()), }, }, + PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic { + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { + specifier, + range, + } => DiagnosticLocation::PositionInFile { + specifier: Cow::Borrowed(specifier), + source_pos: DiagnosticSourcePos::SourcePos(range.start), + }, + }, } } fn snippet(&self) -> Option> { match &self { - PublishDiagnostic::FastCheck { diagnostic } => { + PublishDiagnostic::FastCheck(diagnostic) => { diagnostic.range().map(|range| DiagnosticSnippet { source: DiagnosticSnippetSource::Specifier(Cow::Borrowed( diagnostic.specifier(), @@ -114,14 +132,29 @@ impl Diagnostic for PublishDiagnostic { }, }) } + PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic { + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { + specifier, + range, + } => Some(DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Borrowed(specifier)), + highlight: DiagnosticSnippetHighlight { + style: DiagnosticSnippetHighlightStyle::Warning, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::SourcePos(range.start), + end: DiagnosticSourcePos::SourcePos(range.end), + }, + description: Some("the unanalyzable dynamic import".into()), + }, + }), + }, } } fn hint(&self) -> Option { match &self { - PublishDiagnostic::FastCheck { diagnostic } => { - Some(diagnostic.fix_hint()) - } + PublishDiagnostic::FastCheck(diagnostic) => Some(diagnostic.fix_hint()), + PublishDiagnostic::ImportMapUnfurl(_) => None, } } @@ -131,7 +164,7 @@ impl Diagnostic for PublishDiagnostic { fn info(&self) -> Cow<'_, [Cow<'_, str>]> { match &self { - PublishDiagnostic::FastCheck { diagnostic } => { + PublishDiagnostic::FastCheck(diagnostic) => { let infos = diagnostic .additional_info() .iter() @@ -139,14 +172,24 @@ impl Diagnostic for PublishDiagnostic { .collect(); Cow::Owned(infos) } + PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic { + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } => Cow::Borrowed(&[ + Cow::Borrowed("after publishing this package, imports from the local import map do not work"), + Cow::Borrowed("dynamic imports that can not be analyzed at publish time will not be rewritten automatically"), + Cow::Borrowed("make sure the dynamic import is resolvable at runtime without an import map") + ]), + }, } } fn docs_url(&self) -> Option { match &self { - PublishDiagnostic::FastCheck { diagnostic } => { + PublishDiagnostic::FastCheck(diagnostic) => { Some(format!("https://jsr.io/go/{}", diagnostic.code())) } + PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic { + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } => None, + }, } } } diff --git a/cli/tools/registry/graph.rs b/cli/tools/registry/graph.rs index 29c825242c..4d061e3da5 100644 --- a/cli/tools/registry/graph.rs +++ b/cli/tools/registry/graph.rs @@ -94,9 +94,8 @@ pub fn collect_fast_check_type_graph_diagnostics( { continue; } - diagnostics_collector.push(PublishDiagnostic::FastCheck { - diagnostic: diagnostic.clone(), - }); + diagnostics_collector + .push(PublishDiagnostic::FastCheck(diagnostic.clone())); if matches!( diagnostic, FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. } diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index 8eaf612583..96f2d0f13b 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -11,9 +11,9 @@ use deno_config::ConfigFile; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; +use deno_core::futures::FutureExt; use deno_core::serde_json; use deno_core::serde_json::json; -use deno_core::unsync::JoinHandle; use deno_core::unsync::JoinSet; use deno_runtime::colors; use deno_runtime::deno_fetch::reqwest; @@ -89,6 +89,7 @@ async fn prepare_publish( deno_json: &ConfigFile, source_cache: Arc, import_map: Arc, + diagnostics_collector: &PublishDiagnosticsCollector, ) -> Result, AnyError> { let config_path = deno_json.specifier.to_file_path().unwrap(); let dir_path = config_path.parent().unwrap().to_path_buf(); @@ -134,11 +135,13 @@ async fn prepare_publish( .to_files_config() .map(|files| files.map(|f| f.exclude).unwrap_or_default())?; + let diagnostics_collector = diagnostics_collector.clone(); let tarball = deno_core::unsync::spawn_blocking(move || { let unfurler = ImportMapUnfurler::new(&import_map); tar::create_gzipped_tarball( &dir_path, &*source_cache, + &diagnostics_collector, &unfurler, &exclude_patterns, ) @@ -666,8 +669,13 @@ async fn prepare_packages_for_publishing( ) .await?; let mut prepared_package_by_name = HashMap::with_capacity(1); - let package = - prepare_publish(&deno_json, source_cache.clone(), import_map).await?; + let package = prepare_publish( + &deno_json, + source_cache.clone(), + import_map, + diagnostics_collector, + ) + .await?; let package_name = format!("@{}/{}", package.scope, package.package); let publish_order_graph = PublishOrderGraph::new_single(package_name.clone()); @@ -692,29 +700,31 @@ async fn prepare_packages_for_publishing( let publish_order_graph = publish_order::build_publish_order_graph(&graph, &roots)?; - let results = - workspace_config - .members - .iter() - .cloned() - .map(|member| { - let import_map = import_map.clone(); - let source_cache = source_cache.clone(); - deno_core::unsync::spawn(async move { - let package = prepare_publish(&member.config_file, source_cache, import_map) - .await - .with_context(|| { - format!("Failed preparing '{}'.", member.package_name) - })?; - Ok((member.package_name, package)) - }) - }) - .collect::), AnyError>>, - >>(); + let results = workspace_config + .members + .iter() + .cloned() + .map(|member| { + let import_map = import_map.clone(); + async move { + let package = prepare_publish( + &member.config_file, + source_cache.clone(), + import_map.clone(), + diagnostics_collector, + ) + .await + .with_context(|| { + format!("Failed preparing '{}'.", member.package_name) + })?; + Ok::<_, AnyError>((member.package_name, package)) + } + .boxed() + }) + .collect::>(); let results = deno_core::futures::future::join_all(results).await; for result in results { - let (package_name, package) = result??; + let (package_name, package) = result?; prepared_package_by_name.insert(package_name, package); } Ok((publish_order_graph, prepared_package_by_name)) diff --git a/cli/tools/registry/tar.rs b/cli/tools/registry/tar.rs index 0f6edbc3a0..9bd7f098ee 100644 --- a/cli/tools/registry/tar.rs +++ b/cli/tools/registry/tar.rs @@ -15,6 +15,9 @@ use tar::Header; use crate::util::import_map::ImportMapUnfurler; use deno_config::glob::PathOrPatternSet; +use super::diagnostics::PublishDiagnostic; +use super::diagnostics::PublishDiagnosticsCollector; + #[derive(Debug, Clone, PartialEq)] pub struct PublishableTarballFile { pub path: PathBuf, @@ -32,6 +35,7 @@ pub struct PublishableTarball { pub fn create_gzipped_tarball( dir: &Path, source_cache: &dyn deno_graph::ParsedSourceStore, + diagnostics_collector: &PublishDiagnosticsCollector, unfurler: &ImportMapUnfurler, exclude_patterns: &PathOrPatternSet, ) -> Result { @@ -72,9 +76,11 @@ pub fn create_gzipped_tarball( }); let content = match source_cache.get_parsed_source(&url) { Some(parsed_source) => { - let (content, unfurl_diagnostics) = - unfurler.unfurl(&url, &parsed_source); - diagnostics.extend_from_slice(&unfurl_diagnostics); + let mut reporter = |diagnostic| { + diagnostics_collector + .push(PublishDiagnostic::ImportMapUnfurl(diagnostic)); + }; + let content = unfurler.unfurl(&url, &parsed_source, &mut reporter); content.into_bytes() } None => data, diff --git a/cli/util/import_map.rs b/cli/util/import_map.rs index e26c7cb0ce..2656389b8c 100644 --- a/cli/util/import_map.rs +++ b/cli/util/import_map.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use deno_ast::ParsedSource; +use deno_ast::SourceRange; use deno_core::serde_json; use deno_core::ModuleSpecifier; use deno_graph::DefaultModuleAnalyzer; @@ -14,8 +15,6 @@ use deno_semver::jsr::JsrPackageReqReference; use deno_semver::npm::NpmPackageReqReference; use import_map::ImportMap; -use crate::graph_util::format_range_with_colors; - pub fn import_map_deps(value: &serde_json::Value) -> HashSet { let Some(obj) = value.as_object() else { return Default::default(); @@ -69,6 +68,30 @@ fn values_to_set<'a>( entries } +#[derive(Debug, Clone)] +pub enum ImportMapUnfurlDiagnostic { + UnanalyzableDynamicImport { + specifier: ModuleSpecifier, + range: SourceRange, + }, +} + +impl ImportMapUnfurlDiagnostic { + pub fn code(&self) -> &'static str { + match self { + Self::UnanalyzableDynamicImport { .. } => "unanalyzable-dynamic-import", + } + } + + pub fn message(&self) -> &'static str { + match self { + Self::UnanalyzableDynamicImport { .. } => { + "unable to analyze dynamic import" + } + } + } +} + pub struct ImportMapUnfurler<'a> { import_map: &'a ImportMap, } @@ -82,8 +105,8 @@ impl<'a> ImportMapUnfurler<'a> { &self, url: &ModuleSpecifier, parsed_source: &ParsedSource, - ) -> (String, Vec) { - let mut diagnostics = Vec::new(); + diagnostic_reporter: &mut dyn FnMut(ImportMapUnfurlDiagnostic), + ) -> String { let mut text_changes = Vec::new(); let module_info = DefaultModuleAnalyzer::module_info(parsed_source); let analyze_specifier = @@ -117,14 +140,17 @@ impl<'a> ImportMapUnfurler<'a> { ); if !success { - diagnostics.push( - format!("Dynamic import was not analyzable and won't use the import map once published.\n at {}", - format_range_with_colors(&deno_graph::Range { - specifier: url.clone(), - start: dep.range.start.clone(), - end: dep.range.end.clone(), - }) - ) + let start_pos = + parsed_source.text_info().line_start(dep.range.start.line) + + dep.range.start.character; + let end_pos = + parsed_source.text_info().line_start(dep.range.end.line) + + dep.range.end.character; + diagnostic_reporter( + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { + specifier: url.to_owned(), + range: SourceRange::new(start_pos, end_pos), + }, ); } } @@ -160,7 +186,7 @@ impl<'a> ImportMapUnfurler<'a> { parsed_source.text_info().text_str(), text_changes, ); - (rewritten_text, diagnostics) + rewritten_text } } @@ -311,10 +337,26 @@ const test6 = await import(`${expr}`); "#; let specifier = ModuleSpecifier::parse("file:///dev/mod.ts").unwrap(); let source = parse_ast(&specifier, source_code); - let (unfurled_source, d) = unfurler.unfurl(&specifier, &source); + let mut d = Vec::new(); + let mut reporter = |diagnostic| d.push(diagnostic); + let unfurled_source = unfurler.unfurl(&specifier, &source, &mut reporter); assert_eq!(d.len(), 2); - assert!(d[0].starts_with("Dynamic import was not analyzable and won't use the import map once published.")); - assert!(d[1].starts_with("Dynamic import was not analyzable and won't use the import map once published.")); + assert!( + matches!( + d[0], + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } + ), + "{:?}", + d[0] + ); + assert!( + matches!( + d[1], + ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } + ), + "{:?}", + d[1] + ); let expected_source = r#"import express from "npm:express@5";" import foo from "./lib/foo.ts"; import bar from "./lib/bar.ts";