Re: [PATCH] C++: fix fix-it hints for misspellings within explicit namespaces

[Jason Merrill <jason@redhat.com>]

On Fri, Jan 13, 2017 at 5:05 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2017-01-04 at 14:58 -0500, Jason Merrill wrote:
>> On Tue, Jan 3, 2017 at 8:28 PM, David Malcolm <dmalcolm@redhat.com>
>> wrote:
>> > PR c++/77829 and PR c++/78656 identify an issue within the C++
>> > frontend
>> > where it issues nonsensical fix-it hints for misspelled name
>> > lookups
>> > within an explicitly given namespace: it finds the closest name
>> > within
>> > all namespaces, and uses the location of the namespace for the
>> > replacement,
>> > rather than the name.
>> >
>> > For example, for this error:
>> >
>> >   #include <memory>
>> >   void* allocate(std::size_t n)
>> >   {
>> >     return std::alocator<char>().allocate(n);
>> >   }
>> >
>> > we currently emit an erroneous fix-it hint that would generate this
>> > nonsensical patch:
>> >
>> >    {
>> >   -  return std::alocator<char>().allocate(n);
>> >   +  return allocate::alocator<char>().allocate(n);
>> >    }
>> >
>> > whereas we ought to emit a fix-it hint that would generate this
>> > patch:
>> >
>> >    {
>> >   -  return std::alocator<char>().allocate(n);
>> >   +  return std::allocator<char>().allocate(n);
>> >    }
>> >
>> > This patch fixes the suggestions, in two parts:
>> >
>> > The incorrect name in the suggestion is fixed by introducing a
>> > new function "suggest_alternative_in_explicit_scope"
>> > for use by qualified_name_lookup_error when handling failures
>> > in explicitly-given namespaces, looking for hint candidates within
>> > just that namespace.  The function suggest_alternatives_for gains a
>> > "suggest_misspellings" bool param, so that we can disable fuzzy
>> > name
>> > lookup for the case where we've ruled out hint candidates in the
>> > explicitly-given namespace.
>> >
>> > This lets us suggest "allocator" (found in "std") rather "allocate"
>> > (found in the global ns).
>>
>> This looks good.
>>
>> > The patch fixes the location for the replacement by updating
>> > local "unqualified_id" in cp_parser_id_expression from tree to
>> > cp_expr to avoid implicitly dropping location information, and
>> > passing this location to a new param of finish_id_expression.
>> > There are multiple users of finish_id_expression, and it wasn't
>> > possible to provide location information for the id for all of them
>> > so the new location information is assumed to be optional there.
>>
>> > This fixes the underlined location, and ensures that the fix-it
>> > hint
>> > replaces "alocator", rather than "std".
>>
>> I'm dubious about this approach, as I think this will fix some cases
>> and not others.  The general problem is that we aren't sure what to
>> do
>> with the location of a qualified-id: sometimes we use the location of
>> the unqualified-id, sometimes we use the beginning of the first
>> nested-name-specifier.  I think the right answer is probably to use a
>> rich location with the caret on the unqualified-id.  Then the fix-it
>> hint can use the caret location for the fix-it.  Does that make
>> sense?
>
> Sorry, I'm not sure I follow you.
>
> By "rich location" do you mean providing multiple ranges (e.g. one for
> the nested-name-specifier, another for the unqualified-id)?
>
> e.g.
>
>   ::foo::bar
>     ~~~  ^~~
>      |    |
>      |    `-(primary location and range)
>      `-(secondary range)
>
> or:
>
>   ::foo::bar
>   ~~~~~  ^~~
>      |    |
>      |    `-(primary location and
> range)
>      `-(secondary range)
>
> (if that ASCII art makes sense)
>
> Note that such a rich location (with more than one range) can only
> exist during the emission of a diagnostic; it's not something we can
> use as the location of a tree node.
>
> Or do you mean that we should supply a range for the unqualified-id,
> with the caret at the start of the unqualified-id, e.g.:
>
>    ::foo::bar
>           ^~~
>
> (a tree node code can have such a location).
>
> It seems to me that we ought to have
>
>    ::foo::bar
>    ^~~~~~~~~~
>
> as the location of such a tree node, and only use the range of just
> "bar" as the location when handling name lookup errors (such as when
> emitting fix-it hints).

Yes, sorry, I meant range.  I was thinking

::foo::bar
~~~~~~~^~~

And then the fix-it would know to replace from the caret to the end of
the range?

Jason