[m-rev.] for post-commit review: fix github issue #118

[Julien Fischer]

Hi Zoltan,

On Thu, 16 Mar 2023, Zoltan Somogyi wrote:

> The diff contains no test case, because the bug occurs when a compiler
> invocation gets, from a .int0 file, a mode declaration that specifies
> a determinism that is invalid, given the argument types of the predicate
> in question. None of the invalid* test directories is set up to handle
> test cases like this, so the test case (which is attached) has nowhere to go.
> I did check manually that the diff fixes the problem.

1. The diff below contains comments suggesting it was added (presumably
    written before you realised it wouldn't work?)

2. The new test case should go somewhere, even if we currently lack the
    means to run it; not adding it simply risks losing it.  I suggest we
    add the directory tests/manual_invalid and use it to to house such
    orphan test cases. (At the least, this lets us check that everything in
    such a directory works before a release.)

> The algorithm we use to decide what goes into .int0 files is very primitive,
> being inherited almost unchanged from Fergus's "everything is a list of items"
> design, and consists of copying items from the parse tree of the .m file
> almost unchanged to the .int0 file. We could change this, as we have already
> started doing for invalid combinations of type, inst and mode declarations
> in .int/.int2 files, but that requires more discussion up front.

...

> Fix compiler abort on non-model_det procedures.
> 
> Github issue #118 reports a compiler abort that happens when
> table_gen.m attempts to perform that transformation that implements
> the tabling of I/O actions for declarative debugging, and a sanity check
> finds that the procedure's determinism is not model_det.
> 
> We could change table_gen.m to not transform the affected predicate
> in such cases. This change could fix *this specific* compiler abort,
> but it is better to fix the root cause, which is that our semantic analysis
> passes have not detected and reported a violation of our semantic rules,
> and have allowed the affected predicate to flow through to the middle end
> to be processed.
> 
> compiler/det_analysis.m:
> compiler/det_report.m:
>     We used to check whether predicates with I/O state arguments have
>     one of the permitted model_det determinisms as part of det_infer_proc.
>     Github issue #118 arose because det_infer_proc processes only
>     predicates defined in the current module. It was also strange that
>     some properties of mode declarations were checked by det_infer_proc
>     in det_analysis.m (which can be invoked on a procedure more than once
>     during mode inference), while others were checked by
>     check_determinism_of_procs in det_report.m (which is only ever invoked
>     on a procedure once, at the end of determinism analysis).
>
>     Move the checks on mode declarations all to det_report.m. Partly
>     this is to fix the above code smell, but mainly so that we can add
>     check_determinism_of_imported_procs, which performs the part of
>     the job of check_determinism_of_procs that is appropriate for
>     imported procedures.
>
>     Make the error message for invalid mode declarations more specific
>     by listing the given invalid determinism as well as the possible
>     valid determinisms.
>
>     To make the above possible without doing unnecessary checks on
>     compiler-constructed procedures that are "born correct", separate out
>     imported procedures from born-correct procedures. Previously, they
>     were lumped together as "no inference needed" procedures.
> 
> tests/invalid/ho_unique_error.err_exp:
> tests/invalid/mostly_uniq1.err_exp:
> tests/invalid/mostly_uniq2.err_exp:
>     Expect the extra detail in error messages.
> 
> tests/invalid/io_in_ite_cond.err_exp:
> tests/invalid/magicbox.err_exp:
> tests/invalid/try_detism.err_exp:
>     Expect an error message about the invalid determinism of a procedure
>     with I/O state args. Previously, we did not generate an error message
>     for these issues.

These test cases changes were not included in the diff, but what you'e
committed looks fine.

...

> diff --git a/compiler/det_report.m b/compiler/det_report.m
> index c47f9969d..9725fb8a1 100644
> --- a/compiler/det_report.m
> +++ b/compiler/det_report.m

...

> @@ -75,6 +75,23 @@
>      module_info::in, module_info::out,
>      list(error_spec)::in, list(error_spec)::out) is det.
> 
> +    % Check the determinism declarations of the specified procedures
> +    % whose bodies det_analysis.m did not analyze because they are
> +    % imported from another module (and whose bodies are therefore
> +    % not available, at least not without intermodule optimization).
> +    % However, we can still perform some checks on the mode declarations
> +    % themselves.
> +    %
> +    % Note that any errors with those declarations should also be caught
> +    % when the module that we are importing the declaration from is compiled.
> +    % We do the checks we do here to prevent later passes of *this* compiler
> +    % invocation being given data that violates our rules of static semantics.
> +    % (See tests/invalid/gh118.m for an example.)

I thought you weren't adding this test case?

> @@ -553,6 +627,109 @@ func_primary_mode_det_msg = [words("In Mercury,"),
>
>  %---------------------------------------------------------------------------%
> 
> +:- pred check_io_state_proc_detism(module_info::in, pred_proc_id::in,
> +    pred_info::in, proc_info::in,
> +    list(error_spec)::in, list(error_spec)::out) is det.
> +
> +check_io_state_proc_detism(ModuleInfo, PredProcId, PredInfo, ProcInfo,
> +        !Specs) :-
> +    ( if
> +        proc_info_has_io_state_pair(ModuleInfo, ProcInfo, _InArg, _OutArg),
> +        proc_info_get_inferred_determinism(ProcInfo, Detism),
> +        not
> +            ( Detism = detism_det
> +            ; Detism = detism_cc_multi
> +            ; Detism = detism_erroneous
> +            )
> +    then
> +        pred_info_get_module_name(PredInfo, PredModuleName),
> +        module_info_get_name(ModuleInfo, ModuleName),
> +        % Almost all error messages this predicate will generate
> +        % will refer to a procedure that is local to the module being compiled,
> +        % and for these, pring the module name would only be clutter.

s/pring/printing/

> +        % However, in rare cases such as the tests/invalid/gh118 test case,

Again, you're not adding the test?

> +        % we may need to generate a message about an imported predicate,
> +        % and for this case, the name of the module containing the predicate
> +        % that we complain about is crucial information.
> +        ( if ModuleName = PredModuleName then
> +            ShouldModuleQual = should_not_module_qualify
> +        else
> +            ShouldModuleQual = should_module_qualify
> +        ),

...

> +%---------------------------------------------------------------------------%
> +
> diff --git a/tests/invalid/Mmakefile b/tests/invalid/Mmakefile
> index 21eae26c4..183ee4223 100644
> --- a/tests/invalid/Mmakefile
> +++ b/tests/invalid/Mmakefile
> @@ -42,6 +42,7 @@ SPECIAL_RULE_SINGLEMODULE_PROGS = \
>  MULTIMODULE_PROGS = \
>  	abstract_eqv \
>  	exported_unify \
> +	gh118 \

Ditto.

That looks fine otherwise.

Julien.