[m-rev.] for review: better error messages for bad inst and mode names

[Julien Fischer]

Hi Zoltan,

On Fri, 22 Apr 2016, Zoltan Somogyi wrote:

> Generate better error messages for bad insts and modes.
> 
> compiler/parse_inst_mode_name.m:
>     When trying to parse an inst or mode name and failing, we used to just
>     print the whole term (including the correct parts) and a message about
>     the mode being invalid. We now print a message for each incorrect *part*
>     of a term, and make its message specific to the particular problem.

...

> diff --git a/compiler/parse_class.m b/compiler/parse_class.m
> index d606a82..cde77bd 100644
> --- a/compiler/parse_class.m
> +++ b/compiler/parse_class.m
> @@ -567,9 +567,36 @@ parse_arbitrary_constraint_list(VarSet, HeadTerm, TailTerms, Result) :-
>
>  parse_arbitrary_constraint(VarSet, ConstraintTerm, Result) :-
>      ( if
> -        parse_inst_constraint(ConstraintTerm, InstVar, Inst)
> +        ConstraintTerm = term.functor(term.atom("=<"), [LHSTerm, RHSTerm], _)
> +    then
> +        (
> +            LHSTerm = term.variable(InstVar0, _),
> +            term.coerce_var(InstVar0, InstVar1),
> +            MaybeInstVar = ok1(InstVar1)
> +        ;
> +            LHSTerm = term.functor(_, _, LHSContext),
> +            LHSTermStr = describe_error_term(VarSet, LHSTerm),
> +            LHSPieces = [words("Error: a non-variable inst such as"),

Add some commas there.

     Error: a non-variable inst, such as `foo', may not be the

> diff --git a/compiler/parse_inst_mode_name.m b/compiler/parse_inst_mode_name.m
> index 489d710..6de7a7d 100644
> --- a/compiler/parse_inst_mode_name.m
> +++ b/compiler/parse_inst_mode_name.m
> @@ -15,39 +15,56 @@

...

>
>      % XXX These predicates should take a ContextPieces argument, be det,
>      % and return a maybe1(...) result.
>      %
> -:- pred convert_mode_list(allow_constrained_inst_var::in, list(term)::in,
> -    list(mer_mode)::out) is semidet.
> -:- pred convert_mode(allow_constrained_inst_var::in, term::in, mer_mode::out)
> -    is semidet.
> +:- pred parse_modes(allow_constrained_inst_var::in, varset::in,
> +    cord(format_component)::in, list(term)::in, maybe1(list(mer_mode))::out)
> +    is det.
> +:- pred parse_mode(allow_constrained_inst_var::in, varset::in,
> +    cord(format_component)::in, term::in, maybe1(mer_mode)::out) is det.

Presumably you can delete the XXX comment above now?

>  %---------------------------------------------------------------------------%
> @@ -60,62 +77,137 @@
>  :- import_module mdbcomp.prim_data.
>  :- import_module mdbcomp.sym_name.
>  :- import_module parse_tree.parse_sym_name.
> +:- import_module parse_tree.parse_tree_out_term.
>  :- import_module parse_tree.parse_util.
>  :- import_module parse_tree.prog_util.
>
>  :- import_module bool.
>  :- import_module set.
> +:- import_module string.

...

> +parse_mode(AllowConstrainedInstVar, VarSet, ContextPieces, Term, MaybeMode) :-
> +    (
> +        Term = term.variable(_, Context),
> +        TermStr = describe_error_term(VarSet, Term),
> +        Pieces = cord.list(ContextPieces) ++ [lower_case_next_if_not_first,
> +            words("Error: a variable such as"), quote(TermStr),
> +            words("is not a valid mode."), nl],
> +        Spec = error_spec(severity_error, phase_term_to_parse_tree,
> +            [simple_msg(Context, [always(Pieces)])]),
> +        MaybeMode = error1([Spec])
> +    ;
> +        Term = term.functor(TermFunctor, ArgTerms0, Context),
> +        (
> +            (
> +                TermFunctor = term.integer(_),
> +                Name = "an integer"
> +            ;
> +                TermFunctor = term.big_integer(_, _),
> +                Name = "an integer"
> +            ;
> +                TermFunctor = term.float(_),
> +                Name = "a floating point number"
> +            ;
> +                TermFunctor = term.string(_),
> +                Name = "a string"
> +            ;
> +                TermFunctor = term.implementation_defined(_),
> +                Name = "an implementation defined literal"
> +            ),
> +            TermStr = describe_error_term(VarSet, Term),
> +            Pieces = cord.list(ContextPieces) ++ [lower_case_next_if_not_first,
> +                words("Error:"), words(Name), words("such as"), quote(TermStr),

Ditto w.r.t to commas.

...

> @@ -499,65 +787,199 @@ convert_higher_order_inst(AllowConstrainedInstVar, BeforeIsTerm, DetTerm,
>              ;
>                  IsAny = yes,
>                  Inst = any(shared, higher_order(FuncInst))
> +            ),
> +            MaybeInst = ok1(Inst)
> +        else
> +            Specs = get_any_errors1(MaybeArgModes0)
> +                ++ get_any_errors1(MaybeRetMode)
> +                ++ get_any_errors1(MaybeDetism),
> +            MaybeInst = error1(Specs)
>          )
> +    else
> +        Pieces = cord.list(ContextPieces) ++ [lower_case_next_if_not_first,
> +            words("Error: a higher-order ints should have"),

Typo: s/ints/inst/

> +            words("one of the following forms:"), nl,
> +            quote("pred(<inst1>, ...) is <detism>"), nl,
> +            quote("any_pred(<inst1>, ...) is <detism>"), nl,
> +            quote("func(<inst1>, ...) = <return_inst> is <detism>"), nl,
> +            quote("any_func(<inst1>, ...) = <return_inst> is <detism>"), nl,
> +            suffix("."), nl],
> +        Spec = error_spec(severity_error, phase_term_to_parse_tree,
> +            [simple_msg(get_term_context(BeforeIsTerm), [always(Pieces)])]),
> +        MaybeInst = error1([Spec])
>      ).

....

> +            TermStr = describe_error_term(VarSet, Term),
> +            Pieces = cord.list(ContextPieces) ++ [lower_case_next_if_not_first,
> +                words("Error: an implementation defined literal"),
> +                words("such as"), quote(TermStr),

Add commas.

> +                words("may not be a used as a bound inst."), nl],
> +            Spec = error_spec(severity_error, phase_term_to_parse_tree,
> +                [simple_msg(Context, [always(Pieces)])]),
> +            MaybeBoundInst = error1([Spec])
>          ;
>              ( Functor = term.integer(_)
>              ; Functor = term.big_integer(_, _)
>              ; Functor = term.float(_)
>              ; Functor = term.string(_)
>              ),
> -        Args1 = Args0,
> -        list.length(Args1, Arity),
> -        make_functor_cons_id(Functor, Arity, ConsId)
> +            (
> +                ArgTerms0 = [],
> +                det_make_functor_cons_id(Functor, 0, ConsId),
> +                BoundInst = bound_functor(ConsId, []),
> +                MaybeBoundInst = ok1(BoundInst)
> +            ;
> +                ArgTerms0 = [_ | _],
> +                ( Functor = term.integer(_),        FunctorStr = "integer"
> +                ; Functor = term.big_integer(_, _), FunctorStr = "integer"
> +                ; Functor = term.float(_),          FunctorStr = "float"
> +                ; Functor = term.string(_),         FunctorStr = "string"
>                  ),
> -    convert_inst_list(AllowConstrainedInstVar, Args1, Args),
> -    BoundInst = bound_functor(ConsId, Args).
> +                TermStr = describe_error_term(VarSet, Term),
> +                Pieces = cord.list(ContextPieces) ++
> +                    [lower_case_next_if_not_first, words("Error:"),
> +                    words("a"), words(FunctorStr), words("such as"),

For FunctorStr = "integer", that should read "*an* integer" not "a integer".
Also, add commas around "such as ...".

> +                    quote(TermStr), words("may not have any arguments."), nl],
> +                Spec = error_spec(severity_error, phase_term_to_parse_tree,
> +                    [simple_msg(Context, [always(Pieces)])]),
> +                MaybeBoundInst = error1([Spec])
> +            )
> +        )
> +    ).

...

> diff --git a/compiler/parse_item.m b/compiler/parse_item.m
> index 35d89c1..f708d53 100644
> --- a/compiler/parse_item.m
> +++ b/compiler/parse_item.m
> @@ -808,8 +808,11 @@ parse_pred_or_func_decl_item(ModuleName, VarSet, Functor, ArgTerms,
>      ( if ArgTerms = [Term] then
>          parse_determinism_suffix(VarSet, Term, BeforeDetismTerm,
>              MaybeMaybeDetism),
> -        parse_with_inst_suffix(BeforeDetismTerm, BeforeWithInstTerm,
> -            MaybeWithInst),
> +        WithInstContextPieces = cord.from_list([
> +            words("In the with_inst annotation of a"),

Quote wth_inst.

> +            words(pred_or_func_to_string(PredOrFunc)), words("declaration:")]),
> +        parse_with_inst_suffix(VarSet, WithInstContextPieces,
> +            BeforeDetismTerm, BeforeWithInstTerm, MaybeWithInst),
>          parse_with_type_suffix(VarSet, BeforeWithInstTerm, BeforeWithTypeTerm,
>              MaybeWithType),
>          BaseTerm = BeforeWithTypeTerm,

The rest looks fine.

Julien.