diff options
| -rw-r--r-- | cli/diagnostics.rs | 9 | ||||
| -rw-r--r-- | cli/tests/integration/publish_tests.rs | 10 | ||||
| -rw-r--r-- | cli/tests/testdata/publish/invalid_import.out | 32 | ||||
| -rw-r--r-- | cli/tests/testdata/publish/invalid_import/deno.json | 10 | ||||
| -rw-r--r-- | cli/tests/testdata/publish/invalid_import/mod.ts | 9 | ||||
| -rw-r--r-- | cli/tests/testdata/publish/symlink.out | 1 | ||||
| -rw-r--r-- | cli/tools/registry/diagnostics.rs | 127 | ||||
| -rw-r--r-- | cli/tools/registry/graph.rs | 73 | ||||
| -rw-r--r-- | cli/tools/registry/mod.rs | 4 | ||||
| -rw-r--r-- | ext/node/ops/crypto/x509.rs | 1 |
10 files changed, 237 insertions, 39 deletions
diff --git a/cli/diagnostics.rs b/cli/diagnostics.rs index 015dea178..846cd7751 100644 --- a/cli/diagnostics.rs +++ b/cli/diagnostics.rs @@ -49,6 +49,12 @@ pub struct DiagnosticSourceRange { pub enum DiagnosticSourcePos { SourcePos(SourcePos), ByteIndex(usize), + LineAndCol { + // 0-indexed line number + line: usize, + // 0-indexed column number + column: usize, + }, } impl DiagnosticSourcePos { @@ -56,6 +62,9 @@ impl DiagnosticSourcePos { match self { DiagnosticSourcePos::SourcePos(pos) => *pos, DiagnosticSourcePos::ByteIndex(index) => source.range().start() + *index, + DiagnosticSourcePos::LineAndCol { line, column } => { + source.line_start(*line) + *column + } } } } diff --git a/cli/tests/integration/publish_tests.rs b/cli/tests/integration/publish_tests.rs index 0e917eab9..59486602d 100644 --- a/cli/tests/integration/publish_tests.rs +++ b/cli/tests/integration/publish_tests.rs @@ -3,6 +3,7 @@ use deno_core::serde_json::json; use test_util::assert_contains; use test_util::assert_not_contains; +use test_util::env_vars_for_npm_tests; use test_util::TestContextBuilder; static TEST_REGISTRY_URL: &str = "http://127.0.0.1:4250"; @@ -49,6 +50,15 @@ itest!(symlink { exit_code: 0, }); +itest!(invalid_import { + args: "publish --token 'sadfasdf' --dry-run", + output: "publish/invalid_import.out", + cwd: Some("publish/invalid_import"), + envs: env_vars_for_npm_tests(), + exit_code: 1, + http_server: true, +}); + itest!(javascript_missing_decl_file { args: "publish --token 'sadfasdf'", output: "publish/javascript_missing_decl_file.out", diff --git a/cli/tests/testdata/publish/invalid_import.out b/cli/tests/testdata/publish/invalid_import.out new file mode 100644 index 000000000..ca9d20eed --- /dev/null +++ b/cli/tests/testdata/publish/invalid_import.out @@ -0,0 +1,32 @@ +Download http://localhost:4545/welcome.ts +Download http://localhost:4545/echo.ts +Download http://localhost:4545/npm/registry/chalk +Download http://localhost:4545/npm/registry/chalk/chalk-5.0.1.tgz +Checking fast check type graph for errors... +Ensuring type checks... +Check file://[WILDCARD]mod.ts +error[invalid-external-import]: invalid import to a non-JSR 'http' specifier + --> [WILDCARD]mod.ts:1:8 + | +1 | import "http://localhost:4545/welcome.ts"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the specifier + = hint: replace this import with one from jsr or npm, or vendor the dependency into your package + + info: the import was resolved to 'http://localhost:4545/welcome.ts' + info: this specifier is not allowed to be imported on jsr + info: jsr only supports importing `jsr:`, `npm:`, and `data:` specifiers + docs: https://jsr.io/go/invalid-external-import + +error[invalid-external-import]: invalid import to a non-JSR 'http' specifier + --> [WILDCARD]mod.ts:2:8 + | +2 | import "$echo"; + | ^^^^^^^ the specifier + = hint: replace this import with one from jsr or npm, or vendor the dependency into your package + + info: the import was resolved to 'http://localhost:4545/echo.ts' + info: this specifier is not allowed to be imported on jsr + info: jsr only supports importing `jsr:`, `npm:`, and `data:` specifiers + docs: https://jsr.io/go/invalid-external-import + +error: Found 2 problems diff --git a/cli/tests/testdata/publish/invalid_import/deno.json b/cli/tests/testdata/publish/invalid_import/deno.json new file mode 100644 index 000000000..49b666d22 --- /dev/null +++ b/cli/tests/testdata/publish/invalid_import/deno.json @@ -0,0 +1,10 @@ +{ + "name": "@foo/bar", + "version": "1.0.0", + "imports": { + "$echo": "http://localhost:4545/echo.ts" + }, + "exports": { + ".": "./mod.ts" + } +} diff --git a/cli/tests/testdata/publish/invalid_import/mod.ts b/cli/tests/testdata/publish/invalid_import/mod.ts new file mode 100644 index 000000000..bdaf010e2 --- /dev/null +++ b/cli/tests/testdata/publish/invalid_import/mod.ts @@ -0,0 +1,9 @@ +import "http://localhost:4545/welcome.ts"; +import "$echo"; + +import "data:application/javascript,console.log(1)"; +import "npm:chalk@5"; + +export function foobar(): string { + return "string"; +} diff --git a/cli/tests/testdata/publish/symlink.out b/cli/tests/testdata/publish/symlink.out index 5befec4f9..d226fa18e 100644 --- a/cli/tests/testdata/publish/symlink.out +++ b/cli/tests/testdata/publish/symlink.out @@ -7,5 +7,6 @@ warning[unsupported-file-type]: unsupported file type 'symlink' info: only files and directories are supported info: the file was ignored and will not be published + docs: https://jsr.io/go/unsupported-file-type Warning Aborting due to --dry-run diff --git a/cli/tools/registry/diagnostics.rs b/cli/tools/registry/diagnostics.rs index 45090aa2c..bd2a64d35 100644 --- a/cli/tools/registry/diagnostics.rs +++ b/cli/tools/registry/diagnostics.rs @@ -63,60 +63,72 @@ impl PublishDiagnosticsCollector { pub enum PublishDiagnostic { FastCheck(FastCheckDiagnostic), ImportMapUnfurl(ImportMapUnfurlDiagnostic), - InvalidPath { path: PathBuf, message: String }, - DuplicatePath { path: PathBuf }, - UnsupportedFileType { specifier: Url, kind: String }, + InvalidPath { + path: PathBuf, + message: String, + }, + DuplicatePath { + path: PathBuf, + }, + UnsupportedFileType { + specifier: Url, + kind: String, + }, + InvalidExternalImport { + kind: String, + imported: Url, + referrer: deno_graph::Range, + }, } impl Diagnostic for PublishDiagnostic { fn level(&self) -> DiagnosticLevel { + use PublishDiagnostic::*; match self { - PublishDiagnostic::FastCheck( - FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. }, - ) => DiagnosticLevel::Warning, - PublishDiagnostic::FastCheck(_) => DiagnosticLevel::Error, - PublishDiagnostic::ImportMapUnfurl(_) => DiagnosticLevel::Warning, - PublishDiagnostic::InvalidPath { .. } => DiagnosticLevel::Error, - PublishDiagnostic::DuplicatePath { .. } => DiagnosticLevel::Error, - PublishDiagnostic::UnsupportedFileType { .. } => DiagnosticLevel::Warning, + FastCheck(FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { + .. + }) => DiagnosticLevel::Warning, + FastCheck(_) => DiagnosticLevel::Error, + ImportMapUnfurl(_) => DiagnosticLevel::Warning, + InvalidPath { .. } => DiagnosticLevel::Error, + DuplicatePath { .. } => DiagnosticLevel::Error, + UnsupportedFileType { .. } => DiagnosticLevel::Warning, + InvalidExternalImport { .. } => DiagnosticLevel::Error, } } fn code(&self) -> impl Display + '_ { + use PublishDiagnostic::*; match &self { - PublishDiagnostic::FastCheck(diagnostic) => diagnostic.code(), - PublishDiagnostic::ImportMapUnfurl(diagnostic) => diagnostic.code(), - PublishDiagnostic::InvalidPath { .. } => "invalid-path", - PublishDiagnostic::DuplicatePath { .. } => { - "case-insensitive-duplicate-path" - } - PublishDiagnostic::UnsupportedFileType { .. } => "unsupported-file-type", + FastCheck(diagnostic) => diagnostic.code(), + ImportMapUnfurl(diagnostic) => diagnostic.code(), + InvalidPath { .. } => "invalid-path", + DuplicatePath { .. } => "case-insensitive-duplicate-path", + UnsupportedFileType { .. } => "unsupported-file-type", + InvalidExternalImport { .. } => "invalid-external-import", } } fn message(&self) -> impl Display + '_ { + use PublishDiagnostic::*; match &self { - PublishDiagnostic::FastCheck(diagnostic) => { - Cow::Owned(diagnostic.to_string()) - } - PublishDiagnostic::ImportMapUnfurl(diagnostic) => { - Cow::Borrowed(diagnostic.message()) - } - PublishDiagnostic::InvalidPath { message, .. } => { - Cow::Borrowed(message.as_str()) - } - PublishDiagnostic::DuplicatePath { .. } => { + FastCheck(diagnostic) => Cow::Owned(diagnostic.to_string()) , + ImportMapUnfurl(diagnostic) => Cow::Borrowed(diagnostic.message()), + InvalidPath { message, .. } => Cow::Borrowed(message.as_str()), + DuplicatePath { .. } => { Cow::Borrowed("package path is a case insensitive duplicate of another path in the package") } - PublishDiagnostic::UnsupportedFileType { kind, .. } => { - Cow::Owned(format!("unsupported file type '{kind}'",)) + UnsupportedFileType { kind, .. } => { + Cow::Owned(format!("unsupported file type '{kind}'")) } + InvalidExternalImport { kind, .. } => Cow::Owned(format!("invalid import to a {kind} specifier")), } } fn location(&self) -> DiagnosticLocation { + use PublishDiagnostic::*; match &self { - PublishDiagnostic::FastCheck(diagnostic) => match diagnostic.range() { + FastCheck(diagnostic) => match diagnostic.range() { Some(range) => DiagnosticLocation::ModulePosition { specifier: Cow::Borrowed(diagnostic.specifier()), source_pos: DiagnosticSourcePos::SourcePos(range.range.start), @@ -125,7 +137,7 @@ impl Diagnostic for PublishDiagnostic { specifier: Cow::Borrowed(diagnostic.specifier()), }, }, - PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic { + ImportMapUnfurl(diagnostic) => match diagnostic { ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { specifier, range, @@ -134,15 +146,22 @@ impl Diagnostic for PublishDiagnostic { source_pos: DiagnosticSourcePos::SourcePos(range.start), }, }, - PublishDiagnostic::InvalidPath { path, .. } => { + InvalidPath { path, .. } => { DiagnosticLocation::Path { path: path.clone() } } - PublishDiagnostic::DuplicatePath { path, .. } => { + DuplicatePath { path, .. } => { DiagnosticLocation::Path { path: path.clone() } } - PublishDiagnostic::UnsupportedFileType { specifier, .. } => { - DiagnosticLocation::Module { - specifier: Cow::Borrowed(specifier), + UnsupportedFileType { specifier, .. } => DiagnosticLocation::Module { + specifier: Cow::Borrowed(specifier), + }, + InvalidExternalImport { referrer, .. } => { + DiagnosticLocation::ModulePosition { + specifier: Cow::Borrowed(&referrer.specifier), + source_pos: DiagnosticSourcePos::LineAndCol { + line: referrer.start.line, + column: referrer.start.character, + }, } } } @@ -184,6 +203,27 @@ impl Diagnostic for PublishDiagnostic { PublishDiagnostic::InvalidPath { .. } => None, PublishDiagnostic::DuplicatePath { .. } => None, PublishDiagnostic::UnsupportedFileType { .. } => None, + PublishDiagnostic::InvalidExternalImport { referrer, .. } => { + Some(DiagnosticSnippet { + source: DiagnosticSnippetSource::Specifier(Cow::Borrowed( + &referrer.specifier, + )), + highlight: DiagnosticSnippetHighlight { + style: DiagnosticSnippetHighlightStyle::Error, + range: DiagnosticSourceRange { + start: DiagnosticSourcePos::LineAndCol { + line: referrer.start.line, + column: referrer.start.character, + }, + end: DiagnosticSourcePos::LineAndCol { + line: referrer.end.line, + column: referrer.end.character, + }, + }, + description: Some("the specifier".into()), + }, + }) + } } } @@ -200,6 +240,7 @@ impl Diagnostic for PublishDiagnostic { PublishDiagnostic::UnsupportedFileType { .. } => Some( "remove the file, or add it to 'publish.exclude' in the config file", ), + PublishDiagnostic::InvalidExternalImport { .. } => Some("replace this import with one from jsr or npm, or vendor the dependency into your package") } } @@ -234,6 +275,11 @@ impl Diagnostic for PublishDiagnostic { Cow::Borrowed("only files and directories are supported"), Cow::Borrowed("the file was ignored and will not be published") ]), + PublishDiagnostic::InvalidExternalImport { imported, .. } => Cow::Owned(vec![ + Cow::Owned(format!("the import was resolved to '{}'", imported)), + Cow::Borrowed("this specifier is not allowed to be imported on jsr"), + Cow::Borrowed("jsr only supports importing `jsr:`, `npm:`, and `data:` specifiers"), + ]), } } @@ -251,7 +297,12 @@ impl Diagnostic for PublishDiagnostic { PublishDiagnostic::DuplicatePath { .. } => { Some("https://jsr.io/go/case-insensitive-duplicate-path".to_owned()) } - PublishDiagnostic::UnsupportedFileType { .. } => None, + PublishDiagnostic::UnsupportedFileType { .. } => { + Some("https://jsr.io/go/unsupported-file-type".to_owned()) + } + PublishDiagnostic::InvalidExternalImport { .. } => { + Some("https://jsr.io/go/invalid-external-import".to_owned()) + } } } } diff --git a/cli/tools/registry/graph.rs b/cli/tools/registry/graph.rs index 4d061e3da..2a3b4cc17 100644 --- a/cli/tools/registry/graph.rs +++ b/cli/tools/registry/graph.rs @@ -10,7 +10,11 @@ use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_graph::FastCheckDiagnostic; +use deno_graph::ModuleEntryRef; use deno_graph::ModuleGraph; +use deno_graph::ResolutionResolved; +use deno_graph::WalkOptions; +use lsp_types::Url; use super::diagnostics::PublishDiagnostic; use super::diagnostics::PublishDiagnosticsCollector; @@ -64,6 +68,75 @@ pub fn resolve_config_file_roots_from_exports( Ok(exports) } +pub fn collect_invalid_external_imports( + graph: &ModuleGraph, + diagnostics_collector: &PublishDiagnosticsCollector, +) { + let mut visited = HashSet::new(); + let mut skip_specifiers: HashSet<Url> = HashSet::new(); + + let mut collect_if_invalid = + |skip_specifiers: &mut HashSet<Url>, resolution: &ResolutionResolved| { + if visited.insert(resolution.specifier.clone()) { + match resolution.specifier.scheme() { + "file" | "data" => {} + "jsr" | "npm" => { + skip_specifiers.insert(resolution.specifier.clone()); + } + "http" | "https" => { + skip_specifiers.insert(resolution.specifier.clone()); + diagnostics_collector.push( + PublishDiagnostic::InvalidExternalImport { + kind: format!("non-JSR '{}'", resolution.specifier.scheme()), + imported: resolution.specifier.clone(), + referrer: resolution.range.clone(), + }, + ); + } + _ => { + skip_specifiers.insert(resolution.specifier.clone()); + diagnostics_collector.push( + PublishDiagnostic::InvalidExternalImport { + kind: format!("'{}'", resolution.specifier.scheme()), + imported: resolution.specifier.clone(), + referrer: resolution.range.clone(), + }, + ); + } + } + } + }; + + let options = WalkOptions { + check_js: true, + follow_dynamic: true, + follow_type_only: true, + }; + let mut iter = graph.walk(&graph.roots, options); + while let Some((specifier, entry)) = iter.next() { + if skip_specifiers.contains(specifier) { + iter.skip_previous_dependencies(); + continue; + } + + let ModuleEntryRef::Module(module) = entry else { + continue; + }; + let Some(module) = module.esm() else { + continue; + }; + + for (_, dep) in &module.dependencies { + if let Some(resolved) = dep.maybe_code.ok() { + collect_if_invalid(&mut skip_specifiers, resolved); + } + if let Some(resolved) = dep.maybe_type.ok() { + collect_if_invalid(&mut skip_specifiers, resolved); + } + } + } +} + /// Collects diagnostics from the module graph for the given packages. /// Returns true if any diagnostics were collected. pub fn collect_fast_check_type_graph_diagnostics( diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index 1c5344d27..cb43e0df8 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -34,6 +34,7 @@ use crate::http_util::HttpClient; use crate::tools::check::CheckOptions; use crate::tools::registry::diagnostics::PublishDiagnosticsCollector; use crate::tools::registry::graph::collect_fast_check_type_graph_diagnostics; +use crate::tools::registry::graph::collect_invalid_external_imports; use crate::tools::registry::graph::get_workspace_member_roots; use crate::tools::registry::graph::resolve_config_file_roots_from_exports; use crate::tools::registry::graph::MemberRoots; @@ -752,6 +753,9 @@ async fn build_and_check_graph_for_publish( .await?, ); graph.valid()?; + + collect_invalid_external_imports(&graph, diagnostics_collector); + log::info!("Checking fast check type graph for errors..."); let has_fast_check_diagnostics = collect_fast_check_type_graph_diagnostics( &graph, diff --git a/ext/node/ops/crypto/x509.rs b/ext/node/ops/crypto/x509.rs index 440e7c291..2e0aec011 100644 --- a/ext/node/ops/crypto/x509.rs +++ b/ext/node/ops/crypto/x509.rs @@ -106,7 +106,6 @@ pub fn op_node_x509_check_email( if let Some(subject_alt) = subject_alt { for name in &subject_alt.general_names { - dbg!(name); if let extensions::GeneralName::RFC822Name(n) = name { if *n == email { return Ok(true); |