[m-rev.] for review: --inform-incomplete-switch

[Julien Fischer]

Hi Zoltan,

On Sun, 22 Jan 2017, Zoltan Somogyi wrote:

> I have now implemented both improvements above, minus the configurability
> of the "how many cons_ids to print maximum", and the diff is ready for
> review, by anyone, though Julien is the logical reviewer.
>
> The two INCOMPLETE* files attached show the messages we get with the
> threshold set at 50 and 60% respectively. These, and printing a maximum of
> 10 cons_ids per message, have cut down the total size of the messages
> a lot.

> Add a new compiler option --inform-incomplete-switch.
>

...

> diff --git a/compiler/det_report.m b/compiler/det_report.m
> index fe3ba65..71989e8 100644
> --- a/compiler/det_report.m
> +++ b/compiler/det_report.m

...

> @@ -969,48 +990,123 @@ det_diagnose_orelse_goals([Goal | Goals], InstMap0, Desired, SwitchContexts0,
>      Msgs = Msgs1 ++ Msgs2.
>
>  %-----------------------------------------------------------------------------%
> +%
> +% There are two reasons why we may want to report that a switch is incomplete: 
> +%
> +% - because the switch is wrapped in a require_complete_switch scope, and
> +% - because the --inform-incomplete-switch option is set.
> +
> +    % This type says whether the --inform-incomplete-switch option is set.
> +    %
> +:- type maybe_inform_incomplete_switches
> +    --->    do_not_inform_incomplete_switches
> +    ;       inform_incomplete_switches.
> +
> +    % This type says which of the above two reasons causes us to report
> +    % the given incomplete switch.
> +    %
> +:- type why_report_incomplete_switch
> +    --->    switch_required_to_be_complete
> +    ;       inform_incomplete_switch_option.
> +
> +    % It is possible for *both* reasons to apply to the same incomplete switch.
> +    % In such cases, we want to generate only the error required by the scope,
> +    % and not the information message that the option calls for. We process
> +    % procedure bodied top down, so we process the scope goal before

s/bodied/bodies/

> +    % the switch it wraps, so when we generate an error report for an
> +    % incomplete switch, we pass along its details when we process the
> +    % goal inside the scope (which may either be the incomplete switch,
> +    % or a conjunction of feature_lifted_by_cse deconstruction unifications
> +    % followed by the incomplete switch).
> +    %
> +:- type reported_switch
> +    --->    reported_switch(
> +                prog_context,
> +                prog_var,
> +                list(case)
> +            ).

...

> @@ -1376,61 +1573,162 @@ lambda_update_instmap([Var - Mode | VarsModes], ModuleInfo, !InstMap) :-
>
>  %-----------------------------------------------------------------------------%
> 
> -:- pred find_missing_cons_ids(det_info::in, instmap::in, prog_var::in,
> -    list(case)::in, string::out, maybe(list(format_component))::out) is det.
> +:- type missing_cons_id_info
> +    --->    missing_cons_id_info(
> +                % The number of cons_ids that the switch variable could
> +                % be bound to on entry to the switch. May be the number of
> +                % function symbols in the variable's type, or it may be
> +                % the number of cons_ids that appear in the variable's
> +                % initial bound(...) inst.
> +                int,
> +
> +                % The number of these cons_ids that do NOT occur
> +                % in any of the cases.
> +                int,
> +
> +                % The diagnostic listing the missing cons_ids.
> +                %
> +                % If there are more missing cons_ids than the
> +                % specified limit, we return two different versions
> +                % of the error message fragment. The first is the version
> +                % to use if the user doesn't want verbose errors,
> +                % the second is the version to use if he/she does want them.
> +                %
> +                % If the number of missing cons_ids is below the limit,
> +                % or if the caller did not specify the limit,
> +                % the second list will be empty.
> +                list(format_component),
> +                list(format_component)
> +            ).
> +
> +:- pred find_missing_cons_ids(det_info::in, maybe(int)::in, instmap::in,
> +    list(switch_context)::in, prog_var::in, list(case)::in,
> +    list(format_component)::out, string::out,
> +    maybe(missing_cons_id_info)::out) is det.
> +
> +find_missing_cons_ids(DetInfo, MaybeLimit, InstMap0, SwitchContexts,
> +        Var, Cases, NestingPieces, VarStr, MaybeMissingInfo) :-
> +    det_diagnose_switch_context(DetInfo, SwitchContexts, NestingPieces),
> 
> -find_missing_cons_ids(DetInfo, InstMap0, Var, Cases, VarStr,
> -        MaybeMissingPieces) :-
>      det_get_proc_info(DetInfo, ProcInfo),
>      proc_info_get_varset(ProcInfo, VarSet),
>      VarStr = mercury_var_to_name_only(VarSet, Var),
>      det_info_get_module_info(DetInfo, ModuleInfo),
> +    instmap_lookup_var(InstMap0, Var, VarInst),
>      ( if
>          ( if
> -            instmap_lookup_var(InstMap0, Var, VarInst),
>              inst_is_bound_to_functors(ModuleInfo, VarInst, BoundInsts)
>          then
>              det_info_get_vartypes(DetInfo, VarTypes),
>              lookup_var_type(VarTypes, Var, VarType),
>              type_to_ctor_det(VarType, VarTypeCtor),
> -            list.map(bound_inst_to_cons_id(VarTypeCtor),
> -                BoundInsts, ConsIds)
> +            bound_insts_to_cons_ids(VarTypeCtor, BoundInsts, BoundConsIds),
> +            list.sort_and_remove_dups(BoundConsIds, SortedBoundConsIds),
> +            set_tree234.sorted_list_to_set(SortedBoundConsIds,
> +                BoundConsIdsSet),
> +            % Since should insist that all insts used in predicate

Since _we_ should ...

> +            % declarations be specific to the type of the arguments
> +            % they apply to. However, I (zs) don't think we enforce this
> +            % yet with any consistency in the mode checker. The intersection
> +            % operation below should compenate for this, but we can invoke it

s/compenate/compensate/

> +            % only for the switch variable's type is a du type, and not
> +            % a builtin type such as int or string.
> +            ( if
> +                det_lookup_var_type(ModuleInfo, ProcInfo, Var, TypeDefn),
> +                hlds_data.get_type_defn_body(TypeDefn, TypeBody),
> +                ConsTable = TypeBody ^ du_type_cons_tag_values
> +            then
> +                map.keys(ConsTable, SortedTypeConsIds),
> +                set_tree234.sorted_list_to_set(SortedTypeConsIds,
> +                    TypeConsIdsSet),
> +                set_tree234.intersect(TypeConsIdsSet, BoundConsIdsSet,
> +                    PossibleConsIdsSet)
> +            else

...

> @@ -1668,6 +1964,7 @@ failing_context_description(ModuleInfo, VarSet, FailingContext) = Msg :-
>          FailingGoal = incomplete_switch(Var),
>          VarStr = mercury_var_to_name_only(VarSet, Var),
>          Pieces = [words("Switch on"), fixed(VarStr), words("is incomplete.")]
> +        % ZZZ
>      ;

That ZZZ is presumably meaningful to you?

The rest looks fine.

Julien.