[m-rev.] for review: don't allow nondefault mode functions in terms

[Peter Wang]

On Fri, 30 Oct 2015 13:58:43 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> The language reference manual already disallows putting functions with
> nondefault mode and determinism signatures into terms (in section 8.3)
> We allowed it so far, and even used such code ourselves, even though
> it could lead to crashes. So this diff, which disallows such code, would
> de facto, though not de jure, change the language. Any objections to that?

As mentioned, the manual doesn't disallow it completely.  I agree with
disallowing construction of terms with a non-default mode function as a
limitation of the implementation.

> Don't allow putting functions with nondefault signatures into terms.
> 
> The code that takes the function out of the term can use the function
> as if it had the default signature. If it doesn't, this can lead to
> This fixes Mantis bug 264.

lead to crashes.

> 
> compiler/mode_errors.m:
>     Provide a way to record such errors, and to print error messages for them.
> 
> compiler/modecheck_unify.m:
>     Look for such errors when constructing terms.
> 
> browser/declarative_user.m:
>     Don't put a function with a nondefault signature (it used a subtype)
>     into a term. Enforce the subtype with a runtime assertion instead.
> 
> tests/invalid/default_ho_inst.{m,err_exp}:
>     The test case for Mantis bug 264.
> 
> tests/invalid/Mmakefile:
>     Enable the new test case.
> 
> tests/valid/Mmakefile:
>     Disable the old ho_func_call test case, since it RELIES on putting
>     functions with nondefault modes into terms. It also uses I/O states
>     in ui modes.
> 

> diff --git a/compiler/mode_errors.m b/compiler/mode_errors.m
> index db0f6cd..bde9f12 100644
> --- a/compiler/mode_errors.m
> +++ b/compiler/mode_errors.m
> @@ -142,6 +142,13 @@
>              % One of the head variables did not have the expected final inst
>              % on exit from the proc.
>  
> +    ;       mode_error_nondefault_mode_func(prog_var, cons_id,
> +                list(prog_var), one_or_more(prog_var))
> +            % mode_error_nondefault_mode_func(X, ConsId, Ys, BadYs):
> +            % In the unification X = ConsId(Ys), the BadYs are functions
> +            % that do NOT have the default mode and determinism, i.e.
> +            % they aren't functions of the form f(in, ..., in) = out is det.
> +
>      ;       purity_error_should_be_in_promise_purity_scope(
>                  negated_context_desc, prog_var)
>              % The condition of an if-then-else or the body of a negation
> @@ -344,6 +351,11 @@ mode_error_to_spec(ModeInfo, ModeError) = Spec :-
>          Spec = mode_error_unify_var_functor_to_spec(ModeInfo, Var, Name,
>              Args, Inst, ArgInsts)
>      ;
> +        ModeError = mode_error_nondefault_mode_func(Var, Name,
> +            AllArgVars, BadArgVars),
> +        Spec = mode_error_nondefault_mode_func_to_spec(ModeInfo, Var, Name,
> +            AllArgVars, BadArgVars)
> +    ;
>          ModeError = mode_error_conj(Errors, Culprit),
>          Spec = mode_error_conj_to_spec(ModeInfo, Errors, Culprit)
>      ;
> @@ -1118,6 +1130,44 @@ mode_error_unify_var_functor_to_spec(ModeInfo, X, ConsId, Args,
>  
>  %-----------------------------------------------------------------------------%
>  
> +:- func mode_error_nondefault_mode_func_to_spec(mode_info, prog_var,
> +    cons_id, list(prog_var), one_or_more(prog_var)) = error_spec.
> +
> +mode_error_nondefault_mode_func_to_spec(ModeInfo, X, ConsId,
> +        AllArgVars, BadArgVars) = Spec :-
> +    Preamble = mode_info_context_preamble(ModeInfo),
> +    mode_info_get_context(ModeInfo, Context),
> +    mode_info_get_varset(ModeInfo, VarSet),
> +    mode_info_get_module_info(ModeInfo, ModuleInfo),
> +    FunctorConsIdStr = functor_cons_id_to_string(ModuleInfo, VarSet,
> +        print_name_only, ConsId, AllArgVars),
> +    BadArgVars = one_or_more(HeadBadArgVar, TailBadArgVars),
> +    (
> +        TailBadArgVars = [],
> +        ArgPieces = [words("Argument"),
> +            quote(mercury_var_to_name_only(VarSet, HeadBadArgVar)),
> +            words("is a function")]
> +    ;
> +        TailBadArgVars = [_ | _],
> +        ArgPieces = [words("Arguments"),
> +            quote(mercury_vars_to_name_only(VarSet,
> +                [HeadBadArgVar | TailBadArgVars])),
> +            words("are functions")]
> +    ),
> +    Pieces = [words("mode error in unification of"),
> +        quote(mercury_var_to_name_only(VarSet, X)),
> +        words("and"), words_quote(FunctorConsIdStr), suffix("."), nl] ++
> +        ArgPieces ++ [words("whose argument modes and/or determinism"),
> +        words("differ from the default function signature"),
> +        words("(f(in, ..., in) = out is det)."),
> +        words("This is not allowed, because taking such arguments"),
> +        words("out of the term being constructed"),
> +        words("would lose their mode and determinism information."), nl],
> +    Spec = error_spec(severity_error, phase_mode_check(report_in_any_mode),
> +        [simple_msg(Context, [always(Preamble ++ Pieces)])]).

You can delete the comma after "allowed".

The information is not necessarily lost so I suggest replacing "would
lose" with "may lose" or "would likely lose".

> diff --git a/compiler/modecheck_unify.m b/compiler/modecheck_unify.m
> index 6a51d63..4ff5272 100644
> --- a/compiler/modecheck_unify.m
> +++ b/compiler/modecheck_unify.m
...
> +:- pred is_input_mode(mer_mode::in) is semidet.
> +
> +is_input_mode(Mode) :-
> +    (
> +        % The structural version of the input mode.
> +        Mode = (ground_inst -> ground_inst)
> +    ;
> +        % The named version of the input mode.
> +        Mode = in_mode
> +    ).
> +
> +:- pred is_output_mode(mer_mode::in) is semidet.
> +
> +is_output_mode(Mode) :-
> +    (
> +        % The structural version of the output mode.
> +        Mode = (free -> ground_inst)
> +    ;
> +        % The named version of the output mode.
> +        Mode = out_mode
> +    ).

Can you use mode_is_fully_input/output here?
Or perhaps move these to mode_util.m.

Peter