refactor(hugrv2)!: combine TypeEnum with Term, no RV parametrization#2895
refactor(hugrv2)!: combine TypeEnum with Term, no RV parametrization#2895
Conversation
…erm::List, mangle_name, skip simple_alias
Merging this PR will degrade performance by 43.47%
Performance Changes
Comparing Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2895 +/- ##
==========================================
+ Coverage 81.07% 81.09% +0.01%
==========================================
Files 242 241 -1
Lines 45183 45054 -129
Branches 38951 38822 -129
==========================================
- Hits 36633 36536 -97
+ Misses 6585 6542 -43
- Partials 1965 1976 +11
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:
|
25c7fdd to
3a41dda
Compare
|
@aborgna-q some improvements on the conversions, thanks, including generalization of Term::new_list which was a big win, some questions remaining at #2895 (comment). Also unsure about aliases. Searching for commented-out code mentioning alias might be useful when we revamp....is it any good to include |
85eb0fc to
80f1934
Compare
I think that's all the serious concerns (in particular, all those marked ALAN), plenty of smaller ones which I can have a look over but ATM it looks like they haven't raised any strong opinions ;) |
aborgna-q
left a comment
There was a problem hiding this comment.
LGTM! Just some additional comments.
| SerSimpleType::G(sig) => Type::new_function(*sig).into(), | ||
| SerSimpleType::Sum(st) => Type::from(st).into(), | ||
| SerSimpleType::Opaque(o) => Type::new_extension(o).into(), | ||
| SerSimpleType::Alias(_) => todo!("alias?"), |
There was a problem hiding this comment.
What are we doing with the alias stubs? If this stays here, it should at least give a better error.
| /// Wraps the given Term, without checking its type. | ||
| pub fn new_unchecked(t: impl Into<Term>) -> Self { | ||
| Self(t.into()) | ||
| } |
There was a problem hiding this comment.
Are we OK with this being public? The rest of the code assumes these are valid by construction so this feels like a footgun.
We could have a public try_new if needed, an keep the no-checks version internal-only.
There was a problem hiding this comment.
I first made TypeRowRV::new_unchecked crate-private, then thought - do we really need it? Can't we just use try_into().expect()? The latter works fine, and doesn't change anything on CodSpeed (although -10% on capnp roundtrip either way is not great, hmmm)
62e058d to
9fe64c5
Compare
|
aborgna-q
left a comment
There was a problem hiding this comment.
Another suggestion to remove the vestigial commented-out alias code, but otherwise ![]()
| @@ -352,7 +352,7 @@ mod test { | |||
| }; | |||
| assert_matches!(build_result, Ok(_)); | |||
| Ok(()) | |||
| } | |||
| }*/ | |||
|
|
|||
| #[inline] | ||
| pub fn new_list_concat(lists: impl IntoIterator<Item = Self>) -> Self { | ||
| Self::ListConcat(lists.into_iter().collect()) | ||
| pub fn concat_lists(lists: impl IntoIterator<Item = Self>) -> Self { |
There was a problem hiding this comment.
Ok 👍
Should we still update the comment with the current behaviour?
I had actually tried to but had missed a couple! Thanks :) |
Term::Runtime(Type), addTerm::RuntimeFunction(FuncValueType),Term::RuntimeSum(SumType)andTerm::RuntimeExtension(CustomType)and removeTypeEnum. That is, runtime types are now just variants ofTerm.Typenow just wraps aTerm, but guarantees by construction that the Term inside it represents a single runtime type. (There is no way to bypass this; there isTryFrom<Term> for Type; there are utility constructors onTypethat takeTypes thus preserving the invariant - the samenew_extension,new_tuple,new_sum,new_functionas before).TypeRV. It never had a consistent meaning: it was sometimes a type, and sometimes a list. Also removetrait MaybeRV,enum NoRVandstruct RowVariable.TypeRowRV(much like Type) wraps aTerm, but guarantees it represents a list of types (of perhaps unknown length). That is, it could be aTerm::List(whose elements are single types), or aTerm::Variable(a "row variable" i.e. ranging over lists of types, of unknown length), or aTerm::ListConcat(whose elements are one of these three). Again there are utility constructors preserving the invariant, tho there is also aTypeRowRV::new_unchecked.new+try_new+new_uncheckedconstructors, removed in that commit). However I think with a few nifty TypeRowRV conversions and methods (just_row_varandconcat) the wrapper-struct is easier to use as well as giving more static checking.TypeRowremains as a list ofTypes, i.e. whose length/number-of-types is knowntrait TypeRowLikeallows parametrizingFuncTypeBaseso it can still do bothSignatureandFuncValueType. I have kept this trait crate-hidden, so we keep thesubstituteandvalidatemethods hidden (as before), which does mean external code cannot parametrize in this way.Thus, although the new system offers less Rust-compile-time checking, there is still a reasonable amount, and it's more principled now ;)
Along the way,
Type::as_type_enumwithimpl Deref<Target=Term> for Type, similarly for TypeRowRVTermTypeErroris now used more (it's a subclass of SignatureError but captures all the errors that might happen from e.g. turning a Term into a Type), I've addedimpl From<TermTypeError> for OpLoadError(alsofor ImportErrorInner) - because Rust won't let meimpl <T: From<SignatureError>> From<TermTypeError> for T:-(.Term::new_listto take items that areimpl Into<Term>rather than justTermProposed follow-ups....
TypeintoSumType::General, in perf!: remove bound-caching from Type, do in SumType::General instead #3022TypeRowRV::new_spliced(IntoIterator<Item=Term>)that checks each Term is either a type or a list of types (and panics if not....ok could also havetry_new_spliced), and then assembles the appropriate lists/concats. commit 68fba45 shows this done for FuncValueType whereas here/now it would be for TypeRowRV, and I'm not sure that it's much better than theTypeRowRV::just_row_var,concatandfrom([Type])that this PR has now (in particular, just_row_var/concat have better Rust-compile-time checking than this "splicing" approach). So, probably less urgent / only if TypeRowRV is too awkward in practice.BREAKING CHANGE:
TypeEnumandTypeRVare no more - useTerm.RowVariable,MaybeRVetc. are gone - use appropriate term types and variables.TypeandTypeRowRVmerely wrapTermwith invariants.