C++: suggestions for misspelled private members (PR c++/84993)
authorDavid Malcolm <dmalcolm@redhat.com>
Thu, 11 Oct 2018 19:03:33 +0000 (19:03 +0000)
committerDavid Malcolm <dmalcolm@gcc.gnu.org>
Thu, 11 Oct 2018 19:03:33 +0000 (19:03 +0000)
PR c++/84993 identifies a problem with our suggestions for
misspelled member names in the C++ FE for the case where the
member is private.

For example, given:

class foo
{
public:
  double get_ratio() const { return m_ratio; }

private:
  double m_ratio;
};

void test(foo *ptr)
{
  if (ptr->ratio >= 0.5)
    ;// etc
}

...we currently emit this suggestion:

<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean 'm_ratio'?
   if (ptr->ratio >= 0.5)
            ^~~~~
            m_ratio

...but if the user follows this suggestion, they get:

<source>: In function 'void test(foo*)':
<source>:12:12: error: 'double foo::m_ratio' is private within this context
   if (ptr->m_ratio >= 0.5)
            ^~~~~~~
<source>:7:10: note: declared private here
   double m_ratio;
          ^~~~~~~
<source>:12:12: note: field 'double foo::m_ratio' can be accessed via 'double foo::get_ratio() const'
   if (ptr->m_ratio >= 0.5)
            ^~~~~~~
            get_ratio()

It feels wrong to be emitting a fix-it hint that doesn't compile, so this
patch adds the accessor fix-it hint logic to this case, so that we directly
offer a valid suggestion:

<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean
'double foo::m_ratio'? (accessible via 'double foo::get_ratio() const')
   if (ptr->ratio >= 0.5)
            ^~~~~
            get_ratio()

gcc/cp/ChangeLog:
PR c++/84993
* call.c (enforce_access): Move diagnostics to...
(complain_about_access): ...this new function.
* cp-tree.h (class access_failure_info): Rename split out field
"m_field_decl" into "m_decl" and "m_diag_decl".
(access_failure_info::record_access_failure): Add tree param.
(access_failure_info::was_inaccessible_p): New accessor.
(access_failure_info::get_decl): New accessor.
(access_failure_info::get_diag_decl): New accessor.
(access_failure_info::get_any_accessor): New member function.
(access_failure_info::add_fixit_hint): New static member function.
(complain_about_access): New decl.
* typeck.c (access_failure_info::record_access_failure): Update
for change to fields.
(access_failure_info::maybe_suggest_accessor): Split out into...
(access_failure_info::get_any_accessor): ...this new function...
(access_failure_info::add_fixit_hint): ...and this new function.
(finish_class_member_access_expr): Split out "has no member named"
error-handling into...
(complain_about_unrecognized_member): ...this new function, and
check that the guessed name is accessible along the access path.
Only provide a spell-correction fix-it hint if it is; otherwise,
attempt to issue an accessor fix-it hint.

gcc/testsuite/ChangeLog:
PR c++/84993
* g++.dg/torture/accessor-fixits-9.C: New test.

From-SVN: r265056

gcc/cp/ChangeLog
gcc/cp/call.c
gcc/cp/cp-tree.h
gcc/cp/typeck.c
gcc/testsuite/ChangeLog
gcc/testsuite/g++.dg/torture/accessor-fixits-9.C [new file with mode: 0644]

index da443d9be407d0d06afc871ac48a5346a2cb5272..a3d29d75b36e445d88970e6c7fca80385b6fbd68 100644 (file)
@@ -1,3 +1,29 @@
+2018-10-11  David Malcolm  <dmalcolm@redhat.com>
+
+       PR c++/84993
+       * call.c (enforce_access): Move diagnostics to...
+       (complain_about_access): ...this new function.
+       * cp-tree.h (class access_failure_info): Rename split out field
+       "m_field_decl" into "m_decl" and "m_diag_decl".
+       (access_failure_info::record_access_failure): Add tree param.
+       (access_failure_info::was_inaccessible_p): New accessor.
+       (access_failure_info::get_decl): New accessor.
+       (access_failure_info::get_diag_decl): New accessor.
+       (access_failure_info::get_any_accessor): New member function.
+       (access_failure_info::add_fixit_hint): New static member function.
+       (complain_about_access): New decl.
+       * typeck.c (access_failure_info::record_access_failure): Update
+       for change to fields.
+       (access_failure_info::maybe_suggest_accessor): Split out into...
+       (access_failure_info::get_any_accessor): ...this new function...
+       (access_failure_info::add_fixit_hint): ...and this new function.
+       (finish_class_member_access_expr): Split out "has no member named"
+       error-handling into...
+       (complain_about_unrecognized_member): ...this new function, and
+       check that the guessed name is accessible along the access path.
+       Only provide a spell-correction fix-it hint if it is; otherwise,
+       attempt to issue an accessor fix-it hint.
+
 2018-10-11  Nathan Sidwell  <nathan@acm.org>
 
        * parser.c (cp_parser_translation_unit): Return void.  Don't fail
index 747f837019dc550f58d4cf00c54007c6ce524db9..0baf26e43460e7f061f606e5a6b423c4995333b6 100644 (file)
@@ -6514,6 +6514,38 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
   return error_mark_node;
 }
 
+/* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
+   in the diagnostics.
+
+   If ISSUE_ERROR is true, then issue an error about the
+   access, followed by a note showing the declaration.
+   Otherwise, just show the note.  */
+
+void
+complain_about_access (tree decl, tree diag_decl, bool issue_error)
+{
+  if (TREE_PRIVATE (decl))
+    {
+      if (issue_error)
+       error ("%q#D is private within this context", diag_decl);
+      inform (DECL_SOURCE_LOCATION (diag_decl),
+             "declared private here");
+    }
+  else if (TREE_PROTECTED (decl))
+    {
+      if (issue_error)
+       error ("%q#D is protected within this context", diag_decl);
+      inform (DECL_SOURCE_LOCATION (diag_decl),
+             "declared protected here");
+    }
+  else
+    {
+      if (issue_error)
+       error ("%q#D is inaccessible within this context", diag_decl);
+      inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
+    }
+}
+
 /* If the current scope isn't allowed to access DECL along
    BASETYPE_PATH, give an error.  The most derived class in
    BASETYPE_PATH is the one used to qualify DECL. DIAG_DECL is
@@ -6538,34 +6570,12 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
 
   if (!accessible_p (basetype_path, decl, true))
     {
+      if (flag_new_inheriting_ctors)
+       diag_decl = strip_inheriting_ctors (diag_decl);
       if (complain & tf_error)
-       {
-         if (flag_new_inheriting_ctors)
-           diag_decl = strip_inheriting_ctors (diag_decl);
-         if (TREE_PRIVATE (decl))
-           {
-             error ("%q#D is private within this context", diag_decl);
-             inform (DECL_SOURCE_LOCATION (diag_decl),
-                     "declared private here");
-             if (afi)
-               afi->record_access_failure (basetype_path, diag_decl);
-           }
-         else if (TREE_PROTECTED (decl))
-           {
-             error ("%q#D is protected within this context", diag_decl);
-             inform (DECL_SOURCE_LOCATION (diag_decl),
-                     "declared protected here");
-             if (afi)
-               afi->record_access_failure (basetype_path, diag_decl);
-           }
-         else
-           {
-             error ("%q#D is inaccessible within this context", diag_decl);
-             inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
-             if (afi)
-               afi->record_access_failure (basetype_path, diag_decl);
-           }
-       }
+       complain_about_access (decl, diag_decl, true);
+      if (afi)
+       afi->record_access_failure (basetype_path, decl, diag_decl);
       return false;
     }
 
index efbdad83966ff94f4c558d397f10405322a86ccc..26ded3a921438d80dcfc15d65a8d03f3502b891e 100644 (file)
@@ -6103,18 +6103,27 @@ extern void complain_about_bad_argument (location_t arg_loc,
 class access_failure_info
 {
  public:
-  access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
-    m_field_decl (NULL_TREE) {}
+  access_failure_info () : m_was_inaccessible (false),
+    m_basetype_path (NULL_TREE),
+    m_decl (NULL_TREE), m_diag_decl (NULL_TREE) {}
 
-  void record_access_failure (tree basetype_path, tree field_decl);
+  void record_access_failure (tree basetype_path, tree decl, tree diag_decl);
+
+  bool was_inaccessible_p () const { return m_was_inaccessible; }
+  tree get_decl () const { return m_decl; }
+  tree get_diag_decl () const { return m_diag_decl; }
+  tree get_any_accessor (bool const_p) const;
   void maybe_suggest_accessor (bool const_p) const;
+  static void add_fixit_hint (rich_location *richloc, tree accessor);
 
  private:
   bool m_was_inaccessible;
   tree m_basetype_path;
-  tree m_field_decl;
+  tree m_decl;
+  tree m_diag_decl;
 };
 
+extern void complain_about_access              (tree, tree, bool);
 extern bool enforce_access                     (tree, tree, tree,
                                                 tsubst_flags_t,
                                                 access_failure_info *afi = NULL);
index 8f81959253331427f172988b7a8e3c2e7a2a41f0..c921096cb31f84d5e5dfef1dd107b931917eecd8 100644 (file)
@@ -2708,43 +2708,138 @@ check_template_keyword (tree decl)
 }
 
 /* Record that an access failure occurred on BASETYPE_PATH attempting
-   to access FIELD_DECL.  */
+   to access DECL, where DIAG_DECL should be used for diagnostics.  */
 
 void
 access_failure_info::record_access_failure (tree basetype_path,
-                                           tree field_decl)
+                                           tree decl, tree diag_decl)
 {
   m_was_inaccessible = true;
   m_basetype_path = basetype_path;
-  m_field_decl = field_decl;
+  m_decl = decl;
+  m_diag_decl = diag_decl;
 }
 
 /* If an access failure was recorded, then attempt to locate an
-   accessor function for the pertinent field, and if one is
-   available, add a note and fix-it hint suggesting using it.  */
+   accessor function for the pertinent field.
+   Otherwise, return NULL_TREE.  */
 
-void
-access_failure_info::maybe_suggest_accessor (bool const_p) const
+tree
+access_failure_info::get_any_accessor (bool const_p) const
 {
-  if (!m_was_inaccessible)
-    return;
+  if (!was_inaccessible_p ())
+    return NULL_TREE;
 
   tree accessor
-    = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
+    = locate_field_accessor (m_basetype_path, m_diag_decl, const_p);
   if (!accessor)
-    return;
+    return NULL_TREE;
 
   /* The accessor must itself be accessible for it to be a reasonable
      suggestion.  */
   if (!accessible_p (m_basetype_path, accessor, true))
-    return;
+    return NULL_TREE;
 
-  rich_location richloc (line_table, input_location);
+  return accessor;
+}
+
+/* Add a fix-it hint to RICHLOC suggesting the use of ACCESSOR_DECL, by
+   replacing the primary location in RICHLOC with "accessor()".  */
+
+void
+access_failure_info::add_fixit_hint (rich_location *richloc,
+                                    tree accessor_decl)
+{
   pretty_printer pp;
-  pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
-  richloc.add_fixit_replace (pp_formatted_text (&pp));
+  pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor_decl)));
+  richloc->add_fixit_replace (pp_formatted_text (&pp));
+}
+
+/* If an access failure was recorded, then attempt to locate an
+   accessor function for the pertinent field, and if one is
+   available, add a note and fix-it hint suggesting using it.  */
+
+void
+access_failure_info::maybe_suggest_accessor (bool const_p) const
+{
+  tree accessor = get_any_accessor (const_p);
+  if (accessor == NULL_TREE)
+    return;
+  rich_location richloc (line_table, input_location);
+  add_fixit_hint (&richloc, accessor);
   inform (&richloc, "field %q#D can be accessed via %q#D",
-         m_field_decl, accessor);
+         m_diag_decl, accessor);
+}
+
+/* Subroutine of finish_class_member_access_expr.
+   Issue an error about NAME not being a member of ACCESS_PATH (or
+   OBJECT_TYPE), potentially providing a fix-it hint for misspelled
+   names.  */
+
+static void
+complain_about_unrecognized_member (tree access_path, tree name,
+                                   tree object_type)
+{
+  /* Attempt to provide a hint about misspelled names.  */
+  tree guessed_id = lookup_member_fuzzy (access_path, name,
+                                        /*want_type=*/false);
+  if (guessed_id == NULL_TREE)
+    {
+      /* No hint.  */
+      error ("%q#T has no member named %qE",
+            TREE_CODE (access_path) == TREE_BINFO
+            ? TREE_TYPE (access_path) : object_type, name);
+      return;
+    }
+
+  location_t bogus_component_loc = input_location;
+  gcc_rich_location rich_loc (bogus_component_loc);
+
+  /* Check that the guessed name is accessible along access_path.  */
+  access_failure_info afi;
+  lookup_member (access_path, guessed_id, /*protect=*/1,
+                /*want_type=*/false, /*complain=*/false,
+                &afi);
+  if (afi.was_inaccessible_p ())
+    {
+      tree accessor = afi.get_any_accessor (TYPE_READONLY (object_type));
+      if (accessor)
+       {
+         /* The guessed name isn't directly accessible, but can be accessed
+            via an accessor member function.  */
+         afi.add_fixit_hint (&rich_loc, accessor);
+         error_at (&rich_loc,
+                   "%q#T has no member named %qE;"
+                   " did you mean %q#D? (accessible via %q#D)",
+                   TREE_CODE (access_path) == TREE_BINFO
+                   ? TREE_TYPE (access_path) : object_type,
+                   name, afi.get_diag_decl (), accessor);
+       }
+      else
+       {
+         /* The guessed name isn't directly accessible, and no accessor
+            member function could be found.  */
+         error_at (&rich_loc,
+                   "%q#T has no member named %qE;"
+                   " did you mean %q#D? (not accessible from this context)",
+                   TREE_CODE (access_path) == TREE_BINFO
+                   ? TREE_TYPE (access_path) : object_type,
+                   name, afi.get_diag_decl ());
+         complain_about_access (afi.get_decl (), afi.get_diag_decl (), false);
+       }
+    }
+  else
+    {
+      /* The guessed name is directly accessible; suggest it.  */
+      rich_loc.add_fixit_misspelled_id (bogus_component_loc,
+                                       guessed_id);
+      error_at (&rich_loc,
+               "%q#T has no member named %qE;"
+               " did you mean %qE?",
+               TREE_CODE (access_path) == TREE_BINFO
+               ? TREE_TYPE (access_path) : object_type,
+               name, guessed_id);
+    }
 }
 
 /* This function is called by the parser to process a class member
@@ -2940,27 +3035,8 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
                /* Try again at instantiation time.  */
                goto dependent;
              if (complain & tf_error)
-               {
-                 tree guessed_id = lookup_member_fuzzy (access_path, name,
-                                                        /*want_type=*/false);
-                 if (guessed_id)
-                   {
-                     location_t bogus_component_loc = input_location;
-                     gcc_rich_location rich_loc (bogus_component_loc);
-                     rich_loc.add_fixit_misspelled_id (bogus_component_loc,
-                                                       guessed_id);
-                     error_at (&rich_loc,
-                               "%q#T has no member named %qE;"
-                               " did you mean %qE?",
-                               TREE_CODE (access_path) == TREE_BINFO
-                               ? TREE_TYPE (access_path) : object_type,
-                               name, guessed_id);
-                   }
-                 else
-                   error ("%q#T has no member named %qE",
-                          TREE_CODE (access_path) == TREE_BINFO
-                          ? TREE_TYPE (access_path) : object_type, name);
-               }
+               complain_about_unrecognized_member (access_path, name,
+                                                   object_type);
              return error_mark_node;
            }
          if (member == error_mark_node)
index df8f70c671e62689d080e51fd9720263dfd8114b..a1ba7da09b832142f94da158e3d2bd9dbc0d44eb 100644 (file)
@@ -1,3 +1,8 @@
+2018-10-11  David Malcolm  <dmalcolm@redhat.com>
+
+       PR c++/84993
+       * g++.dg/torture/accessor-fixits-9.C: New test.
+
 2018-10-11  Nathan Sidwell  <nathan@acm.org>
 
        * g++.dg/parse/close-brace.C: New.
diff --git a/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
new file mode 100644 (file)
index 0000000..d9e77ba
--- /dev/null
@@ -0,0 +1,119 @@
+// PR c++/84993
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Misspelling (by omitting a leading "m_") of a private member for which
+   there's a public accessor.
+
+   We expect a fix-it hint suggesting the accessor.  */
+
+class t1
+{
+public:
+  int get_ratio () const { return m_ratio; }
+
+private:
+  int m_ratio;
+};
+
+int test (t1 *ptr_1)
+{
+  return ptr_1->ratio; // { dg-error "'class t1' has no member named 'ratio'; did you mean 'int t1::m_ratio'\\? \\(accessible via 'int t1::get_ratio\\(\\) const'\\)" }
+  /* { dg-begin-multiline-output "" }
+   return ptr_1->ratio;
+                 ^~~~~
+                 get_ratio()
+     { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member for which there's a public accessor.
+
+   We expect a fix-it hint suggesting the accessor.  */
+
+class t2
+{
+public:
+  int get_color () const { return m_color; }
+
+private:
+  int m_color;
+};
+
+int test (t2 *ptr_2)
+{
+  return ptr_2->m_colour; // { dg-error "'class t2' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+  /* { dg-begin-multiline-output "" }
+   return ptr_2->m_colour;
+                 ^~~~~~~~
+                 get_color()
+     { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member via a subclass pointer, for which there's
+   a public accessor in the base class.
+
+   We expect a fix-it hint suggesting the accessor.  */
+
+class t3 : public t2 {};
+
+int test (t3 *ptr_3)
+{
+  return ptr_3->m_colour; // { dg-error "'class t3' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+  /* { dg-begin-multiline-output "" }
+   return ptr_3->m_colour;
+                 ^~~~~~~~
+                 get_color()
+     { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a protected member, for which there's isn't a public
+   accessor.
+
+   We expect no fix-it hint; instead a message identifying where the
+   data member was declared.  */
+
+class t4
+{
+protected:
+  int m_color; // { dg-message "declared protected here" }
+};
+
+int test (t4 *ptr_4)
+{
+  return ptr_4->m_colour; // { dg-error "'class t4' has no member named 'm_colour'; did you mean 'int t4::m_color'\\? \\(not accessible from this context\\)" }
+  /* { dg-begin-multiline-output "" }
+   return ptr_4->m_colour;
+                 ^~~~~~~~
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member, for which the accessor is also private.
+
+   We expect no fix-it hint; instead a message identifying where the
+   data member was declared.  */
+
+class t5
+{
+  int get_color () const { return m_color; }
+  int m_color; // { dg-message "declared private here" }
+};
+
+int test (t5 *ptr_5)
+{
+  return ptr_5->m_colour; // { dg-error "'class t5' has no member named 'm_colour'; did you mean 'int t5::m_color'\\? \\(not accessible from this context\\)" }
+  /* { dg-begin-multiline-output "" }
+   return ptr_5->m_colour;
+                 ^~~~~~~~
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+}