refactor(hugrv2)!: WIP+RFC: Unify Type and Term#2785
refactor(hugrv2)!: WIP+RFC: Unify Type and Term#2785acl-cqc wants to merge 97 commits intoacl/no_1type_rowfrom
Conversation
…conversion to types.rs
This reverts commit 3aad2180f710237a31fc67e5995a2f607d9f0168.
This reverts commit f022c59.
We are no longer parametrizing over it: Signature andFuncValueType are two separate structs. So it serves no purpose. The alternative would be to reintroduce FuncTypeBase. Signature (the <TypeRow> instantiation) would serialize correctly, but we could not mark FuncValueType (the <Term> instantation) with the necessary serde directives. We could work round this by introducing a `struct TypeRowRV(Term)` - with constructors `new`, `try_new` and `new_unchecked` and perhaps validation - and parametrize over that. Might not be sooo bad...??
Merging this PR will degrade performance by 32.53%
Performance Changes
Comparing |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## acl/no_1type_row #2785 +/- ##
====================================================
- Coverage 83.70% 83.64% -0.06%
====================================================
Files 260 259 -1
Lines 52559 52416 -143
Branches 47397 47254 -143
====================================================
- Hits 43995 43845 -150
+ Misses 6175 6163 -12
- Partials 2389 2408 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
318385b to
f5047be
Compare
| [Type::new_function(FuncValueType::new( | ||
| [usize_t()], | ||
| [tv.clone()], | ||
| tv.clone(), |
There was a problem hiding this comment.
This is a nice example of where we go back on #2784: this "row variable" previously had to be written in square brackets so it could be placed as an element of a TypeRow, despite the variable standing for a list itself
| /// | ||
| /// let expected_json = json!({ | ||
| /// "typ": usize_t(), | ||
| /// "typ": {"t": "I"}, // No public way to serialize a Term as a (SerSimple)Type... |
There was a problem hiding this comment.
This would be #[serde_as(as="crate::types::serialize::SerType")] on a rust decl, but we don't have a way to do that here. We could do "type": SerSimpleType::from(usize_t()) but would have to make SerSimpleType public
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
|
Gonna do this a different way, |
Follows #2784; closes #2341.
Term::RuntimeFunction,Term::RuntimeSumandTerm::RuntimeExtension, removingTerm::Runtime(Type)as well asenum Type(now an alias forTerm) andTypeEnum(RIP).Types are now RuntimeXXX's. However this new convention is not consistent with (does not apply to)Term::RuntimeType:-(... so the existing convention, that the name XXXType (e.g.Term::RuntimeType,Term::StringType) means a type ofTerms i.e. in the static system, takes priority.Term::StringKind,Term::StaticKind) and (b) then rename everything RuntimeXXX to XXXType (e.g.Term::FunctionType,Term::ExtensionType). Thus,Term::RuntimeType(TypeBound)->Term::TypeKind(TypeBound)). This will be even more breaking as Rust doesn't have good support for renaming enum variants, note....Term::RuntimeExtension(which would beTerm::ExtensionTypeunder renaming proposed above) is planned to becomeTerm::Extensionin the next PR, i.e. able to produce things other than runtime typesSumTypecaches its bound; this is the only case where TypeEnum's cache was useful (as Extensions already cache insideCustomTypeand Functions are always Copyable) so then we can drop the Type/TypeEnum wrappingTerm::ListConcatall of whose members are lists; this is a much nicer/more-principled solution.TypeRowremains as aVec<Term>(see below) butTypeRowRVis now aTerm: either aTerm::List(of single types, including variables), aTerm::Variable(a variable ranging over lists of types), or aTerm::ListConcat(of sublists each being one of these three, and canonicalized to a mix of the previous two only).Term::new_list_concat) was not that great so I have reverted...thoughts?serde_aswith some new markers (SerType,SerTypeRowRV). This has had some impact, see next two points.#[serde_as(as=SerTypeRowRV)]declaration. However, see next point.TypeRowsignificantly. In general it can store arbitrary Terms (of course), but it serializes into json in the legacy format that only supports Types. MeanwhileTypeRowRVis justTermbut places using it tend to use a#[serde_as(as=SerTypeRowRV)]which serializes into json in the legacy format that supports Types and RowVariables.Signature,FuncValueTypeandSumTypechecking constructors:::try_newchecks the contents are actually types/lists of types;::newdoes the same check and unwraps;::new_uncheckedavoids the check (but will break at JSON-serialization time, and shouldn't really happen anyways).TypeRowRVbe a wrapper-struct aroundTermand instead put these checking constructors on bothTypeRow(i.e. requiring that the elements are single types) andTypeRowRV. However this has problems because of our use ofimpl Into<TypeRow>(/RV) everywhere: eitherimpl Fromwould callnew_unchecked(meaning the checks are bypassed in many cases)impl Fromwould callnew(i.e..into()panics)x: impl Into<TypeRow>intox: impl TryInto<TypeRow>and add Results with that error type, but this would be very disruptive.impl TryFrom<Term> for TypeRowwould becomeTerm::as_list(self) -> Option<Vec<Term>>i.e. a utility for a single-variantmatch- we have plenty of these so that's fine.Typebe a wrapper-struct aroundTermthat checks it's a runtime type (aRuntimeExtension/RuntimeFunction/RuntimeSum/Variable-of-appropriate-type), again via checking constructors, but I think this would be tooooo painfulSummary This PR is substantially painful, although I hope that removing the RV hack can be a significant win in itself right now, if we can do it right. The bigger gains are expected to come in later PRs (model-style Custom constructors in hugr-core, i.e. TypeDef -> TermDef #2296 and Unify
ValuewithTerm#2756, eventually allowing "inspectable" custom constants with efficient serialization).TODOs/unresolved questions:
TypeArg,TypeParamand the newType? (Perhaps alsoTypeRowRVbut the above suggests making it a non-alias struct with guarantees; I guess we could do the same forTypeRVtoo)TypeRowto guarantee its elements are runtime-types, thenTermRow....or just remove itBREAKING CHANGE: Term gains
RuntimeFunction,RuntimeSumandRuntimeExtensionvariants; Type is now an alias for Term. Usecheck_term_type(tm, &TypeBound::Linear.into())to check thattm: &Termrepresents a runtime type. No more aliases.