[m-rev.] for review: mlds_enum_class_defn

[Julien Fischer]

On Tue, 13 Jun 2023, Zoltan Somogyi wrote:

> For review by anyone. The diff is with -b. I have
> bootchecked the diff in hlc.gc, java and csharp grades.
>
> Does anyone know what the intention was behind creating
> the never-written-out field variable in enum classes? And would
> anyone object to deleting it?

I have included some speculation below ...


> Separate enum classes from other classes in MLDS.
> 
> Classes representing enums have always treated differently from the
> other kinds of classes by many pieces of code that operate on classes.
> Representing them using a separate construct codifies this, and allows us
> to encode the unique invariants applying to enum classes in the types.

...


> diff --git a/compiler/mlds.m b/compiler/mlds.m
> index c7e286237..7223e5d23 100644
> --- a/compiler/mlds.m
> +++ b/compiler/mlds.m
> @@ -375,6 +375,7 @@
>                  % (unless we are compiling to a target that does not need them
>                  % to be flattened).
>                  mlds_type_defns         :: list(mlds_class_defn),
> +                mlds_enum_defns         :: list(mlds_enum_class_defn),
>
>                  % Definitions of the structures that hold the tables
>                  % of tabled procedures.
> @@ -618,6 +619,46 @@
>                  mcd_ctors           :: list(mlds_function_defn)
>              ).
> 
> +    % Classes representing enum types and dummy types. We use these when
> +    % targeting C# and Java, not when targeting C (since we use low-level
> +    % data representation for C).
> +:- type mlds_enum_class_defn
> +    --->    mlds_enum_class_defn(
> +                mecd_class_name     :: mlds_class_name,
> +                mecd_class_arity    :: arity,
> +                mecd_context        :: prog_context,
> +                % We don't store any flags; they are always
> +                % mlds_class_decl_flags(class_public, overridable, modifiable)
> +
> +                % The enum class inherits one base class when targeting Java,
> +                % and no base classes when targeting C#.
> +                mecd_inherits       :: mlds_enum_class_inherits,
> +
> +                % This enum class implements one interface when targeting Java,
> +                % and no interfaces when targeting C#.
> +                mecd_implements     :: maybe(mlds_interface_id),
> +
> +                % Type parameters.
> +                mcd_tparams         :: list(type_param),
> +
> +                % XXX ml_type_gen.m generates this "value" field variable
> +                % for each enum type. However, this field var's definition
> +                % can never be used, because it is not written out by either
> +                % mlds_to_cs_class.m or mlds_to_java_class.m, and of course
> +                % mlds_to_c_class.m does not do anything with
> +                % mlds_enum_class_defns at all.
> +                mecd_value_field    :: mlds_field_var_defn,

The "value" field is present of the Java backend it's defined in
java/runtime/MercuryEnum.java, which is the base class that all Mercury-Java
enumerations extend. My guess it that field being present in the MLDS is either
a left-over from the MLDS->IL backend or something that was thought to be
required for one of the target languages for which support was never
implemented.

...


> @@ -688,6 +732,25 @@
>                  mfvd_gc                 :: mlds_gc_statement
>              ).
> 
> +:- type mlds_enum_const_defn
> +    --->    mlds_enum_const_defn(
> +                mecd_name               :: mlds_field_var_name,
> +                mecd_context            :: prog_context,
> +                % We don't store any flags for enum const definitions;
> +                % they are implicitly one_copy and const.
> +
> +                % We don't store a type for enum const definitions either;
> +                % they are always implicitly int. (Even when their value
> +                % is given by a string, that string is the name of an integer
> +                % constant in a foreign language.)

This may change, for example, if we ever support a version of of foreign_enums
for Java that maps on to actual Java enums.

...

> diff --git a/compiler/mlds_to_java_class.m b/compiler/mlds_to_java_class.m
> index 998b0ae01..9430d47d4 100644
> --- a/compiler/mlds_to_java_class.m
> +++ b/compiler/mlds_to_java_class.m

...

> @@ -248,36 +298,32 @@ output_enum_ctor_for_java(Stream, Indent, ClassName, ClassArity, !IO) :-
>      io.format(Stream, "%s}\n", [s(IndentStr)], !IO).
>
>  :- pred output_enum_constant_for_java(io.text_output_stream::in,
> -    indent::in, mlds_class_name::in, arity::in, mlds_field_var_defn::in,
> +    indent::in, mlds_class_name::in, arity::in, mlds_enum_const_defn::in,
>      io::di, io::uo) is det.
>
>  output_enum_constant_for_java(Stream, Indent, ClassName, ClassArity,
> -        FieldVarDefn, !IO) :-
> -    FieldVarDefn = mlds_field_var_defn(FieldVarName, _Context, _Flags,
> -        _Type, Initializer, _GCStmt),
> +        EnumConstDefn, !IO) :-
> +    EnumConstDefn = mlds_enum_const_defn(FieldVarName, _Context, EnumConst),
>      % Make a static instance of the constant. The MLDS doesn't retain enum
>      % constructor names (that shouldn't be hard to change now) so it is
>      % easier to derive the name of the constant later by naming them after
>      % the integer values.
> -    (
> -        Initializer = init_obj(Rval),
> -        ( if Rval = ml_const(mlconst_enum(N, _)) then
>      IndentStr = indent2_string(Indent),
>      UnqualClassNameStr =
>          unqual_class_name_to_string_for_java(ClassName, ClassArity),
>      FieldVarNameStr = field_var_name_to_string_for_java(FieldVarName),
> +    (
> +        EnumConst = mlds_enum_const_uint(EnumUInt),
>          io.format(Stream,
> -                "%spublic static final %s K%d = new %s(%d); /* %s */\n",
> -                [s(IndentStr), s(UnqualClassNameStr), i(N),
> -                s(UnqualClassNameStr), i(N), s(FieldVarNameStr)], !IO)
> -        else
> -            unexpected($pred, "not mlconst_enum")
> -        )
> +            "%spublic static final %s K%u = new %s(%u); /* %s */\n",
> +            [s(IndentStr), s(UnqualClassNameStr), u(EnumUInt),
> +            s(UnqualClassNameStr), u(EnumUInt), s(FieldVarNameStr)], !IO)
>      ;
> -        ( Initializer = no_initializer
> -        ; Initializer = init_struct(_, _)
> -        ; Initializer = init_array(_)
> -        ),
> +        EnumConst = mlds_enum_const_foreign(_Lang, _EnumNameStr, _Type),
> +        % XXX JAVA ENUM The code previously here got the enum value from
> +        % an mlds_initializer, and insisted that it be bound to a term
> +        % of the init_obj(ml_const(mlconst_enum(_, _))). This meant that
> +        % it did not allow for an enum defined by foreign language code.

Foreign enums are not (currently) supported by the Java backend.

The diff looks fine.

Julien.