PR driver/69265: add hint for options with misspelled arguments

opts-common.c's cmdline_handle_error handles invalid arguments
for options with CL_ERR_ENUM_ARG by building a strings listing the
valid arguments.  By also building a vec of valid arguments, we
can use find_closest_string and provide a hint if we see a close
misspelling.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/ChangeLog:
	PR driver/69265
	* Makefile.in (GCC_OBJS): Move spellcheck.o to...
	(OBJS-libcommon-target): ...here.
	* opts-common.c: Include spellcheck.h.
	(cmdline_handle_error): Build a vec of valid options and use it
	to suggest provide hints for misspelled arguments.

gcc/testsuite/ChangeLog:
	PR driver/69265
	* gcc.dg/spellcheck-options-11.c: New test case.
---
 gcc/Makefile.in                              |  4 ++--
 gcc/opts-common.c                            | 11 ++++++++++-
 gcc/testsuite/gcc.dg/spellcheck-options-11.c |  7 +++++++
 3 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/spellcheck-options-11.c

On Mon, May 09, 2016 at 08:14:47PM -0400, David Malcolm wrote:

> -      inform (loc, "valid arguments to %qs are: %s", option->opt_text, s);
> +      const char *hint = find_closest_string (arg, &candidates);
> +      if (hint)
> +	inform (loc, "valid arguments to %qs are: %s; did you mean %qs?",
> +		option->opt_text, s, hint);
> +      else
> +	inform (loc, "valid arguments to %qs are: %s", option->opt_text, s);
> +

btw.. did you consider a format specifier for this ";did you mean %qs"
common stanza?

inform (loc, "valid arguments to %qs are: %s%qhs", x, y, hint);
(let's say) where the specifier would end up empty if hint was NULL.

I don't really like all these odd conditionals on the hints and it also
adds some bloat to have all these duplicate error strings.

Not sure but maybe this would even help translators?

Is there a -Wno-spelling-suggestions to turn the suggestions off
globally, btw?

thanks,

On Tue, 2016-05-10 at 22:09 +0200, Bernhard Reutner-Fischer wrote:
> On Mon, May 09, 2016 at 08:14:47PM -0400, David Malcolm wrote:
> 
> > -      inform (loc, "valid arguments to %qs are: %s", option
> > ->opt_text, s);
> > +      const char *hint = find_closest_string (arg, &candidates);
> > +      if (hint)
> > +	inform (loc, "valid arguments to %qs are: %s; did you mean
> > %qs?",
> > +		option->opt_text, s, hint);
> > +      else
> > +	inform (loc, "valid arguments to %qs are: %s", option
> > ->opt_text, s);
> > +
> 
> btw.. did you consider a format specifier for this ";did you mean
> %qs"
> common stanza?
> 
> inform (loc, "valid arguments to %qs are: %s%qhs", x, y, hint);
> (let's say) where the specifier would end up empty if hint was NULL.

It would save a conditional, but it seems a bit too much like "magic"
to me; I prefer the explicitness of the existing approach.

> I don't really like all these odd conditionals on the hints and it
> also
> adds some bloat to have all these duplicate error strings.
> 
> Not sure but maybe this would even help translators?

I'm not sure either, but it's generally a bad idea from an i18n
perpective to be building strings at run-time, and the %qhs idea above
feels a bit like that to me.

> Is there a -Wno-spelling-suggestions to turn the suggestions off
> globally, btw?

There isn't one.


Dave

On 10/05/16 22:03, David Malcolm wrote:
> On Tue, 2016-05-10@22:09 +0200, Bernhard Reutner-Fischer wrote:
>> On Mon, May 09, 2016@08:14:47PM -0400, David Malcolm wrote:
>>
>>> -      inform (loc, "valid arguments to %qs are: %s", option
>>> ->opt_text, s);
>>> +      const char *hint = find_closest_string (arg, &candidates);
>>> +      if (hint)
>>> +	inform (loc, "valid arguments to %qs are: %s; did you mean
>>> %qs?",
>>> +		option->opt_text, s, hint);
>>> +      else
>>> +	inform (loc, "valid arguments to %qs are: %s", option
>>> ->opt_text, s);
>>> +
>>
>> btw.. did you consider a format specifier for this ";did you mean
>> %qs"
>> common stanza?
>>
>> inform (loc, "valid arguments to %qs are: %s%qhs", x, y, hint);
>> (let's say) where the specifier would end up empty if hint was NULL.
>
> It would save a conditional, but it seems a bit too much like "magic"
> to me; I prefer the explicitness of the existing approach.

Also, I'm pretty sure that %qX will print `; did you mean hint?', that is, with 
the quotes all messed up. GCC has the same issue when printing typedefs (aka). 
GCC prints:

error: member reference base type 'pid_t {aka int}' is not a structure or union

instead of the more correct quoting used by Clang:

error: member reference base type 'pid_t' (aka 'int') is not a structure or union

But perhaps a wrapper or a dedicated overload can hide some of the details 
within diagnostic.c without requiring a new format specifier:

inform_with_hint (const char *target, const auto_vec<const char *> *candidates,
                   location_t, const char *gmsgid, ...)

Eventually, we should move to an API like the rich_location one, where 
diagnostics can be explicitly constructed, properties can be dynamically added 
and then the diagnostic is explicitly emitted.

Perhaps even an API that uses the Named Parameter Idiom?

diagnostic.at(rich_loc)
	  .with_hint(target, candidates)
	  .inform("something").print()

Cheers,
	Manuel.

On 05/09/2016 06:14 PM, David Malcolm wrote:
> opts-common.c's cmdline_handle_error handles invalid arguments
> for options with CL_ERR_ENUM_ARG by building a strings listing the
> valid arguments.  By also building a vec of valid arguments, we
> can use find_closest_string and provide a hint if we see a close
> misspelling.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> gcc/ChangeLog:
> 	PR driver/69265
> 	* Makefile.in (GCC_OBJS): Move spellcheck.o to...
> 	(OBJS-libcommon-target): ...here.
> 	* opts-common.c: Include spellcheck.h.
> 	(cmdline_handle_error): Build a vec of valid options and use it
> 	to suggest provide hints for misspelled arguments.
>
> gcc/testsuite/ChangeLog:
> 	PR driver/69265
> 	* gcc.dg/spellcheck-options-11.c: New test case.
OK.
jeff