Indexes 2: Add `new_unchecked()` constructors to spk schema objects by dcookspi · Pull Request #1337 · spkenv/spk

dcookspi · 2026-03-19T22:01:04Z

Note: for info on benefits of indexing for spk solves see #1340 (5 of 5). Maybe start there and work back down to this PR if you prefer to review PRs top down.

This adds new_unchecked() constructor methods to various spk schema objects. These allow direct creation of those objects from other existing object pieces, e.g. from other pieces of those objects in index data. This is one of the changes that supports adding indexes and index based packages to Spk repositories. It allows indexes to avoid reparsing data from text for some objects.

This is 2 of 5 chained PRs for adding indexes to spk solves:

dcookspi · 2026-03-19T22:03:16Z

crates/spk-schema/crates/foundation/src/version/compat/mod.rs

+// 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.


This refers to something that is worked around in later Indexing changes. But it could also be something that gets addressed in future, by separating the two uses into distinct types.

I do like the idea of there being a newtype that wraps the other and giving them their own distinct Display implementation, to get away from the "magic" alternate() mechanism.

Turned this into an issue #1347

crates/spk-schema/crates/foundation/src/version_range/mod.rs

dcookspi · 2026-03-19T22:10:07Z

crates/parsedbuf/src/lib.rs

        }

-        // Allow tests to manufacture owned instances with known good values.
+        // Allow tests and indexes to manufacture owned instances with known good values.


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.

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.

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).

crates/spk-schema/crates/foundation/src/version/compat/mod.rs

codecov · 2026-03-19T23:02:13Z

Codecov Report

❌ Patch coverage is 0% with 100 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/spk-schema/src/option.rs	0.00%	34 Missing ⚠️
crates/spk-schema/src/component_spec.rs	0.00%	20 Missing ⚠️
crates/spk-schema/src/v0/embedded_package_spec.rs	0.00%	18 Missing ⚠️
...hema/crates/foundation/src/ident_build/build_id.rs	0.00%	5 Missing ⚠️
...schema/crates/foundation/src/version/compat/mod.rs	0.00%	4 Missing ⚠️
...-schema/crates/foundation/src/version_range/mod.rs	0.00%	4 Missing ⚠️
...a/crates/foundation/src/ident_ops/parsing/ident.rs	0.00%	3 Missing ⚠️
...es/spk-schema/crates/foundation/src/version/mod.rs	0.00%	3 Missing ⚠️
...ates/spk-schema/src/component_embedded_packages.rs	0.00%	3 Missing ⚠️
crates/spk-schema/src/component_spec_list.rs	0.00%	3 Missing ⚠️
... and 1 more

📢 Thoughts on this report? Let us know!

jrray

This is gonna hurt...

jrray · 2026-03-26T17:43:12Z

crates/parsedbuf/src/lib.rs

        }

-        // Allow tests to manufacture owned instances with known good values.
+        // Allow tests and indexes to manufacture owned instances with known good values.


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.

crates/spk-schema/crates/foundation/src/ident_build/build_id.rs

crates/spk-schema/crates/foundation/src/ident_ops/parsing/ident.rs

jrray · 2026-03-26T17:50:51Z

crates/spk-schema/crates/foundation/src/version/compat/mod.rs

+// 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.


I do like the idea of there being a newtype that wraps the other and giving them their own distinct Display implementation, to get away from the "magic" alternate() mechanism.

crates/spk-schema/crates/foundation/src/version/compat/mod.rs

crates/spk-schema/src/component_embedded_packages.rs

crates/spk-schema/src/component_spec.rs

crates/spk-schema/src/option.rs

crates/spk-schema/src/requirements_list.rs

These allow for directly creating those objects from other existing object pieces, e.g. from index data objects. Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

jrray · 2026-04-02T20:24:12Z

crates/spk-schema/crates/foundation/src/version/compat/mod.rs

+    /// # Safety
+    ///
+    /// The caller must ensure the string parses as a valid compat.
+    pub unsafe fn new_unchecked(compat: &str) -> Result<Self> {


This uses from_str and returns a Result. Is it really unsafe? It has to successfully parse so I don't think using this skips any validation.

jrray · 2026-04-02T20:25:33Z

crates/spk-schema/crates/foundation/src/version/mod.rs

+    /// The caller must make sure the string can be parsed as a valid
+    /// Version.
+    pub unsafe fn new_unchecked(version_str: &str) -> Result<Self> {
+        Version::try_from(version_str)


Same thought here, really unsafe?

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?

jrray · 2026-04-02T20:34:15Z

crates/spk-schema/src/v0/embedded_package_spec.rs

+    /// The caller must make sure the pieces combine to make a valid
+    /// EmbeddedPackageSpec.


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.

jrray · 2026-04-02T20:35:30Z

crates/spk-schema/src/requirements_list.rs

+    ///
+    /// # Safety
+    ///
+    /// The caller must make sure the requests are valid and the they


Suggested change

/// The caller must make sure the requests are valid and the they

/// The caller must make sure the requests are valid and they

jrray

Typo, and maybe some things are unsafe that don't have to be.

I'm not expecting you to update the safety statements but can you create an issue for it to acknowledge it?

dcookspi self-assigned this Mar 19, 2026

dcookspi added SPI AOI Area of interest for SPI pr-chain This PR doesn't target the main branch, don't merge! labels Mar 19, 2026

dcookspi commented Mar 19, 2026

View reviewed changes

crates/spk-schema/crates/foundation/src/version_range/mod.rs Show resolved Hide resolved

dcookspi commented Mar 19, 2026

View reviewed changes

crates/spk-schema/crates/foundation/src/version/compat/mod.rs Outdated Show resolved Hide resolved

dcookspi added the enhancement New feature or request label Mar 19, 2026

dcookspi requested review from jrray and rydrman March 19, 2026 22:53

dcookspi force-pushed the index-1-package-trait-changes branch from 2f40d65 to fb6e0dd Compare March 25, 2026 00:42

dcookspi force-pushed the index-2-new-unchecked-additions branch from d83388a to a59523b Compare March 25, 2026 00:56

jrray requested changes Mar 26, 2026

View reviewed changes

dcookspi force-pushed the index-1-package-trait-changes branch from fb6e0dd to 9bd9685 Compare March 26, 2026 23:06

Adds new_unchecked() constructor methods to various spk schema objects.

3e1ba06

These allow for directly creating those objects from other existing object pieces, e.g. from index data objects. Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

dcookspi force-pushed the index-2-new-unchecked-additions branch from a59523b to 5c8e4db Compare March 27, 2026 00:48

Updarted new_unchecked methods to be unsafe

33938c3

Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

dcookspi force-pushed the index-2-new-unchecked-additions branch from 5c8e4db to 33938c3 Compare March 27, 2026 00:54

dcookspi mentioned this pull request Mar 30, 2026

CompatRules vs a single CompatRule - should be different types #1347

Open

dcookspi requested a review from jrray March 30, 2026 18:17

dcookspi mentioned this pull request Apr 2, 2026

Indexes 6: Changes version_filter field in index schema #1344

Open

jrray reviewed Apr 2, 2026

View reviewed changes

jrray requested changes Apr 2, 2026

View reviewed changes

		/// The caller must make sure the pieces combine to make a valid
		/// EmbeddedPackageSpec.

	/// The caller must make sure the requests are valid and the they
	/// The caller must make sure the requests are valid and they

Conversation

dcookspi commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcookspi Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcookspi Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jrray left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrray Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrray left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dcookspi commented Mar 19, 2026 •

edited

Loading

dcookspi Mar 19, 2026 •

edited

Loading

dcookspi Mar 19, 2026 •

edited

Loading

codecov bot commented Mar 19, 2026 •

edited

Loading

jrray Apr 2, 2026 •

edited

Loading

jrray left a comment •

edited

Loading