[m-rev.] for review: unify getopt and getopt_io

[Peter Wang]

On Fri, 09 Oct 2020 06:07:04 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> For review by anyone, though I would like both Peter and Julien
> to have a look at the concept, and at the additional help for
> diagnosing breaking changes.
> 
> I intend to post a diff for NEWS tomorrow, covering both this change,
> and other recent changes to getopt*.m.
> 
> Zoltan.

> diff --git a/compiler/typecheck_errors.m b/compiler/typecheck_errors.m
> index 527c205e6..cc61da53c 100644
> --- a/compiler/typecheck_errors.m
> +++ b/compiler/typecheck_errors.m

> +:- pred report_special_type_mismatches_loop(is_first::in,
> +    list(type_mismatch_special)::in, list(format_component)::out) is det.
> +
> +report_special_type_mismatches_loop(_IsFirst, [], []).
> +report_special_type_mismatches_loop(IsFirst, [HeadSpecial | TailSpecials],
> +    Pieces) :-

Indent.

> +    report_special_type_mismatches_loop(is_not_first, TailSpecials,
> +        TailPieces),
> +    HeadPieces = report_special_type_mismatch(IsFirst, HeadSpecial),
> +    Pieces = HeadPieces ++ TailPieces.
> +
> +:- func report_special_type_mismatch(is_first, type_mismatch_special)
> +    = list(format_component).
> +
> +report_special_type_mismatch(IsFirst, MismatchSpecial) = Pieces :-
> +    (
> +        IsFirst = is_first,
> +        ReasonIsPieces =
> +            [words("One possible reason for the error is that")]
> +    ;
> +        IsFirst = is_not_first,
> +        ReasonIsPieces =
> +            [words("Another possible reason for the error is that")]
> +    ),
> +    (
> +        MismatchSpecial = type_mismatch_special_getopt_error(GetoptModule),
> +        Pieces = ReasonIsPieces ++
> +            [words("the signatures of the option processing predicates"),
> +            words("in the"), quote(GetoptModule), words("module"),
> +            words("have changed recently."),
> +            words("Errors are now returned in a structured form,"),
> +            words("which can be converted to a string by calling the"),
> +            quote("option_error_to_string"), words("function."), nl]
> +    ).

Don't need the parentheses.

> diff --git a/library/getopt_io.m b/library/getopt_io.m
> index e6361d1e2..a0afaf32e 100644
> --- a/library/getopt_io.m
> +++ b/library/getopt_io.m
> @@ -105,28 +71,24 @@
>  % "primitive" options could be handled by a special handler which sets those
>  % "primitive" options.
>  %
> +% It is an error to use a "special" option other than file_special
> +% for which there is no handler, or for which the handler fails.
> +%

I suggest to put "other than file_special" in parentheses to make the
sentence easier to parse.

> +% The reading of the file obviously requires doing I/O, which means that
> +% only the predicate variants that take an I/O state pair of arguments
> +% support file_special options. If a call to a predicate variant that
> +% does not take a pair of I/O states as argument does nevertheless specify

Fix "argument", or delete "as argument".

> +% a file_special option, that predicate will generate an error message
> +% for that option.

Perhaps simply:

    then an error will be reported.

> @@ -428,69 +378,107 @@
>              % The option is a file_special option whose argument is the file
>              % named by the argument. This file contained some non-option
>              % arguments.
> -% GETOPT_IO_ONLY_END
>  
>  %---------------------------------------------------------------------------%
>  
> -    % process_options(OptionOps, Args, NonOptionArgs, Result, !IO):
> -    % process_options(OptionOps, Args, OptionArgs, NonOptionArgs, Result, !IO):
> +    % process_options(OptionOps, Args, NonOptionArgs, Result):
> +    % process_options(OptionOps, Args, OptionArgs, NonOptionArgs, Result):
> +    % process_option_io(OptionOps, Args, NonOptionArgs,
> +    %   Result, !IO):
> +    % process_option_io(OptionOps, Args, OptionArgs, NonOptionArgs,
> +    %   Result, !IO):

Missing "s" on the two _io predicates.

> +    %
> +    % These four predicates do effectively the same job, differing
> +    % from each other in two minor ways.
> +    %
> +    % The job they all do is scanning through 'Args' looking for options.
> +    % The various fields of the OptionOps structure specify the names
> +    % (both short and long) of the options to look for, as well as their
> +    % defaul values, and possibly the handler for the special options.

default

> +    % The structure of the `OptionOps' argument is documented above,
> +    % at the definition of the option_ops type.
>      %
> -    % Scans through 'Args' looking for options. Places all the option arguments
> -    % in `OptionArgs', places all the non-option arguments in 'NonOptionArgs',
> -    % and records the options in the `OptionTable'. `OptionTable' is a map
> -    % from a user-defined option type to option_data.
> +    % All these predicates place all the non-option arguments in
> +    % 'NonOptionArgs', and the predicates that have an `OptionArgs' argument
> +    % place the option arguments there. (While some callers will want
> +    % the arguments contain the options, other callers will not, considering
> +    % that the only information they want from them is that contained in
> +    % the option table.)

I think we should deprecate

    process_options(OptionOps, Args, NonOptionArgs, Result)

and don't introduce

    process_options_io(OptionOps, Args, NonOptionArgs, Result, !IO)

>      %
> -    % If an invalid option is encountered, we return `error(Message)',
> -    % otherwise we return `ok(OptionTable)' in 'Result'.
> +    % If they find a problem, such as an unknown option name, an option
> +    % being given an argument of the wrong type, or the failure of the handler
> +    % for a special option, all the predicates will put into `Result'
> +    % an `error' wrapped around an error code. That error code can be turned
> +    % into an error message using the option_error_to_string function below.
>      %
> -    % The structure of the `OptionOps' argument is documented above.
> +    % In they do not find a problem, all these predicates will place into

If they

> +    % `Result' an `ok' wrapped around an option table, which maps each option
> +    % to its final value. This will be its default value as updated by
> +    % all the options in `Args'.
>      %

The mention of "default values" being "updated" was confusing to me
(of course the intended meaning can be grasped eventually).
Maybe replace the last part with something like:

    which maps each option to a value, as set by the options in `Args'.

> -    % The two different arity versions differ only in that the lower arity
> -    % version does not return the arguments that contained the options.
> -    % While some callers will want those arguments, other callers will not,
> -    % considering that the only information they want from them is that
> -    % contained in the option table.
> +    % The predicate versions whose names end in `io' take a pair of I/O state
> +    % arguments. This is so that they can handle file_special options, which
> +    % require reading a named file, and treating its contents as specifying

treating the file's contents

> +    % additional options. The predicate versions whose names do not end in `io'
> +    % cannot do I/O, and therefore cannot handle file_special options.
> +    % If they encounter them anyway, they will report that fact as an error.

I suggest:

    cannot do I/O, and will report an error if they encounter a
    file_special option.

>      % process_options_track(OptionOps, Args, OptionArgs, NonOptionArgs,
> +    %   OptionTable0, Result, OptionsSet):
> +    % process_options_track_io(OptionOps, Args, OptionArgs, NonOptionArgs,
>      %   OptionTable0, Result, OptionsSet, !IO):
>      %
> -    % This predicate differs from the two variants of process_options
> -    % above in only two respects.
> +    % These predicates differ from the non-track variants above
> +    % in only two respects.
>      %
> -    % First, it expects the caller to supply an argument containing
> +    % First, they expects the caller to supply an argument containing

expect

>      % the initial contents of the option table, instead of calling
> -    % the initialization predicate itself. The point of this is that
> +    % the initialization predicate themselves. The point of this is that
>      % it allows the option table to be initialized once (using either

I suggest replacing:

    The point of this is that it allows ...

with:

    It allows the option table ...

> -    % init_option_table or init_option_table_multi below), and then
> -    % process_options_track to be called several times with different
> -    % sets of arguments, perhaps obtained from different sources
> -    % (command line, configuration file, etc).
> +    % the init_option_table or the init_option_table_multi predicate below),
> +    % and then process_options_track or process_options_track_io
> +    % can be called several times with different sets of arguments,
> +    % perhaps obtained from different sources (such as command line,
> +    % configuration file, and so on).
>      %
> -    % Second, each call to process_options_track returns the set of options
> -    % that were set by that call. This helps with the same objective;
> -    % for example, it can tell the caller whether an option was set
> -    % from a configuration file, the command line, both, or neither.
> +    % Second, each call to these predicates returns the set of options

return

>      % process_options_userdata(OptionOps, Args, OptionArgs, NonOptionArgs,
> +    %   MaybeError, OptionsSet, !OptionTable, !UserData):
> +    % process_options_userdata_io(OptionOps, Args, OptionArgs, NonOptionArgs,
>      %   MaybeError, OptionsSet, !OptionTable, !UserData, !IO):
>      %
> -    % This predicate is similar to process_options_track, but differs
> +    % These predicates are similar to the track predicates above, but differ
>      % in two ways.
>      %
> -    % - It also threads a piece of state of a user-specified "userdata" type
> +    % - They also threads a piece of state of a user-specified "userdata" type

thread

>      %   through all the handlers of special options, so that each
>      %   special handler can read from and/or write to this state.
>      %   Amongst other things, this can be used by the caller to recover
> @@ -498,33 +486,17 @@
>      %   information that is not present in the (orderless) set
>      %   of specified options.
>      %
> -    % - Even if it finds an error, it returns the option table as it was
> +    % - Even if they find an error, they returns the option table as it was

return

> @@ -580,8 +552,18 @@
>  
>  %---------------------------------------------------------------------------%
>  
> +    % Convert our structured representation of an error to an error message.
> +    %
>  :- func option_error_to_string(option_error(OptionType)) = string.

s/our/from the/

> +%---------------------%
> +
> +    % If the argument represents an error, then that error from
> +    % our structured representation to an error message.
> +    %

convert that error from the structured representation to

> +:- func decode_maybe_option_table(maybe_option_table_se(OptionType))
> +    = maybe_option_table(OptionType).
> +

"decode" seems a strange name for this function, but I can only suggest
"convert_to_maybe_option_table".

The rest looks fine to me.

Peter