On Sat, 2017-01-14 at 09:50 -0500, Jason Merrill wrote:
> On Fri, Jan 13, 2017 at 5:05 PM, David Malcolm <[email protected]>
> wrote:
> > On Wed, 2017-01-04 at 14:58 -0500, Jason Merrill wrote:
> > > On Tue, Jan 3, 2017 at 8:28 PM, David Malcolm <
> > > [email protected]>
> > > 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?
>
Thanks.
Here's a version of the patch which simply tweaks
cp_parser_primary_expression's call to finish_id_expression so that
it passes the location of the id_expression, rather than that of
id_expr_token.
The id_expression in question came from cp_parser_id_expression,
whereas the id_expr_token is the first token within the id-expression.
The location passed here to finish_id_expression only affects:
the location used for name-lookup errors, and for the resulting
decl cp_expr. Given that the following code immediately does this:
decl.set_location (id_expr_token->location);
to override the latter, nothing is changing apart from the reported
location (and it uses it for the replacement range for the fix-it
hint).
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
Is this more minimal version of the patch OK for trunk as-is, or do you
want me to do the extended ranges idea discussed above?
gcc/cp/ChangeLog:
PR c++/77829
PR c++/78656
* cp-tree.h (suggest_alternatives_for): Add bool param.
(suggest_alternative_in_explicit_scope): New decl.
* error.c (qualified_name_lookup_error): When SCOPE is a namespace
that isn't the global one, call new function
suggest_alternative_in_explicit_scope, only calling
suggest_alternatives_for if it fails, and disabling near match
searches fort that case. When SCOPE is the global namespace,
pass true for new param to suggest_alternatives_for to allow for
fuzzy name lookups.
* lex.c (unqualified_name_lookup_error): Pass true for new param
to suggest_alternatives_for.
* name-lookup.c (consider_binding_level): Add forward decl.
(suggest_alternatives_for): Add "suggest_misspellings" param,
using it to conditionalize the fuzzy name-lookup code.
(suggest_alternative_in_explicit_scope): New function.
* parser.c (cp_parser_primary_expression): When calling
finish_id_expression, pass location of id_expression rather
than that of id_expr_token.
(cp_parser_id_expression): Convert local "unqualified_id" from
tree to cp_expr to avoid implicitly dropping location information.
gcc/testsuite/ChangeLog:
PR c++/77829
PR c++/78656
* g++.dg/spellcheck-pr77829.C: New test case.
* g++.dg/spellcheck-pr78656.C: New test case.
---
gcc/cp/cp-tree.h | 3 +-
gcc/cp/error.c | 5 +-
gcc/cp/lex.c | 2 +-
gcc/cp/name-lookup.c | 55 ++++++++--
gcc/cp/parser.c | 4 +-
gcc/testsuite/g++.dg/spellcheck-pr77829.C | 167 ++++++++++++++++++++++++++++++
gcc/testsuite/g++.dg/spellcheck-pr78656.C | 39 +++++++
7 files changed, 261 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/spellcheck-pr77829.C
create mode 100644 gcc/testsuite/g++.dg/spellcheck-pr78656.C
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 24de346..edbe1ca 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6928,7 +6928,8 @@ extern tree cp_fully_fold (tree);
extern void clear_fold_cache (void);
/* in name-lookup.c */
-extern void suggest_alternatives_for (location_t, tree);
+extern void suggest_alternatives_for (location_t, tree, bool);
+extern bool suggest_alternative_in_explicit_scope (location_t, tree, tree);
extern tree strip_using_decl (tree);
/* in constraint.cc */
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 72044a9..4f4c11d 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3777,11 +3777,12 @@ qualified_name_lookup_error (tree scope, tree name,
else if (scope != global_namespace)
{
error_at (location, "%qD is not a member of %qD", name, scope);
- suggest_alternatives_for (location, name);
+ if (!suggest_alternative_in_explicit_scope (location, name, scope))
+ suggest_alternatives_for (location, name, false);
}
else
{
error_at (location, "%<::%D%> has not been declared", name);
- suggest_alternatives_for (location, name);
+ suggest_alternatives_for (location, name, true);
}
}
diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c
index 797dd96..60a70e9 100644
--- a/gcc/cp/lex.c
+++ b/gcc/cp/lex.c
@@ -441,7 +441,7 @@ unqualified_name_lookup_error (tree name, location_t loc)
if (!objc_diagnose_private_ivar (name))
{
error_at (loc, "%qD was not declared in this scope", name);
- suggest_alternatives_for (loc, name);
+ suggest_alternatives_for (loc, name, true);
}
/* Prevent repeated error messages by creating a VAR_DECL with
this NAME in the innermost block scope. */
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index c422d2f..af00005 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -48,6 +48,10 @@ static bool lookup_using_namespace (tree, struct
scope_binding *, tree,
tree, int);
static bool qualified_lookup_using_namespace (tree, tree,
struct scope_binding *, int);
+static void consider_binding_level (tree name, best_match <tree, tree> &bm,
+ cp_binding_level *lvl,
+ bool look_within_fields,
+ enum lookup_name_fuzzy_kind kind);
static tree lookup_type_current_level (tree);
static tree push_using_directive (tree);
static tree lookup_extern_c_fun_in_all_ns (tree);
@@ -4424,10 +4428,13 @@ remove_hidden_names (tree fns)
/* Suggest alternatives for NAME, an IDENTIFIER_NODE for which name
lookup failed. Search through all available namespaces and print out
- possible candidates. */
+ possible candidates. If no exact matches are found, and
+ SUGGEST_MISSPELLINGS is true, then also look for near-matches and
+ suggest the best near-match, if there is one. */
void
-suggest_alternatives_for (location_t location, tree name)
+suggest_alternatives_for (location_t location, tree name,
+ bool suggest_misspellings)
{
vec<tree> candidates = vNULL;
vec<tree> namespaces_to_search = vNULL;
@@ -4474,13 +4481,16 @@ suggest_alternatives_for (location_t location, tree
name)
or do nothing. */
if (candidates.is_empty ())
{
- const char *fuzzy_name = lookup_name_fuzzy (name, FUZZY_LOOKUP_NAME);
- if (fuzzy_name)
+ if (suggest_misspellings)
{
- gcc_rich_location richloc (location);
- richloc.add_fixit_replace (fuzzy_name);
- inform_at_rich_loc (&richloc, "suggested alternative: %qs",
- fuzzy_name);
+ const char *fuzzy_name = lookup_name_fuzzy (name, FUZZY_LOOKUP_NAME);
+ if (fuzzy_name)
+ {
+ gcc_rich_location richloc (location);
+ richloc.add_fixit_replace (fuzzy_name);
+ inform_at_rich_loc (&richloc, "suggested alternative: %qs",
+ fuzzy_name);
+ }
}
return;
}
@@ -4495,6 +4505,35 @@ suggest_alternatives_for (location_t location, tree name)
candidates.release ();
}
+/* Look for alternatives for NAME, an IDENTIFIER_NODE for which name
+ lookup failed within the explicitly provided SCOPE. Suggest the
+ the best meaningful candidates (if any) as a fix-it hint.
+ Return true iff a suggestion was provided. */
+
+bool
+suggest_alternative_in_explicit_scope (location_t location, tree name,
+ tree scope)
+{
+ cp_binding_level *level = NAMESPACE_LEVEL (scope);
+
+ best_match <tree, tree> bm (name);
+ consider_binding_level (name, bm, level, false, FUZZY_LOOKUP_NAME);
+
+ /* See if we have a good suggesion for the user. */
+ tree best_id = bm.get_best_meaningful_candidate ();
+ if (best_id)
+ {
+ const char *fuzzy_name = IDENTIFIER_POINTER (best_id);
+ gcc_rich_location richloc (location);
+ richloc.add_fixit_replace (fuzzy_name);
+ inform_at_rich_loc (&richloc, "suggested alternative: %qs",
+ fuzzy_name);
+ return true;
+ }
+
+ return false;
+}
+
/* Unscoped lookup of a global: iterate over current namespaces,
considering using-directives. */
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 7b3ee30..8a7ae32 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -5332,7 +5332,7 @@ cp_parser_primary_expression (cp_parser *parser,
template_p, done, address_p,
template_arg_p,
&error_msg,
- id_expr_token->location));
+ id_expression.get_location ()));
if (error_msg)
cp_parser_error (parser, error_msg);
decl.set_location (id_expr_token->location);
@@ -5425,7 +5425,7 @@ cp_parser_id_expression (cp_parser *parser,
tree saved_scope;
tree saved_object_scope;
tree saved_qualifying_scope;
- tree unqualified_id;
+ cp_expr unqualified_id;
bool is_template;
/* See if the next token is the `template' keyword. */
diff --git a/gcc/testsuite/g++.dg/spellcheck-pr77829.C
b/gcc/testsuite/g++.dg/spellcheck-pr77829.C
new file mode 100644
index 0000000..2f75779
--- /dev/null
+++ b/gcc/testsuite/g++.dg/spellcheck-pr77829.C
@@ -0,0 +1,167 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Various tests of name lookup within a namespace, both within an explicitly
+ given namespace, or implicitly. */
+
+namespace detail {
+ /* Various things to look for. */
+
+ typedef int some_typedef;
+
+ int _foo(int i) { return i; }
+
+ template <typename T>
+ T something_else (T i) { return i; }
+}
+
+/* Tests of lookup of a typedef. */
+
+void fn_1_explicit ()
+{
+ detail::some_type i; // { dg-error ".some_type. is not a member of .detail."
}
+ // { dg-message "suggested alternative: .some_typedef." "" { target *-*-* }
.-1 }
+ /* { dg-begin-multiline-output "" }
+ detail::some_type i;
+ ^~~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ detail::some_type i;
+ ^~~~~~~~~
+ some_typedef
+ { dg-end-multiline-output "" } */
+}
+
+namespace detail {
+
+void fn_1_implicit ()
+{
+ some_type i; // { dg-error ".some_type. was not declared in this scope" }
+ // { dg-message "suggested alternative: .some_typedef." "" { target *-*-* }
.-1 }
+ /* { dg-begin-multiline-output "" }
+ some_type i;
+ ^~~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ some_type i;
+ ^~~~~~~~~
+ some_typedef
+ { dg-end-multiline-output "" } */
+}
+
+} // namespace detail
+
+
+/* Tests of lookup of a function. */
+
+void fn_2_explicit (int i) {
+ detail::foo(i); // { dg-error ".foo. is not a member of .detail." }
+ // { dg-message "suggested alternative: ._foo." "" { target *-*-* } .-1 }
+ /* { dg-begin-multiline-output "" }
+ detail::foo(i);
+ ^~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ detail::foo(i);
+ ^~~
+ _foo
+ { dg-end-multiline-output "" } */
+}
+
+namespace detail {
+
+void fn_2_implicit (int i) {
+ foo(i); // { dg-error ".foo. was not declared in this scope" }
+ // { dg-message "suggested alternative: ._foo." "" { target *-*-* } .-1 }
+ /* { dg-begin-multiline-output "" }
+ foo(i);
+ ^~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ foo(i);
+ ^~~
+ _foo
+ { dg-end-multiline-output "" } */
+}
+
+} // namespace detail
+
+
+/* Examples using a template. */
+
+void fn_3_explicit (int i) {
+ detail::something_els(i); // { dg-error ".something_els. is not a member of
.detail." }
+ // { dg-message "suggested alternative: .something_else." "" { target *-*-*
} .-1 }
+ /* { dg-begin-multiline-output "" }
+ detail::something_els(i);
+ ^~~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+
+ /* { dg-begin-multiline-output "" }
+ detail::something_els(i);
+ ^~~~~~~~~~~~~
+ something_else
+ { dg-end-multiline-output "" } */
+}
+
+namespace detail {
+
+void fn_3_implicit (int i) {
+ something_els(i); // { dg-error ".something_els. was not declared in this
scope" }
+ // { dg-message "suggested alternative: .something_else." "" { target *-*-*
} .-1 }
+ /* { dg-begin-multiline-output "" }
+ something_els(i);
+ ^~~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+
+ /* { dg-begin-multiline-output "" }
+ something_els(i);
+ ^~~~~~~~~~~~~
+ something_else
+ { dg-end-multiline-output "" } */
+}
+
+} // namespace detail
+
+
+/* Tests of lookup for which no hint is available. */
+
+void fn_4_explicit (int i) {
+ detail::not_recognized(i); // { dg-error ".not_recognized. is not a member
of .detail." }
+ /* { dg-begin-multiline-output "" }
+ detail::not_recognized(i);
+ ^~~~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+}
+
+namespace detail {
+
+void fn_4_implicit (int i)
+{
+ not_recognized(i); // { dg-error ".not_recognized. was not declared in this
scope" }
+ /* { dg-begin-multiline-output "" }
+ not_recognized(i);
+ ^~~~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+}
+
+} // namespace detail
+
+
+/* Test for failed lookup explicitly within global namespace. */
+
+typedef int another_typedef;
+
+void fn_5 ()
+{
+ ::another_type i; // { dg-error ".::another_type. has not been declared" }
+ // { dg-message "suggested alternative: .another_typedef." "" { target *-*-*
} .-1 }
+ /* { dg-begin-multiline-output "" }
+ ::another_type i;
+ ^~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ ::another_type i;
+ ^~~~~~~~~~~~
+ another_typedef
+ { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/g++.dg/spellcheck-pr78656.C
b/gcc/testsuite/g++.dg/spellcheck-pr78656.C
new file mode 100644
index 0000000..ded4bb6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/spellcheck-pr78656.C
@@ -0,0 +1,39 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+#include <memory>
+
+void* allocate(std::size_t n)
+{
+ return std::allocate<char>().allocate(n); // { dg-error ".allocate. is not a
member of .std." }
+ // { dg-message "suggested alternative: .allocator." "" { target *-*-* } .-1
}
+ /* { dg-begin-multiline-output "" }
+ return std::allocate<char>().allocate(n);
+ ^~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ return std::allocate<char>().allocate(n);
+ ^~~~~~~~
+ allocator
+ { dg-end-multiline-output "" } */
+
+ // Various errors follow that we don't care about; suppress them:
+ // { dg-excess-errors "7: " }
+}
+
+void* test_2(std::size_t n)
+{
+ return std::alocator<char>().allocate(n); // { dg-error ".alocator. is not a
member of .std." }
+ // { dg-message "suggested alternative: .allocator." "" { target *-*-* } .-1
}
+ /* { dg-begin-multiline-output "" }
+ return std::alocator<char>().allocate(n);
+ ^~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ return std::alocator<char>().allocate(n);
+ ^~~~~~~~
+ allocator
+ { dg-end-multiline-output "" } */
+
+ // Various errors follow that we don't care about; suppress them:
+ // { dg-excess-errors "25: " }
+}
--
1.8.5.3
