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

[Zoltan Somogyi]

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.

> @@ -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.

> +        ;
> +            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.

> --- 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"

> +            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.

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

> @@ -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.

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

>           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.

> +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.

Apart from that, the diff is fine.

Zoltan.