[m-rev.] for review: pragma format_call

[Julien Fischer]

On Tue, 20 Sep 2022, Zoltan Somogyi wrote:

> I have not made any changes to the reference manual since addressing
> the review of its diff.
>
> This diff has a ZZZ in add_pragma.m, on code that checks the status
> of the new pragma the same way that we check the status of other
> declarative pragmas: we generate an error if the predicate (or function)
> to which the pragma applies is exported, but the pragma itself is not exported.
>
> I think this test is wrong. We should generate an error in the *opposite*
> case: the pragma is exported, but the pred/func it applies to is not exported.

We should definitely be doing that.

> The case when the pred/func is exported but the declarative pragma is not
> exported should merit only a warning, because the other modules that
> import that pred/func can survive without knowing that e.g. that pred/func
> is obsolete, or that calls to it can be checked the same way as calls to e.g.
> string.format.

That seems reasonable.

> Add the format_call pragma to the language.
> 
> doc/reference_manual.texi:
> NEWS:
>     Document and announce the new pragma.
> 
> compiler/prog_data_pragma.m:
> compiler/prog_item.m:
>     Provide a representation for the new pragma. The part that ends up
>     being referred to from the HLDS goes into prog_data_pragma.m,
>     the part that is not needed once the HLDS has been constructed
>     goes into prog_item.m.
> 
> 
> tests/warnings/format_call_warning.{m,exp}:
>     A new test case to see whether we generate the expected set of error
>     messages for incorrect calls to a predicate with a format_call pragma.

...

> diff --git a/compiler/check_pragma_format_call.m b/compiler/check_pragma_format_call.m
> index e69de29bb..611aa17ba 100644
> --- a/compiler/check_pragma_format_call.m
> +++ b/compiler/check_pragma_format_call.m

...

> +:- pred check_for_non_input_arg_nums_in_proc(module_info::in,
> +    int::in, pair(proc_id, proc_info)::in, int::in, int::out,
> +    list(int)::in, list(int)::out) is det.
> +
> +check_for_non_input_arg_nums_in_proc(ModuleInfo, ArgNum, _ProcId - ProcInfo,
> +        !CurModeNum, !BadModeNums) :-
> +    proc_info_get_argmodes(ProcInfo, ArgModes),
> +    list.det_index1(ArgModes, ArgNum, ArgMode),
> +    ( if mode_is_input(ModuleInfo, ArgMode) then
> +        true
> +    else
> +        !:BadModeNums = [!.CurModeNum | !.BadModeNums]
> +    ),
> +    !:CurModeNum = !.CurModeNum + 1.
> +
> +%---------------------%
> +
> +% XXX should the references to format_string_values below be quoted?

Yes, I think they should be quoted.

> +
> +:- func format_call_error_too_large_arg_num(pred_info, prog_context,
> +    int, int, int, int, string) = error_spec.
> +
> +format_call_error_too_large_arg_num(PredInfo, Context, NumFormatArgs,
> +        FormatArgNum, MaxArgNum, ArgNum, FirstOrSecond) = Spec :-
> +    PredOrFunc = pred_info_is_pred_or_func(PredInfo),
> +    Pieces =
> +        format_call_error_prelude(PredInfo, NumFormatArgs, FormatArgNum) ++
> +        [words("the"), words(FirstOrSecond), words("argument of"),
> +        words("format_string_values specifies argument number"),
> +        int_fixed(ArgNum), suffix(","), words("but the"), p_or_f(PredOrFunc),
> +        words("has only"), int_fixed(MaxArgNum), words("arguments."), nl],
> +    Spec = simplest_spec($pred, severity_error, phase_parse_tree_to_hlds,
> +        Context, Pieces).
> +
> +:- func format_call_error_too_small_arg_num(pred_info, prog_context,
> +    int, int, int, string) = error_spec.
> +
> +format_call_error_too_small_arg_num(PredInfo, Context, NumFormatArgs,
> +        FormatArgNum, ArgNum, FirstOrSecond) = Spec :-
> +    Pieces =
> +        format_call_error_prelude(PredInfo, NumFormatArgs, FormatArgNum) ++
> +        [words("the"), words(FirstOrSecond), words("argument of"),
> +        words("format_string_values specifies argument number"),
> +        int_fixed(ArgNum), suffix(","),
> +        words("but argument numbers start at 1."), nl],
> +    Spec = simplest_spec($pred, severity_error, phase_parse_tree_to_hlds,
> +        Context, Pieces).
> +
> +:- func format_call_error_wrong_type(pred_info, prog_context,
> +        int, int, int, string, string, string) = error_spec.
> +

...

> diff --git a/compiler/format_call.m b/compiler/format_call.m
> index f23869245..9e1c7ec45 100644
> --- a/compiler/format_call.m
> +++ b/compiler/format_call.m
> @@ -12,16 +12,26 @@
>  %
>  % This module has two related jobs.
>  %
> -% - The first job is to generate warnings about calls to string.format,
> -%   io.format and stream.string_writer.format in which the format string
> -%   and the supplied lists of values do not agree.
> +% - The first job is to check calls to
> +%
> +%   - the Mercury library predicates and functions string.format,
> +%     io.format and stream.string_writer.format, and
> +%
> +%   - any user-written predicates or functions which have a format_call pragma
> +%
> +%   to see whether the format string and the supplied lists of values agree,
> +%   and to generate warnings for calls in which they do not.
>  %
>  %   The difficult part of this job is actually finding the values of the
>  %   variables representing the format string and the list of values to be
>  %   printed.
>  %
> -% - The second job is to try to transform well formed calls into code
> +% - The second job is to try to transform well formed calls to
> +%   string.format, io.format and stream.string_writer.format into code
>  %   that interprets the format string at compile time, rather than runtime.
> +%   (We cannot do the same for user-written predicates or functions with
> +%   format_call pragmas, because we don't know what callee intends to do

    what *the* callee

> +%   with the checked arguments.)
>  %
>  % Our general approach to the first job is a backwards traversal of the
>  % procedure body. During this traversal, we assign an id to every conjunction

...

> diff --git a/compiler/hlds_pred.m b/compiler/hlds_pred.m
> index cb59365a5..e2a3706a9 100644
> --- a/compiler/hlds_pred.m
> +++ b/compiler/hlds_pred.m

...

> @@ -701,6 +719,8 @@
>  :- pred pred_info_set_obsolete_in_favour_of(
>      maybe(list(sym_name_arity))::in,
>      pred_info::in, pred_info::out) is det.
> +:- pred pred_info_set_format_call(maybe(format_call)::in,
> +    pred_info::in, pred_info::out) is det.
>  :- pred pred_info_set_instance_method_arg_types(list(mer_type)::in,
>      pred_info::in, pred_info::out) is det.
>  :- pred pred_info_set_clauses_info(clauses_info::in,
> @@ -1145,6 +1165,32 @@ marker_name(marker_fact_table_semantic_errors, "fact_table_semantic_errors").
>                  % as obsolete, this will be "no".
>                  psi_obsolete_in_favour_of       :: maybe(list(sym_name_arity)),
> 
> +                % If this field contain yes(FormatCall), then this predicate

s/contain/contains/

> +                % has a format_call pragma, and FormatCall contains both the
> +                % <format string, values list> argument number pairs
> +                % specified in that pragma, and the context of that pragma.
> +                % If this field contains no, then the predicate does not have
> +                % a format_call pragma.

...

> diff --git a/compiler/prog_data_pragma.m b/compiler/prog_data_pragma.m
> index 49eb41211..cfb354d15 100644
> --- a/compiler/prog_data_pragma.m
> +++ b/compiler/prog_data_pragma.m
> @@ -435,6 +435,39 @@ tabled_eval_method_to_table_type(EvalMethod) = TableTypeStr :-
>              % (in the case of user-defined equality or comparison) or
>              % propagating an exception from them.
> 
> +%---------------------------------------------------------------------------%
> +%
> +% Stuff for the `format_call' pragma.
> +%
> +
> +:- interface.
> +
> +:- type format_string_values
> +    --->    format_string_values(
> +                % The format_call pragma allows uses to specify pairs of

s/uses/users/

...

> diff --git a/tests/warnings/format_call_warning.m b/tests/warnings/format_call_warning.m
> index e69de29bb..8f52b5666 100644
> --- a/tests/warnings/format_call_warning.m
> +++ b/tests/warnings/format_call_warning.m
> @@ -0,0 +1,68 @@
> +%---------------------------------------------------------------------------%
> +% vim: ts=4 sw=4 et ft=mercury
> +%---------------------------------------------------------------------------%
> +
> +:- module format_call_warning.
> +:- interface.
> +
> +:- import_module io.
> +
> +:- pred main(io::di, io::uo) is det.
> +
> +%---------------------------------------------------------------------------%
> +%---------------------------------------------------------------------------%
> +
> +:- implementation.
> +
> +:- import_module int.
> +:- import_module list.
> +:- import_module string.
> +
> +main(!IO) :-
> +    CurLevel = 6,
> +    Msg = "The record temperature is",
> +    TempA = 42.3,
> +    TempB = 43.2,
> +
> +    % This call should get warnings for all four fs/vl pairs.
> +    maybe_log_msg(CurLevel, 1,
> +        "%%s: 5.2f", [f(TempA)], [f(TempB)],
> +        "%5.2f", [s(Msg), f(TempA)], [s(Msg), f(TempB)], !IO),
> +
> +    % This call should get a warning for only the second fs/vl pair.
> +    maybe_log_msg(CurLevel, 1,
> +        "%5.2f", [f(TempA)], [f(TempB), s(Msg)],
> +        "%s: %5.2f", [s(Msg), f(TempA)], [s(Msg), f(TempB)], !IO),
> +
> +    % This call should not get any warnings.
> +    maybe_log_msg(CurLevel, 1,
> +        "%5.2f", [f(TempA)], [f(TempB)],
> +        "%s: %5.2f", [s(Msg), f(TempA)], [s(Msg), f(TempB)], !IO).
> +
> +%---------------------------------------------------------------------------%
> +
> +:- pred maybe_log_msg(int::in, int::in,
> +    string::in, list(poly_type)::in, list(poly_type)::in,
> +    string::in, list(poly_type)::in, list(poly_type)::in,
> +    io::di, io::uo) is det.
> +:- pragma format_call(pred(maybe_log_msg/10),
> +    [format_string_values(3, 4), format_string_values(3, 5),
> +     format_string_values(6, 7), format_string_values(6, 8)]).
> +
> +maybe_log_msg(CurLogLevel, MsgLevel,
> +        FormatStrA, ValuesListA1, ValuesListA2,
> +        FormatStrB, ValuesListB1, ValuesListB2, !IO) :-
> +    Diff = MsgLevel - CurLogLevel,
> +    ( if Diff >= 15 then
> +        io.format(FormatStrA, ValuesListA1, !IO)
> +    else if Diff >= 10 then
> +        io.format(FormatStrA, ValuesListA2, !IO)
> +    else if Diff >= 5 then
> +        io.format(FormatStrB, ValuesListB1, !IO)
> +    else if Diff >= 0 then
> +        io.format(FormatStrB, ValuesListB2, !IO)
> +    else
> +        io.format("%d", ValuesListB2, !IO)

Add a comment saying that this last call is intended to generate a warning
about there begin unknown format values in the call to io.format.

The testing here is not comprehensive enough. We should also have:

1. A test of the case where the new pragam has a single format_string_values
argument (i.e. the common case), since the error message has a different form
in that case.

2. A multi-module test case that checks that calls to imported predicates that
are the subject of format_call pragmas have the checks applied.

3. A format_call pragma applied to a multi-moded predicate.  (I will add
such a test after this is committed, since I can extract something from G12
for it.)

The diff looks fine otherwise.

Julien.