From 7cef8ca44a610f5842f8eb27b357b8f1fde8c221 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Fri, 23 Dec 2016 18:47:12 -0500
Subject: [PATCH 18/26] C++: fix-it hints suggesting accessors for private
 fields
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 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.

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 86c7647..26295aa 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 5be5dfe..031e432 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

