[m-dev.] tightening up require_complete_switch

[Zoltan Somogyi]

On Tue, 19 Apr 2016 11:25:19 +1000, Peter Wang <novalazy at gmail.com> wrote: 
> Assuming that require_switch_arms_* scopes could be changed as well,
> removing the [X] part means that all require_* checks would have the
> same form `require_something GOAL', a more consistent language design.

True, but I think that would be a *very* minor advantage.

> require_complete_switch is quite prevalent in code outside the compiler
> so I don't want to advocate a breaking change.  If it were possible to
> remove the [X] part with a significant transition period, then I would
> like to see that happen. 

Why? Don't you find the variable useful documentation? There are some
disjunctions that can be looked at as switches on *more* than one variable
(e.g. assoc_list.from_corresponding_lists); I find the variable name slightly
useful in that it tells me "you don't have to search for another variable
that this disjunction is a switch on". We could in future even use the
variable name in the scope to control switch detection, by checking
whether the disjunction it is given is a switch on the named variable
*before* checking to see if it is a switch on any other variables.
(In fact, I think this should be easy to implement.)

> I do not think error messages would be any harder to understand.
> I assumed reqscope_check_scope would look something like this:
> 
>         Reason = require_complete_switch(MaybeRequiredVar),
>         SubGoal = hlds_goal(SubGoalExpr, _),
>         ( if SubGoalExpr = switch(SwitchVar, CanFail, Cases) then
> 	    ... report if SwitchVar != RequiredVar (if supplied)
> 	    ... report if switch on SwitchVar is incomplete
>         else
> 	    ... report that SubGoal is not a switch
>         )

Those are not the error messages I am thinking of. I am thinking
of error messages that refer to (a) syntax errors (b) during the necessary
transition period when we support both the presence and the absence
of the [X]. I have absolutely no idea what library/parser.m will do
when given an operator that is both a unary prefix and a binary prefix
operator.

> > diff --git a/doc/reference_manual.texi b/doc/reference_manual.texi
> > index ef57532..611093e 100644
> > --- a/doc/reference_manual.texi
> > +++ b/doc/reference_manual.texi
> > @@ -950,15 +950,13 @@ for all the function symbols in the type of @var{Var},
> >  then @code{require_complete_switch [@var{Var}] @var{Goal}}
> >  is equivalent to @var{Goal}.
> >  If @var{Goal} is a switch on @var{Var} but it is not a complete switch,
> > -i.e. it has no arm
> > -for at least one of the function symbols in the type of @var{Var},
> > -then the compiler is required to generate an error message.
> > +i.e. if some of the function symbols in the type of @var{Var}
> > +that @var{Var} could be bound to at this point in the code
> > +are not covered by any arm of the switch,
> > +then the compiler is required to generate an error message,
> > +preferably one that lists the uncovered function symbols.
> 
> The "preferably one that lists the uncovered function symbols" part is
> jarring when read from a user's point of view (the main audience), so I
> suggest deleting it.

If there is nothing there, then the text would say the same thing, i.e.
"is required to generate an error message", for both (a) the subgoal
is a switch on X, but it is not complete, and (b) the subgoal is not
a switch on X. I was trying to differentiate the two; (a) is the intended
purpose of the scope, while (b) is just a technicality. Do you not think
that emphasizing the intended purpose is worthwhile? Do you have
a better way to do it?

I think the text would read better without the "preferably", but the
checking code does not seem to *guarantee* finding the relevant function
symbols, and we shouldn't promise what we can't deliver.

Thanks for the review.

Zoltan.