Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/parsedbuf/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ macro_rules! parsed {
}
}

// Allow tests to manufacture owned instances with known good values.
// Allow tests and indexes to manufacture owned instances with known good values.
Copy link
Copy Markdown
Collaborator Author

@dcookspi dcookspi Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new_checked() methods were added for these String based types here. But in a later indexes PRs there are several uses of unsafe { ... } around an existing method on these types. It may be we want to fold those into a new_unchecked() method for non-test cases use.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big picture I think we want to be able to say that values in the index were already validated before put into the index and therefore should be trusted without having to re-validate them when reading the index.

However we need a way to introduce changes to spk that may change validation rules. For example let's say we stop allowing '-' in package names. How do we want to put some kind of version number in the index metadata so at runtime the spk process can check that the index conforms to its expectations?

I'm interested in having a way to validate the whole index at once, in a sense, instead of having to validate every individual value found in the index.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added unsafe's to the new_checked methods and updated the callers to use unsafe blocks around them.

Validating the index will be addressed in PR3 (#1338).

#[allow(dead_code)]
impl $owned_type_name {
$crate::paste::paste! {
Expand Down
12 changes: 12 additions & 0 deletions crates/spk-schema/crates/foundation/src/ident_build/build_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ impl BuildId {
Self(chars)
}

/// Make a new BuildId from a list of chars without checking
/// them.
///
/// # Safety
///
/// The caller must make sure the chars represent a valid BuildId.
pub unsafe fn new_unchecked(value: Vec<char>) -> Self {
let mut chars = [0 as char; Self::SIZE];
chars.copy_from_slice(&value);
Self::new(chars)
}

pub fn new_from_bytes(bytes: &[u8]) -> Self {
let encoded = data_encoding::BASE32.encode(bytes);
Self(
Expand Down
11 changes: 11 additions & 0 deletions crates/spk-schema/crates/foundation/src/ident_ops/parsing/ident.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ impl NormalizedVersionString {
pub fn as_str(&self) -> &str {
&self.0
}

/// Make a new NormalizedVersionString from a string without
/// checking its validity.
///
/// # Safety
///
/// The caller must make sure the string is a valid normalized
/// version string.
pub unsafe fn new_unchecked(normalized_version_string: String) -> Self {
Self(normalized_version_string)
}
}

impl std::fmt::Display for NormalizedVersionString {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ use crate::version::parsing::{version, version_str};
mod ident;
mod request;

pub use ident::{IdentParts, IdentPartsBuf, ident_parts, ident_parts_with_components};
pub use ident::{
IdentParts,
IdentPartsBuf,
NormalizedVersionString,
ident_parts,
ident_parts_with_components,
};
pub use request::{range_ident_pkg_name, request_pkg_name_and_version};

pub static KNOWN_REPOSITORY_NAMES: Lazy<HashSet<&'static str>> = Lazy::new(|| {
Expand Down
19 changes: 18 additions & 1 deletion crates/spk-schema/crates/foundation/src/version/compat/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,20 @@ pub enum CompatRule {
Binary,
}

// TODO: CompatRules that allow ::None are used in Compat
// structs. CompatRules that do not allow ::None are used in
// required_compat's in Requests. They should be separate types,
// perhaps one wrapping the other, to clarify where ::None is and is
// not a valid value.
Comment thread
dcookspi marked this conversation as resolved.
impl std::fmt::Display for CompatRule {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
if f.alternate() {
// Request for alternate (long form) names.
f.write_str(match self {
CompatRule::None => unreachable!(),
CompatRule::None => unreachable!(
"CompatRule '{}' fmt() cannot be displayed when using the alternate flag",
self
),
CompatRule::API => API_STR,
CompatRule::Binary => BINARY_STR,
})
Expand Down Expand Up @@ -690,6 +698,15 @@ impl Compat {
}
}

/// Create a compat from the given string without checking it. For
/// internal use only for data from a index.
///
pub fn new_from_compat_str(compat: &str) -> Result<Self> {
// TODO: change this to be more direct once Compat objects are
// directly represented in indexes.
Self::from_str(compat)
}

/// Return true if the two versions are api compatible by this compat rule.
pub fn is_api_compatible(&self, base: &Version, other: &Version) -> Compatibility {
self.check_compat(base, other, CompatRule::API)
Expand Down
6 changes: 6 additions & 0 deletions crates/spk-schema/crates/foundation/src/version/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,12 @@ impl Version {
}
}

/// Make a new Version from a string without checking it.
///
pub fn new_from_version_str(version_str: &str) -> Result<Self> {
Version::try_from(version_str)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thought here, really unsafe?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this, the followup question would be could the current users of Version::new_unchecked just switch to Version::try_from and then this constructor is not needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really unsafe here, I'll take it out.

For the followup question: Yes it could be replaced for this PR. But for one of the the later ones, or a coming improvement (I can't remember which, but one of them wull change the string in index storage to the component number pieces), the new_checked() implementation would change to directly set the fields in the Version struct. I suppose I could take it out and then put it back, but I wanted to put all the new_unchecked() methods in one PR.

Copy link
Copy Markdown
Collaborator Author

@dcookspi dcookspi Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken the unsafe out.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, remove the safety notice and rename to something that doesn't use "unchecked".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this.

}

/// The major version number (first component)
pub fn major(&self) -> u32 {
self.parts.first().copied().unwrap_or_default()
Expand Down
13 changes: 13 additions & 0 deletions crates/spk-schema/crates/foundation/src/version_range/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,19 @@ impl VersionFilter {
}
}

/// Makes a VersionFilter from the given string, without checking
/// that it is valid. This is used by indexing.
///
/// # Safety
///
/// The caller must make sure the string parses ad a valid
/// VersionFilter.
pub unsafe fn new_unchecked(filter_string: &str) -> Self {
VersionFilter::from_str(filter_string).expect(
"A filter string given to VersionFilter::new_unchecked() should be a valid VersionFilter when parsed",
)
}

Comment thread
dcookspi marked this conversation as resolved.
pub fn single(item: VersionRange) -> Self {
let mut filter = Self::default();
filter.rules.insert(item);
Expand Down
11 changes: 11 additions & 0 deletions crates/spk-schema/src/component_embedded_packages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ impl ComponentEmbeddedPackage {
}
}

/// Create a new `ComponentEmbeddedPackage` directly from the
/// given pieces without checking them.
///
/// # Safety
///
/// The caller must make sure the given pieces can make a valid
/// component embedded package.
pub unsafe fn new_unchecked(pkg: OptVersionIdent, components: BTreeSet<Component>) -> Self {
Self { pkg, components }
}

#[inline]
pub fn components(&self) -> &BTreeSet<Component> {
&self.components
Expand Down
34 changes: 31 additions & 3 deletions crates/spk-schema/src/component_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use spk_schema_foundation::name::PkgName;
use spk_schema_foundation::option_map::OptionMap;
use spk_schema_foundation::spec_ops::{ComponentFileMatchMode, HasBuildIdent};

use super::RequirementsList;
use super::{ComponentEmbeddedPackagesList, RequirementsList};
use crate::component_spec_list::ComponentSpecDefaults;
use crate::foundation::ident_component::Component;
use crate::foundation::spec_ops::{ComponentOps, FileMatcher};
Expand All @@ -32,7 +32,7 @@ struct RawComponentSpec {
#[serde(default)]
requirements: super::RequirementsList<PinnedRequest>,
#[serde(default)]
embedded: super::ComponentEmbeddedPackagesList,
embedded: ComponentEmbeddedPackagesList,
#[serde(default)]
file_match_mode: ComponentFileMatchMode,
}
Expand Down Expand Up @@ -72,7 +72,7 @@ pub struct ComponentSpec {
default,
skip_serializing_if = "super::ComponentEmbeddedPackagesList::is_fabricated"
)]
pub embedded: super::ComponentEmbeddedPackagesList,
pub embedded: ComponentEmbeddedPackagesList,

#[serde(default)]
pub file_match_mode: ComponentFileMatchMode,
Expand Down Expand Up @@ -193,6 +193,34 @@ impl ComponentSpec {
file_match_mode,
})
}

/// Create a component spec from parts without checking them.
///
/// # Safety
///
/// The caller must make sure the pieces make up a valid
/// ComponentSpec.
#[allow(clippy::too_many_arguments)]
pub unsafe fn new_unchecked(
name: Component,
uses: Vec<Component>,
// This is needed because some cases call for empty rules and
// others for a single wild card rule.
files: FileMatcher,
options: &OptionMap,
requirements: RequirementsList<PinnedRequest>,
embedded: ComponentEmbeddedPackagesList,
) -> ComponentSpec {
ComponentSpec {
name,
uses,
files,
requirements_with_options: (options.iter(), &requirements).into(),
requirements,
embedded,
file_match_mode: Default::default(),
}
}
}

impl ComponentSpecDefaults for ComponentSpec {
Expand Down
4 changes: 4 additions & 0 deletions crates/spk-schema/src/component_spec_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ impl<ComponentSpecT> ComponentSpecList<ComponentSpecT>
where
ComponentSpecT: ComponentOps,
{
pub fn new(component_specs: Vec<ComponentSpecT>) -> Self {
ComponentSpecList(component_specs)
}

/// Collect the names of all components in this list
pub fn names(&self) -> HashSet<&Component> {
self.iter().map(|i| i.name()).collect()
Expand Down
2 changes: 1 addition & 1 deletion crates/spk-schema/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub mod validation;
pub mod variant;

pub use build_spec::BuildSpec;
pub use component_embedded_packages::ComponentEmbeddedPackagesList;
pub use component_embedded_packages::{ComponentEmbeddedPackage, ComponentEmbeddedPackagesList};
pub use component_spec::ComponentSpec;
pub use component_spec_list::ComponentSpecList;
pub use deprecate::{Deprecate, DeprecateMut};
Expand Down
47 changes: 47 additions & 0 deletions crates/spk-schema/src/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,30 @@ impl VarOpt {
})
}

/// Create a VarOpt from the given pieces without checking them.
///
/// # Safety
///
/// The caller must make sure the pieces make up a valid var opt.
pub unsafe fn new_unchecked<S: AsRef<str>>(
var: S,
inheritance: Inheritance,
compat: Option<Compat>,
required: bool,
value: Option<String>,
) -> Self {
Self {
var: unsafe { OptNameBuf::from_string(var.as_ref().to_string()) },
default: String::default(),
choices: IndexSet::default(),
inheritance,
description: None,
compat,
required,
value,
}
}

pub fn get_value(&self, given: Option<&str>) -> Option<String> {
if let Some(v) = &self.value
&& !v.is_empty()
Expand Down Expand Up @@ -668,6 +692,29 @@ impl PkgOpt {
})
}

/// Create a PkgOpt from the given pieces without checking them.
///
/// # Safety
///
/// The caller must make sure the given pieces can combine into a
/// valid pkg opt.
pub unsafe fn new_unchecked(
name: PkgNameBuf,
components: ComponentBTreeSetBuf,
prerelease_policy: Option<PreReleasePolicy>,
value: Option<String>,
required_compat: Option<CompatRule>,
) -> Self {
Self {
pkg: name,
components,
default: String::default(),
prerelease_policy,
value,
required_compat,
}
}

pub fn get_value(&self, given: Option<&str>) -> Option<String> {
if let Some(v) = &self.value {
Some(v.clone())
Expand Down
11 changes: 11 additions & 0 deletions crates/spk-schema/src/requirements_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,17 @@ impl RequirementsList<RequestWithOptions> {
}
Ok(out)
}

/// Construct a RequirementsList from a RequestWithOptions list,
/// without checking them, or merging them together.
///
/// # Safety
///
/// The caller must make sure the requests are valid and they
/// have already been merged if needed.
pub unsafe fn new_checked(requests_with_options: Vec<RequestWithOptions>) -> Self {
Self(requests_with_options)
}
}

impl<R> std::fmt::Display for RequirementsList<R>
Expand Down
26 changes: 26 additions & 0 deletions crates/spk-schema/src/v0/embedded_package_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,32 @@ impl EmbeddedPackageSpec {
}
}

/// Create a spec for the identified package from the given values
/// without checking them.
///
/// # Safety
///
/// The caller must make sure the pieces combine to make a valid
/// EmbeddedPackageSpec.
Comment on lines +112 to +113
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the need to say that this safety message (and the others) amount to saying "you have responsibilities to use this correctly" but without giving any details on how to use it correctly. I get it that you don't necessarily know what those are. I couldn't tell you what they are right now on the spot either.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the safety messages are a linting requirement. Looking up the specific details is very much left to the caller. I couldn't say what they are for the different cases either - other than make sure you pass valid x, y, z objects in. I suspect the actual details could be found in the parsing methods, maybe?

pub unsafe fn new_unchecked(
ident: BuildIdent,
build: EmbeddedBuildSpec,
requirements_with_options: RequirementsList<RequestWithOptions>,
install: EmbeddedInstallSpec,
) -> Self {
EmbeddedPackageSpec {
pkg: ident,
meta: Meta::default(),
compat: Compat::default(),
deprecated: bool::default(),
sources: Vec::new(),
build,
tests: Vec::new(),
install,
install_requirements_with_options: requirements_with_options,
}
}

/// Read-only access to the build spec
#[inline]
pub fn build(&self) -> &EmbeddedBuildSpec {
Expand Down
Loading