[m-rev.] for review: reversing operator priorities

[Julien Fischer]

Hi Zoltan,

On Sun, 6 Nov 2022, Zoltan Somogyi wrote:

> This implements what we discussed recently.
>
> The diff contains several "XXX OPS" markers noting further
> opportunities for improvement; I would like your feedback on these.
> I would like to resolve the ones in ops.m before commit; but intend
> to leave the others for a later commit.

> We probably should keep the old form of the ops module around,
> possibly named old_ops.m or prolog_ops.m, for anyone who
> wants it. I will make the necessary changes, including to NEWS,
> once we get to a concensus on this.

I suggest adding old_ops to extras (similar to what we until recently
had with old_term_parser.)

...

> Make higher operator priorities bind tighter.
> 
> Mercury inherited its original system of operator priorities from Prolog,
> because during its initial development, we wanted to be able execute
> the Mercury compiler using NU-Prolog and later SICStus Prolog.
> That consideration has long been obsolete, and now we may fix the
> design error that gifted Prolog with its counter-intuitive system
> of operator priorities, in which higher *numerical* operator priorities
> mean lower *actual* priorities. This diff does that.
> 
> library/ops.m:
>     Change the meaning of operator priorities, to make higher numerical
>     priorities mean also higher actual priorities.
>
>     This semantic change requires corresponding changes in any other module
>     that uses ops.m. To force this change to occur, change the type
>     representing priorities from being a synonym for a bare int to being
>     a notag wrapper around an uint.
>
>     Add a method named tightest_priority to replace the use of 0 as a priority.
>
>     Rename the max_priority method as the loosest_priority method.
>
>     Add a method named universal_priority to replace the use of
>     max_priority + 1 as a priority.
>
>     Add a function to return the priority of the comma operator,
>     to allow other modules to stop hardcoding it.
>
>     Add operations for comparing priorities and for incrementing/decrementing
>     priorities.



> diff --git a/library/ops.m b/library/ops.m
> index 653362ce5..8cb920916 100644
> --- a/library/ops.m
> +++ b/library/ops.m
> @@ -32,6 +32,11 @@
>
>      % An class describes what structure terms constructed with an operator
>      % of that class are allowed to take.
> +    %
> +    % XXX OPS For binary ops, we should specify the arg priorities as one of
> +    % left_assoc, right_assoc and non_assoc.
> +    % XXX OPS For unary ops, we should specify the arg priority using
> +    % the arg_priority type.

I'm not quite sure what you are proposing here.

>  :- type class
>      --->    infix(ops.assoc, ops.assoc)             % term Op term
>      ;       prefix(ops.assoc)                       % Op term
> @@ -46,14 +51,29 @@
>      --->    x
>      ;       y.
> 
> -    % Operators with a low "priority" bind more tightly than those
> -    % with a high "priority". For example, given that `+' has
> -    % priority 500 and `*' has priority 400, the term `2 * X + Y'
> -    % would parse as `(2 * X) + Y'.
> +% XXX OPS not yet used
> +:- type arg_priority
> +    --->    arg_prio_gt_op_prio
> +            % `The argument's priority must be higher than the priority
> +            % of the operator, i.e. the argument must bind tighter.
> +    ;       arg_prio_ge_op_prio.
> +            % `The argument's priority must be higher than or equal to
> +            % the priority of the operator, i.e. the argument must bind
> +            % at least as tightly as the operator.
> +
> +    % Operators with a higher priority bind more tightly than those
> +    % with a low priority. For example, given that `+' has priority 1000
> +    % and `*' has priority 1100, the string "2 + X * Y" would parse as
> +    % `2 + (X * Y)'.
> +    %
> +    % The range of valid operator priorities is 1 to 1500, with 1 being
> +    % the loosest and 1500 being the tightest.
>      %
> -    % The lowest priority is 0.
> +    % The universal priority 0 describes contexts that accept terms
> +    % whose principal functor may be any operator.
>      %
> -:- type priority == int.
> +:- type priority
> +    --->    prio(uint).
>
>  :- type op_info
>      --->    op_info(
> @@ -108,15 +128,31 @@
>      pred lookup_operator_term(Table::in, ops.priority::out,
>          ops.assoc::out, ops.assoc::out) is semidet,
> 
> -        % Returns the highest priority number (the lowest is zero).
> +        % Returns a priority that accepts even terms whose top functor
> +        % has the loosest priority as arguments.
> +        % XXX OPS The terminology and documentation here could be improved.
>          %
> -    func max_priority(Table) = ops.priority,
> +    func universal_priority(Table) = ops.priority,
> 
> -        % The maximum priority of an operator appearing as the top-level
> +        % Returns the loosest priority.
> +        % XXX OPS Should this be loosest_OP_priority?

For consistency with the other method names?  Yes, although that does
then leave lookup_operator_term being the odd one out.

> +        %
> +    func loosest_priority(Table) = ops.priority,
> +
> +        % Returns the tightest priority.
> +        % XXX OPS Should this be tightest_OP_priority?
> +        %
> +    func tightest_priority(Table) = ops.priority,
> +
> +        % XXX OPS Should there be a separate method for comma_priority?
> +        % All users of this module need it, since they compute arg_priority
> +        % from it.

Doesn't that assume that comma is in your operator table?

> +
> +        % The minimum priority of an operator appearing as the top-level
>          % functor of an argument of a compound term.
>          %
> -        % This will generally be the precedence of `,/2' less one.
> -        % If `,/2' does not appear in the op_table, ops.max_priority plus one
> +        % This will generally be the precedence of `,/2' plus one.
> +        % If `,/2' does not appear in the op_table, ops.loosest_priority - 1
>          % may be a reasonable value.
>          %
>      func arg_priority(Table) = ops.priority

> @@ -395,8 +477,51 @@ mercury_op_table_postfix_op(Info, MaybeOtherInfo, Priority, LeftAssoc) :-
>
>  :- pragma inline(pred(adjust_priority_for_assoc/3)).
> 
> -adjust_priority_for_assoc(Priority, y, Priority).
> -adjust_priority_for_assoc(Priority, x, Priority - 1).
> +adjust_priority_for_assoc(Priority, Assoc, AdjustedPriority) :-
> +    (
> +        Assoc = y,
> +        AdjustedPriority = Priority
> +    ;
> +        Assoc = x,
> +        AdjustedPriority = increment_priority(Priority)
> +    ).
> +
> +% XXX OPS This predicate is unused.

Specifically, it is unused by the Mercury compiler; user code
may have a use for it.

> +decrement_priority(prio(P)) = prio(DecP) :-
> +    trace [compile_time(flag("enforce_ops_priority_bounds"))] (
> +        ( if prio(P) = mercury_op_table_loosest_priority then
> +            unexpected($pred, "decrementing loosest priority")
> +        else
> +            true
> +        )
> +    ),
> +    ( if P = 0u then
> +        unexpected($pred, "decrementing 0")
> +    else
> +        DecP = P - 1u
> +    ).

That fine otherwise; I only gave your conversion of the priorities in
the table a cursory glance -- I'm assuming a bootcheck would catch
any non-obvious errors.

Julien.