[m-rev.] for review: check bound insts in coerce ops more systematically

[Peter Wang]

On Wed, 29 May 2024 18:17:10 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> Report coerce inst errors more systematically.
> 
> compiler/modecheck_coerce.m:
>     When checking the bound_insts occurring in the initial inst of the
>     coerced term, use the same mechanism to handle
>     
>     - any cons_ids they contain that do not occur in the input type,
>     - any cons_ids they contain that do occur in the input type but are
>       used in the bound_inst with the wrong arity, and
>     - any cons_ids they contain that do not occur in the result type.
> 
>     The new mechanism allows us to catch any combination of these kinds
>     of errors in the same list of bound insts. The old mechanism could not
>     do this, partially because it only ever tried collect at most one
>     error of some of the above kinds.
> 
>     Use more standard variable names.
> 
> compiler/mode_errors.m:
>     Change the representation of coerce mode errors to accommodate
>     the new capability in modecheck_coerce.m, and change the code
>     that reports such mode errors accordingly.
> 
> tests/invalid/coerce_int.m:
>     Make this case just a bit tougher.
> 
> tests/invalid/coerce_int.err_exp:
> tests/invalid/coerce_mode_error2.err_exp:
> tests/invalid/coerce_recursive_type.err_exp:
>     Expect updated diagnostics.

> diff --git a/compiler/mode_errors.m b/compiler/mode_errors.m
> index 2bbe89cd2..8f97c924b 100644
> --- a/compiler/mode_errors.m
> +++ b/compiler/mode_errors.m
> @@ -314,17 +314,28 @@
>  
>  :- type coerce_error_reason
>      --->    input_inst_not_ground(mer_inst)
> -    ;       invalid_inst_for_input_type(mer_inst)
> -    ;       invalid_cons_ids_for_result_type(list(cons_id))
> +    ;       cons_id_errors(mer_inst,
> +                bound_inst_cons_id_error, list(bound_inst_cons_id_error))
>      ;       has_inst_expect_upcast(mer_inst).
>  
> +:- type bound_inst_cons_id_error
> +    --->    bad_cons_id_input(cons_id)
> +            % The cons_id does not exists in the input type.

does not exist

> +    ;       bad_cons_id_input_inst_arity(cons_id, arity, arity)
> +            % The cons_id exists in the input type, but the inst specifies
> +            % the wrong arity for it. XXX How?
> +            % The cons_id, its arity in the inst of X, and the expected arity.

I wasn't able to (quickly) construct a test case which triggers this
error.

I may have just have put that arity check in
modecheck_coerce_from_bound_make_bound_functor out of caution,
since as the comment says:

    % The user may have provided an inst that does not make sense for the type.
    % The compiler does not check for that elsewhere (probably it should)
    % so we try to be careful about that here.

You may want to merge bad_cons_id_input_inst_arity into
bad_cons_id_input.

> +    ;       bad_cons_id_result(cons_id).
> +            % The cons_id does not exists in the result type.

does not exist

> diff --git a/compiler/modecheck_coerce.m b/compiler/modecheck_coerce.m
> index 5b21b1b05..524dffed8 100644
> --- a/compiler/modecheck_coerce.m
> +++ b/compiler/modecheck_coerce.m
...
> @@ -354,14 +354,15 @@ is_user_inst(InstName) :-
>      list(bound_inst)::in, maybe_coerce_error(mer_inst)::out) is det.
>  
>  modecheck_coerce_from_bound_make_bound_inst(ModuleInfo, TVarSet, LiveX, UniqX,
> -        RevTermPath0, TypeX, TypeY, ExpandedInsts0, InstX, FunctorsX, Res) :-
> +        RevTermPath0, TypeX, TypeY, ExpandedInsts0, InstX, BoundInstsX,
> +        Result) :-
>      modecheck_coerce_from_bound_make_bound_functors(ModuleInfo, TVarSet, LiveX,
> -        RevTermPath0, TypeX, TypeY, ExpandedInsts0, InstX,
> -        FunctorsX, FunctorsY, BadConsIds, no, MaybeError0),
> +        RevTermPath0, TypeX, TypeY, ExpandedInsts0, BoundInstsX, BoundInstsY,
> +        BadConsIdErrors, [], DeeperErrors),
>      (
> -        BadConsIds = [],
> +        BadConsIdErrors = [],
>          (
> -            MaybeError0 = no,
> +            DeeperErrors = [],
>              UniqY = uniqueness_for_coerce_result(LiveX, UniqX),
>              % XXX A better approximation of InstResults is probably possible.
>              InstResults = inst_test_results(
> @@ -372,75 +373,76 @@ modecheck_coerce_from_bound_make_bound_inst(ModuleInfo, TVarSet, LiveX, UniqX,
>                  inst_result_contains_types_unknown,
>                  inst_result_no_type_ctor_propagated
>              ),
> -            InstY = bound(UniqY, InstResults, FunctorsY),
> -            Res = ok(InstY)
> +            InstY = bound(UniqY, InstResults, BoundInstsY),
> +            Result = ok1(InstY)
>          ;
> -            MaybeError0 = yes(Error),
> -            Res = coerce_error(Error)
> +            DeeperErrors = [_ | _],
> +            Result = error1(DeeperErrors)
>          )
>      ;
> -        BadConsIds = [_ | _],
> +        BadConsIdErrors = [HeadBadConsIdError | TailBadConsIdErrors],
>          % This will prefer to report any invalid functors in the current
>          % `bound()' inst over an error found deeper in the inst tree
>          % (in MaybeError0).

Update or delete this comment (MaybeError0 to DeeperErrors).

>          list.reverse(RevTermPath0, TermPath),
> -        Reason = invalid_cons_ids_for_result_type(BadConsIds),
> -        Error = coerce_error(TermPath, TypeX, TypeY, Reason),
> -        Res = coerce_error(Error)
> +        Reason = cons_id_errors(InstX,
> +            HeadBadConsIdError, TailBadConsIdErrors),
> +        ConsIdError = coerce_error(TermPath, TypeX, TypeY, Reason),
> +        Result = error1([ConsIdError | DeeperErrors])
>      ).
>  

That looks fine.

Peter