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

[Julien Fischer]

On Sat, 20 Apr 2024, Zoltan Somogyi wrote:

> On 2024-04-06 01:41 +11:00 AEST, "Julien Fischer" <jfischer at opturion.com> wrote:
>
>> 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

Done.

>>  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 :-.

There's already one there.

>
>> +    % 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).

We already have add_suffix. I have added add_prefix to the string
module.

>> 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.

I have changed it to report_option_error.

>> +:- 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 \= ""?

Too much Java recently ;-)

> 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").

Done.

>> +:- 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".

I have changed it to long_table.

> The diff is otherwise fine.

Thanks for the review.

Julien.