[m-rev.] for review: do not hardcode the list of builtin primtive types in so many places

[Julien Fischer]

Hi Zoltan,

On Mon, 10 Apr 2017, Zoltan Somogyi wrote:

>
>> On 10 Apr 2017, at 00:50, Julien Fischer <jfischer at opturion.com> wrote:
>>
>>    Add a comment document other places in the compiler that will need
>>    to be updated if a new primitive type is added.
>
> A word is missing here.

Fixed.

>> compiler/add_foreign_enum.m:
>> compiler/module_qual.qualify_items.m:
>> compiler/special_pred.m:
>> compiler/write_module_interface_files.m:
>>     Use the new predicates rather than listing the primitive types
>>     directly.
>
> The key to the correctness of this change is not the modules that
> have been updated to use the new predicates, but the modules that haven’t,
> even though they should be. May I presume that you have looked at all
> the modules in the compiler that mention all of “character”, “string”,
> “int” and “float”?

Yes.

>> --- a/compiler/add_foreign_enum.m
>> +++ b/compiler/add_foreign_enum.m
>> @@ -55,12 +55,7 @@ add_pragma_foreign_export_enum(FEEInfo, _TypeStatus, Context,
>>         % Emit an error message for foreign_export_enum pragmas for the
>>         % builtin atomic types.
>>         TypeArity = 0,
>> -        ( TypeName = unqualified("character")
>> -        ; TypeName = unqualified("float")
>> -        ; TypeName = unqualified("uint")
>> -        ; TypeName = unqualified("int")
>> -        ; TypeName = unqualified("string")
>> -        )
>> +        is_builtin_type_sym_name(TypeName)
>
> Indentation seems to be off, both here and below.

It's fine in the actual file.

>> index 14b0c9a..eeb14d6 100644
>> --- a/compiler/prog_data.m
>> +++ b/compiler/prog_data.m
>> @@ -482,6 +482,16 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>>             % A type expression with an explicit kind annotation.
>>             % (These are not yet used.)
>>
>> +
>> +    % This type enumerates all of the builtin primitive types in Mercury.
>> +    % If you add a new alternative then you may also need to update the
>> +    % following predicates:
>> +    %
>> +    %     - parse_type_name.is_known_type_name_args/3
>> +    %     - inst_check.check_inst_defn_has_matching_type/7
>> +    %     - llds_out_data.output_type_ctor_addr/5
>> +    %     - type_util.classify_type_ctor/2
>> +    %
>> :- type builtin_type
>>     --->    builtin_type_int
>>     ;       builtin_type_uint
>> @@ -489,6 +499,10 @@ cons_id_is_const_struct(ConsId, ConstNum) :-
>>     ;       builtin_type_string
>>     ;       builtin_type_char.
>>
>> +:- pred is_builtin_type_sym_name(sym_name::in) is semidet.
>> +
>> +:- pred is_builtin_type_name(string::in) is semidet.
>> +
>>
>
> I am surprised that there is no place in the compiler that would want
> the builtin_ctor that corresponds to the name.
>
> Coming back to this: I see that type_ctor_info.m has a version of this
> code that does return the builtin_ctor. Is there any reason why you
> haven’t moved that functionality here? It would reduce code duplication.

It seems that type_ctor_info.m is the *only* place that wants it.  In
any case, the table in type_ctor_info.m relates type names to values of
builtin_ctor0 type, not builtin_type/0.  Moving into prog_data.m seems
like the wrong thing since the rest of the compliler doesn't need to
know about it.

>> --- a/compiler/special_pred.m
>> +++ b/compiler/special_pred.m
>> @@ -340,11 +340,7 @@ can_generate_special_pred_clauses_for_type(ModuleInfo, TypeCtor, TypeBody) :-
>> is_builtin_type_special_preds_defined_in_mercury(TypeCtor, TypeName) :-
>>     Builtin = mercury_public_builtin_module,
>>     TypeCtor = type_ctor(qualified(Builtin, TypeName), 0),
>> -    ( TypeName = "int"
>> -    ; TypeName = "uint"
>> -    ; TypeName = "string"
>> -    ; TypeName = "character"
>> -    ; TypeName = "float"
>> +    ( is_builtin_type_name(TypeName)
>>     ; TypeName = "pred"
>>     ).
>
> This code expects the symname to have builtin.m as the qualifier module.
> The other code above all expected an *unqualified* name.
>
> Either this code is wrong (and has been for a while), or the tests on
> symnames in prog_data.m should accept both forms.

It's not a new problem, see https://bugs.mercurylang.org/view.php?id=13.
I'm not sure what the intended behaviour is meant to be (if indeed there
was ever such a thing):

    1. all builtin types have unqualified names
    2. all buitlin types have qualified names (e.g. with builtin)
    3. there's a point in the compiler where the names switch from being
       unqualified to qualified.

If it's either (1) or (2) then there are several points in the compiler
that are broken; if it's (3) then it's not sufficiently well documented.

In the interests of not inadvertently breaking stuff (further), I will
leave the above code as-is for now.  (Note that type_ctor_info.m also
assumes that the builtin types are qualified.)

Julien.