Refactor type normalization

Some of these checks were redundant, and we probably don't actually need
substitution to wrap success/failure, since it isn't expected to fail
anyway... Now the logic is much simpler, and adding more built-in type
definitions should be easy.

src/hz.erl

@@ -1652,18 +1652,14 @@ annotate_variants([{Name, Elems} | Rest], Types, Acc) ->
 annotate_variants([], _Types, Acc) ->
     {ok, lists:reverse(Acc)}.
 
-normalize_opaque_type(T, Types) ->
-    case type_is_expanded(T) of
-        false -> normalize_opaque_type(T, Types, true);
-        true  -> {ok, true, T, T}
-    end.
-
 % This function evaluates type aliases in a loop, until eventually a usable
 % definition is found.
 %
 % It also evaluates built-in and standard library types such as options and
 % names, to their defined variant representation, as well as evaluating
 % certain binary types like hash, fp, and fr, into their byte representations.
+normalize_opaque_type(T, Types) -> normalize_opaque_type(T, Types, true).
+
 % FIXME detect infinite loops
 % FIXME detect builtins with the wrong number of arguments
 % FIXME should nullary types have an empty list of arguments added before now?
@@ -1675,106 +1671,92 @@ normalize_opaque_type(hash, _Types, IsFirst) ->
     % For coercion purposes, hash is indistinguishable from bytes(32), so we
     % treat it like a type alias.
     {ok, IsFirst, hash, {bytes, [32]}};
+normalize_opaque_type(T, _Types, IsFirst) when is_atom(T) ->
+    % Once we have eliminated the above rewrite cases, all other cases are
+    % handled explicitly by the coerce logic, and so are considered normalized.
+    {ok, IsFirst, T, T};
+normalize_opaque_type(Type = {T, _}, _Types, IsFirst) when is_atom(T) ->
+    % Once we have eliminated the above rewrite cases, all other cases are
+    % handled explicitly by the coerce logic, and so are considered normalized.
+    {ok, IsFirst, Type, Type};
 normalize_opaque_type(T, Types, IsFirst) when is_list(T) ->
+    % Lists/strings indicate userspace types, which may require arg
+    % substitutions. Convert to an explicit but empty arg list, for uniformity.
     normalize_opaque_type({T, []}, Types, IsFirst);
 normalize_opaque_type({T, TypeArgs}, Types, IsFirst) when is_list(T) ->
     case maps:find(T, Types) of
-        %{error, invalid_aci}; % FIXME more info
         error ->
-            {ok, IsFirst, {T, TypeArgs}, {unknown_type, TypeArgs}};
+            % We couldn't find this named type... Keep building the AACI, but
+            % mark this type expression as unknown, so that FATE coercions
+            % aren't attempted.
+            {ok, IsFirst, {T, TypeArgs}, unknown_type};
         {ok, {TypeParamNames, Definition}} ->
+            % We have a definition for this type, including names for whatever
+            % args we have been given. Subtitute our args into this.
+            NewType = substitute_opaque_type(TypeParamNames, Definition, TypeArgs),
+            % Now continue on to see if we need to restart the loop or not.
+            normalize_opaque_type2(IsFirst, {T, TypeArgs}, NewType, Types)
+    end.
+
+normalize_opaque_type2(IsFirst, PrevType, NextType = {variant, _}, _) ->
+    % We have reduced to a variant. Report the type name as the normalized
+    % type, but also provide the variant definition itself as the candidate
+    % flattened type for further annotation.
+    {ok, IsFirst, PrevType, NextType};
+normalize_opaque_type2(IsFirst, PrevType, NextType = {record, _}, _) ->
+    % We have reduced to a record. Report the type name as the normalized
+    % type, but also provide the record definition itself as the candidate
+    % flattened type for further annotation.
+    {ok, IsFirst, PrevType, NextType};
+normalize_opaque_type2(_, _, NextType, Types) ->
+    % Not a variant or record yet, so go back to the start of the loop.
+    % It will no longer be the first iteration.
+    normalize_opaque_type(NextType, Types, false).
+
+% Perform a beta-reduction on a type expression.
+substitute_opaque_type([], Definition, _) ->
+    % There are no parameters to substitute. This is the simplest way of
+    % defining type aliases, records, and variants, so we should make sure to
+    % short circuit all the recursive descent logic, since it won't actually
+    % do anything.
+    Definition;
+substitute_opaque_type(TypeParamNames, Definition, TypeArgs) ->
+    % Bundle the param names alongside the args that we want to substitute, so
+    % that we can keyfind the one list.
     Bindings = lists:zip(TypeParamNames, TypeArgs),
-            normalize_opaque_type2(T, TypeArgs, Types, IsFirst, Bindings, Definition)
+    substitute_opaque_type(Bindings, Definition).
-    end.
 
-normalize_opaque_type2(T, TypeArgs, Types, IsFirst, Bindings, Definition) ->
-    SubResult =
-        case Bindings of
-            [] -> {ok, Definition};
-            _  -> substitute_opaque_type(Bindings, Definition)
-        end,
-    case SubResult of
-        % Type names were already normalized if they were ADTs or records,
-        % since for those connectives the name is considered part of the type.
-        {ok, NextT = {variant, _}} ->
-            {ok, IsFirst, {T, TypeArgs}, NextT};
-        {ok, NextT = {record, _}} ->
-            {ok, IsFirst, {T, TypeArgs}, NextT};
-        % Everything else has to be substituted down to a built-in connective
-        % to be considered normalized.
-        {ok, NextT} ->
-            normalize_opaque_type3(NextT, Types);
-        Error ->
-            Error
-    end.
-
-% while this does look like normalize_opaque_type/2, it sets IsFirst to false
-% instead of true, and is part of the loop, instead of being an initial
-% condition for the loop.
-normalize_opaque_type3(NextT, Types) ->
-    case type_is_expanded(NextT) of
-        false -> normalize_opaque_type(NextT, Types, false);
-        true  -> {ok, false, NextT, NextT}
-    end.
-
-% Strings indicate names that should be substituted. Atoms indicate built in
-% types, which don't need to be expanded, except for option.
-% TODO: Stop calling this, so that we can stop redundantly enumerating all the
-% built in types.
-type_is_expanded({option, _})            -> false;
-type_is_expanded(hash)                   -> false;
-type_is_expanded(X) when is_atom(X)      -> true;
-type_is_expanded({X, _}) when is_atom(X) -> true;
-type_is_expanded(_)                      -> false.
-
-% Skip traversal if there is nothing to substitute. This will often be the
-% most common case.
 substitute_opaque_type(Bindings, {var, VarName}) ->
     case lists:keyfind(VarName, 1, Bindings) of
-        false        -> {error, invalid_aci};
+        {_, TypeArg} -> TypeArg;
-        {_, TypeArg} -> {ok, TypeArg}
+        % No valid ACI will create this case. Regardless, the user should
-    end;
+        % still be able to specify arbitrary gmb FATE terms for whatever this
-substitute_opaque_type(Bindings, {variant, Args}) ->
+        % is meant to be.
-    case substitute_variant_types(Bindings, Args, []) of
+        false        -> unknown_type
-        {ok, Result} -> {ok, {variant, Result}};
-        Error        -> Error
-    end;
-substitute_opaque_type(Bindings, {record, Args}) ->
-    case substitute_record_types(Bindings, Args, []) of
-        {ok, Result} -> {ok, {record, Result}};
-        Error        -> Error
     end;
+substitute_opaque_type(Bindings, {variant, Variants}) ->
+    Each = fun({VariantName, Elements}) ->
+               NewElements = substitute_opaque_types(Bindings, Elements),
+               {VariantName, NewElements}
+           end,
+    NewVariants = lists:map(Each, Variants),
+    {variant, NewVariants};
+substitute_opaque_type(Bindings, {record, Fields}) ->
+    Each = fun({FieldName, FieldType}) ->
+               NewType = substitute_opaque_type(Bindings, FieldType),
+               {FieldName, NewType}
+           end,
+    NewFields = lists:map(Each, Fields),
+    {record, NewFields};
 substitute_opaque_type(Bindings, {Connective, Args}) ->
-    case substitute_opaque_types(Bindings, Args, []) of
+    NewArgs = substitute_opaque_types(Bindings, Args),
-        {ok, Result} -> {ok, {Connective, Result}};
+    {Connective, NewArgs};
-        Error        -> Error
-    end;
 substitute_opaque_type(_Bindings, Type) ->
-    {ok, Type}.
+    Type.
 
-substitute_variant_types(Bindings, [{VariantName, Elements} | Rest], Acc) ->
+substitute_opaque_types(Bindings, Types) ->
-    case substitute_opaque_types(Bindings, Elements, []) of
+    Each = fun(Type) -> substitute_opaque_type(Bindings, Type) end,
-        {ok, Result} -> substitute_variant_types(Bindings, Rest, [{VariantName, Result} | Acc]);
+    lists:map(Each, Types).
-        Error        -> Error
-    end;
-substitute_variant_types(_Bindings, [], Acc) ->
-    {ok, lists:reverse(Acc)}.
-
-substitute_record_types(Bindings, [{ElementName, Type} | Rest], Acc) ->
-    case substitute_opaque_type(Bindings, Type) of
-        {ok, Result} -> substitute_record_types(Bindings, Rest, [{ElementName, Result} | Acc]);
-        Error        -> Error
-    end;
-substitute_record_types(_Bindings, [], Acc) ->
-    {ok, lists:reverse(Acc)}.
-
-substitute_opaque_types(Bindings, [Next | Rest], Acc) ->
-    case substitute_opaque_type(Bindings, Next) of
-        {ok, Result} -> substitute_opaque_types(Bindings, Rest, [Result | Acc]);
-        Error        -> Error
-    end;
-substitute_opaque_types(_Bindings, [], Acc) ->
-    {ok, lists:reverse(Acc)}.
 
 coerce_bindings(VarTypes, Terms, Direction) ->
     DefLength = length(VarTypes),