[m-rev.] for review: better error message for bad type names

[Zoltan Somogyi]

On Wed, 20 Apr 2016 15:20:57 +1000, Mark Brown <mark at mercurylang.org> wrote:
> > For review by anyone, but I would like Mark to have a look
> > at the XXXs on the definition of the why_no_ho_inst_info type
> > in parse_type_name.m, which lists the contexts in which types
> > may not contain higher order inst information.
> 
> They should all be possible with varying degrees of additional
> programming. I left them NYI because I needed all my brain capacity
> for the core problem.

OK, I replaced the XXXs with NYIs, and a comment explaining the NYIs.

> > Generate better error messages for bad type names.
> >
> > compiler/parse_type_name.m:
> >     When trying to parse a type name and failing, we used to just print
> >     the whole term (including the correct parts) and a message of the form
> >     "ill-formed type". We know print a message for each incorrect *part*
> 
> s/know/now/

Done.
 
> >     of a type term, and make its message specific to the particular problem.
> >
> >     To help do this, base the new code on the type-name-parsing code
> >     that was already in this module (which could generate no error messages
> >     at all), but on better, though still not perfect, code that used to be
> >     in parse_item.m.
> 
> Do you mean base the new code *not* on the type-name-parsing code?

Yes. Fixed.

> > diff --git a/compiler/parse_class.m b/compiler/parse_class.m
> > index 371baad..d606a82 100644
> > --- a/compiler/parse_class.m
> > +++ b/compiler/parse_class.m
> > @@ -578,9 +578,9 @@ parse_arbitrary_constraint(VarSet, ConstraintTerm, Result) :-
> >          try_parse_sym_name_and_args(ConstraintTerm, ClassName, Args0)
> >      then
> >          ArgsResultContextPieces =
> > -            cord.singleton(words("In class or inst constraint:")),
> > -        parse_types(no_allow_ho_inst_info, VarSet, ArgsResultContextPieces,
> > -            Args0, ArgsResult),
> > +            cord.singleton(words("In class constraint:")),
> > +        parse_types(no_allow_ho_inst_info(wnhii_class_constraint),
> > +            VarSet, ArgsResultContextPieces, Args0, ArgsResult),
> >          (
> >              ArgsResult = ok1(Args),
> >              Constraint = constraint(ClassName, Args),
> 
> The change to the message makes sense given what is on this code path,
> but note that it can still be reached for an inst constraint.
> 
> It's not required as part of this change, but parse_inst_constraint/3
> ought to succeed whenever the functor is '=<'/2, producing error specs
> if the arguments cannot be parsed. That would prevent the above code
> path being taken.

Agreed. I will do that soon.

> > diff --git a/tests/invalid/combined_ho_type_inst.err_exp b/tests/invalid/combined_ho_type_inst.err_exp
> > index 3a89720..8d35d43 100644
> > --- a/tests/invalid/combined_ho_type_inst.err_exp
> > +++ b/tests/invalid/combined_ho_type_inst.err_exp
> > +combined_ho_type_inst.m:035: Error: invalid determinism category `semiet'.
> 
> That's actually a typo in the test case, which I hadn't noticed with
> the previous error messages.

Yes, I noticed that, but it turned out to be a good test anyway; it just
tests something other than what it was *intended* to test :-)

> The rest looks fine.

Thanks for the review.

Zoltan.