From 7089a79b1e36b007da39e4eae8f305113e598b57 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 11 Jun 2025 22:00:20 +0200 Subject: [PATCH 1/9] I want to get all of this working --- crates/typst-kit/src/package.rs | 2 +- crates/typst-library/src/diag.rs | 9 +++++++-- pending/import1.typ | 1 + pending/import2.typ | 1 + pending/import3.typ | 1 + pending/import4.typ | 1 + pending/import5.typ | 1 + pending/runall.sh | 5 +++++ tests/suite/scripting/import.typ | 13 +++++++++++++ 9 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 pending/import1.typ create mode 100644 pending/import2.typ create mode 100644 pending/import3.typ create mode 100644 pending/import4.typ create mode 100644 pending/import5.typ create mode 100755 pending/runall.sh diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index 584ec83c0..f08eb897c 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -185,7 +185,7 @@ impl PackageStorage { if let Ok(version) = self.determine_latest_version(&spec.versionless()) { return Err(PackageError::VersionNotFound(spec.clone(), version)); } else { - return Err(PackageError::NotFound(spec.clone())); + return Err(PackageError::NotFound(spec.clone(), Some(eco_format!("attempted to download")))); } } Err(err) => { diff --git a/crates/typst-library/src/diag.rs b/crates/typst-library/src/diag.rs index 41b92ed65..6a55a21da 100644 --- a/crates/typst-library/src/diag.rs +++ b/crates/typst-library/src/diag.rs @@ -523,7 +523,9 @@ pub type PackageResult = Result; #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum PackageError { /// The specified package does not exist. - NotFound(PackageSpec), + /// Optionally provides information on where we tried to find the package, + /// defaults to simply "searched for" if absent. + NotFound(PackageSpec, Option), /// The specified package found, but the version does not exist. VersionNotFound(PackageSpec, PackageVersion), /// Failed to retrieve the package through the network. @@ -539,9 +541,12 @@ impl std::error::Error for PackageError {} impl Display for PackageError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self { - Self::NotFound(spec) => { + Self::NotFound(spec, None) => { write!(f, "package not found (searched for {spec})",) } + Self::NotFound(spec, Some(attempted)) => { + write!(f, "package not found ({attempted} {spec})",) + } Self::VersionNotFound(spec, latest) => { write!( f, diff --git a/pending/import1.typ b/pending/import1.typ new file mode 100644 index 000000000..36894382d --- /dev/null +++ b/pending/import1.typ @@ -0,0 +1 @@ +#import "@local/mypkg:0.0.1" diff --git a/pending/import2.typ b/pending/import2.typ new file mode 100644 index 000000000..7d1515e1a --- /dev/null +++ b/pending/import2.typ @@ -0,0 +1 @@ +#import "@nope/mypkg:0.0.1" diff --git a/pending/import3.typ b/pending/import3.typ new file mode 100644 index 000000000..5010e4daa --- /dev/null +++ b/pending/import3.typ @@ -0,0 +1 @@ +#import "@local/penpo:5.0.0" diff --git a/pending/import4.typ b/pending/import4.typ new file mode 100644 index 000000000..cdd6c755a --- /dev/null +++ b/pending/import4.typ @@ -0,0 +1 @@ +#import "@local/noperm:0.0.1" diff --git a/pending/import5.typ b/pending/import5.typ new file mode 100644 index 000000000..f354b64e3 --- /dev/null +++ b/pending/import5.typ @@ -0,0 +1 @@ +#import "@preview/nope:0.0.1" diff --git a/pending/runall.sh b/pending/runall.sh new file mode 100755 index 000000000..bfe75c63d --- /dev/null +++ b/pending/runall.sh @@ -0,0 +1,5 @@ +#! /usr/bin/env bash + +for file in $(ls pending/*.typ); do + cargo run -- compile $file +done diff --git a/tests/suite/scripting/import.typ b/tests/suite/scripting/import.typ index 382e444cc..1866af3d6 100644 --- a/tests/suite/scripting/import.typ +++ b/tests/suite/scripting/import.typ @@ -483,3 +483,16 @@ This is never reached. --- import-from-file-package-lookalike --- // Error: 9-28 file not found (searched at tests/suite/scripting/#test/mypkg:1.0.0) #import "#test/mypkg:1.0.0": * + +--- import-from-package-namespace-not-found --- +// Error: todo +#import "@missingdir/test:0.0.0": * + +--- import-from-package-not-found --- +// Error: todo +#import "@test/missingpkg:0.0.0": * + +--- import-from-package-version-not-found --- +// Error: todo +#import "@test/mypkg:5.0.0": * + From 407b006dc659faf3ea535fff56fa32cf3054e654 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 11 Jun 2025 22:32:10 +0200 Subject: [PATCH 2/9] first attempt --- crates/typst-kit/src/package.rs | 51 +++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index f08eb897c..d0fb0ffef 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -87,13 +87,9 @@ impl PackageStorage { ) -> PackageResult { let subdir = format!("{}/{}/{}", spec.namespace, spec.name, spec.version); - if let Some(packages_dir) = &self.package_path { - let dir = packages_dir.join(&subdir); - if dir.exists() { - return Ok(dir); - } - } - + // QUESTION: is it sometimes the case that both package_path + // and package_cache_path are Some? Is it a problem if we change + // this order? if let Some(cache_dir) = &self.package_cache_path { let dir = cache_dir.join(&subdir); if dir.exists() { @@ -102,14 +98,39 @@ impl PackageStorage { // Download from network if it doesn't exist yet. if spec.namespace == DEFAULT_NAMESPACE { - self.download_package(spec, cache_dir, progress)?; - if dir.exists() { - return Ok(dir); - } + return self.download_package(spec, cache_dir, progress); } } - Err(PackageError::NotFound(spec.clone())) + // In all paths from now, we either succeed or fail with a NotFound. + let not_found = |msg| Err(PackageError::NotFound(spec.clone(), msg)); + + let Some(packages_dir) = &self.package_path else { + // QUESTION: when is this code path hit? + return not_found(None); + }; + // We try the fast path + let dir = packages_dir.join(&subdir); + if dir.exists() { + return Ok(dir); + } + // Fast path failed. Instead we try to diagnose the issue by going + // down the subdirectories of the expected path one at a time. + let namespace_dir = packages_dir.join(format!("{}", spec.namespace)); + if !namespace_dir.exists() { + return not_found(Some(eco_format!("did not find namespace '{}' while searching at {} for", spec.namespace, namespace_dir.display()))); + } + let package_dir = namespace_dir.join(format!("{}", spec.name)); + if !package_dir.exists() { + return not_found(Some(eco_format!("did not find package '{}' while searching at {} for", spec.name, package_dir.display()))); + } + let version_dir = package_dir.join(format!("{}", spec.version)); + if !version_dir.exists() { + return not_found(Some(eco_format!("did not find version {} while searching at {} for", spec.version, version_dir.display()))); + } + // We'll never hit this path as long as subdir is indeed namespace/name/version. + // What's Typst's policy on `unreachable`? + unreachable!() } /// Tries to determine the latest version of a package. @@ -171,7 +192,7 @@ impl PackageStorage { spec: &PackageSpec, cache_dir: &Path, progress: &mut dyn Progress, - ) -> PackageResult<()> { + ) -> PackageResult { assert_eq!(spec.namespace, DEFAULT_NAMESPACE); let url = format!( @@ -235,8 +256,8 @@ impl PackageStorage { // broken packages still occur even with the rename safeguard, we might // consider more complex solutions like file locking or checksums. match fs::rename(&tempdir, &package_dir) { - Ok(()) => Ok(()), - Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(()), + Ok(()) => Ok(package_dir), + Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(package_dir), Err(err) => Err(error("failed to move downloaded package directory", err)), } } From 3292aeaaa1969fc3115a19bad9d9cbcc7fba95ef Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 11 Jun 2025 22:43:14 +0200 Subject: [PATCH 3/9] What's with the namespacing on the unit tests ? --- tests/suite/scripting/import.typ | 6 +++--- tests/testit | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) create mode 100644 tests/testit diff --git a/tests/suite/scripting/import.typ b/tests/suite/scripting/import.typ index 1866af3d6..9fbd889fa 100644 --- a/tests/suite/scripting/import.typ +++ b/tests/suite/scripting/import.typ @@ -485,14 +485,14 @@ This is never reached. #import "#test/mypkg:1.0.0": * --- import-from-package-namespace-not-found --- -// Error: todo +// Error: 9-33 package not found #import "@missingdir/test:0.0.0": * --- import-from-package-not-found --- -// Error: todo +// Error: 9-33 package not found #import "@test/missingpkg:0.0.0": * --- import-from-package-version-not-found --- -// Error: todo +// Error: 9-28 package not found #import "@test/mypkg:5.0.0": * diff --git a/tests/testit b/tests/testit new file mode 100644 index 000000000..2be10fee3 --- /dev/null +++ b/tests/testit @@ -0,0 +1,2 @@ +alias testit="cargo test --workspace --test tests --" + From d148f3ebe6cbc81f36d5639c57b2ca1ffb51fe39 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Thu, 24 Jul 2025 09:27:41 +0200 Subject: [PATCH 4/9] those were temporary files --- crates/typst-kit/src/package.rs | 70 ++++++++++++++++++++------------ crates/typst-library/src/diag.rs | 30 +++++++------- pending/import1.typ | 1 - pending/import2.typ | 1 - pending/import3.typ | 1 - pending/import4.typ | 1 - pending/import5.typ | 1 - pending/runall.sh | 5 --- 8 files changed, 58 insertions(+), 52 deletions(-) delete mode 100644 pending/import1.typ delete mode 100644 pending/import2.typ delete mode 100644 pending/import3.typ delete mode 100644 pending/import4.typ delete mode 100644 pending/import5.typ delete mode 100755 pending/runall.sh diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index d0fb0ffef..3bfb46c3e 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -87,9 +87,15 @@ impl PackageStorage { ) -> PackageResult { let subdir = format!("{}/{}/{}", spec.namespace, spec.name, spec.version); - // QUESTION: is it sometimes the case that both package_path - // and package_cache_path are Some? Is it a problem if we change - // this order? + // By default, search for the package locally + if let Some(packages_dir) = &self.package_path { + let dir = packages_dir.join(&subdir); + if dir.exists() { + return Ok(dir); + } + } + + // As a fallback, look into the cache and possibly download from network. if let Some(cache_dir) = &self.package_cache_path { let dir = cache_dir.join(&subdir); if dir.exists() { @@ -102,38 +108,42 @@ impl PackageStorage { } } - // In all paths from now, we either succeed or fail with a NotFound. + // None of the strategies above found the package, so all code paths + // from now on fail. The rest of the function is only to determine the + // cause of the failure. let not_found = |msg| Err(PackageError::NotFound(spec.clone(), msg)); let Some(packages_dir) = &self.package_path else { - // QUESTION: when is this code path hit? - return not_found(None); + return not_found(eco_format!("cannot access local package storage")); }; - // We try the fast path - let dir = packages_dir.join(&subdir); - if dir.exists() { - return Ok(dir); - } // Fast path failed. Instead we try to diagnose the issue by going // down the subdirectories of the expected path one at a time. let namespace_dir = packages_dir.join(format!("{}", spec.namespace)); if !namespace_dir.exists() { - return not_found(Some(eco_format!("did not find namespace '{}' while searching at {} for", spec.namespace, namespace_dir.display()))); + return not_found(eco_format!( + "namespace @{} should be located at {}", + spec.namespace, + namespace_dir.display() + )); } let package_dir = namespace_dir.join(format!("{}", spec.name)); if !package_dir.exists() { - return not_found(Some(eco_format!("did not find package '{}' while searching at {} for", spec.name, package_dir.display()))); + return not_found(eco_format!( + "{} does not have package '{}'", + namespace_dir.display(), + spec.name + )); } - let version_dir = package_dir.join(format!("{}", spec.version)); - if !version_dir.exists() { - return not_found(Some(eco_format!("did not find version {} while searching at {} for", spec.version, version_dir.display()))); - } - // We'll never hit this path as long as subdir is indeed namespace/name/version. - // What's Typst's policy on `unreachable`? - unreachable!() + let latest = self.determine_latest_version(&spec.versionless()).ok(); + Err(PackageError::VersionNotFound( + spec.clone(), + latest, + eco_format!("{}", namespace_dir.display()), + )) } /// Tries to determine the latest version of a package. + /// TODO improve here too pub fn determine_latest_version( &self, spec: &VersionlessPackageSpec, @@ -195,18 +205,26 @@ impl PackageStorage { ) -> PackageResult { assert_eq!(spec.namespace, DEFAULT_NAMESPACE); - let url = format!( - "{DEFAULT_REGISTRY}/{DEFAULT_NAMESPACE}/{}-{}.tar.gz", - spec.name, spec.version - ); + let namespace_url = format!("{DEFAULT_REGISTRY}/{DEFAULT_NAMESPACE}"); + let url = format!("{namespace_url}/{}-{}.tar.gz", spec.name, spec.version); let data = match self.downloader.download_with_progress(&url, progress) { Ok(data) => data, Err(ureq::Error::Status(404, _)) => { if let Ok(version) = self.determine_latest_version(&spec.versionless()) { - return Err(PackageError::VersionNotFound(spec.clone(), version)); + return Err(PackageError::VersionNotFound( + spec.clone(), + Some(version), + eco_format!("{namespace_url}"), + )); } else { - return Err(PackageError::NotFound(spec.clone(), Some(eco_format!("attempted to download")))); + return Err(PackageError::NotFound( + spec.clone(), + eco_format!( + "{namespace_url} does not have package '{}'", + spec.name + ), + )); } } Err(err) => { diff --git a/crates/typst-library/src/diag.rs b/crates/typst-library/src/diag.rs index 6a55a21da..4c7424875 100644 --- a/crates/typst-library/src/diag.rs +++ b/crates/typst-library/src/diag.rs @@ -519,15 +519,15 @@ pub type PackageResult = Result; /// An error that occurred while trying to load a package. /// -/// Some variants have an optional string can give more details, if available. +/// Some variants have an optional string that can give more details, if available. #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum PackageError { /// The specified package does not exist. - /// Optionally provides information on where we tried to find the package, - /// defaults to simply "searched for" if absent. - NotFound(PackageSpec, Option), + /// Additionally provides information on where we tried to find the package. + NotFound(PackageSpec, EcoString), /// The specified package found, but the version does not exist. - VersionNotFound(PackageSpec, PackageVersion), + /// TODO: make the registry part of the error better typed + VersionNotFound(PackageSpec, Option, EcoString), /// Failed to retrieve the package through the network. NetworkFailed(Option), /// The package archive was malformed. @@ -541,18 +541,16 @@ impl std::error::Error for PackageError {} impl Display for PackageError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self { - Self::NotFound(spec, None) => { - write!(f, "package not found (searched for {spec})",) + Self::NotFound(spec, detail) => { + write!(f, "package not found: {detail} (searching for {spec})",) } - Self::NotFound(spec, Some(attempted)) => { - write!(f, "package not found ({attempted} {spec})",) - } - Self::VersionNotFound(spec, latest) => { - write!( - f, - "package found, but version {} does not exist (latest is {})", - spec.version, latest, - ) + Self::VersionNotFound(spec, latest, registry) => { + write!(f, "package found, but version {} does not exist", spec.version,)?; + if let Some(version) = latest { + write!(f, " (latest version provided by {registry} is {version})") + } else { + write!(f, " ({registry} contains no versions for this package)") + } } Self::NetworkFailed(Some(err)) => { write!(f, "failed to download package ({err})") diff --git a/pending/import1.typ b/pending/import1.typ deleted file mode 100644 index 36894382d..000000000 --- a/pending/import1.typ +++ /dev/null @@ -1 +0,0 @@ -#import "@local/mypkg:0.0.1" diff --git a/pending/import2.typ b/pending/import2.typ deleted file mode 100644 index 7d1515e1a..000000000 --- a/pending/import2.typ +++ /dev/null @@ -1 +0,0 @@ -#import "@nope/mypkg:0.0.1" diff --git a/pending/import3.typ b/pending/import3.typ deleted file mode 100644 index 5010e4daa..000000000 --- a/pending/import3.typ +++ /dev/null @@ -1 +0,0 @@ -#import "@local/penpo:5.0.0" diff --git a/pending/import4.typ b/pending/import4.typ deleted file mode 100644 index cdd6c755a..000000000 --- a/pending/import4.typ +++ /dev/null @@ -1 +0,0 @@ -#import "@local/noperm:0.0.1" diff --git a/pending/import5.typ b/pending/import5.typ deleted file mode 100644 index f354b64e3..000000000 --- a/pending/import5.typ +++ /dev/null @@ -1 +0,0 @@ -#import "@preview/nope:0.0.1" diff --git a/pending/runall.sh b/pending/runall.sh deleted file mode 100755 index bfe75c63d..000000000 --- a/pending/runall.sh +++ /dev/null @@ -1,5 +0,0 @@ -#! /usr/bin/env bash - -for file in $(ls pending/*.typ); do - cargo run -- compile $file -done From 3846955ce6aaa56be55418fa802e0a4d73352dc2 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Thu, 12 Jun 2025 15:22:51 +0200 Subject: [PATCH 5/9] Report path on a FileError --- crates/typst-cli/src/world.rs | 6 +++-- crates/typst-kit/src/package.rs | 1 - crates/typst-library/src/diag.rs | 39 +++++++++++++++++++++----------- crates/typst/src/lib.rs | 2 +- tests/testit | 2 -- 5 files changed, 31 insertions(+), 19 deletions(-) delete mode 100644 tests/testit diff --git a/crates/typst-cli/src/world.rs b/crates/typst-cli/src/world.rs index 95bee235c..3dd0a6337 100644 --- a/crates/typst-cli/src/world.rs +++ b/crates/typst-cli/src/world.rs @@ -404,7 +404,9 @@ fn system_path( // Join the path to the root. If it tries to escape, deny // access. Note: It can still escape via symlinks. - id.vpath().resolve(root).ok_or(FileError::AccessDenied) + id.vpath() + .resolve(root) + .ok_or_else(|| FileError::AccessDenied(id.vpath().as_rootless_path().into())) } /// Reads a file from a `FileId`. @@ -427,7 +429,7 @@ fn read( fn read_from_disk(path: &Path) -> FileResult> { let f = |e| FileError::from_io(e, path); if fs::metadata(path).map_err(f)?.is_dir() { - Err(FileError::IsDirectory) + Err(FileError::IsDirectory(path.into())) } else { fs::read(path).map_err(f) } diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index 3bfb46c3e..b51c0a7c8 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -143,7 +143,6 @@ impl PackageStorage { } /// Tries to determine the latest version of a package. - /// TODO improve here too pub fn determine_latest_version( &self, spec: &VersionlessPackageSpec, diff --git a/crates/typst-library/src/diag.rs b/crates/typst-library/src/diag.rs index 4c7424875..2214b63f4 100644 --- a/crates/typst-library/src/diag.rs +++ b/crates/typst-library/src/diag.rs @@ -440,13 +440,13 @@ pub enum FileError { /// A file was not found at this path. NotFound(PathBuf), /// A file could not be accessed. - AccessDenied, + AccessDenied(PathBuf), /// A directory was found, but a file was expected. - IsDirectory, + IsDirectory(PathBuf), /// The file is not a Typst source file, but should have been. - NotSource, + NotSource(PathBuf), /// The file was not valid UTF-8, but should have been. - InvalidUtf8, + InvalidUtf8(Option), /// The package the file is part of could not be loaded. Package(PackageError), /// Another error. @@ -460,11 +460,11 @@ impl FileError { pub fn from_io(err: io::Error, path: &Path) -> Self { match err.kind() { io::ErrorKind::NotFound => Self::NotFound(path.into()), - io::ErrorKind::PermissionDenied => Self::AccessDenied, + io::ErrorKind::PermissionDenied => Self::AccessDenied(path.into()), io::ErrorKind::InvalidData if err.to_string().contains("stream did not contain valid UTF-8") => { - Self::InvalidUtf8 + Self::InvalidUtf8(Some(path.into())) } _ => Self::Other(Some(eco_format!("{err}"))), } @@ -479,10 +479,19 @@ impl Display for FileError { Self::NotFound(path) => { write!(f, "file not found (searched at {})", path.display()) } - Self::AccessDenied => f.pad("failed to load file (access denied)"), - Self::IsDirectory => f.pad("failed to load file (is a directory)"), - Self::NotSource => f.pad("not a typst source file"), - Self::InvalidUtf8 => f.pad("file is not valid utf-8"), + Self::AccessDenied(path) => { + write!(f, "failed to load file {} (access denied)", path.display()) + } + Self::IsDirectory(path) => { + write!(f, "failed to load file {} (is a directory)", path.display()) + } + Self::NotSource(path) => { + write!(f, "{} is not a typst source file", path.display()) + } + Self::InvalidUtf8(Some(path)) => { + write!(f, "file {} is not valid utf-8", path.display()) + } + Self::InvalidUtf8(None) => f.pad("file is not valid utf-8"), Self::Package(error) => error.fmt(f), Self::Other(Some(err)) => write!(f, "failed to load file ({err})"), Self::Other(None) => f.pad("failed to load file"), @@ -492,13 +501,13 @@ impl Display for FileError { impl From for FileError { fn from(_: Utf8Error) -> Self { - Self::InvalidUtf8 + Self::InvalidUtf8(None) } } impl From for FileError { fn from(_: FromUtf8Error) -> Self { - Self::InvalidUtf8 + Self::InvalidUtf8(None) } } @@ -545,7 +554,11 @@ impl Display for PackageError { write!(f, "package not found: {detail} (searching for {spec})",) } Self::VersionNotFound(spec, latest, registry) => { - write!(f, "package found, but version {} does not exist", spec.version,)?; + write!( + f, + "package '{}' found, but version {} does not exist", + spec.name, spec.version + )?; if let Some(version) = latest { write!(f, " (latest version provided by {registry} is {version})") } else { diff --git a/crates/typst/src/lib.rs b/crates/typst/src/lib.rs index a6bb4fe38..9d388f3bb 100644 --- a/crates/typst/src/lib.rs +++ b/crates/typst/src/lib.rs @@ -190,7 +190,7 @@ fn hint_invalid_main_file( file_error: FileError, input: FileId, ) -> EcoVec { - let is_utf8_error = matches!(file_error, FileError::InvalidUtf8); + let is_utf8_error = matches!(file_error, FileError::InvalidUtf8(_)); let mut diagnostic = SourceDiagnostic::error(Span::detached(), EcoString::from(file_error)); diff --git a/tests/testit b/tests/testit deleted file mode 100644 index 2be10fee3..000000000 --- a/tests/testit +++ /dev/null @@ -1,2 +0,0 @@ -alias testit="cargo test --workspace --test tests --" - From ddaec8aa85c2b75599ab3067f0cd8b010435ded8 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Thu, 12 Jun 2025 15:53:36 +0200 Subject: [PATCH 6/9] Update in tests/ too the PathBuf in FileError --- tests/src/world.rs | 6 ++++-- tests/suite/scripting/import.typ | 16 ++-------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/tests/src/world.rs b/tests/src/world.rs index 6fa1cf60f..03bbb20b1 100644 --- a/tests/src/world.rs +++ b/tests/src/world.rs @@ -172,7 +172,9 @@ pub(crate) fn system_path(id: FileId) -> FileResult { None => PathBuf::new(), }; - id.vpath().resolve(&root).ok_or(FileError::AccessDenied) + id.vpath() + .resolve(&root) + .ok_or_else(|| FileError::AccessDenied(id.vpath().as_rootless_path().into())) } /// Read a file. @@ -186,7 +188,7 @@ pub(crate) fn read(path: &Path) -> FileResult> { let f = |e| FileError::from_io(e, path); if fs::metadata(path).map_err(f)?.is_dir() { - Err(FileError::IsDirectory) + Err(FileError::IsDirectory(path.into())) } else { fs::read(path).map(Cow::Owned).map_err(f) } diff --git a/tests/suite/scripting/import.typ b/tests/suite/scripting/import.typ index 9fbd889fa..f094042bc 100644 --- a/tests/suite/scripting/import.typ +++ b/tests/suite/scripting/import.typ @@ -302,11 +302,11 @@ #import 5 as x --- import-from-string-invalid --- -// Error: 9-11 failed to load file (is a directory) +// Error: 9-11 failed to load file tests/suite/scripting (is a directory) #import "": name --- import-from-string-renamed-invalid --- -// Error: 9-11 failed to load file (is a directory) +// Error: 9-11 failed to load file tests/suite/scripting (is a directory) #import "" as x --- import-file-not-found-invalid --- @@ -484,15 +484,3 @@ This is never reached. // Error: 9-28 file not found (searched at tests/suite/scripting/#test/mypkg:1.0.0) #import "#test/mypkg:1.0.0": * ---- import-from-package-namespace-not-found --- -// Error: 9-33 package not found -#import "@missingdir/test:0.0.0": * - ---- import-from-package-not-found --- -// Error: 9-33 package not found -#import "@test/missingpkg:0.0.0": * - ---- import-from-package-version-not-found --- -// Error: 9-28 package not found -#import "@test/mypkg:5.0.0": * - From 3b84ce91e330b9a67cd948b9e08065cd8d6f1202 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Tue, 24 Jun 2025 14:42:23 +0200 Subject: [PATCH 7/9] One comment out of date --- crates/typst-kit/src/package.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index b51c0a7c8..5b5c23c42 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -111,13 +111,13 @@ impl PackageStorage { // None of the strategies above found the package, so all code paths // from now on fail. The rest of the function is only to determine the // cause of the failure. + // We try `namespace/` then `namespace/name/` then `namespace/name/version/` + // and see where the first error occurs. let not_found = |msg| Err(PackageError::NotFound(spec.clone(), msg)); let Some(packages_dir) = &self.package_path else { return not_found(eco_format!("cannot access local package storage")); }; - // Fast path failed. Instead we try to diagnose the issue by going - // down the subdirectories of the expected path one at a time. let namespace_dir = packages_dir.join(format!("{}", spec.namespace)); if !namespace_dir.exists() { return not_found(eco_format!( From 26f0a1358cc4a6e94be39586acba48f933a3ba50 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Thu, 24 Jul 2025 09:25:58 +0200 Subject: [PATCH 8/9] Move parts of the messages to hints --- crates/typst-cli/src/init.rs | 5 +- crates/typst-kit/src/package.rs | 6 +- crates/typst-library/src/diag.rs | 162 +++++++++++++++++++------------ crates/typst/src/lib.rs | 7 +- 4 files changed, 111 insertions(+), 69 deletions(-) diff --git a/crates/typst-cli/src/init.rs b/crates/typst-cli/src/init.rs index 9a77fb470..2806c7eae 100644 --- a/crates/typst-cli/src/init.rs +++ b/crates/typst-cli/src/init.rs @@ -29,8 +29,9 @@ pub fn init(command: &InitCommand) -> StrResult<()> { })?; // Find or download the package. - let package_path = - package_storage.prepare_package(&spec, &mut PrintDownload(&spec))?; + let package_path = package_storage + .prepare_package(&spec, &mut PrintDownload(&spec)) + .map_err(|e| eco_format!("{e}"))?; // Parse the manifest. let manifest = parse_manifest(&package_path)?; diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index 5b5c23c42..0a794d49b 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -121,7 +121,7 @@ impl PackageStorage { let namespace_dir = packages_dir.join(format!("{}", spec.namespace)); if !namespace_dir.exists() { return not_found(eco_format!( - "namespace @{} should be located at {}", + "the namespace @{} should be located at {}", spec.namespace, namespace_dir.display() )); @@ -129,7 +129,7 @@ impl PackageStorage { let package_dir = namespace_dir.join(format!("{}", spec.name)); if !package_dir.exists() { return not_found(eco_format!( - "{} does not have package '{}'", + "the registry at {} does not have package '{}'", namespace_dir.display(), spec.name )); @@ -220,7 +220,7 @@ impl PackageStorage { return Err(PackageError::NotFound( spec.clone(), eco_format!( - "{namespace_url} does not have package '{}'", + "the registry at {namespace_url} does not have package '{}'", spec.name ), )); diff --git a/crates/typst-library/src/diag.rs b/crates/typst-library/src/diag.rs index 2214b63f4..682ef2650 100644 --- a/crates/typst-library/src/diag.rs +++ b/crates/typst-library/src/diag.rs @@ -315,6 +315,24 @@ impl Trace for SourceResult { /// A result type with a string error message. pub type StrResult = Result; +/// A common trait to add a span to any error +pub trait ErrAt { + /// Attach a span to a more specialized error and turn it into + /// a generic error, optionally with hints. + fn err_at(self, span: Span) -> SourceDiagnostic; +} + +/// Blanket implementation for anything that doesn't implement its own +/// convertion to a SourceDiagnostic. +impl ErrAt for S +where + S: Into, +{ + fn err_at(self, span: Span) -> SourceDiagnostic { + SourceDiagnostic::error(span, self) + } +} + /// Convert a [`StrResult`] or [`HintedStrResult`] to a [`SourceResult`] by /// adding span information. pub trait At { @@ -324,18 +342,10 @@ pub trait At { impl At for Result where - S: Into, + S: ErrAt, { fn at(self, span: Span) -> SourceResult { - self.map_err(|message| { - let mut diagnostic = SourceDiagnostic::error(span, message); - if diagnostic.message.contains("(access denied)") { - diagnostic.hint("cannot read file outside of project root"); - diagnostic - .hint("you can adjust the project root with the --root argument"); - } - eco_vec![diagnostic] - }) + self.map_err(|e| eco_vec![e.err_at(span)]) } } @@ -469,6 +479,29 @@ impl FileError { _ => Self::Other(Some(eco_format!("{err}"))), } } + + fn write_hints(&self) -> EcoVec { + match self { + Self::NotFound(_) => eco_vec![], + Self::AccessDenied(_) => { + eco_vec![ + eco_format!("cannot read file outside of project root"), + eco_format!( + "you can adjust the project root with the --root argument" + ), + ] + } + Self::IsDirectory(_) => eco_vec![], + Self::NotSource(_) => eco_vec![], + Self::InvalidUtf8(Some(path)) => { + eco_vec![eco_format!("tried to read {}", path.display())] + } + Self::InvalidUtf8(None) => eco_vec![], + Self::Package(error) => error.write_hints(), + Self::Other(Some(err)) => eco_vec![eco_format!("{err}")], + Self::Other(None) => eco_vec![], + } + } } impl std::error::Error for FileError {} @@ -479,26 +512,31 @@ impl Display for FileError { Self::NotFound(path) => { write!(f, "file not found (searched at {})", path.display()) } - Self::AccessDenied(path) => { - write!(f, "failed to load file {} (access denied)", path.display()) - } - Self::IsDirectory(path) => { - write!(f, "failed to load file {} (is a directory)", path.display()) - } Self::NotSource(path) => { write!(f, "{} is not a typst source file", path.display()) } - Self::InvalidUtf8(Some(path)) => { - write!(f, "file {} is not valid utf-8", path.display()) + Self::InvalidUtf8(_) => write!(f, "file is not valid utf-8"), + Self::IsDirectory(path) => { + write!(f, "failed to load file {} (is a directory)", path.display()) } - Self::InvalidUtf8(None) => f.pad("file is not valid utf-8"), - Self::Package(error) => error.fmt(f), - Self::Other(Some(err)) => write!(f, "failed to load file ({err})"), - Self::Other(None) => f.pad("failed to load file"), + Self::AccessDenied(path) => { + write!(f, "failed to load file {} (access denied)", path.display()) + } + Self::Other(_) => { + write!(f, "failed to load file") + } + Self::Package(error) => write!(f, "{error}"), } } } +impl ErrAt for FileError { + fn err_at(self, span: Span) -> SourceDiagnostic { + let hints = self.write_hints(); + SourceDiagnostic::error(span, eco_format!("{}", self)).with_hints(hints) + } +} + impl From for FileError { fn from(_: Utf8Error) -> Self { Self::InvalidUtf8(None) @@ -517,12 +555,6 @@ impl From for FileError { } } -impl From for EcoString { - fn from(err: FileError) -> Self { - eco_format!("{err}") - } -} - /// A result type with a package-related error. pub type PackageResult = Result; @@ -545,48 +577,58 @@ pub enum PackageError { Other(Option), } +impl PackageError { + fn write_hints(&self) -> EcoVec { + match self { + Self::NotFound(_, detail) => { + eco_vec![eco_format!("{detail}")] + } + Self::VersionNotFound(spec, latest, registry) => { + if let Some(version) = latest { + eco_vec![ + eco_format!( + "the package {} exists, but not with version {}", + spec.name, + spec.version + ), + eco_format!( + "the registry at {registry} provides up to version {version}" + ), + ] + } else { + eco_vec![eco_format!( + "the registry at {registry} contains no versions for this package" + )] + } + } + Self::NetworkFailed(Some(err)) => eco_vec![eco_format!("{err}")], + Self::NetworkFailed(None) => eco_vec![], + Self::MalformedArchive(Some(err)) => eco_vec![eco_format!("{err}")], + Self::MalformedArchive(None) => { + eco_vec![eco_format!("the archive is malformed")] + } + Self::Other(Some(err)) => eco_vec![eco_format!("{err}")], + Self::Other(None) => eco_vec![], + } + } +} + impl std::error::Error for PackageError {} impl Display for PackageError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self { - Self::NotFound(spec, detail) => { - write!(f, "package not found: {detail} (searching for {spec})",) + Self::NotFound(spec, _) => write!(f, "package {spec} not found"), + Self::VersionNotFound(spec, _, _) => { + write!(f, "package version {spec} not found") } - Self::VersionNotFound(spec, latest, registry) => { - write!( - f, - "package '{}' found, but version {} does not exist", - spec.name, spec.version - )?; - if let Some(version) = latest { - write!(f, " (latest version provided by {registry} is {version})") - } else { - write!(f, " ({registry} contains no versions for this package)") - } - } - Self::NetworkFailed(Some(err)) => { - write!(f, "failed to download package ({err})") - } - Self::NetworkFailed(None) => f.pad("failed to download package"), - Self::MalformedArchive(Some(err)) => { - write!(f, "failed to decompress package ({err})") - } - Self::MalformedArchive(None) => { - f.pad("failed to decompress package (archive malformed)") - } - Self::Other(Some(err)) => write!(f, "failed to load package ({err})"), - Self::Other(None) => f.pad("failed to load package"), + Self::NetworkFailed(_) => write!(f, "failed to download package"), + Self::MalformedArchive(_) => write!(f, "failed to decompress package"), + Self::Other(_) => f.pad("failed to load package"), } } } -impl From for EcoString { - fn from(err: PackageError) -> Self { - eco_format!("{err}") - } -} - /// A result type with a data-loading-related error. pub type LoadResult = Result; diff --git a/crates/typst/src/lib.rs b/crates/typst/src/lib.rs index 9d388f3bb..8d37f8dae 100644 --- a/crates/typst/src/lib.rs +++ b/crates/typst/src/lib.rs @@ -41,9 +41,9 @@ pub use typst_utils as utils; use std::collections::HashSet; use comemo::{Track, Tracked, Validate}; -use ecow::{eco_format, eco_vec, EcoString, EcoVec}; +use ecow::{eco_format, eco_vec, EcoVec}; use typst_library::diag::{ - bail, warning, FileError, SourceDiagnostic, SourceResult, Warned, + bail, warning, ErrAt, FileError, SourceDiagnostic, SourceResult, Warned, }; use typst_library::engine::{Engine, Route, Sink, Traced}; use typst_library::foundations::{StyleChain, Styles, Value}; @@ -191,8 +191,7 @@ fn hint_invalid_main_file( input: FileId, ) -> EcoVec { let is_utf8_error = matches!(file_error, FileError::InvalidUtf8(_)); - let mut diagnostic = - SourceDiagnostic::error(Span::detached(), EcoString::from(file_error)); + let mut diagnostic = file_error.err_at(Span::detached()); // Attempt to provide helpful hints for UTF-8 errors. Perhaps the user // mistyped the filename. For example, they could have written "file.pdf" From 5d792c46ca8a5006ecc19de515fe5bd952797da0 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Thu, 24 Jul 2025 10:10:39 +0200 Subject: [PATCH 9/9] Fix local issue with Rust version --- crates/typst/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/typst/src/lib.rs b/crates/typst/src/lib.rs index 16f7e6051..f6204d9eb 100644 --- a/crates/typst/src/lib.rs +++ b/crates/typst/src/lib.rs @@ -42,13 +42,13 @@ use std::collections::HashSet; use std::sync::LazyLock; use comemo::{Track, Tracked, Validate}; -use ecow::{eco_format, eco_vec, EcoVec}; +use ecow::{EcoVec, eco_format, eco_vec}; +use typst_html::HtmlDocument; use typst_library::diag::{ - bail, warning, ErrAt, FileError, SourceDiagnostic, SourceResult, Warned + ErrAt, FileError, SourceDiagnostic, SourceResult, Warned, bail, warning, }; use typst_library::engine::{Engine, Route, Sink, Traced}; use typst_library::foundations::{NativeRuleMap, StyleChain, Styles, Value}; -use typst_library::html::HtmlDocument; use typst_library::introspection::Introspector; use typst_library::layout::PagedDocument; use typst_library::routines::Routines;