diff mbox

[c++] fix PR 45330, suggest alternatives for failed name lookups

Message ID 20101110181356.GM7991@nightcrawler
State New
Headers show

Commit Message

Nathan Froyd Nov. 10, 2010, 6:13 p.m. UTC 
The testcase in PR 45330 is:

namespace N
{
 int foo;
}

int bar()
{
  return foo;
}

and the perceived problem is that GCC should suggest `N::foo' for the
unqualified `foo' in `bar'.  The below patch does just that: when name
lookup fails, we grovel through all known namespaces for potential
matches and suggest them to the user.

I put the declaration for the function in cp-tree.h rather than
name-lookup.h because it seemed like a better place than including
name-lookup.h in lex.c.  I'm happy to move the declaration to
name-lookup.h and add the include.  (Or, more radically, move
unqualified_name_lookup_error and unqualified_fn_lookup_error to
name-lookup.c and adjust includes appropriately.)

The error.c change is to avoid dump_expr complaining (as occurs in
g++.dg/lookup/koenig5.C).

I didn't include a testcase, as the testcase tweaks indicated there are
a number of testcases testing similar things.
(g++.old-deja/g++.mike/ns5.C is almost identical to the testcase in the
PR, for instance).

Tested on x86_64-unknown-linux-gnu.  OK to commit?

-Nathan

gcc/cp/
	PR c++/45330
	* cp-tree.h (suggest_alternatives_for): Declare.
	* error.c (dump_expr): Handle TYPE_DECL.
	* lex.c (unqualified_name_lookup_error): Call it.
	* name-lookup.c (suggest_alternatives_for_1): New function.
	(suggest_alternatives_for): New function.

gcc/testsuite/
	PR c++/45330
	* g++.dg/ext/builtin3.C: Adjust.
	* g++.dg/lookup/error1.C: Adjust.
	* g++.dg/lookup/koenig5.C: Adjust.
	* g++.dg/overload/koenig1.C: Adjust.
	* g++.dg/parse/decl-specifier-1.C: Adjust.
	* g++.dg/template/static10.C: Adjust.
	* g++.old-deja/g++.mike/ns5.C: Adjust.
	* g++.old-deja/g++.mike/ns7.C: Adjust.
	* g++.old-deja/g++.ns/koenig5.C: Adjust.
	* g++.old-deja/g++.ns/koenig9.C: Adjust.
	* g++.old-deja/g++.other/lineno5.C: Adjust.

Comments

Mark Mitchell Nov. 17, 2010, 2:31 a.m. UTC | #1 
On 11/10/2010 10:13 AM, Nathan Froyd wrote:

> and the perceived problem is that GCC should suggest `N::foo' for the
> unqualified `foo' in `bar'.  The below patch does just that: when name
> lookup fails, we grovel through all known namespaces for potential
> matches and suggest them to the user.

I thought about this for a while from two angles:

(a) Will that be incredibly slow?
(b) Is there any reasonable way to limit the set of namespaces searched?

I decided (a) doesn't really matter; we're issuing an error, worst-case
we probably have a few thousand namespaces, should be OK.

For (b), the obvious thing would be to limit to parents and children
(but not siblings) of current namespaces.  So, for:

  namespace A {
    namespace B { }
    namespace C {
      namespace D { }
    }
  }

if the error is inside C, look in D, but not B.  But, that's probably
dumb; I don't really believe that this is more likely than B.  So, it
would only be a useful heuristic if we decided we really need to prune
the search.  And, of course, we could refuse to search more than 100
namespaces, if we wanted.  I think you should probably add a --param
(max-namespace-for-diagnostic-help) for that, and start with 1000 as the
default, just to ensure sanity?

> I didn't include a testcase, as the testcase tweaks indicated there are
> a number of testcases testing similar things.

OK, but can you make at least one of the testcases check for the proper
alternative name?  (Right now, it looks like you're just checking the
line numbers.)

Thank you,
diff mbox

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 67f4f93..a323501 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5639,6 +5639,9 @@  extern tree cxx_omp_clause_dtor			(tree, tree);
 extern void cxx_omp_finish_clause		(tree);
 extern bool cxx_omp_privatize_by_reference	(const_tree);
 
+/* in name-lookup.c */
+extern void suggest_alternatives_for (tree);
+
 /* -- end of C++ */
 
 #endif /* ! GCC_CP_TREE_H */
diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c
index c583d7d..9c6be41 100644
--- a/gcc/cp/lex.c
+++ b/gcc/cp/lex.c
@@ -450,7 +450,10 @@  unqualified_name_lookup_error (tree name)
   else
     {
       if (!objc_diagnose_private_ivar (name))
-        error ("%qD was not declared in this scope", name);
+	{
+	  error ("%qD was not declared in this scope", name);
+	  suggest_alternatives_for (name);
+	}
       /* Prevent repeated error messages by creating a VAR_DECL with
 	 this NAME in the innermost block scope.  */
       if (current_function_decl)
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 1560fc6..de45efe 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -1700,6 +1700,7 @@  dump_expr (tree t, int flags)
     case NAMESPACE_DECL:
     case LABEL_DECL:
     case OVERLOAD:
+    case TYPE_DECL:
     case IDENTIFIER_NODE:
       dump_decl (t, (flags & ~TFF_DECL_SPECIFIERS) | TFF_NO_FUNCTION_ARGUMENTS);
       break;
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 0a93da8..1f2f4e5 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -30,6 +30,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "timevar.h"
 #include "toplev.h"
 #include "diagnostic-core.h"
+#include "intl.h"
 #include "debug.h"
 #include "c-family/c-pragma.h"
 
@@ -3919,6 +3920,62 @@  remove_hidden_names (tree fns)
   return fns;
 }
 
+/* Helper function for suggest_alternatives_for: lookup NAME in
+   namespace SCOPE and record any matches in CANDIDATES.  */
+
+static VEC(tree,heap) *
+suggest_alternatives_for_1 (tree name, tree scope,
+			    VEC(tree,heap) *candidates)
+{
+  struct scope_binding binding = EMPTY_SCOPE_BINDING;
+  struct cp_binding_level *level = NAMESPACE_LEVEL (scope);
+  tree t;
+
+  /* Look in this namespace.  */
+  qualified_lookup_using_namespace (name, scope, &binding, 0);
+
+  if (binding.value)
+    VEC_safe_push (tree, heap, candidates, binding.value);
+
+  /* Lookup in child namespaces.  */
+  for (t = level->namespaces; t; t = DECL_CHAIN (t))
+    candidates = suggest_alternatives_for_1 (name, t, candidates);
+
+  return candidates;
+}
+
+/* Suggest alternatives for NAME, an IDENTIFIER_NODE for which name
+   lookup failed.  Search through all available namespaces and print out
+   possible candidates.  */
+
+void
+suggest_alternatives_for (tree name)
+{
+  VEC(tree,heap) *candidates
+    = suggest_alternatives_for_1 (name, global_namespace, NULL);
+  const char *str;
+  char *spaces;
+  tree t;
+  unsigned ix;
+
+  /* Nothing useful to report.  */
+  if (VEC_empty (tree, candidates))
+    return;
+
+  str = (VEC_length(tree, candidates) > 1
+	 ? _("suggested alternatives:")
+	 : _("suggested alternative:"));
+  spaces = NULL;
+
+  FOR_EACH_VEC_ELT (tree, candidates, ix, t)
+    {
+      inform (input_location, "%s %qE", (spaces ? spaces : str), t);
+      spaces = spaces ? spaces : get_spaces (str);
+    }
+
+  VEC_free (tree, heap, candidates);
+}
+
 /* Unscoped lookup of a global: iterate over current namespaces,
    considering using-directives.  */
 
diff --git a/gcc/testsuite/g++.dg/ext/builtin3.C b/gcc/testsuite/g++.dg/ext/builtin3.C
index 3d06dd7..a9ebce0 100644
--- a/gcc/testsuite/g++.dg/ext/builtin3.C
+++ b/gcc/testsuite/g++.dg/ext/builtin3.C
@@ -10,4 +10,5 @@  extern "C" int printf(char*, ...);
 
 void foo() {
   printf("abc"); 		// { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 12 }
 }
diff --git a/gcc/testsuite/g++.dg/lookup/error1.C b/gcc/testsuite/g++.dg/lookup/error1.C
index 2264b23..3b34ee3 100644
--- a/gcc/testsuite/g++.dg/lookup/error1.C
+++ b/gcc/testsuite/g++.dg/lookup/error1.C
@@ -4,6 +4,7 @@ 
 
 namespace N { int i; }
 void foo() { i; }   // { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 6 }
 
 using namespace N;
 void bar() { i; }
diff --git a/gcc/testsuite/g++.dg/lookup/koenig5.C b/gcc/testsuite/g++.dg/lookup/koenig5.C
index 6ecc25d..bc1dc8c 100644
--- a/gcc/testsuite/g++.dg/lookup/koenig5.C
+++ b/gcc/testsuite/g++.dg/lookup/koenig5.C
@@ -32,10 +32,12 @@  void g (N::A *a, M::B *b, O::C *c)
   One (a); // ok
   One (a, b); // ok
   One (b); // { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 34 }
 
   Two (c); // ok
   Two (a, c); // ok
   Two (a); // { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 39 }
   Two (a, a); // error masked by earlier error
   Two (b); // error masked by earlier error
   Two (a, b); // error masked by earlier error
@@ -43,4 +45,5 @@  void g (N::A *a, M::B *b, O::C *c)
   Three (b); // ok
   Three (a, b); // ok
   Three (a); // { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 47 }
 }
diff --git a/gcc/testsuite/g++.dg/overload/koenig1.C b/gcc/testsuite/g++.dg/overload/koenig1.C
index 1ed7bce..be0be69 100644
--- a/gcc/testsuite/g++.dg/overload/koenig1.C
+++ b/gcc/testsuite/g++.dg/overload/koenig1.C
@@ -14,5 +14,6 @@  void g ()
   B *bp;
   N::A *ap;
   f (bp);			// { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 16 }
   f (ap);
 }
diff --git a/gcc/testsuite/g++.dg/parse/decl-specifier-1.C b/gcc/testsuite/g++.dg/parse/decl-specifier-1.C
index e81fbab..48fc7fa 100644
--- a/gcc/testsuite/g++.dg/parse/decl-specifier-1.C
+++ b/gcc/testsuite/g++.dg/parse/decl-specifier-1.C
@@ -13,4 +13,5 @@  N::X X;                           // { dg-error "" "" }
 int main()
 {
     return sizeof(X);             // { dg-error "" "" }
+    // { dg-message "note" "suggested alternative" { target *-*-* } 15 }
 }
diff --git a/gcc/testsuite/g++.dg/template/static10.C b/gcc/testsuite/g++.dg/template/static10.C
index ab857bd..2f60f0c 100644
--- a/gcc/testsuite/g++.dg/template/static10.C
+++ b/gcc/testsuite/g++.dg/template/static10.C
@@ -20,4 +20,5 @@  namespace std
 {
   template<> void
   vector<int, allocator<int> >::swap(vector<int, allocator<int> >&) { } // { dg-error "" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 22 }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.mike/ns5.C b/gcc/testsuite/g++.old-deja/g++.mike/ns5.C
index 9d806ca..f13da04 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/ns5.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/ns5.C
@@ -4,3 +4,4 @@  namespace A {
 }
 
 int j = i;		// { dg-error "" } 
+  // { dg-message "note" "suggested alternative" { target *-*-* } 6 }
diff --git a/gcc/testsuite/g++.old-deja/g++.mike/ns7.C b/gcc/testsuite/g++.old-deja/g++.mike/ns7.C
index 57008db..31a71dd 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/ns7.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/ns7.C
@@ -6,4 +6,5 @@  namespace A {
 
 namespace B {
   int j = i;	// { dg-error "" } 
+  // { dg-message "note" "suggested alternative" { target *-*-* } 8 }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C b/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C
index 33061ad..67b781d 100644
--- a/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C
+++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C
@@ -15,4 +15,5 @@  void g()
 			 // foo variable first, and therefore do not
 			 // perform argument-dependent lookup.
   bar(new X);            // { dg-error "not declared" }
+  // { dg-message "note" "suggested alternative" { target *-*-* } 17 }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C b/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C
index 78b0e8b..f246639 100644
--- a/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C
+++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C
@@ -10,4 +10,5 @@  void foo(const char*,...);
 inline void
 bar() {
   foo("",count);    //  { dg-error "" } multiple overloaded count functions
+  // { dg-message "note" "suggested alternative" { target *-*-* } 12 }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
index d14bd90..20e49bc 100644
--- a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
+++ b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
@@ -16,4 +16,5 @@  namespace tmp {
 class A {
   public:
   int kaka(tmp::B = b);		// { dg-error "" } no b in scope
+  // { dg-message "note" "suggested alternative" { target *-*-* } 18 }
 };