[m-rev.] for review: checked defns in parse_tree_module_src

[Julien Fischer]

Hi Zoltan,

On Fri, 15 Oct 2021, Zoltan Somogyi wrote:

> With this diff, there is one test case failure. abstract_type_decl.m,
> current in tests/invalid, has code like this:
>
> :- interface.
> :- type foo.
> :- implementation.
> :- type foo.
> :- type foo ---> foo(int).
>
> Before this diff, the compiler rejected this code, with an error
> saying that the second declaration conflicts with the first,
> due to them being in different sections.
>
> With this diff, we now generate only warnings for redundant
> declarations in the implementation section, and we don't even
> do that for redundant declarations in the interface section
> (since in library modules such as map.m, we will have
> a declaration of e.g. map/2 in the interface section we include
> in the manual, and an actual definition in a second interface
> section we do NOT include in the manual).
>
> Since the section mismatch between the two declarations
> was the only error in the test case, making it not an error
> makes the test case fail by having the compiler exit
> with a success exit code.
>
> I could make the compiler generate an error for this case,
> but I don't think that would be a good idea, since that would
> complicate the relatively simple rule "redundant declarations
> get at most a warning". This would require moving the test case
> to tests/warnings. Does anyone object to this move?

No.

> Use checked types/insts/modes in parse_tree_module_src.

...

> compiler/convert_parse_tree.m:
>     Check type, inst and mode definitions in raw_compilation_units
>     when creating parse_tree_module_srcs.
> 
> compiler/comp_unit_interface.m:
>     Make the code constructing interface files work from the checked maps
>     in parse_tree_module_srcs.
> 
> compiler/make_hlds_passes.m:
>     Set the flags that say we have found invalid type, inst or mode definitions
>     from the error specs constructed during the creation of the checked
>     type, inst and mode maps.
> 
> compiler/add_type.m:
>     Comment out an error message for a condition that will be detected and
>     reported by check_type_inst_mode_defns.m.

Is there a reason you are not removing it?

...

> compiler/check_raw_comp_unit.m:
> compiler/equiv_type.m:
> compiler/get_dependencies.m:
> compiler/grab_modules.m:
> compiler/module_qual.collect_mq_info.m:
> compiler/module_qual.qualify_items.m:
>     Conform to the changes above.
> 
> compiler/parse_type_defn.m:
>     Fix misleading error error message for ":- type <name> = <type>".

Doubled-up word: error.

...

> diff --git a/compiler/check_type_inst_mode_defns.m b/compiler/check_type_inst_mode_defns.m
> index 9ab9517f4..938836d40 100644
> --- a/compiler/check_type_inst_mode_defns.m
> +++ b/compiler/check_type_inst_mode_defns.m

...

> @@ -1053,27 +1123,55 @@ foreign_int_report_any_foreign_defn_in_imp(TypeCtor, IntForeignContext,
>          words("must be in the interface section as well."), nl],

...

> -report_any_nonabstract_solver_type_in_int(TypeCtor, MaybeDefn, !Specs) :-
> +report_any_nonabstract_solver_type_in_int(TypeCtor, IntMaybeDefn,
> +        IntMaybeAbstractDefn0, IntMaybeAbstractDefn,
> +        ImpMaybeDefn0, ImpMaybeDefn, !Specs) :-
>      (
> -        MaybeDefn = no
> +        IntMaybeDefn = no,
> +        IntMaybeAbstractDefn = IntMaybeAbstractDefn0,
> +        ImpMaybeDefn = ImpMaybeDefn0
>      ;
> -        MaybeDefn = yes(Defn),
> +        IntMaybeDefn = yes(IntDefn),
>          Pieces = [words("Error: a solver type such as"),
>              unqual_type_ctor(TypeCtor), words("may be defined"),
>              words("(as opposed to declared)"),
>              words("only in the implementation section."), nl],
> -        Spec = simplest_spec($pred, severity_warning, phase_term_to_parse_tree,
> -            Defn ^ td_context, Pieces),
> -        !:Specs = [Spec | !.Specs]
> +        Spec = simplest_spec($pred, severity_error,
> +            phase_type_inst_mode_check, IntDefn ^ td_context, Pieces),
> +        !:Specs = [Spec | !.Specs],
> +        % To avoid avalanche errors about this solver being unknown,

s/solver/solver type/

> +        % ensure that it has a declaration.
> +        (
> +            IntMaybeAbstractDefn0 = no,
> +            IntAbstractDefn = IntDefn ^ td_ctor_defn := abstract_solver_type,
> +            IntMaybeAbstractDefn = yes(IntAbstractDefn)
> +        ;
> +            IntMaybeAbstractDefn0 = yes(_),
> +            IntMaybeAbstractDefn = IntMaybeAbstractDefn0
> +        ),

That looks fine otherwise.

Julien.