[m-rev.] for review: warn about unneeded initial and final state vars

[Peter Wang]

On Thu, 22 May 2025 12:21:48 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> Refine unused final statevar warnings.
> 

> diff --git a/compiler/pre_typecheck.m b/compiler/pre_typecheck.m
> index f36558037..ce43cb464 100644
> --- a/compiler/pre_typecheck.m
> +++ b/compiler/pre_typecheck.m
> @@ -423,64 +436,224 @@ warn_about_any_unneeded_initial_statevars(PredInfo, HeadVars,
>  
>  %---------------------%
>  
> +:- pred maybe_warn_about_unneeded_final_statevar(module_info::in,
> +    pred_info::in, set(string)::in, prog_context::in, list(clause)::in,
> +    uint::in, init_and_final::in,
> +    list(error_spec)::in, list(error_spec)::out) is det.
> +
> +maybe_warn_about_unneeded_final_statevar(ModuleInfo, PredInfo,
> +        BodyVarSVarNameSet, HeadClauseContext, TailClauses,
> +        InitArgNum, InitAndFinal, !Specs) :-
> +    InitAndFinal = init_and_final(FinalArgNum, SVarNameSet),
> +    set.intersect(BodyVarSVarNameSet, SVarNameSet, SVarNameSetInBody),
> +    ( if set.is_non_empty(SVarNameSetInBody) then
> +        % The body of at least one clause in the definition of this predicate
> +        % (as opposed to the clause heads) contains a reference to an initial
> +        % or to a final version of this state var. We should therefore report
> +        % that only the *final* statevar argument is unneeded.
> +        %
> +        % There is one exception to this. For state variables that represent
> +        % unique state, accessing the initial version *and preserving
> +        % uniqueness in a way that is visible to the caller* requires
> +        % returning a new version of the state var, so that its mode
> +        % can tell the caller that the returned reference is still unique.
> +        ( if
> +            pred_args_are_free_of_declared_uniqueness(ModuleInfo, PredInfo,
> +                InitArgNum, FinalArgNum)
> +        then
> +            warn_about_unneeded_final_statevar(PredInfo, HeadClauseContext,
> +                TailClauses, FinalArgNum, SVarNameSet, !Specs)
> +        else
> +            true
> +        )
> +    else
> +        % The definition of this predicate is missing not only any middle
> +        % occurrence of this state var, it is also missing any initial
> +        % or final occurrence, except in the clause heads.
> +        % We should therefore report that *both* arguments are unneeded.
> +        %
> +        % Uniqueness concerns do not apply here, because if the programmer
> +        % deletes both arguments, the reference to the unique state that
> +        % the caller would have passed to PredInfo is still available to it.

PredInfo -> to the predicate

> +        warn_about_unneeded_initial_final_statevar(PredInfo, HeadClauseContext,
> +            TailClauses, InitArgNum, FinalArgNum, SVarNameSet, !Specs)
> +    ).
> +
> +%---------------------%
> +
> +:- pred gather_clause_body_vars(clause::in,
> +    set_of_progvar::in, set_of_progvar::out) is det.

I suggest renaming this to gather_clause_body_non_svar_copy_vars
or similar.

> +
> +gather_clause_body_vars(Clause, !BodyVars) :-
> +    BodyGoal = Clause ^ clause_body,
> +    % Note that even for clauses that are facts in the source code,
> +    % such as "noop(!IO).", will contain a nonempty body goal, since
> +    % the conversion to HLDS will create a clause of the form
> +    %
> +    %   noop(STATE_VARIABLE_IO_0, STATE_VARIABLE_IO) :-
> +    %       STATE_VARIABLE_IO = STATE_VARIABLE_IO_0.
> +    %
> +    % What our caller wants to know, for a given state var, is not
> +    % "do progvars representing versions of this state var occur in the
> +    % body goal of the clause", because we know in advance that for any
> +    % state var that occurs in the clause head, the answer must be "yes".
> +    % What it wants to know instead is "do progvars representing versions
> +    % of this state var occur in the body goal of the clause *outside of"
> +    % unify goals that just copy state var values".

I suggest:

"body goal of the clause" -> "clause body"

"outside of" -> "other than"

> +    %
> +    % Here we gather the data that allows our caller to answer that question
> +    % for any state var.
> +    non_svar_copy_goal_vars(BodyGoal, BodyGoalVars),
> +    set_of_var.union(BodyGoalVars, !BodyVars).
> +

> diff --git a/compiler/state_var.m b/compiler/state_var.m
> index 8527a4de6..b113f56eb 100644
> --- a/compiler/state_var.m
> +++ b/compiler/state_var.m
> @@ -605,6 +614,57 @@ new_state_var_instance(StateVar, NameSource, Var, !UrInfo) :-
>  initial_state_var_name(SVarName) = ProgVarName :-
>      ProgVarName = string.format("STATE_VARIABLE_%s_0", [s(SVarName)]).
>  
> +is_prog_var_for_some_state_var(VarSet, Var, SVarName) :-
> +    % All prog_vars representing state vars are named.
> +    varset.search_name(VarSet, Var, VarName),
> +    string.remove_prefix("STATE_VARIABLE_", VarName, AfterStdPrefix),
> +    UnderscoreSeparatedPieces = string.split_at_char('_', AfterStdPrefix),
> +    (
> +        UnderscoreSeparatedPieces = [],
> +        % There is no underscore, so there can be no numerical suffix.
> +        SVarName = AfterStdPrefix
> +    ;
> +        UnderscoreSeparatedPieces = [_ | _],
> +        list.det_last(UnderscoreSeparatedPieces, LastPiece),
> +        string.to_int(LastPiece, _N),
> +        % This is either the initial or a middle version of SVarName.
> +        % (Initial if _N is zero, and middle otherwise)
> +        NumericalSuffix = "_" ++ LastPiece,
> +        SVarName = string.det_remove_suffix(AfterStdPrefix, NumericalSuffix)
> +    ).

You could use string.find_last_char and string.unsafe_between.

> diff --git a/tests/warnings/unneeded_final_statevar.m b/tests/warnings/unneeded_final_statevar.m
> index e69de29bb..6e24ec7c4 100644
> --- a/tests/warnings/unneeded_final_statevar.m
> +++ b/tests/warnings/unneeded_final_statevar.m
> @@ -0,0 +1,44 @@
> +%---------------------------------------------------------------------------%
> +% vim: ts=4 sw=4 et ft=mercury
> +%---------------------------------------------------------------------------%
> +
> +:- module unneeded_final_statevar.
> +:- interface.
> +:- import_module io.
> +
> +:- pred p1(io::di, io::uo) is det.
> +:- pred p2(int::mdi, int::muo) is semidet.
> +:- pred p3(int::mdi, int::muo) is semidet.
> +
> +%---------------------------------------------------------------------------%
> +
> +:- implementation.
> +:- import_module int.
> +
> +%---------------------------------------------------------------------------%
> +
> +% p1 should not get a warning, due to the heuristic rule that says
> +% "do not generate unused final statevar warnings for a predicate
> +% if it is defineing clauses are all facts".

defining

I have tested the patch, and it appears to work well. Thanks.

Peter