[m-rev.] for post-commit review: more specific warnings about unresolved polymorphism

[Julien Fischer]

On Sat, 3 May 2025 at 17:46, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
> Make a warning more specific.

...

> diff --git a/compiler/var_origins.m b/compiler/var_origins.m
> new file mode 100644
> index 000000000..1540fc3ba
> --- /dev/null
> +++ b/compiler/var_origins.m
> @@ -0,0 +1,809 @@
> +%---------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%---------------------------------------------------------------------------%
> +% Copyright (C) 2025 The Mercury team.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%---------------------------------------------------------------------------%
> +%
> +% This module provides mechanisms to
> +%
> +% - map each variable in a predicate to its earliest occurrence in each
> +%   clause of a given predicate (its origins), and
> +%
> +% - to map descriptions of such origins into text that can be included
> +%   in error messages.
> +%
> +% The overall intention is to improve the readability of error messages
> +% such as
> +%
> +%   Warning: unresolved polymorphism in predicate `maybe_throw_pred'/2.
> +%   The variable with an unbound type was:
> +%       `V_7': exception.exception_result(T).
> +%   The unbound type variable `T' will be implicitly bound to
> +%   the builtin type `void'.
> +%
> +% Each of these lines has traditionally been prefixed by the context
> +% of the predicate as a whole. When the subject of the diagnostic is
> +% a named variable, this does not matter, since users can look for
> +% occurrences of that variable in the predicate's code. However, for
> +% unnamed variables, they cannot. This is why this module provides
> +% the mechanisms needed to extend the diagnostic with text such as
> +%
> +%   `V_7' represents the term `exception(Exception)'.
> +%
> +% with the preceding context pinpointing the line where the unnamed
> +% variable first occurs. See tests/warnings/unresolved_polymorphism_anon.m.
> +%
> +% Because this intention is to help with error messages created during
> +% type analysis, this module does not look at, or use, any part of the HLDS
> +% that is meaningful during type analysis, such as instmap_deltas.

I think you mean mode analysis there.

...

> +:- type var_origin
> +    --->    var_origin_clause_head(
> +                % The variable's first occurrence is in the clause head.
> +                voch_context    :: prog_context,
> +
> +                % The number of the clause in the predicate's definition.
> +                % The first clause is clause #1.
> +                voch_clause     :: uint,
> +
> +                % The position of the variable in the clause head.
> +                % The first argument is argument #1.
> +                voch_arg_num    :: uint
> +            )
> +    ;       var_origin_lambda_head(
> +                % The variable's first occurrence is in the head of
> +                % a lambda expression.
> +                volh_context    :: prog_context,
> +                volh_p_or_f     :: pred_or_func,
> +                volh_arg_num    :: uint
> +            )
> +    ;       var_origin_unify_var(
> +                vouv_context    :: prog_context,
> +                % The other variable that the variable mapped to this origin
> +                % is unified with.
> +                vouv_lorhs      :: lhs_or_rhs
> +            )
> +    ;       var_origin_unify_func(
> +                vouf_context    :: prog_context,
> +
> +                % The cons_id in the unification.
> +                vouf_cons_id    :: cons_id,
> +
> +                % If the variable mapped to this origin is the LHS var,
> +                % then this lists the RHS vars.
> +                % If the variable mapped to this origin is one of the RHS vars,
> +                % then this gives its position on the RHS, as well as
> +                % the LHS var.
> +                vouf_lorhsa     :: lhs_or_rhs_arg
> +            )
> +    ;       var_origin_plain_call(
> +                vopc_context    :: prog_context,
> +                % The id of the callee, and the number of the argument
> +                % position that is occupied by the variable mapped
> +                % to this origin. (If that variable is only a *component*
> +                % of the argument, and not the whole argument, then its
> +                % origin will be unification introduced by the code of
> +                % superhomoneous.m.)
> +                vopc_callee     :: pred_id,
> +                vopc_arg_num    :: uint
> +            )
> +    ;       var_origin_foreign_call(
> +                vofc_context    :: prog_context,
> +                % We treat foreign language calls the same way as plain calls.
> +                vofc_callee     :: pred_id,
> +                vofc_arg_num    :: uint
> +            )
> +    ;       var_origin_generic_call(
> +                vogc_context    :: prog_context,
> +                % We treat treat language calls similarly to plain calls,
> +                % just with a different specification of the callee.
> +                vogc_callee     :: generic_call,
> +                vogc_arg_num    :: uint
> +            ).
> +
> +:- type lhs_or_rhs
> +    --->    lor_lhs(
> +                    rhs_var     :: prog_var
> +            )
> +    ;       lor_rhs(
> +                    lhs_var     :: prog_var
> +            ).
> +
> +:- type lhs_or_rhs_arg
> +    --->    lora_lhs(
> +                rhs_argvars     :: list(prog_var)
> +            )
> +    ;       lora_rhs(
> +                lhs_var         :: prog_var,
> +                rhs_arg_num     :: uint
> +            ).
> +
> +    % Values of this type map each variable to
> +    %
> +    % - its origin, if it has only one origin (that we have seen so far),
> +    %
> +    % - but if the variable has its origin in a (possibly nested) branched
> +    %   control structure, it will contain its origin on each branch.
> +    %   Note that multiple clauses, being equivalent to multiple disjuncts,
> +    %   count as branches of a branched control structure.
> +:- type var_origins_map == map(prog_var, set(var_origin)).
> +
> +    % compute_var_origins_in_pred(CollectPred, PredInfo0, OriginsMap, !Acc):
> +    %
> +    % Given a predicate, construct and return OriginsMap, which will map
> +    % each variable in its clauses to its set of origins.
> +    %
> +    % Note that one kind of goal, negations, introduce scopes that 'forget'
> +    % origins. This is because if a variable is generated in a negated goal,
> +    % it cannot be referred to from outside the negation. Therefore any
> +    % additions to OriginsMap inside a negated goal will be effectivel

s/effectivel/effectively/

> +    % forgotten when the traversal leaves the negation.
> +    %
> +    % This is why instead of letting our caller post-process OriginsMap,
> +    % we allow it to process each addition to OriginsMap as it is made.
> +    % The traversal will call CollectPred after each such addition,
> +    % passing it both the already-updated OriginsMap, and the pair of
> +    % <variable, origin> that was just added.
> +    %
> +:- pred compute_var_origins_in_pred(
> +    record_var_origin(T)::in(record_var_origin),
> +    pred_info::in, var_origins_map::out, T::in, T::out) is det.

...

> +:- pred update_var_origin(record_var_origin(T)::in(record_var_origin),
> +    prog_var::in, var_origin::in,
> +    var_origins_map::in, var_origins_map::out, T::in, T::out) is det.
> +
> +update_var_origin(CollectPred, Var, VarOrigin, !OriginsMap, !Acc) :-
> +    ( if map.search(!.OriginsMap, Var, _VarOriginsCord0) then
> +        % set.insert(VarOrigin, VarOriginsCord0, VarOriginsCord),
> +        % map.det_update(Var, VarOriginsCord, !OriginsMap)
> +        % ZZZ


What's the ZZZ for?


> +        true
> +    else
> +        VarOriginsCord = set.make_singleton_set(VarOrigin),
> +        map.det_insert(Var, VarOriginsCord, !OriginsMap),
> +        CollectPred(!.OriginsMap, Var, VarOrigin, !Acc)
> +    ).

...

> diff --git a/tests/warnings/unresolved_polymorphism_anon.m b/tests/warnings/unresolved_polymorphism_anon.m
> new file mode 100644
> index 000000000..a45bf4ff7
> --- /dev/null
> +++ b/tests/warnings/unresolved_polymorphism_anon.m
> @@ -0,0 +1,68 @@
> +%---------------------------------------------------------------------------%
> +% vim: ts=4 sw=4 et ft=mercury
> +%---------------------------------------------------------------------------%
> +%
> +% When the compiler generates a warning about unresolved polymorphism,
> +% sometimes the variable being complained about is an anonymous variable.
> +% Since by definition users cannot see such variables in their programs,
> +% the error message does not give them any guidance of what they can change
> +% to make the warning go away.
> +%
> +% This test case tests the operation of the code in the compiler
> +% (var_origins.m) that generates messages to tell users
> +%
> +% - both the context of the first occurrence of the anonymous variable
> +%   as a filename/linenumber pair,
> +%
> +% - and how the variable fits in with the code in which it occurs,
> +%   which should help focus their attention on the affected *part* of that
> +%   line.
> +%
> +% This test case is a cut-down form of the code that motivated the change
> +% being tested here: an unresolved polymorphism in extras/odbc/odbc.m.
> +%
> +% XXX It would be nice to test variable origins other than just "this variable
> +% represents a term constructed here". We can of coyrse add test cases

s/coyrse/course/

> +% for such situations as we come across them.

The rest looks fine.

Julien.