[m-rev.] for review: better error message for bad type names
zoltan.somogyi at runbox.com
Wed Apr 20 17:02:03 AEST 2016
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*
> > 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?
> > 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.
More information about the reviews