-
Notifications
You must be signed in to change notification settings - Fork 48
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
Functional record updates #124
Conversation
Updates allow "changing" the type of a member.
Involved some corrections for typing record patterns.
It wasn't capturing the original record.
Adding several fields before tried to build an Alpaca map add AST including Core Erlang nodes.
There was a unification issue because when we instantiated a record as a constructor argument we didn't include the row variable for the record's type. This led to unification errors when a record contained more members than the constructor was strictly looking for.
I opened issue #125 for the information loss problem. |
src/alpaca.erl
Outdated
"test_files/use_update_record.alp"], | ||
[M1, M2] = compile_and_load(Files, []), | ||
|
||
?assertEqual(#{'__struct__' => record, x => 5, b => 2}, M2:main(unit)), |
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.
I find it a bit confusing to use unit
as the parameter in a call to an alpaca function let main () = ...
, when the actual representation of unit is the empty tuple ()
(or {}
in Erlang syntax). Speaking of this, shouldn't "let main () = " be compiled to a pattern match against {}
, so this would crash?
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.
We could do that. I've always just treated it as a throwaway value and not bothered to check. I don't have particularly strong opinions on this but our code gen does output an empty Erlang tuple. I suppose this is more of an issue for Erlang calling Alpaca.
I'll change this particular occurrence to an empty tuple, happy to have the broader "do we crash on anything not an empty tuple" in a separate issue.
src/alpaca_typer.erl
Outdated
@@ -1037,6 +1068,8 @@ inst_type_arrow(EnvIn, #type_constructor{}=TC) -> | |||
Default = {error, {bad_constructor, Line, Name}}, | |||
%% constructors defined in this module or imported by it: | |||
Available = EnvIn#env.type_constructors, | |||
io:format("Get constructor for ~s~n", [Name]), |
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.
Do you want to keep this debug output?
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.
Definitely not, thanks for catching that :)
I wouldn't call it an "update" if it changes the type of the record. A better name would be record transformation. The mental model then (using your example) is that it is a shortcut for writing a function |
I like "record transformation", thanks for the suggestion. I'll update docs and AST nodes (e.g. |
Per @danabr's suggestion we'll refer to non-destructive record extensions and changes as record transformations. AST and docs changed accordingly.
Just convention for now but it's more consistent with the representation of unit in Alpaca itself.
Fixes #45
Pretty basic syntax to add or update a field, e.g to add an
x: int
field to an existing record:Since record updates are non-destructive and thus not (strictly speaking) updates, I've allowed for members' types to change, e.g. the following is legal:
I think this makes sense both because our updates are non-destructive and since we allow for records to be declared anywhere as literals without them specified as types prior. This PR includes a test showing information loss when using type constructors for which I'll file a new issue. I'm not entirely certain it's something we want to consider a real problem but I'll leave it for discussion in the issue itself once filed.