-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: Extension
requires a version
#1367
Changes from all commits
3cbb454
09a9e25
0bdd95d
8606db2
0817170
8824b7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
//! TODO: YAML declaration and parsing. This should be similar to a plugin | ||
//! system (outside the `types` module), which also parses nested [`OpDef`]s. | ||
|
||
pub use semver::Version; | ||
use std::collections::btree_map; | ||
use std::collections::hash_map; | ||
use std::collections::{BTreeMap, BTreeSet, HashMap}; | ||
|
@@ -55,21 +56,17 @@ impl ExtensionRegistry { | |
pub fn try_new( | ||
value: impl IntoIterator<Item = Extension>, | ||
) -> Result<Self, ExtensionRegistryError> { | ||
let mut exts = BTreeMap::new(); | ||
let mut res = ExtensionRegistry(BTreeMap::new()); | ||
|
||
for ext in value.into_iter() { | ||
let prev = exts.insert(ext.name.clone(), ext); | ||
if let Some(prev) = prev { | ||
return Err(ExtensionRegistryError::AlreadyRegistered( | ||
prev.name().clone(), | ||
)); | ||
}; | ||
res.register(ext)?; | ||
} | ||
|
||
// Note this potentially asks extensions to validate themselves against other extensions that | ||
// may *not* be valid themselves yet. It'd be better to order these respecting dependencies, | ||
// or at least to validate the types first - which we don't do at all yet: | ||
// TODO https://github.com/CQCL/hugr/issues/624. However, parametrized types could be | ||
// cyclically dependent, so there is no perfect solution, and this is at least simple. | ||
let res = ExtensionRegistry(exts); | ||
for ext in res.0.values() { | ||
ext.validate(&res) | ||
.map_err(|e| ExtensionRegistryError::InvalidSignature(ext.name().clone(), e))?; | ||
|
@@ -82,13 +79,35 @@ impl ExtensionRegistry { | |
/// Returns a reference to the registered extension if successful. | ||
pub fn register(&mut self, extension: Extension) -> Result<&Extension, ExtensionRegistryError> { | ||
match self.0.entry(extension.name().clone()) { | ||
btree_map::Entry::Occupied(_) => Err(ExtensionRegistryError::AlreadyRegistered( | ||
btree_map::Entry::Occupied(prev) => Err(ExtensionRegistryError::AlreadyRegistered( | ||
extension.name().clone(), | ||
prev.get().version().clone(), | ||
extension.version().clone(), | ||
)), | ||
btree_map::Entry::Vacant(ve) => Ok(ve.insert(extension)), | ||
} | ||
} | ||
|
||
/// Registers a new extension to the registry, keeping most up to date if extension exists. | ||
/// | ||
/// If extension IDs match, the extension with the higher version is kept. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I note we are not really doing anything with Semver here (it could be any totally-ordered thing), and I wonder whether this should be the place. Replacing an extension with a breaking-change-upgrade could be problematic; should we
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I think I prefer to keep the logic here simple and flexible and deal with valid Maybe I should add a "remove" function so the registry is actually fully flexible, and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added remove |
||
/// If versions match, the original extension is kept. | ||
/// Returns a reference to the registered extension if successful. | ||
pub fn register_updated( | ||
&mut self, | ||
extension: Extension, | ||
) -> Result<&Extension, ExtensionRegistryError> { | ||
match self.0.entry(extension.name().clone()) { | ||
btree_map::Entry::Occupied(mut prev) => { | ||
if prev.get().version() < extension.version() { | ||
*prev.get_mut() = extension; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worth a note that ideally we would check the extensions were identical but this is not possible (no |
||
Ok(prev.into_mut()) | ||
} | ||
btree_map::Entry::Vacant(ve) => Ok(ve.insert(extension)), | ||
} | ||
} | ||
|
||
/// Returns the number of extensions in the registry. | ||
pub fn len(&self) -> usize { | ||
self.0.len() | ||
|
@@ -103,6 +122,11 @@ impl ExtensionRegistry { | |
pub fn iter(&self) -> impl Iterator<Item = (&ExtensionId, &Extension)> { | ||
self.0.iter() | ||
} | ||
|
||
/// Delete an extension from the registry and return it if it was present. | ||
pub fn remove_extension(&mut self, name: &ExtensionId) -> Option<Extension> { | ||
self.0.remove(name) | ||
} | ||
} | ||
|
||
impl IntoIterator for ExtensionRegistry { | ||
|
@@ -264,6 +288,8 @@ pub type ExtensionId = IdentList; | |
/// A extension is a set of capabilities required to execute a graph. | ||
#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] | ||
pub struct Extension { | ||
/// Extension version, follows semver. | ||
pub version: Version, | ||
/// Unique identifier for the extension. | ||
pub name: ExtensionId, | ||
/// Other extensions defining types used by this extension. | ||
|
@@ -286,21 +312,25 @@ pub struct Extension { | |
|
||
impl Extension { | ||
/// Creates a new extension with the given name. | ||
pub fn new(name: ExtensionId) -> Self { | ||
Self::new_with_reqs(name, ExtensionSet::default()) | ||
} | ||
|
||
/// Creates a new extension with the given name and requirements. | ||
pub fn new_with_reqs(name: ExtensionId, extension_reqs: impl Into<ExtensionSet>) -> Self { | ||
pub fn new(name: ExtensionId, version: Version) -> Self { | ||
Self { | ||
name, | ||
extension_reqs: extension_reqs.into(), | ||
version, | ||
extension_reqs: Default::default(), | ||
types: Default::default(), | ||
values: Default::default(), | ||
operations: Default::default(), | ||
} | ||
} | ||
|
||
/// Extend the requirements of this extension with another set of extensions. | ||
pub fn with_reqs(self, extension_reqs: impl Into<ExtensionSet>) -> Self { | ||
Self { | ||
extension_reqs: self.extension_reqs.union(extension_reqs.into()), | ||
..self | ||
} | ||
} | ||
|
||
/// Allows read-only access to the operations in this Extension | ||
pub fn get_op(&self, name: &OpNameRef) -> Option<&Arc<op_def::OpDef>> { | ||
self.operations.get(name) | ||
|
@@ -321,6 +351,11 @@ impl Extension { | |
&self.name | ||
} | ||
|
||
/// Returns the version of the extension. | ||
pub fn version(&self) -> &Version { | ||
&self.version | ||
} | ||
|
||
/// Iterator over the operations of this [`Extension`]. | ||
pub fn operations(&self) -> impl Iterator<Item = (&OpName, &Arc<OpDef>)> { | ||
self.operations.iter() | ||
|
@@ -382,8 +417,8 @@ impl PartialEq for Extension { | |
#[derive(Debug, Clone, Error, PartialEq, Eq)] | ||
pub enum ExtensionRegistryError { | ||
/// Extension already defined. | ||
#[error("The registry already contains an extension with id {0}.")] | ||
AlreadyRegistered(ExtensionId), | ||
#[error("The registry already contains an extension with id {0} and version {1}. New extension has version {2}.")] | ||
AlreadyRegistered(ExtensionId, Version, Version), | ||
/// A registered extension has invalid signatures. | ||
#[error("The extension {0} contains an invalid signature, {1}.")] | ||
InvalidSignature(ExtensionId, #[source] SignatureError), | ||
|
@@ -544,6 +579,53 @@ pub mod test { | |
// We re-export this here because mod op_def is private. | ||
pub use super::op_def::test::SimpleOpDef; | ||
|
||
use super::*; | ||
|
||
impl Extension { | ||
/// Create a new extension for testing, with a 0 version. | ||
pub(crate) fn new_test(name: ExtensionId) -> Self { | ||
Self::new(name, Version::new(0, 0, 0)) | ||
} | ||
} | ||
|
||
#[test] | ||
fn test_register_update() { | ||
let mut reg = ExtensionRegistry::try_new([]).unwrap(); | ||
let ext_1_id = ExtensionId::new("ext1").unwrap(); | ||
let ext_2_id = ExtensionId::new("ext2").unwrap(); | ||
let ext1 = Extension::new(ext_1_id.clone(), Version::new(1, 0, 0)); | ||
let ext1_1 = Extension::new(ext_1_id.clone(), Version::new(1, 1, 0)); | ||
let ext1_2 = Extension::new(ext_1_id.clone(), Version::new(0, 2, 0)); | ||
let ext2 = Extension::new(ext_2_id, Version::new(1, 0, 0)); | ||
|
||
reg.register(ext1.clone()).unwrap(); | ||
assert_eq!(reg.get("ext1").unwrap().version(), &Version::new(1, 0, 0)); | ||
|
||
// normal registration fails | ||
assert_eq!( | ||
reg.register(ext1_1.clone()), | ||
Err(ExtensionRegistryError::AlreadyRegistered( | ||
ext_1_id.clone(), | ||
Version::new(1, 0, 0), | ||
Version::new(1, 1, 0) | ||
)) | ||
); | ||
|
||
// register with update works | ||
reg.register_updated(ext1_1.clone()).unwrap(); | ||
assert_eq!(reg.get("ext1").unwrap().version(), &Version::new(1, 1, 0)); | ||
|
||
// register with lower version does not change version | ||
reg.register_updated(ext1_2.clone()).unwrap(); | ||
assert_eq!(reg.get("ext1").unwrap().version(), &Version::new(1, 1, 0)); | ||
|
||
reg.register(ext2.clone()).unwrap(); | ||
assert_eq!(reg.get("ext2").unwrap().version(), &Version::new(1, 0, 0)); | ||
assert_eq!(reg.len(), 2); | ||
|
||
assert!(reg.remove_extension(&ext_1_id).unwrap().version() == &Version::new(1, 1, 0)); | ||
assert_eq!(reg.len(), 1); | ||
} | ||
mod proptest { | ||
|
||
use ::proptest::{collection::hash_set, prelude::*}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub so downstream crates don't need to add a
semver
dependency