[m-rev.] for review: disable_warning [unknown_format_calls]

[Julien Fischer]

Hi Zoltan,

On Wed, 7 Sep 2022, Zoltan Somogyi wrote:

> On Wed, 7 Sep 2022 16:45:35 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
>> Unrelated: is it feasible to extend the checking done by compiler/format_call.m
>> to check wrappers around io.format etc. like pd_debug_message?
>
> It is doable, but only for some use cases, not all.
>
> Specifically, your proposal would not help two of the scopes in DIFF.ufc2:
>
> 1. In parse_tree_out_info.m, there is output_format/5. Putting this pragma on that
> predicate would not help, because it is never called directly. Instead of calling
> output_format, mercury_to_mercury.m calls add_format/5, the typeclass method
> that is implemented by output_format. So the pragma would have to be placed
> on add_format, and (at the moment) we do not allow pragmas to be placed
> on typeclass methods. (The attached diff deletes this method.)
>
> 2. In string.m, we have
>
> +    disable_warning [unknown_format_calls] (
> +        format("%#." ++ int_to_string(Prec) ++ "g", [f(Float)], Tmp)
> +    ),
>
> In this case, the reason why we need the scope is that the result of
> int_to_string(Prec) is not a constant, because Prec is an input. For this,
> the right fix is not the pragma, but an expansion of formats to
> support the specification of parameters such as precisions
> not in the format string, but in polytypes. C's printf supports
> something similar, e.g. with printf("%.*s", 3, "abcdef").
> This may be worthwhile, but I think it is far from urgent.
>
> But the other instances of disable_warning scopes for this warning
> *could* be avoided with a pragma such as you suggest.

My intent was not avoid the disable_warnings scopes, but to check calls
to the wrapper predicates.  The issue, which seeing pd_debug_message in
this diff reminded me of, is that when writing things like logging
frameworks it is quite common to have a logging predicate that wraps
{io,string,string_writer}.format. For example, this one from G12:

    :- pred log_msg(int, string, list(string.poly_type), S, S) <= g12.state(S).
    :- mode log_msg(in, in, in, mdi, muo) is det.
    :- mode log_msg(in, in, in,  di,  uo) is det.

And here's an example from a Mercury SLF4J binding that I wrote:

    :- pred format_info(logger::in, string::in, list(poly_type)::in,
       io::di, io::uo) is det.

    :- pred format_info(logger::in, marker::in, string::in, list(poly_type)::in,
       io::di, io::uo) is det.

I have been bitten a non-trivial number of times by calls to log_msg/5
above containing invalid format strings. Had these been calls to, for
example, string.format the compiler would have caught such errors.

> The pragma would need its own field in pred_sub_info,
> which is distributed fat, but only a small amount.
>
>> For example,
>> if we add a pragma like:
>>
>>      :- pragma check_format_call(pred(pd_debug_message/5), [format_args(2, 3)]).
>>
>> where format_args(2, 3) identifies the arguments containing the format string
>> and its corresponding list of poly_types. (GCC supports a function attribute
>> that does something similar for printf()).
>
> I would propose that intead of format_args(2, 3) (I see no need for the []s),
> we have two separate and more descriptive arguments, like this:
>
> :- pragma check_format_call(pred(pd_debug_message/5),
>  format_string(2), poly_types(3)).

I don't have any objections to that.  The reason I used a list was that,
in princple, you can certainly write predicates with multiple format
string and multiple list of poly_type arguments. (It's argubly, not a
particularly useful thing to do.)

Julien.