On Wed, 2017-01-04 at 14:58 -0500, Jason Merrill wrote:
> On Tue, Jan 3, 2017 at 8:28 PM, David Malcolm <dmalc...@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
