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

[Julien Fischer]

Hi Zoltan,

On Mon, 13 Mar 2017, Zoltan Somogyi wrote:

> On Mon, 13 Mar 2017 16:55:26 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
>> diff --git a/compiler/parse_pragma.m b/compiler/parse_pragma.m
>> index f7732bd..a3e3fb4 100644
>> --- a/compiler/parse_pragma.m
>> +++ b/compiler/parse_pragma.m
>> @@ -411,6 +404,7 @@ parse_pragma_foreign_type(ModuleName, VarSet, ErrorTerm, PragmaTerms,
>>           ),
>>           Assertions = foreign_type_assertions(AssertionsSet),
>>           ( if
>> +            MaybeForeignLang = ok1(_),
>>               MaybeMaybeUC = ok1(MaybeUC),
>>               MaybeForeignType = ok1(ForeignType),
>>               MaybeTypeDefnHead = ok2(MercuryTypeSymName, MercuryParams),
>> @@ -427,6 +421,7 @@ parse_pragma_foreign_type(ModuleName, VarSet, ErrorTerm, PragmaTerms,
>>               Specs = get_any_errors1(MaybeMaybeUC) ++
>>                   get_any_errors1(MaybeForeignType) ++
>>                   get_any_errors2(MaybeTypeDefnHead) ++
>> +                get_any_errors1(MaybeForeignLang) ++
>>                   AssertionSpecs,
>>               MaybeIOM = error1(Specs)
>>           )
>
> I put the tests for ok/1 and the get_any_errors calls in the *same* order.

Done.  I have also reordered them to match the argument order of the
pragma.

>> @@ -454,10 +449,13 @@ parse_foreign_type_assertions(VarSet, Term, !Assertions, !Specs) :-
>>               then
>>                   true
>>               else
>> -                TermStr = mercury_term_to_string(VarSet, print_name_only,
>> -                    Term),
>> -                Pieces = [words("Error: foreign type assertion"),
>> -                    quote(TermStr), words("is repeated."), nl],
>> +                HeadTermStr = mercury_term_to_string(VarSet, print_name_only,
>> +                    HeadTerm),
>> +                Pieces = [
>> +                    words("In fourth argument of"), pragma_decl("foreign_type"),
>> +                    words("declaration: error:"), words("foreign type assertion"),
>> +                    quote(HeadTermStr), words("is repeated.")
>> +                ],
>
> Here and elsewhere: I think this code would be more robust in the face of
> possible future changes in syntax if the "In the nth arg of xxx:" part of that
> error message was passed in by the caller as ContextPieces. That way,
> the code that picks up the nth arg and the code decides on the context pieces
> are next to each other.

Done.

>> +        ;
>> +            MaybeLanguage = error1(_),
>> +            MaybeForeignLangType = error1([])   % Dummy value.
>>           )
>
> I think this calls for a comment to say that if get here, MaybeForeignLang
> will be error, and give the user the required error message.

Done.

>> --- a/compiler/parse_type_defn.m
>> +++ b/compiler/parse_type_defn.m
>> @@ -38,13 +38,22 @@
>>       list(term)::in, prog_context::in, int::in, is_solver_type::in,
>>       maybe1(item_or_marker)::out) is det.
>>
>> -    % parse_type_defn_head(ModuleName, VarSet, Head, HeadResult):
>> +    % Are we parsing the type defn head in a `:- type' declaration or the in
>> +    % the second argument of a foreign_type pragma?
>
> "the in"

Fixed.

>> +            EndTermStr = describe_error_term(VarSet, EndTerm),
>> +            Pieces = [
>> +                words("In"), pragma_decl("foreign_type"),
>> +                words("declaration: error:"),
>> +                words("unrecognized or unexpected"), quote("where"),
>> +                words("attribute"), quote(EndTermStr), suffix("."), nl],
>
> "unrecognized or unexpected": pick only one.

The existing error message used that wording; I have changed it to just
"unrecognized".

> I would also add a verbose_only message component that lists
> the two attributes that *are* expected in this posititon.

Done, the verbose part reads as follows:

     bad_foreign_type.m:066:   Recognized `where' attributes have the form
     bad_foreign_type.m:066:   `equality is <<equality pred name>>' and
     bad_foreign_type.m:066:   `comparison is <<comparison pred name>>'.

(We should do something similar for regular type declarations as well.)

>> @@ -1358,16 +1376,37 @@ maybe_unify_compare(MaybeEqPred, MaybeCmpPred) =
>>   % Predicates useful for parsing several kinds of type definitions.
>>   %
>>
>> -parse_type_defn_head(ModuleName, VarSet, HeadTerm, MaybeTypeCtorAndArgs) :-
>> +parse_type_defn_head(ParseContext, ModuleName, VarSet, HeadTerm,
>> +        MaybeTypeCtorAndArgs) :-
>>       (
>> -        HeadTerm = term.variable(_, Context),
>> -        Pieces = [words("Error: variable on LHS of type definition."), nl],
>> +        HeadTerm = term.variable(Var, Context),
>> +        (
>> +            ParseContext = tdhpc_type_defn,
>> +            Pieces = [words("Error: variable on LHS of type definition."), nl]
>> +        ;
>> +            ParseContext = tdhpc_foreign_type_pragma,
>> +            VarName = varset.lookup_name(VarSet, Var),
>> +            Pieces = [
>> +                words("In second argument of"), pragma_decl("foreign_type"),
>> +                words("declaration: error: expected a type name declared"),
>> +                words("using a"), decl("type"), words("declaration, got"),
>> +                words("type variable"), quote(VarName), suffix(".")
>> +            ]
>> +        ),
>
> Again, I would pass the ContextPieces here, as an argument of tdhpc_foreign_type_pragma.

Done.

> Also, I would expand LHS in the error message for tdhpc_type_defn.

I've left this for a separate change as it isn't related to foreign_type
pragmas.

>>           Spec = error_spec(severity_error, phase_term_to_parse_tree,
>>               [simple_msg(Context, [always(Pieces)])]),
>>           MaybeTypeCtorAndArgs = error2([Spec])
>>       ;
>>           HeadTerm = term.functor(_, _, HeadContext),
>> -        ContextPieces = cord.singleton(words("In type definition:")),
>> +        (
>> +            ParseContext = tdhpc_type_defn,
>> +            ContextPieces = cord.singleton(words("In type definition:"))
>> +        ;
>> +            ParseContext = tdhpc_foreign_type_pragma,
>> +            ContextPieces = cord.from_list([
>> +                words("In"), pragma_decl("foreign_type"), words("declaration:")
>> +            ])
>> +        ),
>
> You could use the ContextPieces here as well, and they would be more precise,
> since they would give the argument number.

Done.

>> +bad_foreign_type.m:029: In `:- pragma foreign_type' declaration: error: atom
>> +bad_foreign_type.m:029:   expected at 1111.
>> +bad_foreign_type.m:033: In third argument of `:- pragma foreign_type'
>> +bad_foreign_type.m:033:   declaration: error: invalid foreign type descriptor
>> +bad_foreign_type.m:033:   `2222'.
>
> This may be separate from your change, but I think I would prefer to word
> both of those messages following the "expected abc, got def" template.

Addressing the first one will require changing how the predicates in
compiler/parse_sym_name.m handle errors.  That's definitely a separate
change as they're used all over the place.

I've changed the second one so that it now reads as follows:

      bad_foreign_type.m:033: In third argument of `:- pragma foreign_type'
      bad_foreign_type.m:033:   declaration: error: expected a string specifying the
      bad_foreign_type.m:033:   foreign type descriptor, got `2222'.

> Apart from that, the diff is fine.

Thanks for the review,

Julien.