diff mbox

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

Message ID 1493064385-51828-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm April 24, 2017, 8:06 p.m. UTC
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

Comments

Nathan Sidwell April 25, 2017, 11:46 a.m. UTC | #1
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 April 25, 2017, 11:49 a.m. UTC | #2
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
David Malcolm April 25, 2017, 3:58 p.m. UTC | #3
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
Nathan Sidwell April 25, 2017, 4:11 p.m. UTC | #4
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
David Malcolm April 25, 2017, 8:01 p.m. UTC | #5
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
Nathan Sidwell April 25, 2017, 10:22 p.m. UTC | #6
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
David Malcolm April 26, 2017, 4:34 p.m. UTC | #7
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
Nathan Sidwell April 27, 2017, 11:23 a.m. UTC | #8
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
Jason Merrill May 1, 2017, 6:43 p.m. UTC | #9
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
diff mbox

Patch

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