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®rtested 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
