[m-rev.] for review: "Did you mean ..." mesages for long options

[Zoltan Somogyi]

On 2024-04-06 01:41 +11:00 AEST, "Julien Fischer" <jfischer at opturion.com> wrote:
> "Did you mean ..." messages for long options.

I intended to review this when you originally posted this, but forgot about it.
Sorry about that.

> Generate "Did you mean ..." messages for unrecognized long options.
> For example:
> 
>     unrecognized option `--no-half-at-wrn'.
>     (Did you mean `--no-halt-at-warn'?)
> 
> compiler/mercury_compile_main.m:
>     If we have an unrecognized long option or negated long option,
>     then attempt to generate a "Did you mean ..." message for it.
> 
> compiler/error_spec.m:
>     Add a version of maybe_construct_did_you_mean_pieces that lets
>     you add prefix to the strings before including them in the pieces.

add A prefix

>  maybe_construct_did_you_mean_pieces(BaseName, CandidateNames,
>          DidYouMeanPieces) :-
> +    do_maybe_construct_did_you_mean_pieces(BaseName, CandidateNames,
> +        std_util.id, DidYouMeanPieces).
> +
> +maybe_construct_prefixed_did_you_mean_pieces(Prefix, BaseName, CandidateNames,
> +        DidYouMeanPieces) :-
> +    do_maybe_construct_did_you_mean_pieces(BaseName, CandidateNames,
> +        add_prefix(Prefix), DidYouMeanPieces).
> +
> +:- pred do_maybe_construct_did_you_mean_pieces(string::in, list(string)::in,
> +    (func(string) = string)::in, list(format_piece)::out) is det.
> +
> +do_maybe_construct_did_you_mean_pieces(BaseName, CandidateNames,
> +        TransformFunc, DidYouMeanPieces) :-

You may need a space before the :-.

> +    % Add to stdlib? string.append has wrong mode for list map.
> +    %
> +:- func add_prefix(string, string) = string.
> +
> +add_prefix(Prefix, String) = Prefix ++ String.

I would say yes, add add_prefix to string.m, together with add_suffix (which would take
the suffix arg as its first arg).

> diff --git a/compiler/mercury_compile_main.m b/compiler/mercury_compile_main.m
> index 08325be..a58eaf2 100644
> --- a/compiler/mercury_compile_main.m
> +++ b/compiler/mercury_compile_main.m
> @@ -392,10 +392,8 @@ process_options_std(ProgressStream, ErrorStream, DefaultOptionTable,
>          DefaultOptionTable, ArgsOptionTable, cord.init, _UserData, !IO),
>      (
>          MaybeError = yes(OptionError),
> -        OptionErrorStr = option_error_to_string(OptionError),
> -        Spec = simplest_no_context_spec($pred, severity_error,
> -            phase_options, [words(OptionErrorStr), suffix("."), nl]),
> -        Result = opr_failure([Spec])
> +        Specs = make_option_error(OptionError),
> +        Result = opr_failure(Specs)

make_option_error seems a strange name for this function;
I would use something like report_bad_option.

> +:- func make_option_error(option_error(option)) = list(error_spec).
> +
> +make_option_error(OptionError) = Specs :-
> +    OptionErrorStr = option_error_to_string(OptionError),
> +    ( if
> +        OptionError = unrecognized_option(OptionStr),
> +        ( if string.remove_prefix("--no-", OptionStr, BaseOptionStr0) then
> +            IsNegatedOption = yes,
> +            BaseOptionStr = BaseOptionStr0
> +        else if string.remove_prefix("--", OptionStr, BaseOptionStr0) then
> +            IsNegatedOption = no,
> +            BaseOptionStr = BaseOptionStr0
> +        else
> +            false
> +        ),
> +        not is_empty(BaseOptionStr)

Why not is_empty(BaseOptionStr) instead of  BaseOptionStr \= ""?

And this code checks only for long options. On lines 1123 and 1369 of getopt.m,
we can construct unrecognized_option errors for short options as well.
The techniques you use on long options in options.m would work for
short options as well. I presume you rejected that because for short options,
there are no good dym suggestions, so I would add a comment to that effect
just above the "fail" (we usually use that, not "false").

> +:- pred long_option_table(string, option).
> +:- mode long_option_table(in, out) is semidet.
> +:- mode long_option_table(out, out) is multi.

We already have trouble with long option names causing too-long lines.
If we need to switch to a name different than long_table, I would pick
a *shorter* name. I would even be fine with "lo" as the name, with a comment
explaining the line length issue and the fact that it stands for "long option".

The diff is otherwise fine.

Zoltan.