[PATCH] C++: fix-it hints suggesting accessors for private fields

[PATCH] C++: fix-it hints suggesting accessors for private fields

[David Malcolm]

Given e.g.
   class foo
   {
   public:
     int get_field () const { return m_field; }

   private:
     int m_field;
   };

...if the user attempts to access the private field from the
wrong place we emit:

test.cc: In function ‘int test(foo*)’:
test.cc:12:13: error: ‘int foo::m_field’ is private within this context
   return f->m_field;
             ^~~~~~~
test.cc:7:7: note: declared private here
   int m_field;
       ^~~~~~~

This patch adds a note with a fix-it hint to the above, suggesting
the correct accessor to use:

test.cc:12:13: note: field ‘int foo::m_field’ can be accessed via ‘int foo::get_field() const’
   return f->m_field;
             ^~~~~~~
             get_field()

Assuming that an IDE can offer to apply fix-it hints, this should
make it easier to handle refactorings where one makes a field
private and adds a getter.

It also helps by letting the user know that a getter exists, and
the name of the getter ("is it "field", "get_field", etc?").

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/cp/ChangeLog:
	* call.c (maybe_suggest_accessor): New function.
	(enforce_access): Call maybe_suggest_accessor for inaccessible
	decls.
	* cp-tree.h (locate_field_accessor): New decl.
	* search.c (matches_code_and_type_p): New function.
	(field_access_p): New function.
	(direct_accessor_p): New function.
	(reference_accessor_p): New function.
	(field_accessor_p): New function.
	(dfs_locate_field_accessor_pre): New function.
	(locate_field_accessor): New function.

gcc/testsuite/ChangeLog:
	* g++.dg/other/accessor-fixits-1.C: New test case.
	* g++.dg/other/accessor-fixits-2.C: New test case.
	* g++.dg/other/accessor-fixits-3.C: New test case.
---
 gcc/cp/call.c                                  |  28 ++++
 gcc/cp/cp-tree.h                               |   1 +
 gcc/cp/search.c                                | 204 +++++++++++++++++++++++++
 gcc/testsuite/g++.dg/other/accessor-fixits-1.C | 176 +++++++++++++++++++++
 gcc/testsuite/g++.dg/other/accessor-fixits-2.C | 104 +++++++++++++
 gcc/testsuite/g++.dg/other/accessor-fixits-3.C |  15 ++
 6 files changed, 528 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-1.C
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-2.C
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-3.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c15b8e4..67f18aa 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6408,6 +6408,31 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
   return error_mark_node;
 }
 
+/* Helper function for enforce_access when FIELD_DECL is not accessible
+   along BASETYPE_PATH (e.g. due to being private).
+   Attempt to locate an accessor function for the field, and if one is
+   available, add a note and fix-it hint suggesting using it.  */
+
+static void
+maybe_suggest_accessor (tree basetype_path, tree field_decl)
+{
+  tree accessor = locate_field_accessor (basetype_path, field_decl);
+  if (!accessor)
+    return;
+
+  /* The accessor must itself be accessible for it to be a reasonable
+     suggestion.  */
+  if (!accessible_p (basetype_path, accessor, true))
+    return;
+
+  rich_location richloc (line_table, input_location);
+  pretty_printer pp;
+  pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
+  richloc.add_fixit_replace (pp_formatted_text (&pp));
+  inform_at_rich_loc (&richloc, "field %q#D can be accessed via %q#D",
+		      field_decl, accessor);
+}
+
 /* 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
@@ -6441,17 +6466,20 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
 	      error ("%q#D is private within this context", diag_decl);
 	      inform (DECL_SOURCE_LOCATION (diag_decl),
 		      "declared private here");
+	      maybe_suggest_accessor (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");
+	      maybe_suggest_accessor (basetype_path, diag_decl);
 	    }
 	  else
 	    {
 	      error ("%q#D is inaccessible within this context", diag_decl);
 	      inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
+	      maybe_suggest_accessor (basetype_path, diag_decl);
 	    }
 	}
       return false;
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 67dfea2..e5bb6b7 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6323,6 +6323,7 @@ extern tree lookup_fnfields			(tree, tree, int);
 extern tree lookup_member			(tree, tree, int, bool,
 						 tsubst_flags_t);
 extern tree lookup_member_fuzzy		(tree, tree, bool);
+extern tree locate_field_accessor		(tree, tree);
 extern int look_for_overrides			(tree, tree);
 extern void get_pure_virtuals			(tree);
 extern void maybe_suppress_debug_info		(tree);
diff --git a/gcc/cp/search.c b/gcc/cp/search.c
index 09c1b4e..2b1fb2f 100644
--- a/gcc/cp/search.c
+++ b/gcc/cp/search.c
@@ -1993,6 +1993,210 @@ dfs_walk_once_accessible (tree binfo, bool friends_p,
   return rval;
 }
 
+/* Return true iff the code of T is CODE, and it has compatible
+   type with TYPE.  */
+
+static bool
+matches_code_and_type_p (tree t, enum tree_code code, tree type)
+{
+  if (TREE_CODE (t) != code)
+    return false;
+  if (!cxx_types_compatible_p (TREE_TYPE (t), type))
+    return false;
+  return true;
+}
+
+/* Subroutine of direct_accessor_p and reference_accessor_p.
+   Determine if COMPONENT_REF is a simple field lookup of this->FIELD_DECL.
+   We expect a tree of the form:
+	     <component_ref:
+	       <indirect_ref:S>
+		 <nop_expr:P*
+		   <parm_decl (this)>
+		 <field_decl (FIELD_DECL)>>>.  */
+
+static bool
+field_access_p (tree component_ref, tree field_decl, tree field_type)
+{
+  if (!matches_code_and_type_p (component_ref, COMPONENT_REF, field_type))
+    return false;
+
+  tree indirect_ref = TREE_OPERAND (component_ref, 0);
+  if (TREE_CODE (indirect_ref) != INDIRECT_REF)
+    return false;
+
+  tree ptr = STRIP_NOPS (TREE_OPERAND (indirect_ref, 0));
+  if (!is_this_parameter (ptr))
+    return false;
+
+  /* Must access the correct field.  */
+  if (TREE_OPERAND (component_ref, 1) != field_decl)
+    return false;
+  return true;
+}
+
+/* Subroutine of field_accessor_p.
+
+   Assuming that INIT_EXPR has already had its code and type checked,
+   determine if it is a simple accessor for FIELD_DECL
+   (of type FIELD_TYPE).
+
+   Specifically, a simple accessor within struct S of the form:
+       T get_field () { return m_field; }
+   should have a DECL_SAVED_TREE of the form:
+       <return_expr
+	 <init_expr:T
+	   <result_decl:T
+	   <nop_expr:T
+	     <component_ref:
+	       <indirect_ref:S>
+		 <nop_expr:P*
+		   <parm_decl (this)>
+		 <field_decl (FIELD_DECL)>>>.  */
+
+static bool
+direct_accessor_p (tree init_expr, tree field_decl, tree field_type)
+{
+  tree result_decl = TREE_OPERAND (init_expr, 0);
+  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_type))
+    return false;
+
+  tree component_ref = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
+  if (!field_access_p (component_ref, field_decl, field_type))
+    return false;
+
+  return true;
+}
+
+/* Subroutine of field_accessor_p.
+
+   Assuming that INIT_EXPR has already had its code and type checked,
+   determine if it is a "reference" accessor for FIELD_DECL
+   (of type FIELD_REFERENCE_TYPE).
+
+   Specifically, a simple accessor within struct S of the form:
+       T& get_field () { return m_field; }
+   should have a DECL_SAVED_TREE of the form:
+       <return_expr
+	 <init_expr:T&
+	   <result_decl:T&
+	   <nop_expr: T&
+	     <addr_expr: T*
+	       <component_ref:T
+		 <indirect_ref:S
+		   <nop_expr
+		     <parm_decl (this)>>
+		   <field (FIELD_DECL)>>>>>>.  */
+static bool
+reference_accessor_p (tree init_expr, tree field_decl, tree field_type,
+		      tree field_reference_type)
+{
+  tree result_decl = TREE_OPERAND (init_expr, 0);
+  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_reference_type))
+    return false;
+
+  tree field_pointer_type = build_pointer_type (field_type);
+  tree addr_expr = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
+  if (!matches_code_and_type_p (addr_expr, ADDR_EXPR, field_pointer_type))
+    return false;
+
+  tree component_ref = STRIP_NOPS (TREE_OPERAND (addr_expr, 0));
+
+  if (!field_access_p (component_ref, field_decl, field_type))
+    return false;
+
+  return true;
+}
+
+/* Return true if FN is an accessor method for FIELD_DECL.
+   i.e. a method of the form { return FIELD; }, with no
+   conversions. */
+
+static bool
+field_accessor_p (tree fn, tree field_decl)
+{
+  if (TREE_CODE (fn) != FUNCTION_DECL)
+    return false;
+
+  /* We don't yet support looking up static data, just fields.  */
+  if (TREE_CODE (field_decl) != FIELD_DECL)
+    return false;
+
+  tree saved_tree = DECL_SAVED_TREE (fn);
+
+  if (saved_tree == NULL_TREE)
+    return false;
+
+  if (TREE_CODE (saved_tree) != RETURN_EXPR)
+    return false;
+
+  tree init_expr = TREE_OPERAND (saved_tree, 0);
+  if (TREE_CODE (init_expr) != INIT_EXPR)
+    return false;
+
+  /* Determine if this is a simple accessor within struct S of the form:
+       T get_field () { return m_field; }.  */
+   tree field_type = TREE_TYPE (field_decl);
+  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_type))
+    return direct_accessor_p (init_expr, field_decl, field_type);
+
+  /* Failing that, determine if it is an accessor of the form:
+       T& get_field () { return m_field; }.  */
+  tree field_reference_type = cp_build_reference_type (field_type, false);
+  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_reference_type))
+    return reference_accessor_p (init_expr, field_decl, field_type,
+				 field_reference_type);
+
+  return false;
+}
+
+/* Return a FUNCTION_DECL that is an "accessor" method for DATA, a FIELD_DECL,
+   callable via binfo, if one exists, otherwise return NULL_TREE.
+
+   Callback for dfs_walk_once_accessible for use within
+   locate_field_accessor.  */
+
+static tree
+dfs_locate_field_accessor_pre (tree binfo, void *data)
+{
+  tree field_decl = (tree)data;
+  tree type = BINFO_TYPE (binfo);
+
+  vec<tree, va_gc> *method_vec;
+  tree fn;
+  size_t i;
+
+  if (!CLASS_TYPE_P (type))
+    return NULL_TREE;
+
+  method_vec = CLASSTYPE_METHOD_VEC (type);
+  if (!method_vec)
+    return NULL_TREE;
+
+  for (i = 0; vec_safe_iterate (method_vec, i, &fn); ++i)
+    if (fn)
+      if (field_accessor_p (fn, field_decl))
+	return fn;
+
+  return NULL_TREE;
+}
+
+/* Return a FUNCTION_DECL that is an "accessor" method for FIELD_DECL,
+   callable via BASETYPE_PATH, if one exists, otherwise return NULL_TREE.  */
+
+tree
+locate_field_accessor (tree basetype_path, tree field_decl)
+{
+  if (TREE_CODE (basetype_path) != TREE_BINFO)
+    return NULL_TREE;
+
+  /* Walk the hierarchy, looking for a method of some base class that allows
+     access to the field.  */
+  return dfs_walk_once_accessible (basetype_path, /*friends=*/true,
+				   dfs_locate_field_accessor_pre,
+				   NULL, field_decl);
+}
+
 /* Check that virtual overrider OVERRIDER is acceptable for base function
    BASEFN. Issue diagnostic, and return zero, if unacceptable.  */
 
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-1.C b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
new file mode 100644
index 0000000..bf51bae
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
@@ -0,0 +1,176 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+class t1
+{
+public:
+  int get_color () const { return m_color; }
+  int get_shape () const { return m_shape; }
+
+private:
+  int m_color;
+
+protected:
+  int m_shape;
+};
+
+int test_access_t1_color (t1 &ref)
+{
+  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } 10 }
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+              get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_access_t1_shape (t1 &ref)
+{
+  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared protected here" "" { target *-*-* } 13 }
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+              get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_color (t1 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_shape (t1 *ptr)
+{
+  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+               get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+class t2 : public t1
+{
+};
+
+int test_deref_t2_color (t2 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+/* Example of private inheritance.  */
+
+class t3 : private t1
+{
+};
+
+int test_deref_t3_color (t3 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't provide a fix-it hint for this case due to the
+     private inheritance.  */
+}
+
+/* Example of non-public "accessor".  */
+
+class t4
+{
+  int m_field;
+  int get_field () { return m_field; }
+};
+
+int test_deref_t4_field (t4 *ptr)
+{
+  return ptr->m_field; // { dg-error ".int t4::m_field. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_field;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* { dg-begin-multiline-output "" }
+   int m_field;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't provide a fix-it hint for this case, as the accessor is
+     itself private.  */
+}
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-2.C b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
new file mode 100644
index 0000000..e1a2b78
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
@@ -0,0 +1,104 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Test of accessors that return references.  */
+
+class t1
+{
+public:
+  int& get_color () { return m_color; }
+  int& get_shape () { return m_shape; }
+
+private:
+  int m_color;
+
+protected:
+  int m_shape;
+};
+
+int test_access_t1_color (t1 &ref)
+{
+  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } 12 }
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+              get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_access_t1_shape (t1 &ref)
+{
+  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared protected here" "" { target *-*-* } 15 }
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+              get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_color (t1 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_shape (t1 *ptr)
+{
+  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+               get_shape()
+     { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-3.C b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
new file mode 100644
index 0000000..27d2eb4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
@@ -0,0 +1,15 @@
+class foo
+{
+public:
+  static foo& get_singleton () { return s_singleton; }
+
+private:
+  static foo s_singleton;
+};
+
+foo & test_access_singleton ()
+{
+  return foo::s_singleton; // { dg-error ".foo foo::s_singleton. is private within this context" }
+  // { dg-message "declared private here" "" { target *-*-* } 7 }
+  // We don't yet support generating a fix-it hint for this case.
+}
-- 
1.8.5.3

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Nathan Sidwell]

On 04/24/2017 04:06 PM, David Malcolm wrote:

> test.cc:12:13: note: field ‘int foo::m_field’ can be accessed via ‘int foo::get_field() const’
>     return f->m_field;
>               ^~~~~~~
>               get_field()
> 
> Assuming that an IDE can offer to apply fix-it hints, this should
> make it easier to handle refactorings where one makes a field
> private and adds a getter.
> 
> It also helps by letting the user know that a getter exists, and
> the name of the getter ("is it "field", "get_field", etc?").

Neat!

> 
> OK for trunk?
> 
> gcc/cp/ChangeLog:
> 	* call.c (maybe_suggest_accessor): New function.
> 	(enforce_access): Call maybe_suggest_accessor for inaccessible
> 	decls.
> 	* cp-tree.h (locate_field_accessor): New decl.
> 	* search.c (matches_code_and_type_p): New function.
> 	(field_access_p): New function.
> 	(direct_accessor_p): New function.
> 	(reference_accessor_p): New function.
> 	(field_accessor_p): New function.
> 	(dfs_locate_field_accessor_pre): New function.
> 	(locate_field_accessor): New function.

ok.


-- 
Nathan Sidwell

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Nathan Sidwell]

On 04/25/2017 07:46 AM, Nathan Sidwell wrote:
> On 04/24/2017 04:06 PM, David Malcolm wrote:
> 
>> test.cc:12:13: note: field ‘int foo::m_field’ can be accessed via ‘int 
>> foo::get_field() const’
>>     return f->m_field;
>>               ^~~~~~~
>>               get_field()
>>
>> Assuming that an IDE can offer to apply fix-it hints, this should
>> make it easier to handle refactorings where one makes a field
>> private and adds a getter.
>>
>> It also helps by letting the user know that a getter exists, and
>> the name of the getter ("is it "field", "get_field", etc?").
> 
> Neat!
> 
>>
>> OK for trunk?
>>
>> gcc/cp/ChangeLog:
>>     * call.c (maybe_suggest_accessor): New function.
>>     (enforce_access): Call maybe_suggest_accessor for inaccessible
>>     decls.
>>     * cp-tree.h (locate_field_accessor): New decl.
>>     * search.c (matches_code_and_type_p): New function.
>>     (field_access_p): New function.
>>     (direct_accessor_p): New function.
>>     (reference_accessor_p): New function.
>>     (field_accessor_p): New function.
>>     (dfs_locate_field_accessor_pre): New function.
>>     (locate_field_accessor): New function.
> 
> ok.

Oh, what if the field is being accessed for modification or lvalueness? 
Must the accessor return T, or can it return 'T cv &'?  I.e. does it 
need to look for setters too?

nathan
-- 
Nathan Sidwell

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[David Malcolm]

On Tue, 2017-04-25 at 07:49 -0400, Nathan Sidwell wrote:
> On 04/25/2017 07:46 AM, Nathan Sidwell wrote:
> > On 04/24/2017 04:06 PM, David Malcolm wrote:
> > 
> > > test.cc:12:13: note: field ‘int foo::m_field’ can be accessed via
> > > ‘int 
> > > foo::get_field() const’
> > >     return f->m_field;
> > >               ^~~~~~~
> > >               get_field()
> > > 
> > > Assuming that an IDE can offer to apply fix-it hints, this should
> > > make it easier to handle refactorings where one makes a field
> > > private and adds a getter.
> > > 
> > > It also helps by letting the user know that a getter exists, and
> > > the name of the getter ("is it "field", "get_field", etc?").
> > 
> > Neat!
> > 
> > > 
> > > OK for trunk?
> > > 
> > > gcc/cp/ChangeLog:
> > >     * call.c (maybe_suggest_accessor): New function.
> > >     (enforce_access): Call maybe_suggest_accessor for
> > > inaccessible
> > >     decls.
> > >     * cp-tree.h (locate_field_accessor): New decl.
> > >     * search.c (matches_code_and_type_p): New function.
> > >     (field_access_p): New function.
> > >     (direct_accessor_p): New function.
> > >     (reference_accessor_p): New function.
> > >     (field_accessor_p): New function.
> > >     (dfs_locate_field_accessor_pre): New function.
> > >     (locate_field_accessor): New function.
> > 
> > ok.
> 
> Oh, what if the field is being accessed for modification or
> lvalueness? 
> Must the accessor return T, or can it return 'T cv &'?  I.e. does it 
> need to look for setters too?

There's an example of doing so in the patch in:
  g++.dg/other/accessor-fixits-2.C

(see direct_accessor_p vs reference_accessor_p in the patch for how
this is handled).

The patch doesn't take account the context of how the returned field is
used, e.g. whether as an rvalue vs in a modification/lvalue way; it
just looks for a method of the form

  { return FIELD; }

for the correct field, favoring returning T to returning T&.

Is there a way to get at the "style" of access from
call.c:enforce_access?  (to determine if T vs T& would be a better
suggestion)

Otherwise, is the patch OK as-is?

A case the patch doesn't provide a hint for yet is for static data; e.g. for:

   return foo::s_singleton;

(as in g++.dg/other/accessor-fixits-3.C in the patch); would that be OK
to leave as a follow-up?

Thanks
Dave

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Nathan Sidwell]

On 04/25/2017 11:58 AM, David Malcolm wrote:

>    { return FIELD; }
> 
> for the correct field, favoring returning T to returning T&.

Hm, that seems the poorer choice (unless you can suggest both).  After 
all the T& case will meet the rvalue case (const-qualifiers ignoring). I 
suppose if thing is 'T const *':
   thing->field;
you could rule out any non-const qualified accessor?

nathan

-- 
Nathan Sidwell

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[David Malcolm]

On Tue, 2017-04-25 at 12:11 -0400, Nathan Sidwell wrote:
> On 04/25/2017 11:58 AM, David Malcolm wrote:
> 
> >    { return FIELD; }
> > 
> > for the correct field, favoring returning T to returning T&.
> 
> Hm, that seems the poorer choice (unless you can suggest both). 
>  After 
> all the T& case will meet the rvalue case (const-qualifiers
> ignoring). I 
> suppose if thing is 'T const *':
>    thing->field;
> you could rule out any non-const qualified accessor?

Ahhh, the patch erroneously offers get_color as a suggestion for this
case:

class t1
{
public:
  int& get_color () { return m_color; }

private:
  int m_color;
};

int test_const_ptr (const t1 *ptr)
{
  return ptr->m_color;
}

which, if the user applies the bogus suggestion would then fail with:

error: passing ‘const t1’ as ‘this’ argument discards qualifiers
   return ptr->get_color ();
                          ^

I tried adding the kind of filtering you suggest, but the binfo doesn't
seem to have info on const vs non-const qualification of the accessor.

So I tried adding a param for this to maybe_suggest_accessor, but then
we need to pass the information to perform_or_defer_access_check, and
to lookup_member, and it thus becomes quite an invasive change (I don't
have it working yet).

Any thoughts on how to implement this?

Or maybe to narrow the scope of the patch so that it only suggests
const accessors?

Thanks
Dave

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Nathan Sidwell]

On 04/25/2017 04:01 PM, David Malcolm wrote:
> On Tue, 2017-04-25 at 12:11 -0400, Nathan Sidwell wrote:
>> On 04/25/2017 11:58 AM, David Malcolm wrote:
>>
>>>     { return FIELD; }

> I tried adding the kind of filtering you suggest, but the binfo doesn't
> seem to have info on const vs non-const qualification of the accessor.

You need to look at the type of the function-decl.  I think looking at 
the artifical this parm on TYPE_ARG_TYPES?


nathan

-- 
Nathan Sidwell

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[David Malcolm]

On Tue, 2017-04-25 at 18:22 -0400, Nathan Sidwell wrote:
> On 04/25/2017 04:01 PM, David Malcolm wrote:
> > On Tue, 2017-04-25 at 12:11 -0400, Nathan Sidwell wrote:
> > > On 04/25/2017 11:58 AM, David Malcolm wrote:
> > > 
> > > >     { return FIELD; }
> 
> > I tried adding the kind of filtering you suggest, but the binfo
> > doesn't
> > seem to have info on const vs non-const qualification of the
> > accessor.

Sorry, I misspoke slightly here; what I meant to say is that I don't
have access to the const vs non-const qualification of the *access*
i.e. whether the argument at the call site itself is const vs non
-const.

> You need to look at the type of the function-decl.  I think looking
> at 
> the artifical this parm on TYPE_ARG_TYPES?

Thanks - yes; that gives information on the const vs non-const of the
"this" parameter, but doesn't say whether the argument was const vs non
-const.

For example, in:

class t1
{
public:
  int& get_color () { return m_color; }

private:
  int m_color;
};

...we can see that t1::get_color does indeed take a non-const t1 as its
"this" ptr

However, within:

int test_const_ptr (const t1 *ptr)
{
  return ptr->m_color;
}

...when suggesting an accessor:

(gdb) bt
#0  locate_field_accessor (basetype_path=<tree_binfo 0x7ffff1a35c00>, field_decl=<field_decl 0x7ffff1a20ed8 m_color>, 
    foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/search.c:2190
#1  0x0000000000641d31 in maybe_suggest_accessor (basetype_path=<tree_binfo 0x7ffff1a35c00>, 
    field_decl=<field_decl 0x7ffff1a20ed8 m_color>, foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/call.c:6419
#2  0x000000000064221e in enforce_access (basetype_path=<tree_binfo 0x7ffff1a35c00>, decl=<field_decl 0x7ffff1a20ed8 m_color>, 
    diag_decl=<field_decl 0x7ffff1a20ed8 m_color>, complain=3, foo_type=<record_type 0x7ffff1a1fdc8 t1>)
    at ../../src/gcc/cp/call.c:6469
#3  0x0000000000892d92 in perform_or_defer_access_check (binfo=<tree_binfo 0x7ffff1a35c00>, 
    decl=<field_decl 0x7ffff1a20ed8 m_color>, diag_decl=<field_decl 0x7ffff1a20ed8 m_color>, complain=3, 
    foo_type=<record_type 0x7ffff1a1fdc8 t1>) at ../../src/gcc/cp/semantics.c:332
#4  0x000000000088b22c in lookup_member (xbasetype=<tree 0x0>, name=<identifier_node 0x7ffff1a35a80 m_color>, protect=1, 
    want_type=false, complain=3) at ../../src/gcc/cp/search.c:1339
#5  0x0000000000834740 in finish_class_member_access_expr (object=..., name=<identifier_node 0x7ffff1a35a80 m_color>, 
    template_p=false, complain=3) at ../../src/gcc/cp/typeck.c:2837
#6  0x00000000007c9b0c in cp_parser_postfix_dot_deref_expression (parser=0x7ffff7ffbbd0, token_type=CPP_DEREF, 
    postfix_expression=..., for_offsetof=false, idk=0x7fffffffcbfc, location=218274) at ../../src/gcc/cp/parser.c:7493


we have an INDIRECT_REF at cp_parser_postfix_dot_deref_expression:

7067		  postfix_expression
7068		    = cp_parser_postfix_dot_deref_expression
(parser, token->type,
7069							     
 postfix_expression,
7070							     
 false, &idk, loc);


from which we can see the const-ness of the t1:

(gdb) call debug_tree (postfix_expression.m_value)
 <parm_decl 0x7ffff1a37080 ptr
    type <pointer_type 0x7ffff1a1fc78
        type <record_type 0x7ffff1a1fbd0 t1 readonly type_5 type_6 SI
        [...etc...]

but the call to lookup_member from within
finish_class_member_access_expr discards this information, giving just
"access_path": a BINFO that wraps the RECORD_TYPE for t1 directly.

2836		  /* Look up the member.  */
2837		  member = lookup_member (access_path, name, /*protect=*/1,
2838					  /*want_type=*/false, complain);

so that at the point where we'd emit the hint, we don't have that
information anymore.

A somewhat invasive solution would be for lookup_member to grow an extra:
  tree object
parameter, and to pass this information down through the access
-enforcement code, so that locate_field_accessor can look at the const
-ness of the lookup, and avoid suggesting const methods when the object
is const.  The code would probably need to support the new param being
NULL_TREE for cases where we're looking up a static member.  Or maybe
an enum of access style for const vs non-const vs static.
Maybe name the param "access_hint" to signify that it's merely there
for the purpose of hints for the user, and not to affect the parsing
itself?

Another solution would be to not bother offering non-const methods as
accessors.

Thoughts?

Dave

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Nathan Sidwell]

On 04/26/2017 12:34 PM, David Malcolm wrote:

> Thanks - yes; that gives information on the const vs non-const of the
> "this" parameter, but doesn't say whether the argument was const vs non
> -const.

> However, within:
> 
> int test_const_ptr (const t1 *ptr)
> {
>    return ptr->m_color;
> }
> from which we can see the const-ness of the t1:

correct.

> but the call to lookup_member from within
> finish_class_member_access_expr discards this information, giving just
> "access_path": a BINFO that wraps the RECORD_TYPE for t1 directly.

Correct.

lookup_member just looks for a matching name.  the BINFO represents the 
class hierarchy - it's not modified depending on the cvquals of where 
you came from.

> A somewhat invasive solution would be for lookup_member to grow an extra:
>    tree object
> parameter, and to pass this information down through the access
> -enforcement code, so that locate_field_accessor can look at the const
> -ness of the lookup, and avoid suggesting const methods when the object
> is const.  The code would probably need to support the new param being
> NULL_TREE for cases where we're looking up a static member.  Or maybe
> an enum of access style for const vs non-const vs static.
> Maybe name the param "access_hint" to signify that it's merely there
> for the purpose of hints for the user, and not to affect the parsing
> itself?

Hm, that does seem rather unfortunate.
> Another solution would be to not bother offering non-const methods as
> accessors.

I think that would be very unfortunate.

How about adding a tsubst_flag value?

   tf_const_obj = 1 << 11, /* For alternative accessor suggestion help.  */

and pass that in?  the tsubst flags have grown in meaning somewhat since 
they first appeared -- their name is no longer so appropriate.

(of course we have the same problem with volatile, but that's probably 
overkill for first attempt.)

Jason, WDYT?

nathan

-- 
Nathan Sidwell

Re: [PATCH] C++: fix-it hints suggesting accessors for private fields

[Jason Merrill]

On Thu, Apr 27, 2017 at 7:23 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 04/26/2017 12:34 PM, David Malcolm wrote:
>
>> Thanks - yes; that gives information on the const vs non-const of the
>> "this" parameter, but doesn't say whether the argument was const vs non
>> -const.
>
>
>> However, within:
>>
>> int test_const_ptr (const t1 *ptr)
>> {
>>    return ptr->m_color;
>> }
>> from which we can see the const-ness of the t1:
>
>
> correct.
>
>> but the call to lookup_member from within
>> finish_class_member_access_expr discards this information, giving just
>> "access_path": a BINFO that wraps the RECORD_TYPE for t1 directly.
>
>
> Correct.
>
> lookup_member just looks for a matching name.  the BINFO represents the
> class hierarchy - it's not modified depending on the cvquals of where you
> came from.
>
>> A somewhat invasive solution would be for lookup_member to grow an extra:
>>    tree object
>> parameter, and to pass this information down through the access
>> -enforcement code, so that locate_field_accessor can look at the const
>> -ness of the lookup, and avoid suggesting const methods when the object
>> is const.  The code would probably need to support the new param being
>> NULL_TREE for cases where we're looking up a static member.  Or maybe
>> an enum of access style for const vs non-const vs static.
>> Maybe name the param "access_hint" to signify that it's merely there
>> for the purpose of hints for the user, and not to affect the parsing
>> itself?
>
> Hm, that does seem rather unfortunate.
>>
>> Another solution would be to not bother offering non-const methods as
>> accessors.
>
>
> I think that would be very unfortunate.
>
> How about adding a tsubst_flag value?
>
>   tf_const_obj = 1 << 11, /* For alternative accessor suggestion help.  */
>
> and pass that in?  the tsubst flags have grown in meaning somewhat since
> they first appeared -- their name is no longer so appropriate.
>
> (of course we have the same problem with volatile, but that's probably
> overkill for first attempt.)
>
> Jason, WDYT?

I'd suggest handling this diagnostic in
finish_class_member_access_expr, rather than try to push down context
information into lookup_member.  Perhaps by adding another parameter
to lookup_member for passing back the inaccessible or ambiguous lookup
result?

Jason

[PATCH v2] C++: fix-it hints suggesting accessors for private fields

[David Malcolm]

On Mon, 2017-05-01 at 14:43 -0400, Jason Merrill wrote:
> On Thu, Apr 27, 2017 at 7:23 AM, Nathan Sidwell <nathan@acm.org>
> wrote:
> > On 04/26/2017 12:34 PM, David Malcolm wrote:
> > 
> > > Thanks - yes; that gives information on the const vs non-const of
> > > the
> > > "this" parameter, but doesn't say whether the argument was const
> > > vs non
> > > -const.
> > 
> > 
> > > However, within:
> > > 
> > > int test_const_ptr (const t1 *ptr)
> > > {
> > >    return ptr->m_color;
> > > }
> > > from which we can see the const-ness of the t1:
> > 
> > 
> > correct.
> > 
> > > but the call to lookup_member from within
> > > finish_class_member_access_expr discards this information, giving
> > > just
> > > "access_path": a BINFO that wraps the RECORD_TYPE for t1
> > > directly.
> > 
> > 
> > Correct.
> > 
> > lookup_member just looks for a matching name.  the BINFO represents
> > the
> > class hierarchy - it's not modified depending on the cvquals of
> > where you
> > came from.
> > 
> > > A somewhat invasive solution would be for lookup_member to grow
> > > an extra:
> > >    tree object
> > > parameter, and to pass this information down through the access
> > > -enforcement code, so that locate_field_accessor can look at the
> > > const
> > > -ness of the lookup, and avoid suggesting const methods when the
> > > object
> > > is const.  The code would probably need to support the new param
> > > being
> > > NULL_TREE for cases where we're looking up a static member.  Or
> > > maybe
> > > an enum of access style for const vs non-const vs static.
> > > Maybe name the param "access_hint" to signify that it's merely
> > > there
> > > for the purpose of hints for the user, and not to affect the
> > > parsing
> > > itself?
> > 
> > Hm, that does seem rather unfortunate.
> > > 
> > > Another solution would be to not bother offering non-const
> > > methods as
> > > accessors.
> > 
> > 
> > I think that would be very unfortunate.
> > 
> > How about adding a tsubst_flag value?
> > 
> >   tf_const_obj = 1 << 11, /* For alternative accessor suggestion
> > help.  */
> > 
> > and pass that in?  the tsubst flags have grown in meaning somewhat
> > since
> > they first appeared -- their name is no longer so appropriate.
> > 
> > (of course we have the same problem with volatile, but that's
> > probably
> > overkill for first attempt.)
> > 
> > Jason, WDYT?
> 
> I'd suggest handling this diagnostic in
> finish_class_member_access_expr, rather than try to push down context
> information into lookup_member.  Perhaps by adding another parameter
> to lookup_member for passing back the inaccessible or ambiguous
> lookup
> result?
> 
> Jason

Thanks.

Here's an updated version of the patch which adds an optional struct
ptr, for writing back the info, which then gets emitted (if set)
in finish_class_member_access_expr (and thus has access to the constness
of the object).

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/cp/ChangeLog:
	* call.c (enforce_access): Add access_failure_info * param and use
	it to record access failures.
	* cp-tree.h (class access_failure_info): New class.
	(enforce_access): Add access_failure_info * param, defaulting to
	NULL.
	(lookup_member): Likewise.
	(locate_field_accessor): New function decl.
	(perform_or_defer_access_check): Add access_failure_info * param,
	defaulting to NULL.
	* search.c (lookup_member): Add access_failure_info * param and
	pass it on to call to perform_or_defer_access_check.
	(matches_code_and_type_p): New function.
	(field_access_p): New function.
	(direct_accessor_p): New function.
	(reference_accessor_p): New function.
	(field_accessor_p): New function.
	(struct locate_field_data): New struct.
	(dfs_locate_field_accessor_pre): New function.
	(locate_field_accessor): New function.
	* semantics.c (perform_or_defer_access_check): Add
	access_failure_info * param, and pass it on to call to
	enforce_access.
	* typeck.c (access_failure_info::record_access_failure): New method.
	(access_failure_info::maybe_suggest_accessor): New method.
	(finish_class_member_access_expr): Pass an access_failure_info
	instance to the lookup_member call, and call its
	maybe_suggest_accessor method afterwards.

gcc/testsuite/ChangeLog:
	* g++.dg/other/accessor-fixits-1.C: New test case.
	* g++.dg/other/accessor-fixits-2.C: New test case.
	* g++.dg/other/accessor-fixits-3.C: New test case.
	* g++.dg/other/accessor-fixits-4.C: New test case.
---
 gcc/cp/call.c                                  |   8 +-
 gcc/cp/cp-tree.h                               |  31 +++-
 gcc/cp/search.c                                | 240 ++++++++++++++++++++++++-
 gcc/cp/semantics.c                             |   8 +-
 gcc/cp/typeck.c                                |  45 ++++-
 gcc/testsuite/g++.dg/other/accessor-fixits-1.C | 178 ++++++++++++++++++
 gcc/testsuite/g++.dg/other/accessor-fixits-2.C | 104 +++++++++++
 gcc/testsuite/g++.dg/other/accessor-fixits-3.C |  15 ++
 gcc/testsuite/g++.dg/other/accessor-fixits-4.C |  48 +++++
 9 files changed, 666 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-1.C
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-2.C
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-3.C
 create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-4.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c15b8e4..9ed4ad0 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6415,7 +6415,7 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
 
 bool
 enforce_access (tree basetype_path, tree decl, tree diag_decl,
-		tsubst_flags_t complain)
+		tsubst_flags_t complain, access_failure_info *afi)
 {
   gcc_assert (TREE_CODE (basetype_path) == TREE_BINFO);
 
@@ -6441,17 +6441,23 @@ enforce_access (tree basetype_path, tree decl, tree diag_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);
 	    }
 	}
       return false;
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 8721ed4..c2b9df8 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5659,8 +5659,30 @@ extern bool can_convert_arg			(tree, tree, tree, int,
 						 tsubst_flags_t);
 extern bool can_convert_arg_bad			(tree, tree, tree, int,
 						 tsubst_flags_t);
+
+/* A class for recording information about access failures (e.g. private
+   fields), so that we can potentially supply a fix-it hint about
+   an accessor (from a context in which the constness of the object
+   is known).  */
+
+class access_failure_info
+{
+ public:
+  access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
+    m_field_decl (NULL_TREE) {}
+
+  void record_access_failure (tree basetype_path, tree field_decl);
+  void maybe_suggest_accessor (bool const_p) const;
+
+ private:
+  bool m_was_inaccessible;
+  tree m_basetype_path;
+  tree m_field_decl;
+};
+
 extern bool enforce_access			(tree, tree, tree,
-						 tsubst_flags_t);
+						 tsubst_flags_t,
+						 access_failure_info *afi = NULL);
 extern void push_defarg_context			(tree);
 extern void pop_defarg_context			(void);
 extern tree convert_default_arg			(tree, tree, tree, int,
@@ -6322,8 +6344,10 @@ extern tree lookup_fnfields_slot_nolazy		(tree, tree);
 extern int class_method_index_for_fn		(tree, tree);
 extern tree lookup_fnfields			(tree, tree, int);
 extern tree lookup_member			(tree, tree, int, bool,
-						 tsubst_flags_t);
+						 tsubst_flags_t,
+						 access_failure_info *afi = NULL);
 extern tree lookup_member_fuzzy		(tree, tree, bool);
+extern tree locate_field_accessor		(tree, tree, bool);
 extern int look_for_overrides			(tree, tree);
 extern void get_pure_virtuals			(tree);
 extern void maybe_suppress_debug_info		(tree);
@@ -6378,7 +6402,8 @@ extern bool perform_access_checks (vec<deferred_access_check, va_gc> *,
 				   tsubst_flags_t);
 extern bool perform_deferred_access_checks	(tsubst_flags_t);
 extern bool perform_or_defer_access_check	(tree, tree, tree,
-						 tsubst_flags_t);
+						 tsubst_flags_t,
+						 access_failure_info *afi = NULL);
 
 /* RAII sentinel to ensures that deferred access checks are popped before
   a function returns.  */
diff --git a/gcc/cp/search.c b/gcc/cp/search.c
index 09c1b4e..2cd6ea5 100644
--- a/gcc/cp/search.c
+++ b/gcc/cp/search.c
@@ -1232,11 +1232,13 @@ build_baselink (tree binfo, tree access_binfo, tree functions, tree optype)
 
    WANT_TYPE is 1 when we should only return TYPE_DECLs.
 
-   If nothing can be found return NULL_TREE and do not issue an error.  */
+   If nothing can be found return NULL_TREE and do not issue an error.
+
+   If non-NULL, failure information is written back to AFI.  */
 
 tree
 lookup_member (tree xbasetype, tree name, int protect, bool want_type,
-	       tsubst_flags_t complain)
+	       tsubst_flags_t complain, access_failure_info *afi)
 {
   tree rval, rval_binfo = NULL_TREE;
   tree type = NULL_TREE, basetype_path = NULL_TREE;
@@ -1337,7 +1339,7 @@ lookup_member (tree xbasetype, tree name, int protect, bool want_type,
       tree decl = is_overloaded_fn (rval) ? get_first_fn (rval) : rval;
       if (!DECL_NONSTATIC_MEMBER_FUNCTION_P (decl)
 	  && !perform_or_defer_access_check (basetype_path, decl, decl,
-					     complain))
+					     complain, afi))
 	rval = error_mark_node;
     }
 
@@ -1993,6 +1995,238 @@ dfs_walk_once_accessible (tree binfo, bool friends_p,
   return rval;
 }
 
+/* Return true iff the code of T is CODE, and it has compatible
+   type with TYPE.  */
+
+static bool
+matches_code_and_type_p (tree t, enum tree_code code, tree type)
+{
+  if (TREE_CODE (t) != code)
+    return false;
+  if (!cxx_types_compatible_p (TREE_TYPE (t), type))
+    return false;
+  return true;
+}
+
+/* Subroutine of direct_accessor_p and reference_accessor_p.
+   Determine if COMPONENT_REF is a simple field lookup of this->FIELD_DECL.
+   We expect a tree of the form:
+	     <component_ref:
+	       <indirect_ref:S>
+		 <nop_expr:P*
+		   <parm_decl (this)>
+		 <field_decl (FIELD_DECL)>>>.  */
+
+static bool
+field_access_p (tree component_ref, tree field_decl, tree field_type)
+{
+  if (!matches_code_and_type_p (component_ref, COMPONENT_REF, field_type))
+    return false;
+
+  tree indirect_ref = TREE_OPERAND (component_ref, 0);
+  if (TREE_CODE (indirect_ref) != INDIRECT_REF)
+    return false;
+
+  tree ptr = STRIP_NOPS (TREE_OPERAND (indirect_ref, 0));
+  if (!is_this_parameter (ptr))
+    return false;
+
+  /* Must access the correct field.  */
+  if (TREE_OPERAND (component_ref, 1) != field_decl)
+    return false;
+  return true;
+}
+
+/* Subroutine of field_accessor_p.
+
+   Assuming that INIT_EXPR has already had its code and type checked,
+   determine if it is a simple accessor for FIELD_DECL
+   (of type FIELD_TYPE).
+
+   Specifically, a simple accessor within struct S of the form:
+       T get_field () { return m_field; }
+   should have a DECL_SAVED_TREE of the form:
+       <return_expr
+	 <init_expr:T
+	   <result_decl:T
+	   <nop_expr:T
+	     <component_ref:
+	       <indirect_ref:S>
+		 <nop_expr:P*
+		   <parm_decl (this)>
+		 <field_decl (FIELD_DECL)>>>.  */
+
+static bool
+direct_accessor_p (tree init_expr, tree field_decl, tree field_type)
+{
+  tree result_decl = TREE_OPERAND (init_expr, 0);
+  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_type))
+    return false;
+
+  tree component_ref = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
+  if (!field_access_p (component_ref, field_decl, field_type))
+    return false;
+
+  return true;
+}
+
+/* Subroutine of field_accessor_p.
+
+   Assuming that INIT_EXPR has already had its code and type checked,
+   determine if it is a "reference" accessor for FIELD_DECL
+   (of type FIELD_REFERENCE_TYPE).
+
+   Specifically, a simple accessor within struct S of the form:
+       T& get_field () { return m_field; }
+   should have a DECL_SAVED_TREE of the form:
+       <return_expr
+	 <init_expr:T&
+	   <result_decl:T&
+	   <nop_expr: T&
+	     <addr_expr: T*
+	       <component_ref:T
+		 <indirect_ref:S
+		   <nop_expr
+		     <parm_decl (this)>>
+		   <field (FIELD_DECL)>>>>>>.  */
+static bool
+reference_accessor_p (tree init_expr, tree field_decl, tree field_type,
+		      tree field_reference_type)
+{
+  tree result_decl = TREE_OPERAND (init_expr, 0);
+  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_reference_type))
+    return false;
+
+  tree field_pointer_type = build_pointer_type (field_type);
+  tree addr_expr = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
+  if (!matches_code_and_type_p (addr_expr, ADDR_EXPR, field_pointer_type))
+    return false;
+
+  tree component_ref = STRIP_NOPS (TREE_OPERAND (addr_expr, 0));
+
+  if (!field_access_p (component_ref, field_decl, field_type))
+    return false;
+
+  return true;
+}
+
+/* Return true if FN is an accessor method for FIELD_DECL.
+   i.e. a method of the form { return FIELD; }, with no
+   conversions.
+
+   If CONST_P, then additionally require that FN be a const
+   method.  */
+
+static bool
+field_accessor_p (tree fn, tree field_decl, bool const_p)
+{
+  if (TREE_CODE (fn) != FUNCTION_DECL)
+    return false;
+
+  /* We don't yet support looking up static data, just fields.  */
+  if (TREE_CODE (field_decl) != FIELD_DECL)
+    return false;
+
+  tree fntype = TREE_TYPE (fn);
+  if (TREE_CODE (fntype) != METHOD_TYPE)
+    return false;
+
+  /* If the field is accessed via a const "this" argument, verify
+     that the "this" parameter is const.  */
+  if (const_p)
+    {
+      tree this_type = type_of_this_parm (fntype);
+      if (!TYPE_READONLY (this_type))
+	return false;
+    }
+
+  tree saved_tree = DECL_SAVED_TREE (fn);
+
+  if (saved_tree == NULL_TREE)
+    return false;
+
+  if (TREE_CODE (saved_tree) != RETURN_EXPR)
+    return false;
+
+  tree init_expr = TREE_OPERAND (saved_tree, 0);
+  if (TREE_CODE (init_expr) != INIT_EXPR)
+    return false;
+
+  /* Determine if this is a simple accessor within struct S of the form:
+       T get_field () { return m_field; }.  */
+   tree field_type = TREE_TYPE (field_decl);
+  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_type))
+    return direct_accessor_p (init_expr, field_decl, field_type);
+
+  /* Failing that, determine if it is an accessor of the form:
+       T& get_field () { return m_field; }.  */
+  tree field_reference_type = cp_build_reference_type (field_type, false);
+  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_reference_type))
+    return reference_accessor_p (init_expr, field_decl, field_type,
+				 field_reference_type);
+
+  return false;
+}
+
+/* Callback data for dfs_locate_field_accessor_pre.  */
+
+struct locate_field_data
+{
+  locate_field_data (tree field_decl_, bool const_p_)
+  : field_decl (field_decl_), const_p (const_p_) {}
+
+  tree field_decl;
+  bool const_p;
+};
+
+/* Return a FUNCTION_DECL that is an "accessor" method for DATA, a FIELD_DECL,
+   callable via binfo, if one exists, otherwise return NULL_TREE.
+
+   Callback for dfs_walk_once_accessible for use within
+   locate_field_accessor.  */
+
+static tree
+dfs_locate_field_accessor_pre (tree binfo, void *data)
+{
+  locate_field_data *lfd = (locate_field_data *)data;
+  tree type = BINFO_TYPE (binfo);
+
+  vec<tree, va_gc> *method_vec;
+  tree fn;
+  size_t i;
+
+  if (!CLASS_TYPE_P (type))
+    return NULL_TREE;
+
+  method_vec = CLASSTYPE_METHOD_VEC (type);
+  if (!method_vec)
+    return NULL_TREE;
+
+  for (i = 0; vec_safe_iterate (method_vec, i, &fn); ++i)
+    if (fn)
+      if (field_accessor_p (fn, lfd->field_decl, lfd->const_p))
+	return fn;
+
+  return NULL_TREE;
+}
+
+/* Return a FUNCTION_DECL that is an "accessor" method for FIELD_DECL,
+   callable via BASETYPE_PATH, if one exists, otherwise return NULL_TREE.  */
+
+tree
+locate_field_accessor (tree basetype_path, tree field_decl, bool const_p)
+{
+  if (TREE_CODE (basetype_path) != TREE_BINFO)
+    return NULL_TREE;
+
+  /* Walk the hierarchy, looking for a method of some base class that allows
+     access to the field.  */
+  locate_field_data lfd (field_decl, const_p);
+  return dfs_walk_once_accessible (basetype_path, /*friends=*/true,
+				   dfs_locate_field_accessor_pre,
+				   NULL, &lfd);
+}
+
 /* Check that virtual overrider OVERRIDER is acceptable for base function
    BASEFN. Issue diagnostic, and return zero, if unacceptable.  */
 
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 4db2462..e6c6bd9 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -305,11 +305,13 @@ perform_deferred_access_checks (tsubst_flags_t complain)
 
 /* Defer checking the accessibility of DECL, when looked up in
    BINFO. DIAG_DECL is the declaration to use to print diagnostics.
-   Return value like perform_access_checks above.  */
+   Return value like perform_access_checks above.
+   If non-NULL, report failures to AFI.  */
 
 bool
 perform_or_defer_access_check (tree binfo, tree decl, tree diag_decl,
-			       tsubst_flags_t complain)
+			       tsubst_flags_t complain,
+			       access_failure_info *afi)
 {
   int i;
   deferred_access *ptr;
@@ -328,7 +330,7 @@ perform_or_defer_access_check (tree binfo, tree decl, tree diag_decl,
   /* If we are not supposed to defer access checks, just check now.  */
   if (ptr->deferring_access_checks_kind == dk_no_deferred)
     {
-      bool ok = enforce_access (binfo, decl, diag_decl, complain);
+      bool ok = enforce_access (binfo, decl, diag_decl, complain, afi);
       return (complain & tf_error) ? true : ok;
     }
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 7aee0d6..66fb7ba 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2650,6 +2650,46 @@ check_template_keyword (tree decl)
     }
 }
 
+/* Record that an access failure occurred on BASETYPE_PATH attempting
+   to access FIELD_DECL.  */
+
+void
+access_failure_info::record_access_failure (tree basetype_path,
+					    tree field_decl)
+{
+  m_was_inaccessible = true;
+  m_basetype_path = basetype_path;
+  m_field_decl = field_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.  */
+
+void
+access_failure_info::maybe_suggest_accessor (bool const_p) const
+{
+  if (!m_was_inaccessible)
+    return;
+
+  tree accessor
+    = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
+  if (!accessor)
+    return;
+
+  /* The accessor must itself be accessible for it to be a reasonable
+     suggestion.  */
+  if (!accessible_p (m_basetype_path, accessor, true))
+    return;
+
+  rich_location richloc (line_table, input_location);
+  pretty_printer pp;
+  pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
+  richloc.add_fixit_replace (pp_formatted_text (&pp));
+  inform_at_rich_loc (&richloc, "field %q#D can be accessed via %q#D",
+		      m_field_decl, accessor);
+}
+
 /* This function is called by the parser to process a class member
    access expression of the form OBJECT.NAME.  NAME is a node used by
    the parser to represent a name; it is not yet a DECL.  It may,
@@ -2834,8 +2874,11 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
       else
 	{
 	  /* Look up the member.  */
+	  access_failure_info afi;
 	  member = lookup_member (access_path, name, /*protect=*/1,
-				  /*want_type=*/false, complain);
+				  /*want_type=*/false, complain,
+				  &afi);
+	  afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
 	  if (member == NULL_TREE)
 	    {
 	      if (dependent_type_p (object_type))
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-1.C b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
new file mode 100644
index 0000000..cc96b87
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
@@ -0,0 +1,178 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+class t1
+{
+public:
+  int get_color () const { return m_color; }
+  int get_shape () const { return m_shape; }
+
+private:
+  int m_color;
+
+protected:
+  int m_shape;
+};
+
+int test_access_t1_color (t1 &ref)
+{
+  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } 10 }
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+              get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_access_t1_shape (t1 &ref)
+{
+  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared protected here" "" { target *-*-* } 13 }
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+              get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_color (t1 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_shape (t1 *ptr)
+{
+  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+               get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+/* Example of public inheritance.  */
+
+class t2 : public t1
+{
+};
+
+int test_deref_t2_color (t2 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+/* Example of private inheritance.  */
+
+class t3 : private t1
+{
+};
+
+int test_deref_t3_color (t3 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't provide a fix-it hint for this case due to the
+     private inheritance.  */
+}
+
+/* Example of non-public "accessor".  */
+
+class t4
+{
+  int m_field;
+  int get_field () { return m_field; }
+};
+
+int test_deref_t4_field (t4 *ptr)
+{
+  return ptr->m_field; // { dg-error ".int t4::m_field. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_field;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* { dg-begin-multiline-output "" }
+   int m_field;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't provide a fix-it hint for this case, as the accessor is
+     itself private.  */
+}
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-2.C b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
new file mode 100644
index 0000000..e1a2b78
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
@@ -0,0 +1,104 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Test of accessors that return references.  */
+
+class t1
+{
+public:
+  int& get_color () { return m_color; }
+  int& get_shape () { return m_shape; }
+
+private:
+  int m_color;
+
+protected:
+  int m_shape;
+};
+
+int test_access_t1_color (t1 &ref)
+{
+  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } 12 }
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_color;
+              ^~~~~~~
+              get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_access_t1_shape (t1 &ref)
+{
+  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared protected here" "" { target *-*-* } 15 }
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+              get_shape()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_color (t1 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+               get_color()
+     { dg-end-multiline-output "" } */
+}
+
+int test_deref_t1_shape (t1 *ptr)
+{
+  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_shape;
+               ^~~~~~~
+               get_shape()
+     { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-3.C b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
new file mode 100644
index 0000000..27d2eb4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
@@ -0,0 +1,15 @@
+class foo
+{
+public:
+  static foo& get_singleton () { return s_singleton; }
+
+private:
+  static foo s_singleton;
+};
+
+foo & test_access_singleton ()
+{
+  return foo::s_singleton; // { dg-error ".foo foo::s_singleton. is private within this context" }
+  // { dg-message "declared private here" "" { target *-*-* } 7 }
+  // We don't yet support generating a fix-it hint for this case.
+}
diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-4.C b/gcc/testsuite/g++.dg/other/accessor-fixits-4.C
new file mode 100644
index 0000000..c03dd4e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/accessor-fixits-4.C
@@ -0,0 +1,48 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+class t1
+{
+public:
+  int& get_color () { return m_color; }
+  int& get_shape () { return m_shape; }
+
+private:
+  int m_color; // { dg-line color_decl }
+  int m_shape; // { dg-line shape_decl }
+};
+
+int test_const_ptr (const t1 *ptr)
+{
+  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ptr->m_color;
+               ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } color_decl }
+  /* { dg-begin-multiline-output "" }
+   int m_color;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't issue a suggestion: the accessor is non-const, and we
+     only have a const ptr.  */
+}
+
+int test_const_reference (const t1 &ref)
+{
+  return ref.m_shape; // { dg-error ".int t1::m_shape. is private within this context" }
+  /* { dg-begin-multiline-output "" }
+   return ref.m_shape;
+              ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  // { dg-message "declared private here" "" { target *-*-* } shape_decl }
+  /* { dg-begin-multiline-output "" }
+   int m_shape;
+       ^~~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* We shouldn't issue a suggestion: the accessor is non-const, and we
+     only have a const ptr.  */
+}
-- 
1.8.5.3

Re: [PATCH v2] C++: fix-it hints suggesting accessors for private fields

[Jason Merrill]

OK.

On Fri, May 5, 2017 at 6:50 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Mon, 2017-05-01 at 14:43 -0400, Jason Merrill wrote:
>> On Thu, Apr 27, 2017 at 7:23 AM, Nathan Sidwell <nathan@acm.org>
>> wrote:
>> > On 04/26/2017 12:34 PM, David Malcolm wrote:
>> >
>> > > Thanks - yes; that gives information on the const vs non-const of
>> > > the
>> > > "this" parameter, but doesn't say whether the argument was const
>> > > vs non
>> > > -const.
>> >
>> >
>> > > However, within:
>> > >
>> > > int test_const_ptr (const t1 *ptr)
>> > > {
>> > >    return ptr->m_color;
>> > > }
>> > > from which we can see the const-ness of the t1:
>> >
>> >
>> > correct.
>> >
>> > > but the call to lookup_member from within
>> > > finish_class_member_access_expr discards this information, giving
>> > > just
>> > > "access_path": a BINFO that wraps the RECORD_TYPE for t1
>> > > directly.
>> >
>> >
>> > Correct.
>> >
>> > lookup_member just looks for a matching name.  the BINFO represents
>> > the
>> > class hierarchy - it's not modified depending on the cvquals of
>> > where you
>> > came from.
>> >
>> > > A somewhat invasive solution would be for lookup_member to grow
>> > > an extra:
>> > >    tree object
>> > > parameter, and to pass this information down through the access
>> > > -enforcement code, so that locate_field_accessor can look at the
>> > > const
>> > > -ness of the lookup, and avoid suggesting const methods when the
>> > > object
>> > > is const.  The code would probably need to support the new param
>> > > being
>> > > NULL_TREE for cases where we're looking up a static member.  Or
>> > > maybe
>> > > an enum of access style for const vs non-const vs static.
>> > > Maybe name the param "access_hint" to signify that it's merely
>> > > there
>> > > for the purpose of hints for the user, and not to affect the
>> > > parsing
>> > > itself?
>> >
>> > Hm, that does seem rather unfortunate.
>> > >
>> > > Another solution would be to not bother offering non-const
>> > > methods as
>> > > accessors.
>> >
>> >
>> > I think that would be very unfortunate.
>> >
>> > How about adding a tsubst_flag value?
>> >
>> >   tf_const_obj = 1 << 11, /* For alternative accessor suggestion
>> > help.  */
>> >
>> > and pass that in?  the tsubst flags have grown in meaning somewhat
>> > since
>> > they first appeared -- their name is no longer so appropriate.
>> >
>> > (of course we have the same problem with volatile, but that's
>> > probably
>> > overkill for first attempt.)
>> >
>> > Jason, WDYT?
>>
>> I'd suggest handling this diagnostic in
>> finish_class_member_access_expr, rather than try to push down context
>> information into lookup_member.  Perhaps by adding another parameter
>> to lookup_member for passing back the inaccessible or ambiguous
>> lookup
>> result?
>>
>> Jason
>
> Thanks.
>
> Here's an updated version of the patch which adds an optional struct
> ptr, for writing back the info, which then gets emitted (if set)
> in finish_class_member_access_expr (and thus has access to the constness
> of the object).
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> gcc/cp/ChangeLog:
>         * call.c (enforce_access): Add access_failure_info * param and use
>         it to record access failures.
>         * cp-tree.h (class access_failure_info): New class.
>         (enforce_access): Add access_failure_info * param, defaulting to
>         NULL.
>         (lookup_member): Likewise.
>         (locate_field_accessor): New function decl.
>         (perform_or_defer_access_check): Add access_failure_info * param,
>         defaulting to NULL.
>         * search.c (lookup_member): Add access_failure_info * param and
>         pass it on to call to perform_or_defer_access_check.
>         (matches_code_and_type_p): New function.
>         (field_access_p): New function.
>         (direct_accessor_p): New function.
>         (reference_accessor_p): New function.
>         (field_accessor_p): New function.
>         (struct locate_field_data): New struct.
>         (dfs_locate_field_accessor_pre): New function.
>         (locate_field_accessor): New function.
>         * semantics.c (perform_or_defer_access_check): Add
>         access_failure_info * param, and pass it on to call to
>         enforce_access.
>         * typeck.c (access_failure_info::record_access_failure): New method.
>         (access_failure_info::maybe_suggest_accessor): New method.
>         (finish_class_member_access_expr): Pass an access_failure_info
>         instance to the lookup_member call, and call its
>         maybe_suggest_accessor method afterwards.
>
> gcc/testsuite/ChangeLog:
>         * g++.dg/other/accessor-fixits-1.C: New test case.
>         * g++.dg/other/accessor-fixits-2.C: New test case.
>         * g++.dg/other/accessor-fixits-3.C: New test case.
>         * g++.dg/other/accessor-fixits-4.C: New test case.
> ---
>  gcc/cp/call.c                                  |   8 +-
>  gcc/cp/cp-tree.h                               |  31 +++-
>  gcc/cp/search.c                                | 240 ++++++++++++++++++++++++-
>  gcc/cp/semantics.c                             |   8 +-
>  gcc/cp/typeck.c                                |  45 ++++-
>  gcc/testsuite/g++.dg/other/accessor-fixits-1.C | 178 ++++++++++++++++++
>  gcc/testsuite/g++.dg/other/accessor-fixits-2.C | 104 +++++++++++
>  gcc/testsuite/g++.dg/other/accessor-fixits-3.C |  15 ++
>  gcc/testsuite/g++.dg/other/accessor-fixits-4.C |  48 +++++
>  9 files changed, 666 insertions(+), 11 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-1.C
>  create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-2.C
>  create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-3.C
>  create mode 100644 gcc/testsuite/g++.dg/other/accessor-fixits-4.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index c15b8e4..9ed4ad0 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -6415,7 +6415,7 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
>
>  bool
>  enforce_access (tree basetype_path, tree decl, tree diag_decl,
> -               tsubst_flags_t complain)
> +               tsubst_flags_t complain, access_failure_info *afi)
>  {
>    gcc_assert (TREE_CODE (basetype_path) == TREE_BINFO);
>
> @@ -6441,17 +6441,23 @@ enforce_access (tree basetype_path, tree decl, tree diag_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);
>             }
>         }
>        return false;
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 8721ed4..c2b9df8 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5659,8 +5659,30 @@ extern bool can_convert_arg                      (tree, tree, tree, int,
>                                                  tsubst_flags_t);
>  extern bool can_convert_arg_bad                        (tree, tree, tree, int,
>                                                  tsubst_flags_t);
> +
> +/* A class for recording information about access failures (e.g. private
> +   fields), so that we can potentially supply a fix-it hint about
> +   an accessor (from a context in which the constness of the object
> +   is known).  */
> +
> +class access_failure_info
> +{
> + public:
> +  access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
> +    m_field_decl (NULL_TREE) {}
> +
> +  void record_access_failure (tree basetype_path, tree field_decl);
> +  void maybe_suggest_accessor (bool const_p) const;
> +
> + private:
> +  bool m_was_inaccessible;
> +  tree m_basetype_path;
> +  tree m_field_decl;
> +};
> +
>  extern bool enforce_access                     (tree, tree, tree,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t,
> +                                                access_failure_info *afi = NULL);
>  extern void push_defarg_context                        (tree);
>  extern void pop_defarg_context                 (void);
>  extern tree convert_default_arg                        (tree, tree, tree, int,
> @@ -6322,8 +6344,10 @@ extern tree lookup_fnfields_slot_nolazy          (tree, tree);
>  extern int class_method_index_for_fn           (tree, tree);
>  extern tree lookup_fnfields                    (tree, tree, int);
>  extern tree lookup_member                      (tree, tree, int, bool,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t,
> +                                                access_failure_info *afi = NULL);
>  extern tree lookup_member_fuzzy                (tree, tree, bool);
> +extern tree locate_field_accessor              (tree, tree, bool);
>  extern int look_for_overrides                  (tree, tree);
>  extern void get_pure_virtuals                  (tree);
>  extern void maybe_suppress_debug_info          (tree);
> @@ -6378,7 +6402,8 @@ extern bool perform_access_checks (vec<deferred_access_check, va_gc> *,
>                                    tsubst_flags_t);
>  extern bool perform_deferred_access_checks     (tsubst_flags_t);
>  extern bool perform_or_defer_access_check      (tree, tree, tree,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t,
> +                                                access_failure_info *afi = NULL);
>
>  /* RAII sentinel to ensures that deferred access checks are popped before
>    a function returns.  */
> diff --git a/gcc/cp/search.c b/gcc/cp/search.c
> index 09c1b4e..2cd6ea5 100644
> --- a/gcc/cp/search.c
> +++ b/gcc/cp/search.c
> @@ -1232,11 +1232,13 @@ build_baselink (tree binfo, tree access_binfo, tree functions, tree optype)
>
>     WANT_TYPE is 1 when we should only return TYPE_DECLs.
>
> -   If nothing can be found return NULL_TREE and do not issue an error.  */
> +   If nothing can be found return NULL_TREE and do not issue an error.
> +
> +   If non-NULL, failure information is written back to AFI.  */
>
>  tree
>  lookup_member (tree xbasetype, tree name, int protect, bool want_type,
> -              tsubst_flags_t complain)
> +              tsubst_flags_t complain, access_failure_info *afi)
>  {
>    tree rval, rval_binfo = NULL_TREE;
>    tree type = NULL_TREE, basetype_path = NULL_TREE;
> @@ -1337,7 +1339,7 @@ lookup_member (tree xbasetype, tree name, int protect, bool want_type,
>        tree decl = is_overloaded_fn (rval) ? get_first_fn (rval) : rval;
>        if (!DECL_NONSTATIC_MEMBER_FUNCTION_P (decl)
>           && !perform_or_defer_access_check (basetype_path, decl, decl,
> -                                            complain))
> +                                            complain, afi))
>         rval = error_mark_node;
>      }
>
> @@ -1993,6 +1995,238 @@ dfs_walk_once_accessible (tree binfo, bool friends_p,
>    return rval;
>  }
>
> +/* Return true iff the code of T is CODE, and it has compatible
> +   type with TYPE.  */
> +
> +static bool
> +matches_code_and_type_p (tree t, enum tree_code code, tree type)
> +{
> +  if (TREE_CODE (t) != code)
> +    return false;
> +  if (!cxx_types_compatible_p (TREE_TYPE (t), type))
> +    return false;
> +  return true;
> +}
> +
> +/* Subroutine of direct_accessor_p and reference_accessor_p.
> +   Determine if COMPONENT_REF is a simple field lookup of this->FIELD_DECL.
> +   We expect a tree of the form:
> +            <component_ref:
> +              <indirect_ref:S>
> +                <nop_expr:P*
> +                  <parm_decl (this)>
> +                <field_decl (FIELD_DECL)>>>.  */
> +
> +static bool
> +field_access_p (tree component_ref, tree field_decl, tree field_type)
> +{
> +  if (!matches_code_and_type_p (component_ref, COMPONENT_REF, field_type))
> +    return false;
> +
> +  tree indirect_ref = TREE_OPERAND (component_ref, 0);
> +  if (TREE_CODE (indirect_ref) != INDIRECT_REF)
> +    return false;
> +
> +  tree ptr = STRIP_NOPS (TREE_OPERAND (indirect_ref, 0));
> +  if (!is_this_parameter (ptr))
> +    return false;
> +
> +  /* Must access the correct field.  */
> +  if (TREE_OPERAND (component_ref, 1) != field_decl)
> +    return false;
> +  return true;
> +}
> +
> +/* Subroutine of field_accessor_p.
> +
> +   Assuming that INIT_EXPR has already had its code and type checked,
> +   determine if it is a simple accessor for FIELD_DECL
> +   (of type FIELD_TYPE).
> +
> +   Specifically, a simple accessor within struct S of the form:
> +       T get_field () { return m_field; }
> +   should have a DECL_SAVED_TREE of the form:
> +       <return_expr
> +        <init_expr:T
> +          <result_decl:T
> +          <nop_expr:T
> +            <component_ref:
> +              <indirect_ref:S>
> +                <nop_expr:P*
> +                  <parm_decl (this)>
> +                <field_decl (FIELD_DECL)>>>.  */
> +
> +static bool
> +direct_accessor_p (tree init_expr, tree field_decl, tree field_type)
> +{
> +  tree result_decl = TREE_OPERAND (init_expr, 0);
> +  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_type))
> +    return false;
> +
> +  tree component_ref = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
> +  if (!field_access_p (component_ref, field_decl, field_type))
> +    return false;
> +
> +  return true;
> +}
> +
> +/* Subroutine of field_accessor_p.
> +
> +   Assuming that INIT_EXPR has already had its code and type checked,
> +   determine if it is a "reference" accessor for FIELD_DECL
> +   (of type FIELD_REFERENCE_TYPE).
> +
> +   Specifically, a simple accessor within struct S of the form:
> +       T& get_field () { return m_field; }
> +   should have a DECL_SAVED_TREE of the form:
> +       <return_expr
> +        <init_expr:T&
> +          <result_decl:T&
> +          <nop_expr: T&
> +            <addr_expr: T*
> +              <component_ref:T
> +                <indirect_ref:S
> +                  <nop_expr
> +                    <parm_decl (this)>>
> +                  <field (FIELD_DECL)>>>>>>.  */
> +static bool
> +reference_accessor_p (tree init_expr, tree field_decl, tree field_type,
> +                     tree field_reference_type)
> +{
> +  tree result_decl = TREE_OPERAND (init_expr, 0);
> +  if (!matches_code_and_type_p (result_decl, RESULT_DECL, field_reference_type))
> +    return false;
> +
> +  tree field_pointer_type = build_pointer_type (field_type);
> +  tree addr_expr = STRIP_NOPS (TREE_OPERAND (init_expr, 1));
> +  if (!matches_code_and_type_p (addr_expr, ADDR_EXPR, field_pointer_type))
> +    return false;
> +
> +  tree component_ref = STRIP_NOPS (TREE_OPERAND (addr_expr, 0));
> +
> +  if (!field_access_p (component_ref, field_decl, field_type))
> +    return false;
> +
> +  return true;
> +}
> +
> +/* Return true if FN is an accessor method for FIELD_DECL.
> +   i.e. a method of the form { return FIELD; }, with no
> +   conversions.
> +
> +   If CONST_P, then additionally require that FN be a const
> +   method.  */
> +
> +static bool
> +field_accessor_p (tree fn, tree field_decl, bool const_p)
> +{
> +  if (TREE_CODE (fn) != FUNCTION_DECL)
> +    return false;
> +
> +  /* We don't yet support looking up static data, just fields.  */
> +  if (TREE_CODE (field_decl) != FIELD_DECL)
> +    return false;
> +
> +  tree fntype = TREE_TYPE (fn);
> +  if (TREE_CODE (fntype) != METHOD_TYPE)
> +    return false;
> +
> +  /* If the field is accessed via a const "this" argument, verify
> +     that the "this" parameter is const.  */
> +  if (const_p)
> +    {
> +      tree this_type = type_of_this_parm (fntype);
> +      if (!TYPE_READONLY (this_type))
> +       return false;
> +    }
> +
> +  tree saved_tree = DECL_SAVED_TREE (fn);
> +
> +  if (saved_tree == NULL_TREE)
> +    return false;
> +
> +  if (TREE_CODE (saved_tree) != RETURN_EXPR)
> +    return false;
> +
> +  tree init_expr = TREE_OPERAND (saved_tree, 0);
> +  if (TREE_CODE (init_expr) != INIT_EXPR)
> +    return false;
> +
> +  /* Determine if this is a simple accessor within struct S of the form:
> +       T get_field () { return m_field; }.  */
> +   tree field_type = TREE_TYPE (field_decl);
> +  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_type))
> +    return direct_accessor_p (init_expr, field_decl, field_type);
> +
> +  /* Failing that, determine if it is an accessor of the form:
> +       T& get_field () { return m_field; }.  */
> +  tree field_reference_type = cp_build_reference_type (field_type, false);
> +  if (cxx_types_compatible_p (TREE_TYPE (init_expr), field_reference_type))
> +    return reference_accessor_p (init_expr, field_decl, field_type,
> +                                field_reference_type);
> +
> +  return false;
> +}
> +
> +/* Callback data for dfs_locate_field_accessor_pre.  */
> +
> +struct locate_field_data
> +{
> +  locate_field_data (tree field_decl_, bool const_p_)
> +  : field_decl (field_decl_), const_p (const_p_) {}
> +
> +  tree field_decl;
> +  bool const_p;
> +};
> +
> +/* Return a FUNCTION_DECL that is an "accessor" method for DATA, a FIELD_DECL,
> +   callable via binfo, if one exists, otherwise return NULL_TREE.
> +
> +   Callback for dfs_walk_once_accessible for use within
> +   locate_field_accessor.  */
> +
> +static tree
> +dfs_locate_field_accessor_pre (tree binfo, void *data)
> +{
> +  locate_field_data *lfd = (locate_field_data *)data;
> +  tree type = BINFO_TYPE (binfo);
> +
> +  vec<tree, va_gc> *method_vec;
> +  tree fn;
> +  size_t i;
> +
> +  if (!CLASS_TYPE_P (type))
> +    return NULL_TREE;
> +
> +  method_vec = CLASSTYPE_METHOD_VEC (type);
> +  if (!method_vec)
> +    return NULL_TREE;
> +
> +  for (i = 0; vec_safe_iterate (method_vec, i, &fn); ++i)
> +    if (fn)
> +      if (field_accessor_p (fn, lfd->field_decl, lfd->const_p))
> +       return fn;
> +
> +  return NULL_TREE;
> +}
> +
> +/* Return a FUNCTION_DECL that is an "accessor" method for FIELD_DECL,
> +   callable via BASETYPE_PATH, if one exists, otherwise return NULL_TREE.  */
> +
> +tree
> +locate_field_accessor (tree basetype_path, tree field_decl, bool const_p)
> +{
> +  if (TREE_CODE (basetype_path) != TREE_BINFO)
> +    return NULL_TREE;
> +
> +  /* Walk the hierarchy, looking for a method of some base class that allows
> +     access to the field.  */
> +  locate_field_data lfd (field_decl, const_p);
> +  return dfs_walk_once_accessible (basetype_path, /*friends=*/true,
> +                                  dfs_locate_field_accessor_pre,
> +                                  NULL, &lfd);
> +}
> +
>  /* Check that virtual overrider OVERRIDER is acceptable for base function
>     BASEFN. Issue diagnostic, and return zero, if unacceptable.  */
>
> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> index 4db2462..e6c6bd9 100644
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -305,11 +305,13 @@ perform_deferred_access_checks (tsubst_flags_t complain)
>
>  /* Defer checking the accessibility of DECL, when looked up in
>     BINFO. DIAG_DECL is the declaration to use to print diagnostics.
> -   Return value like perform_access_checks above.  */
> +   Return value like perform_access_checks above.
> +   If non-NULL, report failures to AFI.  */
>
>  bool
>  perform_or_defer_access_check (tree binfo, tree decl, tree diag_decl,
> -                              tsubst_flags_t complain)
> +                              tsubst_flags_t complain,
> +                              access_failure_info *afi)
>  {
>    int i;
>    deferred_access *ptr;
> @@ -328,7 +330,7 @@ perform_or_defer_access_check (tree binfo, tree decl, tree diag_decl,
>    /* If we are not supposed to defer access checks, just check now.  */
>    if (ptr->deferring_access_checks_kind == dk_no_deferred)
>      {
> -      bool ok = enforce_access (binfo, decl, diag_decl, complain);
> +      bool ok = enforce_access (binfo, decl, diag_decl, complain, afi);
>        return (complain & tf_error) ? true : ok;
>      }
>
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index 7aee0d6..66fb7ba 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -2650,6 +2650,46 @@ check_template_keyword (tree decl)
>      }
>  }
>
> +/* Record that an access failure occurred on BASETYPE_PATH attempting
> +   to access FIELD_DECL.  */
> +
> +void
> +access_failure_info::record_access_failure (tree basetype_path,
> +                                           tree field_decl)
> +{
> +  m_was_inaccessible = true;
> +  m_basetype_path = basetype_path;
> +  m_field_decl = field_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.  */
> +
> +void
> +access_failure_info::maybe_suggest_accessor (bool const_p) const
> +{
> +  if (!m_was_inaccessible)
> +    return;
> +
> +  tree accessor
> +    = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
> +  if (!accessor)
> +    return;
> +
> +  /* The accessor must itself be accessible for it to be a reasonable
> +     suggestion.  */
> +  if (!accessible_p (m_basetype_path, accessor, true))
> +    return;
> +
> +  rich_location richloc (line_table, input_location);
> +  pretty_printer pp;
> +  pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
> +  richloc.add_fixit_replace (pp_formatted_text (&pp));
> +  inform_at_rich_loc (&richloc, "field %q#D can be accessed via %q#D",
> +                     m_field_decl, accessor);
> +}
> +
>  /* This function is called by the parser to process a class member
>     access expression of the form OBJECT.NAME.  NAME is a node used by
>     the parser to represent a name; it is not yet a DECL.  It may,
> @@ -2834,8 +2874,11 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>        else
>         {
>           /* Look up the member.  */
> +         access_failure_info afi;
>           member = lookup_member (access_path, name, /*protect=*/1,
> -                                 /*want_type=*/false, complain);
> +                                 /*want_type=*/false, complain,
> +                                 &afi);
> +         afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
>           if (member == NULL_TREE)
>             {
>               if (dependent_type_p (object_type))
> diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-1.C b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
> new file mode 100644
> index 0000000..cc96b87
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/accessor-fixits-1.C
> @@ -0,0 +1,178 @@
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +class t1
> +{
> +public:
> +  int get_color () const { return m_color; }
> +  int get_shape () const { return m_shape; }
> +
> +private:
> +  int m_color;
> +
> +protected:
> +  int m_shape;
> +};
> +
> +int test_access_t1_color (t1 &ref)
> +{
> +  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_color;
> +              ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared private here" "" { target *-*-* } 10 }
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_color;
> +              ^~~~~~~
> +              get_color()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_access_t1_shape (t1 &ref)
> +{
> +  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_shape;
> +              ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared protected here" "" { target *-*-* } 13 }
> +  /* { dg-begin-multiline-output "" }
> +   int m_shape;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_shape;
> +              ^~~~~~~
> +              get_shape()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_deref_t1_color (t1 *ptr)
> +{
> +  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +               get_color()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_deref_t1_shape (t1 *ptr)
> +{
> +  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_shape;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_shape;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_shape. can be accessed via .int t1::get_shape\\(\\) const." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_shape;
> +               ^~~~~~~
> +               get_shape()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +/* Example of public inheritance.  */
> +
> +class t2 : public t1
> +{
> +};
> +
> +int test_deref_t2_color (t2 *ptr)
> +{
> +  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_color. can be accessed via .int t1::get_color\\(\\) const." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +               get_color()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +/* Example of private inheritance.  */
> +
> +class t3 : private t1
> +{
> +};
> +
> +int test_deref_t3_color (t3 *ptr)
> +{
> +  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* We shouldn't provide a fix-it hint for this case due to the
> +     private inheritance.  */
> +}
> +
> +/* Example of non-public "accessor".  */
> +
> +class t4
> +{
> +  int m_field;
> +  int get_field () { return m_field; }
> +};
> +
> +int test_deref_t4_field (t4 *ptr)
> +{
> +  return ptr->m_field; // { dg-error ".int t4::m_field. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_field;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_field;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* We shouldn't provide a fix-it hint for this case, as the accessor is
> +     itself private.  */
> +}
> diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-2.C b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
> new file mode 100644
> index 0000000..e1a2b78
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/accessor-fixits-2.C
> @@ -0,0 +1,104 @@
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +/* Test of accessors that return references.  */
> +
> +class t1
> +{
> +public:
> +  int& get_color () { return m_color; }
> +  int& get_shape () { return m_shape; }
> +
> +private:
> +  int m_color;
> +
> +protected:
> +  int m_shape;
> +};
> +
> +int test_access_t1_color (t1 &ref)
> +{
> +  return ref.m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_color;
> +              ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared private here" "" { target *-*-* } 12 }
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_color;
> +              ^~~~~~~
> +              get_color()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_access_t1_shape (t1 &ref)
> +{
> +  return ref.m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_shape;
> +              ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared protected here" "" { target *-*-* } 15 }
> +  /* { dg-begin-multiline-output "" }
> +   int m_shape;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_shape;
> +              ^~~~~~~
> +              get_shape()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_deref_t1_color (t1 *ptr)
> +{
> +  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_color. can be accessed via .int& t1::get_color\\(\\)." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +               get_color()
> +     { dg-end-multiline-output "" } */
> +}
> +
> +int test_deref_t1_shape (t1 *ptr)
> +{
> +  return ptr->m_shape; // { dg-error ".int t1::m_shape. is protected within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_shape;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +
> +  /* { dg-begin-multiline-output "" }
> +   int m_shape;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "field .int t1::m_shape. can be accessed via .int& t1::get_shape\\(\\)." "" { target *-*-* } .-12 }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_shape;
> +               ^~~~~~~
> +               get_shape()
> +     { dg-end-multiline-output "" } */
> +}
> diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-3.C b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
> new file mode 100644
> index 0000000..27d2eb4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/accessor-fixits-3.C
> @@ -0,0 +1,15 @@
> +class foo
> +{
> +public:
> +  static foo& get_singleton () { return s_singleton; }
> +
> +private:
> +  static foo s_singleton;
> +};
> +
> +foo & test_access_singleton ()
> +{
> +  return foo::s_singleton; // { dg-error ".foo foo::s_singleton. is private within this context" }
> +  // { dg-message "declared private here" "" { target *-*-* } 7 }
> +  // We don't yet support generating a fix-it hint for this case.
> +}
> diff --git a/gcc/testsuite/g++.dg/other/accessor-fixits-4.C b/gcc/testsuite/g++.dg/other/accessor-fixits-4.C
> new file mode 100644
> index 0000000..c03dd4e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/other/accessor-fixits-4.C
> @@ -0,0 +1,48 @@
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +class t1
> +{
> +public:
> +  int& get_color () { return m_color; }
> +  int& get_shape () { return m_shape; }
> +
> +private:
> +  int m_color; // { dg-line color_decl }
> +  int m_shape; // { dg-line shape_decl }
> +};
> +
> +int test_const_ptr (const t1 *ptr)
> +{
> +  return ptr->m_color; // { dg-error ".int t1::m_color. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ptr->m_color;
> +               ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared private here" "" { target *-*-* } color_decl }
> +  /* { dg-begin-multiline-output "" }
> +   int m_color;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* We shouldn't issue a suggestion: the accessor is non-const, and we
> +     only have a const ptr.  */
> +}
> +
> +int test_const_reference (const t1 &ref)
> +{
> +  return ref.m_shape; // { dg-error ".int t1::m_shape. is private within this context" }
> +  /* { dg-begin-multiline-output "" }
> +   return ref.m_shape;
> +              ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  // { dg-message "declared private here" "" { target *-*-* } shape_decl }
> +  /* { dg-begin-multiline-output "" }
> +   int m_shape;
> +       ^~~~~~~
> +     { dg-end-multiline-output "" } */
> +
> +  /* We shouldn't issue a suggestion: the accessor is non-const, and we
> +     only have a const ptr.  */
> +}
> --
> 1.8.5.3
>