Fix coerce/3 when applied to namespace types, and type parameters inside record types. #1

spivee · 2025-01-24T17:05:15+09:00

spivee commented

2025-01-24 17:05:15 +09:00

I also added some eunit tests right in the middle of the hz module, but these could be removed or moved as desired.

spivee added 4 commits 2025-01-24 17:05:16 +09:00

Add unit tests for some simple coercions 560cdb8cea

Even more unit tests 8eb907091a

Trying to test all the basic types that coerce covers, and a couple more
type parameter and nested cases.

Also prepare AACI for namespace types 72fe195bff

Fix type substitution into variants and records 87477e8c9c

Variants were working by accident, since
{variant, [{"VariantName", [Element]}]} had a similar enough form to
the opaque types that would come from something like
`type1(type2(int))`, but records were not working, since they have a
different form. Now both are handled explicitly so that only the
intended forms of each are handled.

spivee force-pushed spivee/coerce-fixes from 87477e8c9c to f54a33a293

2025-01-24 17:20:04 +09:00

Compare

spivee commented

2025-01-24 17:21:19 +09:00

Force-pushed a rebased version of the branch, on top of b69ababf0f.

- Force-pushed a rebased version of the branch, on top of b69ababf0f.

zxq9 reviewed 2025-01-25 03:29:36 +09:00

src/hz.erl Outdated

						
				@ -1413,3 +1414,2 @@

				    Name = binary_to_list(NameBin),

				    Types2 = maps:put(Name, {[], contract}, Types),

				    Types3 = case maps:find(state, Next) of

				    Types2 = case IsContract of

zxq9 commented

2025-01-25 03:29:36 +09:00

Pet peeve of mine in midwit Erlang: naming important transitions with numbers instead of what the transitions actually are or avoiding proper structure around the transitions themselves (tends to indicate foggy comprehension).

Can we come up with a more descriptive name than "Types2"?

If this is fundamental, then yes, certainly we can.
If this is not fundamental, then usually the operation desired is a pass-through, not a single step in a prolonged process.

In either case, the names (of the transition points or the functions involved) should tell us more about them.
Otherwise we need a book-sized set of descriptive comments.

Pet peeve of mine in midwit Erlang: naming important transitions with numbers instead of what the transitions actually are *or* avoiding proper structure around the transitions themselves (tends to indicate foggy comprehension). Can we come up with a more descriptive name than "Types2"? - If this is fundamental, then yes, certainly we can. - If this is not fundamental, then usually the operation desired is a pass-through, not a single step in a prolonged process. In either case, the names (of the transition points *or* the functions involved) should tell us more about them. Otherwise we need a book-sized set of descriptive comments.

zxq9 commented

2025-02-02 19:48:13 +09:00

Just like to point out that what @spivee did in the end there is great. Makes a "single decision at a time" much easier to see and reason about. Very nice.

spivee marked this conversation as resolved

spivee force-pushed spivee/coerce-fixes from f54a33a293 to 558c4879fa

2025-01-31 11:41:44 +09:00

Compare

spivee force-pushed spivee/coerce-fixes from 558c4879fa to e5d77ea1b2

2025-01-31 11:57:42 +09:00

Compare

spivee added 3 commits 2025-01-31 12:00:24 +09:00

Break up prepare_aaci logic 7a410b08e0

Now we convert the ACI into trees of opaque types, then flatten the tree
into a map and a list of function specs, and only then dereference the
types in the function specs down to our accelerated annotated types.

Rename 'flatten' and so on to 'annotate' f2cf25a0f3

Factor handling of different ACI typedef cases 77c30abf4a

A lot of this complexity was a consequence of trying to avoid redundant
extraction of the namespace's/contract's name, so on the other hand
letting it be redundant made all of the complexity kind of evaporate.
Add to that that we're now building a little deep list and then
flattening it, and this logic was able to get really neat in a way that
I couldn't work out a year ago.

spivee force-pushed spivee/coerce-fixes from 77c30abf4a to 4e71d3215b

2025-02-26 10:04:56 +09:00

Compare

spivee commented

2025-02-26 10:06:56 +09:00

Rebased onto 0.3.0, and the tests still pass. This should be good to go.

zxq9 approved these changes 2025-02-27 21:00:48 +09:00

zxq9 left a comment

Proper coerce is going to lead to great outcomes downstream. Excellent!

src/hz.erl

						
				@ -2143,0 +2225,4 @@

				        false ->

				            erlang:error({from_fate_failed, Sophia, SophiaActual})

				    end,

				    ok.

zxq9 commented

2025-02-27 20:50:11 +09:00

OOC, what is the reason behind the erlang:error instead of a return tuple? Is there a reason for a non-local return?
The reason I ask is that there is a lot of serialization and bytecode library code that throws or errors, and it winds up complicating the calling code, which is sort of the opposite of the approach I tend to prefer (granted, sometimes it is annoying to actually play the return-tuple game).

OOC, what is the reason behind the erlang:error instead of a return tuple? Is there a reason for a non-local return? The reason I ask is that there is a lot of serialization and bytecode library code that throws or errors, and it winds up complicating the calling code, which is sort of the opposite of the approach I tend to prefer (granted, sometimes it is annoying to actually play the return-tuple game).

zxq9 commented

2025-02-27 20:58:28 +09:00

You just told me out of context that this is for the benefit of eunit. Makes sense now. Thanks.

spivee commented

2025-02-27 21:16:05 +09:00

Yeah, it's just to crash eunit tests that aren't giving the correct results. I have fixed up the redundant case A == B of true bit though, something you pointed out a while ago.

Yeah, it's just to crash eunit tests that aren't giving the correct results. I have fixed up the redundant `case A == B of true` bit though, something you pointed out a while ago.

zxq9 marked this conversation as resolved

spivee added 1 commit 2025-02-27 21:16:49 +09:00

Clean up try_coerce slightly 36a9b17b78

spivee merged commit 36a9b17b78 into master

2025-02-27 21:17:11 +09:00

spivee deleted branch spivee/coerce-fixes

2025-02-27 21:17:11 +09:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QPQ-AG/hakuzaru#1