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

[Jason Merrill <jason at redhat dot com>]

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?

Jason