refactor types module - #407
Conversation
|
I'd like to get this in before working on #403 (comment) |
|
LGTM seems to just need fmt |
2f7112b to
eb4ad94
Compare
code move only
Code move only.
Code move only.
These conversions don't really make conceptual sense, since they're converting a "real" type to a component of TypeInner. They're also not used anywhere. They both date back to BlockstreamResearch#42 when the codebase looked very different, and appear to be vestigial.
eb4ad94 to
7b31e94
Compare
|
Rebased. |
|
On 7b31e94 successfully ran local tests |
stringhandler
left a comment
stringhandler
left a comment
There was a problem hiding this comment.
In 7b31e94
Happy with other previous commits, but seems like a regression introduced in the last one.
| (TypeInner::Option(src), TypeInner::Option(dst)) => cast_preserves_enum_identity(src, dst), | ||
| (TypeInner::Either(src_l, src_r), TypeInner::Either(dst_l, dst_r)) => { | ||
| cast_preserves_enum_identity(src_l, dst_l) && cast_preserves_enum_identity(src_r, dst_r) | ||
| for (source, target) in source.post_order_iter().zip(target.post_order_iter()) { |
There was a problem hiding this comment.
In 7b31e94
LLM found this while I was reviewing with it. TL;DR: zip exits on the shorter of the two arrays, so potentially some lost data here.
Happy to provide the full output, but it was quite long. Not sure if there are cases where this might succeed if the shorter post_order_iter is a subset of the longer post_order_iter.
There was a problem hiding this comment.
Ah, damn, you're right, this zip-based algorithm is just wrong. Let me rework it.
There was a problem hiding this comment.
And I guess I should start LLM-reviewing my patches before I submit them..
There was a problem hiding this comment.
The existing algorithm is also buggy (at least in the way mentioned in its doccomment). You can cast an Option<u8> to Either<(), u8> but not if you replace u8 with an enum.
There was a problem hiding this comment.
I've added several regression tests, including ones that fail on this code but not on the old code. I may need to just back out this change and move it to a separate PR because I think a correct algorithm is going to be a fair bit longer than this.
There was a problem hiding this comment.
I just removed the change. But added a commit that adds regression test anyway.
| (Pattern::Array(pats), TypeInner::Array(ty, size)) if pats.len() == *size => { | ||
| stack.extend(pats.iter().zip(std::iter::repeat(ty.as_ref()))); | ||
| Pattern::Array(pats) => { | ||
| if let Some((ty, size)) = ty.as_array() { |
There was a problem hiding this comment.
In 7b31e94 LLM finding:
this line shadows the outer ty with the element type, so the length-mismatch arm reports the wrong type:
this PR: Expected expression of type u8; found something else
master: Expected expression of type [u8; 3]; found something else
Renaming the binding to elem_ty fixes it. Minor: the commit message mentions as_list / lists, but Pattern has no List variant.
There was a problem hiding this comment.
I replaced the explicit return value with an auxiliary closure which avoids the shadowing and also reduces code duplication.
Also updated the commit message.
Several of these are currently broken; I simply did `expect_err` rather than `expect` so that the tests would pass. But we should fix these and fix the tests in parallel.
Our goal is to reduce/eliminate the places outside of the `types` module that use the `TypeInner` type, since this is (ideally) an implementation detail of `ResolvedType`. There aren't a lot of these places, it turns out. This commit removes one "easy" one, in pattern.rs. There, previously we were matching arrays and using a wildcard _ match to return an error on mismatches. By adding a bit of code repetiton (the 'return error' line 3 times) and calling TypeDestructible::as_list and as_array, we can get rid of the wildcard match, which eliminates the `TypeInner` but also is more robust against extensions to the Pattern enum. This leaves the use in ast.rs in `cast_preserves_enum_identity` which seems quite difficult to remove correctly (the existing code is not correct either, but ok, let's leave it alone unless we can fix it completely). We want to make this nonrecursive and correct, but it will have to wait for a later PR. Aside from that, the only parts of the code, outside the ResolvedType module itself, that now need to know the internals of ResolvedType are in value.rs. Since the structure of Value mirrors the structure of ResolvedType probably we will just have to live with this; when we change the representation of ResolvedType we will need to make parallel changes in value.rs.
7b31e94 to
098eb0e
Compare
|
On 098eb0e successfully ran local tests |
3 participants
Just moves a bunch of types around. No behavioral changes. Should make the types module much easier to navigate.