[m-rev.] for review: improve error reporting for foreign_enum pragmas

[Julien Fischer]

Hi Zoltan,

On Thu, 19 May 2016, Zoltan Somogyi wrote:

>> On 18 May 2016, at 11:22, Julien Fischer <jfischer at opturion.com> wrote:
>>
>> @@ -737,42 +725,72 @@ parse_export_enum_attr(VarSet, Term, MaybeAttribute) :-
>> parse_pragma_foreign_enum(VarSet, ErrorTerm, PragmaTerms, Context, SeqNum,
>>         MaybeIOM) :-
>>     ( if PragmaTerms = [LangTerm, MercuryTypeTerm, ValuesTerm] then
>> +
>>         ( if parse_foreign_language(LangTerm, ForeignLang) then
>> -            parse_export_enum_type_ctor(MercuryTypeTerm, MaybeTypeCtor),
>> +            MaybeForeignLang = ok1(ForeignLang)
>> +        else
>> +            LangPieces = [words("Error: invalid foreign language in"),
>> +                pragma_decl("foreign_enum"), words("declaration."),
>> +                nl],
>> +            LangSpec = error_spec(severity_error, phase_term_to_parse_tree,
>> +                [simple_msg(get_term_context(LangTerm), [always(LangPieces)])]),
>> +            MaybeForeignLang = error1([LangSpec])
>> +        ),
>
> I think it could help if the message said e.g. "invalid foreign language `charp’
> in …”, so I would call describe_error_term on LangTerm.

Done.

>> +        parse_type_ctor_name_arity("foreign_enum", MercuryTypeTerm,
>> +            MaybeTypeCtor),
>> +
>> +        UnrecognizedPieces =
>> +            [words("Error: expected a valid mapping element")],
>
> I would say explicitly what the "mapping element” is supposed
> to be, i.e. what it maps to what. I would do that by giving a template,
> since it is not obvious e.g what should separate the source and target
> entities.

I'll do that as a separate change -- I was intending to rework the code
for parsing the third argument of foreign_enum pragmas so we could
return more accurate error messages anyway.  (The existing code has
a tendency to simply fail if it detects an error and output the above
error.)

>> +        % XXX the following doesn't check that foreign values are sensible
>> +        % (e.g. it should reject the empty string).
>
> Would it be easy to add that check?

As part of the above reworking, yes.

>> +        convert_maybe_list("mapping elements", yes(VarSet), ValuesTerm,
>> +            parse_sym_name_string_pair(VarSet, PairContextPieces),
>> +            UnrecognizedPieces, MaybeValues0),
>> +        (
>> +            MaybeValues0 = ok1(Values),
>
> The convention I have been using for code like this, where the initial
> part of the code conditionally puts a value into a maybe1 and the later
> part takes it out again, is to name the first appearance of the variable
> a 0 suffix. So I would rename Values to Values0, and the later ValuesPrime
> to plain Values.

Done.

>> diff --git a/tests/invalid/invalid_pragma.err_exp b/tests/invalid/invalid_pragma.err_exp
>> new file mode 100644
>> index 0000000..6a9fe32
>> --- /dev/null
>> +++ b/tests/invalid/invalid_pragma.err_exp
>> @@ -0,0 +1,2 @@
>> +invalid_pragma.m:006: Error: a `:- pragma' declaration should have the form
>> +invalid_pragma.m:006:   `:- pragma pragma_name(pragma_arguments).’
>
> It may help if this continued “`1235’ is not a valid/recognized pragma name”.

I'll look at this one separately too.

Julien.