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

[David Malcolm]

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

So maybe we should have:

  ::foo::bar
  ~~~~~  ^~~
     |    |
     |    `-(primary location)
     `-(secondary location)

as the location when reporting name lookup errors (and thus using "bar"
as the location for replacement fix-it hints), and:

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

as the location of the tree node, once name-lookup is done?

(this seems invasive; maybe inappropriate for this stage?  I was hoping
for a simpler fix)

finish_id_expression (which can emit the fix-it hint) can be called by
5 different sites:

  * in cp_parser_primary_expression, after cp_parser_id_expression
  * in cp_parser_lambda_introducer, after a call to
cp_parser_lookup_name_simple
  * in cp_parser_decltype_expr, after a call to cp_parser_id_expression
  * in tsubst_copy_and_build
  * in omp_reduction_lookup

My patch used UNKNOWN_LOCATION for the replacement location for all of
these apart from the first one: I was only confident about passing in
correct location information from the first callsite.

Should I attempt to fix all of these cases so that the location that's
passed in is that of the unqualified-id?

Thanks; hope this makes sense
Dave