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

[Julien Fischer]

On Thu, 10 Nov 2022, Zoltan Somogyi wrote:

> The attached updated diff and log message includes not just the
> above fixes, but also action on many of the "XXX OPS" noted in the
> original diff. There are also new XXXs, including one about why some
> predicates in the standard library that should be able to handle arbitrary
> op tables but are currently unable to do so, despite the documentation
> implying that they can. It also includes the update to NEWS. I would
> appreciate post-commit feedback on those parts of the diff.

...

> 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/pprint.m:
>     Conform to the changes above.
>
>     Rename a function to avoid ambiguity.
> 
> library/pretty_printer.m:
> library/stream.string_writer.m:
> library/string.to_string.m:
> library/term_io.m:
>     Conform to the changes above.
> 
> library/string.m:
>     Add a note on an significant old problem.

The diff does not contain any changes to string.m

...

> NEWS:
>     Announce the user-visible changes.
> 
> tests/hard_coded/bug383.m:
>     Update this test case to use the new system of operator priorities.
> 
> tests/hard_coded/term_io_test.{m,inp}:
>     Fix white space.
> 
> extras/old_library_modules/old_mercury_term_parser.m:
> extras/old_library_modules/old_ops.m:
>     The old contents of the mercury_term_parser and ops modules,
>     in case anyone wants to continue using them instead of updating
>     their code to use their updated equivalents.

You should also update extras/README.md

> 
> samples/calculator2.m:
>     Import the old versions of mercury_term_parser and ops.

...


> diff --git a/NEWS b/NEWS
> index 63b705d3d..bdec8651d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,24 @@ Changes that may break compatibility
>
>  * The old random number generator in the `random` module has been removed.
> 
> +* The system of operator priorities in the `ops` module has been inverted.
> +  In the old ops.m, given two operators of different priorities, the one

s/In the old op.m/Previously/

> +  that bound more tightly was the one with the lower numerical priority.
> +  In the new ops.m, operators with higher numerical priorities bind

s/In the new ops.m/Now/

> +  more tightly. There are other changes as well, such as priorities
> +  no longer being interchangeable with other integers, some types and function
> +  symbols being renamed, and an updated op_table type class with a
> +  somewhat different list of methods.
> +
> +  The old contents of the `ops` module is still available as
> +  `extras/old_library_modules/old_ops`.
> +
> +* The `mercury_term_parser` module now expects the operator table
> +  to be specified using the updated op_table typeclass in the new `ops` module.

s/typeclass/type class/

> +
> +  The old contents of the `mercury_term_parser` module is still available as
> +  `extras/old_library_modules/old_mercury_term_parser`.
> +
>  Changes to the Mercury standard library
>  ---------------------------------------
> 
> @@ -298,10 +316,43 @@ Changes to the Mercury standard library
>
>  ### Changes to the `ops` module
> 
> +* The representation of operator priories has been changed. Priorities

s/priories/priorities/

> +  used to be represented as plain signed integers between 0 and 1200,

Delete plain.

> +  with higher numbers representing operators that bind less tightly.
> +  Priorities are now represented using unsigned integers between 0u and 1500u,
> +  with higher numbers representing operators that bind more tightly,
> +  and to guard against priorities being unintentionally confused with
> +  other kinds of numbers, they are now wrapped in the `prio` function symbol.

Turn that last bit into two sentences.

> +* The `assoc` type has been renamed to `arg_prio_gt_or_ge`, which
> +  reflects its semantics. Its function symbols `x` and `y` have been
> +  renamed to `arg_gt` and `arg_ge` respectively.
> +
> +* The list of methods in the op_table type class has been changed.
> +
> + - The method max_priority has been been replaced by the method
> +   loosest_op_priority.
> +
> + - The new method universal_priority has been added. It is intended to
> +   replace uses of `max_priority + 1` to denote a context that accept terms
> +   using any operator, regardless of its priority.
> +
> + - The new method tightest_op_priority has been added. It is intended to
> +   replace uses of the integer constant `0` as a priority.
> +
> + - The new method comma_priority has been added. It is intended to denote
> +   the priority of the comma character, which separates term arguments,
> +   when used as an operator.

...

> diff --git a/library/ops.m b/library/ops.m
> index 653362ce5..bca2bf45d 100644
> --- a/library/ops.m
> +++ b/library/ops.m
> @@ -30,37 +30,82 @@

...


>  :- type op_info
>      --->    op_info(
> -                ops.class,
> -                ops.priority
> +                class,
> +                priority
>              ).
> 
> +    % min_priority_for_arg(OpPriority, GtOrGe) = MinArgPriority:
> +    %
> +    % Given the priority of an operator (OpPriority) and the required
> +    % relationship between this priority and the priority of a term
> +    % in given argument position (GtOrGe), return the minim priority

s/minim/minimum/

...

> @@ -68,29 +113,31 @@
>          % Check whether a string is the name of an infix operator,
>          % and if it is, return its precedence and associativity.
>          %
> -    pred lookup_infix_op(Table::in, string::in, ops.priority::out,
> -        ops.assoc::out, ops.assoc::out) is semidet,
> +    pred lookup_infix_op(Table::in, string::in, priority::out,
> +        arg_prio_gt_or_ge::out, arg_prio_gt_or_ge::out) is semidet,
>
>          % Check whether a string is the name of a prefix operator,
>          % and if it is, return its precedence and associativity.
>          %
>      pred lookup_prefix_op(Table::in, string::in,
> -        ops.priority::out, ops.assoc::out) is semidet,
> +        priority::out, arg_prio_gt_or_ge::out) is semidet,
>
>          % Check whether a string is the name of a binary prefix operator,
>          % and if it is, return its precedence and associativity.
>          %
>      pred lookup_binary_prefix_op(Table::in, string::in,
> -        ops.priority::out, ops.assoc::out, ops.assoc::out) is semidet,
> +        priority::out, arg_prio_gt_or_ge::out, arg_prio_gt_or_ge::out)
> +        is semidet,
>
>          % Check whether a string is the name of a postfix operator,
>          % and if it is, return its precedence and associativity.
>          %
> -    pred lookup_postfix_op(Table::in, string::in, ops.priority::out,
> -        ops.assoc::out) is semidet,
> +    pred lookup_postfix_op(Table::in, string::in, priority::out,
> +        arg_prio_gt_or_ge::out) is semidet,
>
>          % Check whether a string is the name of an operator.
>          %
> +        % XXX operating system

operating system?

>      pred lookup_op(Table::in, string::in) is semidet,
>
>          % Check whether a string is the name of an operator, and if it is,

...

> @@ -185,22 +250,82 @@
>
>  :- interface.
> 
> -    % The Mercury operator table used to be the only one allowed.
> -    % The old names are no longer appropriate.
> +    % We export this type synonym to io.m (for get_op_table/set_op_table)

The whole setup with io.get/set_op_table seems clunky. The only user of
io.get_op_table is term_io and it could just call ops.init_mercury_op_table
directly. It's not clear to me why io.get/set_op_table need to exist.
(Was it required to support user-defined operators some time in the past?)

> +    % and to string.m (for string_ops and string_ops_noncanon). These all
> +    % treat the argument whose type is ops.table as being *any* op_table,
> +    % i.e. as not necessarily being the *Mercury* op table. Indeed, the notion
> +    % of being able to set the op table in io.set_op_table, and of being able
> +    % to pass an arbitrary op table to string_ops and its noncanon version
> +    % *depend* on being able to pass op tables that differ from the standard
> +    % Mercury op table. XXX Yet we do not actually support this. Regardless
> +    % of what op table the user wants to pass to set_op_table or to string_ops,
> +    % the fact "table" is a synonym for "mercury_op_table" means that all
> +    % operations on the op table get mercury_op_table's instance of the
> +    % op_table typeclass, and the instance methods of this typeclass
> +    % for mercury_op_table, quite reasonably, all look up operators in the
> +    % *Mercury* table of operators. XXX There is also the minor issue
> +    % (compared to the issue above) that the references to ops.table in io.m
> +    % and string.m are dangling references from users' points of view,
> +    % since we deliberaly prevent the definition of this type from appearing

s/deliberately/

> +    % the library manual.
> +    %
> +    % We could fix both issues by making the currrent users of this type
> +    % synonym all take arguments of any type that is an instance of the
> +    % op_table type class. That would be a breaking change, but the breakage
> +    % would be comparatively minor.
> +    %
>  :- type table == ops.mercury_op_table.
> 
> -%
> -% For use by mercury_term_parser.m, term_io.m, stream.string_writer.m.
> -%
> +%---------------------------------------------------------------------------%
> 
> -:- pred adjust_priority_for_assoc(ops.priority::in, ops.assoc::in,
> -    ops.priority::out) is det.
> +:- implementation.
> +
> +:- import_module require.
> +:- import_module uint.
>
>  %---------------------------------------------------------------------------%
> 
> -:- implementation.
> +:- pragma inline(func(min_priority_for_arg/2)).
> +
> +min_priority_for_arg(OpPriority, arg_ge) = OpPriority.
> +min_priority_for_arg(OpPriority, arg_gt) = increment_priority(OpPriority).
> +
> +% XXX OPS This predicate is not used by the Mercury compiler.

That shouldn't be an XXX; there are lots of predicates and functions in the
standard library that are not used by the Mercury compiler.

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

...

> +    % The following operators are not useful in Mercury, and are provided
> +    % only for compatibility.
> +    %
> +    %   =:=
> +    %   ==
> +    %   =\\=
> +    %   ?-
> +    %   \\==
> +    %   ~
> +    %   ~=
> +    %   when


== is part of Mercury type syntax.

> +    %
> +    % The operator
> +    %
> +    %   where
> +    %
> +    % was previously also marked as not being useful in Mercury,
> +    % but it is part of typeclass syntax.

This comment about 'where' seem unnecessary.

> +
> +    % The priorities here are derived from the priorities in prolog_ops.m,
> +    % using the equation NP = 1500 - OP, where OP is the old priority in
> +    % prolog_ops.m, and NP is the new priority here.

Replace prolog_ops.m with extras/old_library_modules/old_ops.m.

...

> diff --git a/samples/calculator2.m b/samples/calculator2.m
> index 36407a289..b8cd5c7b7 100644
> --- a/samples/calculator2.m
> +++ b/samples/calculator2.m
> @@ -32,8 +32,9 @@
>  :- import_module list.
>  :- import_module map.
>  :- import_module maybe.
> -:- import_module mercury_term_parser.
> -:- import_module ops.
> +% The next two modules are available in extras/old_library_modules.
> +:- import_module old_mercury_term_parser.
> +:- import_module old_ops.
>  :- import_module pair.
>  :- import_module require.
>  :- import_module string.

This sample is referred to by the documentation of the ops module.  It should
work with the current version of that module in the standard library, not the
one in extras.  (Also, doing this will break compilation of the samples
directory.)

The rest looks fine.

Julien.