* [PATCH] C++: suggestions for misspelled private members (PR c++/84993)
@ 2018-07-12 1:58 David Malcolm
2018-07-27 15:00 ` [PING] " David Malcolm
0 siblings, 1 reply; 6+ messages in thread
From: David Malcolm @ 2018-07-12 1:58 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
PR c++/84993 identifies a problem with our suggestions for
misspelled member names in the C++ FE for the case where the
member is private.
For example, given:
class foo
{
public:
double get_ratio() const { return m_ratio; }
private:
double m_ratio;
};
void test(foo *ptr)
{
if (ptr->ratio >= 0.5)
;// etc
}
...we currently emit this suggestion:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean 'm_ratio'?
if (ptr->ratio >= 0.5)
^~~~~
m_ratio
...but if the user follows this suggestion, they get:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'double foo::m_ratio' is private within this context
if (ptr->m_ratio >= 0.5)
^~~~~~~
<source>:7:10: note: declared private here
double m_ratio;
^~~~~~~
<source>:12:12: note: field 'double foo::m_ratio' can be accessed via 'double foo::get_ratio() const'
if (ptr->m_ratio >= 0.5)
^~~~~~~
get_ratio()
It feels wrong to be emitting a fix-it hint that doesn't compile, so this
patch adds the accessor fix-it hint logic to this case, so that we directly
offer a valid suggestion:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean
'double foo::m_ratio'? (accessible via 'double foo::get_ratio() const')
if (ptr->ratio >= 0.5)
^~~~~
get_ratio()
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;
adds 105 PASS results to g++.sum.
OK for trunk?
gcc/cp/ChangeLog:
PR c++/84993
* call.c (enforce_access): Move diagnostics to...
(complain_about_access): ...this new function.
* cp-tree.h (class access_failure_info): Rename split out field
"m_field_decl" into "m_decl" and "m_diag_decl".
(access_failure_info::record_access_failure): Add tree param.
(access_failure_info::was_inaccessible_p): New accessor.
(access_failure_info::get_decl): New accessor.
(access_failure_info::get_diag_decl): New accessor.
(access_failure_info::get_any_accessor): New member function.
(access_failure_info::add_fixit_hint): New static member function.
(complain_about_access): New decl.
* typeck.c (access_failure_info::record_access_failure): Update
for change to fields.
(access_failure_info::maybe_suggest_accessor): Split out into...
(access_failure_info::get_any_accessor): ...this new function...
(access_failure_info::add_fixit_hint): ...and this new function.
(finish_class_member_access_expr): Split out "has no member named"
error-handling into...
(complain_about_unrecognized_member): ...this new function, and
check that the guessed name is accessible along the access path.
Only provide a spell-correction fix-it hint if it is; otherwise,
attempt to issue an accessor fix-it hint.
gcc/testsuite/ChangeLog:
PR c++/84993
* g++.dg/torture/accessor-fixits-9.C: New test.
---
gcc/cp/call.c | 64 ++++++----
gcc/cp/cp-tree.h | 17 ++-
gcc/cp/typeck.c | 150 +++++++++++++++++------
gcc/testsuite/g++.dg/torture/accessor-fixits-9.C | 119 ++++++++++++++++++
4 files changed, 282 insertions(+), 68 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 209c1fd..121f0ca 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6456,6 +6456,38 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
return error_mark_node;
}
+/* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
+ in the diagnostics.
+
+ If ISSUE_ERROR is true, then issue an error about the
+ access, followed by a note showing the declaration.
+ Otherwise, just show the note. */
+
+void
+complain_about_access (tree decl, tree diag_decl, bool issue_error)
+{
+ if (TREE_PRIVATE (decl))
+ {
+ if (issue_error)
+ error ("%q#D is private within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl),
+ "declared private here");
+ }
+ else if (TREE_PROTECTED (decl))
+ {
+ if (issue_error)
+ error ("%q#D is protected within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl),
+ "declared protected here");
+ }
+ else
+ {
+ if (issue_error)
+ error ("%q#D is inaccessible within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
+ }
+}
+
/* If the current scope isn't allowed to access DECL along
BASETYPE_PATH, give an error. The most derived class in
BASETYPE_PATH is the one used to qualify DECL. DIAG_DECL is
@@ -6480,34 +6512,12 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
if (!accessible_p (basetype_path, decl, true))
{
+ if (flag_new_inheriting_ctors)
+ diag_decl = strip_inheriting_ctors (diag_decl);
if (complain & tf_error)
- {
- if (flag_new_inheriting_ctors)
- diag_decl = strip_inheriting_ctors (diag_decl);
- if (TREE_PRIVATE (decl))
- {
- error ("%q#D is private within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl),
- "declared private here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- else if (TREE_PROTECTED (decl))
- {
- error ("%q#D is protected within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl),
- "declared protected here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- else
- {
- error ("%q#D is inaccessible within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- }
+ complain_about_access (decl, diag_decl, true);
+ if (afi)
+ afi->record_access_failure (basetype_path, decl, diag_decl);
return false;
}
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 0fac7e9..9b7a497 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6054,18 +6054,27 @@ extern location_t get_fndecl_argument_location (tree, int);
class access_failure_info
{
public:
- access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
- m_field_decl (NULL_TREE) {}
+ access_failure_info () : m_was_inaccessible (false),
+ m_basetype_path (NULL_TREE),
+ m_decl (NULL_TREE), m_diag_decl (NULL_TREE) {}
- void record_access_failure (tree basetype_path, tree field_decl);
+ void record_access_failure (tree basetype_path, tree decl, tree diag_decl);
+
+ bool was_inaccessible_p () const { return m_was_inaccessible; }
+ tree get_decl () const { return m_decl; }
+ tree get_diag_decl () const { return m_diag_decl; }
+ tree get_any_accessor (bool const_p) const;
void maybe_suggest_accessor (bool const_p) const;
+ static void add_fixit_hint (rich_location *richloc, tree accessor);
private:
bool m_was_inaccessible;
tree m_basetype_path;
- tree m_field_decl;
+ tree m_decl;
+ tree m_diag_decl;
};
+extern void complain_about_access (tree, tree, bool);
extern bool enforce_access (tree, tree, tree,
tsubst_flags_t,
access_failure_info *afi = NULL);
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index ea4ce96..d03e797 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2703,43 +2703,138 @@ check_template_keyword (tree decl)
}
/* Record that an access failure occurred on BASETYPE_PATH attempting
- to access FIELD_DECL. */
+ to access DECL, where DIAG_DECL should be used for diagnostics. */
void
access_failure_info::record_access_failure (tree basetype_path,
- tree field_decl)
+ tree decl, tree diag_decl)
{
m_was_inaccessible = true;
m_basetype_path = basetype_path;
- m_field_decl = field_decl;
+ m_decl = decl;
+ m_diag_decl = diag_decl;
}
/* If an access failure was recorded, then attempt to locate an
- accessor function for the pertinent field, and if one is
- available, add a note and fix-it hint suggesting using it. */
+ accessor function for the pertinent field.
+ Otherwise, return NULL_TREE. */
-void
-access_failure_info::maybe_suggest_accessor (bool const_p) const
+tree
+access_failure_info::get_any_accessor (bool const_p) const
{
- if (!m_was_inaccessible)
- return;
+ if (!was_inaccessible_p ())
+ return NULL_TREE;
tree accessor
- = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
+ = locate_field_accessor (m_basetype_path, m_diag_decl, const_p);
if (!accessor)
- return;
+ return NULL_TREE;
/* The accessor must itself be accessible for it to be a reasonable
suggestion. */
if (!accessible_p (m_basetype_path, accessor, true))
- return;
+ return NULL_TREE;
- rich_location richloc (line_table, input_location);
+ return accessor;
+}
+
+/* Add a fix-it hint to RICHLOC suggesting the use of ACCESSOR_DECL, by
+ replacing the primary location in RICHLOC with "accessor()". */
+
+void
+access_failure_info::add_fixit_hint (rich_location *richloc,
+ tree accessor_decl)
+{
pretty_printer pp;
- pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
- richloc.add_fixit_replace (pp_formatted_text (&pp));
+ pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor_decl)));
+ richloc->add_fixit_replace (pp_formatted_text (&pp));
+}
+
+/* If an access failure was recorded, then attempt to locate an
+ accessor function for the pertinent field, and if one is
+ available, add a note and fix-it hint suggesting using it. */
+
+void
+access_failure_info::maybe_suggest_accessor (bool const_p) const
+{
+ tree accessor = get_any_accessor (const_p);
+ if (accessor == NULL_TREE)
+ return;
+ rich_location richloc (line_table, input_location);
+ add_fixit_hint (&richloc, accessor);
inform (&richloc, "field %q#D can be accessed via %q#D",
- m_field_decl, accessor);
+ m_diag_decl, accessor);
+}
+
+/* Subroutine of finish_class_member_access_expr.
+ Issue an error about NAME not being a member of ACCESS_PATH (or
+ OBJECT_TYPE), potentially providing a fix-it hint for misspelled
+ names. */
+
+static void
+complain_about_unrecognized_member (tree access_path, tree name,
+ tree object_type)
+{
+ /* Attempt to provide a hint about misspelled names. */
+ tree guessed_id = lookup_member_fuzzy (access_path, name,
+ /*want_type=*/false);
+ if (guessed_id == NULL_TREE)
+ {
+ /* No hint. */
+ error ("%q#T has no member named %qE",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type, name);
+ return;
+ }
+
+ location_t bogus_component_loc = input_location;
+ gcc_rich_location rich_loc (bogus_component_loc);
+
+ /* Check that the guessed name is accessible along access_path. */
+ access_failure_info afi;
+ lookup_member (access_path, guessed_id, /*protect=*/1,
+ /*want_type=*/false, /*complain=*/false,
+ &afi);
+ if (afi.was_inaccessible_p ())
+ {
+ tree accessor = afi.get_any_accessor (TYPE_READONLY (object_type));
+ if (accessor)
+ {
+ /* The guessed name isn't directly accessible, but can be accessed
+ via an accessor member function. */
+ afi.add_fixit_hint (&rich_loc, accessor);
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %q#D? (accessible via %q#D)",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, afi.get_diag_decl (), accessor);
+ }
+ else
+ {
+ /* The guessed name isn't directly accessible, and no accessor
+ member function could be found. */
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %q#D? (not accessible from this context)",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, afi.get_diag_decl ());
+ complain_about_access (afi.get_decl (), afi.get_diag_decl (), false);
+ }
+ }
+ else
+ {
+ /* The guessed name is directly accessible; suggest it. */
+ rich_loc.add_fixit_misspelled_id (bogus_component_loc,
+ guessed_id);
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %qE?",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, guessed_id);
+ }
}
/* This function is called by the parser to process a class member
@@ -2935,27 +3030,8 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
/* Try again at instantiation time. */
goto dependent;
if (complain & tf_error)
- {
- tree guessed_id = lookup_member_fuzzy (access_path, name,
- /*want_type=*/false);
- if (guessed_id)
- {
- location_t bogus_component_loc = input_location;
- gcc_rich_location rich_loc (bogus_component_loc);
- rich_loc.add_fixit_misspelled_id (bogus_component_loc,
- guessed_id);
- error_at (&rich_loc,
- "%q#T has no member named %qE;"
- " did you mean %qE?",
- TREE_CODE (access_path) == TREE_BINFO
- ? TREE_TYPE (access_path) : object_type,
- name, guessed_id);
- }
- else
- error ("%q#T has no member named %qE",
- TREE_CODE (access_path) == TREE_BINFO
- ? TREE_TYPE (access_path) : object_type, name);
- }
+ complain_about_unrecognized_member (access_path, name,
+ object_type);
return error_mark_node;
}
if (member == error_mark_node)
diff --git a/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
new file mode 100644
index 0000000..276c5b8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
@@ -0,0 +1,119 @@
+// PR c++/84993
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Misspelling (by omitting a leading "m_") of a private member for which
+ there's a public accessor.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t1
+{
+public:
+ int get_ratio () const { return m_ratio; }
+
+private:
+ int m_ratio;
+};
+
+int test (t1 *ptr_1)
+{
+ return ptr_1->ratio; // { dg-error "'class t1' has no member named 'ratio'; did you mean 'int t1::m_ratio'\\? \\(accessible via 'int t1::get_ratio\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_1->ratio;
+ ^~~~~
+ get_ratio()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member for which there's a public accessor.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t2
+{
+public:
+ int get_color () const { return m_color; }
+
+private:
+ int m_color;
+};
+
+int test (t2 *ptr_2)
+{
+ return ptr_2->m_colour; // { dg-error "'class t2' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_2->m_colour;
+ ^~~~~~~~
+ get_color()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member via a subclass pointer, for which there's
+ a public accessor in the base class.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t3 : public t2 {};
+
+int test (t3 *ptr_3)
+{
+ return ptr_3->colour; // { dg-error "'class t3' has no member named 'colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_3->colour;
+ ^~~~~~
+ get_color()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a protected member, for which there's isn't a public
+ accessor.
+
+ We expect no fix-it hint; instead a message identifying where the
+ data member was declared. */
+
+class t4
+{
+protected:
+ int m_color; // { dg-message "declared protected here" }
+};
+
+int test (t4 *ptr_4)
+{
+ return ptr_4->colour; // { dg-error "'class t4' has no member named 'colour'; did you mean 'int t4::m_color'\\? \\(not accessible from this context\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_4->colour;
+ ^~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ int m_color;
+ ^~~~~~~
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member, for which the accessor is also private.
+
+ We expect no fix-it hint; instead a message identifying where the
+ data member was declared. */
+
+class t5
+{
+ int get_color () const { return m_color; }
+ int m_color; // { dg-message "declared private here" }
+};
+
+int test (t5 *ptr_5)
+{
+ return ptr_5->colour; // { dg-error "'class t5' has no member named 'colour'; did you mean 'int t5::m_color'\\? \\(not accessible from this context\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_5->colour;
+ ^~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ int m_color;
+ ^~~~~~~
+ { dg-end-multiline-output "" } */
+}
--
1.8.5.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PING] Re: [PATCH] C++: suggestions for misspelled private members (PR c++/84993)
2018-07-12 1:58 [PATCH] C++: suggestions for misspelled private members (PR c++/84993) David Malcolm
@ 2018-07-27 15:00 ` David Malcolm
2018-09-21 21:38 ` [PATCH] v2: " David Malcolm
0 siblings, 1 reply; 6+ messages in thread
From: David Malcolm @ 2018-07-27 15:00 UTC (permalink / raw)
To: gcc-patches
Ping:
https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00594.html
On Wed, 2018-07-11 at 22:42 -0400, David Malcolm wrote:
> PR c++/84993 identifies a problem with our suggestions for
> misspelled member names in the C++ FE for the case where the
> member is private.
>
> For example, given:
>
> class foo
> {
> public:
> double get_ratio() const { return m_ratio; }
>
> private:
> double m_ratio;
> };
>
> void test(foo *ptr)
> {
> if (ptr->ratio >= 0.5)
> ;// etc
> }
>
> ...we currently emit this suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> you mean 'm_ratio'?
> if (ptr->ratio >= 0.5)
> ^~~~~
> m_ratio
>
> ...but if the user follows this suggestion, they get:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'double foo::m_ratio' is private within this
> context
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> <source>:7:10: note: declared private here
> double m_ratio;
> ^~~~~~~
> <source>:12:12: note: field 'double foo::m_ratio' can be accessed via
> 'double foo::get_ratio() const'
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> get_ratio()
>
> It feels wrong to be emitting a fix-it hint that doesn't compile, so
> this
> patch adds the accessor fix-it hint logic to this case, so that we
> directly
> offer a valid suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> you mean
> 'double foo::m_ratio'? (accessible via 'double foo::get_ratio()
> const')
> if (ptr->ratio >= 0.5)
> ^~~~~
> get_ratio()
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;
> adds 105 PASS results to g++.sum.
>
> OK for trunk?
>
> gcc/cp/ChangeLog:
> PR c++/84993
> * call.c (enforce_access): Move diagnostics to...
> (complain_about_access): ...this new function.
> * cp-tree.h (class access_failure_info): Rename split out field
> "m_field_decl" into "m_decl" and "m_diag_decl".
> (access_failure_info::record_access_failure): Add tree param.
> (access_failure_info::was_inaccessible_p): New accessor.
> (access_failure_info::get_decl): New accessor.
> (access_failure_info::get_diag_decl): New accessor.
> (access_failure_info::get_any_accessor): New member function.
> (access_failure_info::add_fixit_hint): New static member
> function.
> (complain_about_access): New decl.
> * typeck.c (access_failure_info::record_access_failure): Update
> for change to fields.
> (access_failure_info::maybe_suggest_accessor): Split out
> into...
> (access_failure_info::get_any_accessor): ...this new
> function...
> (access_failure_info::add_fixit_hint): ...and this new
> function.
> (finish_class_member_access_expr): Split out "has no member
> named"
> error-handling into...
> (complain_about_unrecognized_member): ...this new function, and
> check that the guessed name is accessible along the access
> path.
> Only provide a spell-correction fix-it hint if it is;
> otherwise,
> attempt to issue an accessor fix-it hint.
>
> gcc/testsuite/ChangeLog:
> PR c++/84993
> * g++.dg/torture/accessor-fixits-9.C: New test.
> ---
> gcc/cp/call.c | 64 ++++++----
> gcc/cp/cp-tree.h | 17 ++-
> gcc/cp/typeck.c | 150
> +++++++++++++++++------
> gcc/testsuite/g++.dg/torture/accessor-fixits-9.C | 119
> ++++++++++++++++++
> 4 files changed, 282 insertions(+), 68 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 209c1fd..121f0ca 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -6456,6 +6456,38 @@ build_op_delete_call (enum tree_code code,
> tree addr, tree size,
> return error_mark_node;
> }
>
> +/* Issue diagnostics about a disallowed access of DECL, using
> DIAG_DECL
> + in the diagnostics.
> +
> + If ISSUE_ERROR is true, then issue an error about the
> + access, followed by a note showing the declaration.
> + Otherwise, just show the note. */
> +
> +void
> +complain_about_access (tree decl, tree diag_decl, bool issue_error)
> +{
> + if (TREE_PRIVATE (decl))
> + {
> + if (issue_error)
> + error ("%q#D is private within this context", diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl),
> + "declared private here");
> + }
> + else if (TREE_PROTECTED (decl))
> + {
> + if (issue_error)
> + error ("%q#D is protected within this context", diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl),
> + "declared protected here");
> + }
> + else
> + {
> + if (issue_error)
> + error ("%q#D is inaccessible within this context",
> diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
> + }
> +}
> +
> /* If the current scope isn't allowed to access DECL along
> BASETYPE_PATH, give an error. The most derived class in
> BASETYPE_PATH is the one used to qualify DECL. DIAG_DECL is
> @@ -6480,34 +6512,12 @@ enforce_access (tree basetype_path, tree
> decl, tree diag_decl,
>
> if (!accessible_p (basetype_path, decl, true))
> {
> + if (flag_new_inheriting_ctors)
> + diag_decl = strip_inheriting_ctors (diag_decl);
> if (complain & tf_error)
> - {
> - if (flag_new_inheriting_ctors)
> - diag_decl = strip_inheriting_ctors (diag_decl);
> - if (TREE_PRIVATE (decl))
> - {
> - error ("%q#D is private within this context",
> diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl),
> - "declared private here");
> - if (afi)
> - afi->record_access_failure (basetype_path,
> diag_decl);
> - }
> - else if (TREE_PROTECTED (decl))
> - {
> - error ("%q#D is protected within this context",
> diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl),
> - "declared protected here");
> - if (afi)
> - afi->record_access_failure (basetype_path,
> diag_decl);
> - }
> - else
> - {
> - error ("%q#D is inaccessible within this context",
> diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl), "declared
> here");
> - if (afi)
> - afi->record_access_failure (basetype_path,
> diag_decl);
> - }
> - }
> + complain_about_access (decl, diag_decl, true);
> + if (afi)
> + afi->record_access_failure (basetype_path, decl, diag_decl);
> return false;
> }
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 0fac7e9..9b7a497 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6054,18 +6054,27 @@ extern location_t
> get_fndecl_argument_location (tree, int);
> class access_failure_info
> {
> public:
> - access_failure_info () : m_was_inaccessible (false),
> m_basetype_path (NULL_TREE),
> - m_field_decl (NULL_TREE) {}
> + access_failure_info () : m_was_inaccessible (false),
> + m_basetype_path (NULL_TREE),
> + m_decl (NULL_TREE), m_diag_decl (NULL_TREE) {}
>
> - void record_access_failure (tree basetype_path, tree field_decl);
> + void record_access_failure (tree basetype_path, tree decl, tree
> diag_decl);
> +
> + bool was_inaccessible_p () const { return m_was_inaccessible; }
> + tree get_decl () const { return m_decl; }
> + tree get_diag_decl () const { return m_diag_decl; }
> + tree get_any_accessor (bool const_p) const;
> void maybe_suggest_accessor (bool const_p) const;
> + static void add_fixit_hint (rich_location *richloc, tree
> accessor);
>
> private:
> bool m_was_inaccessible;
> tree m_basetype_path;
> - tree m_field_decl;
> + tree m_decl;
> + tree m_diag_decl;
> };
>
> +extern void complain_about_access (tree, tree, bool);
> extern bool enforce_access (tree, tree, tree,
> tsubst_flags_t,
> access_failure_info
> *afi = NULL);
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index ea4ce96..d03e797 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -2703,43 +2703,138 @@ check_template_keyword (tree decl)
> }
>
> /* Record that an access failure occurred on BASETYPE_PATH
> attempting
> - to access FIELD_DECL. */
> + to access DECL, where DIAG_DECL should be used for
> diagnostics. */
>
> void
> access_failure_info::record_access_failure (tree basetype_path,
> - tree field_decl)
> + tree decl, tree
> diag_decl)
> {
> m_was_inaccessible = true;
> m_basetype_path = basetype_path;
> - m_field_decl = field_decl;
> + m_decl = decl;
> + m_diag_decl = diag_decl;
> }
>
> /* If an access failure was recorded, then attempt to locate an
> - accessor function for the pertinent field, and if one is
> - available, add a note and fix-it hint suggesting using it. */
> + accessor function for the pertinent field.
> + Otherwise, return NULL_TREE. */
>
> -void
> -access_failure_info::maybe_suggest_accessor (bool const_p) const
> +tree
> +access_failure_info::get_any_accessor (bool const_p) const
> {
> - if (!m_was_inaccessible)
> - return;
> + if (!was_inaccessible_p ())
> + return NULL_TREE;
>
> tree accessor
> - = locate_field_accessor (m_basetype_path, m_field_decl,
> const_p);
> + = locate_field_accessor (m_basetype_path, m_diag_decl, const_p);
> if (!accessor)
> - return;
> + return NULL_TREE;
>
> /* The accessor must itself be accessible for it to be a
> reasonable
> suggestion. */
> if (!accessible_p (m_basetype_path, accessor, true))
> - return;
> + return NULL_TREE;
>
> - rich_location richloc (line_table, input_location);
> + return accessor;
> +}
> +
> +/* Add a fix-it hint to RICHLOC suggesting the use of ACCESSOR_DECL,
> by
> + replacing the primary location in RICHLOC with "accessor()". */
> +
> +void
> +access_failure_info::add_fixit_hint (rich_location *richloc,
> + tree accessor_decl)
> +{
> pretty_printer pp;
> - pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME
> (accessor)));
> - richloc.add_fixit_replace (pp_formatted_text (&pp));
> + pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME
> (accessor_decl)));
> + richloc->add_fixit_replace (pp_formatted_text (&pp));
> +}
> +
> +/* If an access failure was recorded, then attempt to locate an
> + accessor function for the pertinent field, and if one is
> + available, add a note and fix-it hint suggesting using it. */
> +
> +void
> +access_failure_info::maybe_suggest_accessor (bool const_p) const
> +{
> + tree accessor = get_any_accessor (const_p);
> + if (accessor == NULL_TREE)
> + return;
> + rich_location richloc (line_table, input_location);
> + add_fixit_hint (&richloc, accessor);
> inform (&richloc, "field %q#D can be accessed via %q#D",
> - m_field_decl, accessor);
> + m_diag_decl, accessor);
> +}
> +
> +/* Subroutine of finish_class_member_access_expr.
> + Issue an error about NAME not being a member of ACCESS_PATH (or
> + OBJECT_TYPE), potentially providing a fix-it hint for misspelled
> + names. */
> +
> +static void
> +complain_about_unrecognized_member (tree access_path, tree name,
> + tree object_type)
> +{
> + /* Attempt to provide a hint about misspelled names. */
> + tree guessed_id = lookup_member_fuzzy (access_path, name,
> + /*want_type=*/false);
> + if (guessed_id == NULL_TREE)
> + {
> + /* No hint. */
> + error ("%q#T has no member named %qE",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type, name);
> + return;
> + }
> +
> + location_t bogus_component_loc = input_location;
> + gcc_rich_location rich_loc (bogus_component_loc);
> +
> + /* Check that the guessed name is accessible along
> access_path. */
> + access_failure_info afi;
> + lookup_member (access_path, guessed_id, /*protect=*/1,
> + /*want_type=*/false, /*complain=*/false,
> + &afi);
> + if (afi.was_inaccessible_p ())
> + {
> + tree accessor = afi.get_any_accessor (TYPE_READONLY
> (object_type));
> + if (accessor)
> + {
> + /* The guessed name isn't directly accessible, but can be
> accessed
> + via an accessor member function. */
> + afi.add_fixit_hint (&rich_loc, accessor);
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %q#D? (accessible via %q#D)",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, afi.get_diag_decl (), accessor);
> + }
> + else
> + {
> + /* The guessed name isn't directly accessible, and no
> accessor
> + member function could be found. */
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %q#D? (not accessible from this
> context)",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, afi.get_diag_decl ());
> + complain_about_access (afi.get_decl (), afi.get_diag_decl
> (), false);
> + }
> + }
> + else
> + {
> + /* The guessed name is directly accessible; suggest it. */
> + rich_loc.add_fixit_misspelled_id (bogus_component_loc,
> + guessed_id);
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %qE?",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, guessed_id);
> + }
> }
>
> /* This function is called by the parser to process a class member
> @@ -2935,27 +3030,8 @@ finish_class_member_access_expr (cp_expr
> object, tree name, bool template_p,
> /* Try again at instantiation time. */
> goto dependent;
> if (complain & tf_error)
> - {
> - tree guessed_id = lookup_member_fuzzy
> (access_path, name,
> - /*want_type
> =*/false);
> - if (guessed_id)
> - {
> - location_t bogus_component_loc =
> input_location;
> - gcc_rich_location rich_loc
> (bogus_component_loc);
> - rich_loc.add_fixit_misspelled_id
> (bogus_component_loc,
> - guessed_id);
> - error_at (&rich_loc,
> - "%q#T has no member named %qE;"
> - " did you mean %qE?",
> - TREE_CODE (access_path) ==
> TREE_BINFO
> - ? TREE_TYPE (access_path) :
> object_type,
> - name, guessed_id);
> - }
> - else
> - error ("%q#T has no member named %qE",
> - TREE_CODE (access_path) == TREE_BINFO
> - ? TREE_TYPE (access_path) : object_type,
> name);
> - }
> + complain_about_unrecognized_member (access_path,
> name,
> + object_type);
> return error_mark_node;
> }
> if (member == error_mark_node)
> diff --git a/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
> b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
> new file mode 100644
> index 0000000..276c5b8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
> @@ -0,0 +1,119 @@
> +// PR c++/84993
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +/* Misspelling (by omitting a leading "m_") of a private member for
> which
> + there's a public accessor.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t1
> +{
> +public:
> + int get_ratio () const { return m_ratio; }
> +
> +private:
> + int m_ratio;
> +};
> +
> +int test (t1 *ptr_1)
> +{
> + return ptr_1->ratio; // { dg-error "'class t1' has no member named
> 'ratio'; did you mean 'int t1::m_ratio'\\? \\(accessible via 'int
> t1::get_ratio\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_1->ratio;
> + ^~~~~
> + get_ratio()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member for which there's a public
> accessor.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t2
> +{
> +public:
> + int get_color () const { return m_color; }
> +
> +private:
> + int m_color;
> +};
> +
> +int test (t2 *ptr_2)
> +{
> + return ptr_2->m_colour; // { dg-error "'class t2' has no member
> named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via
> 'int t2::get_color\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_2->m_colour;
> + ^~~~~~~~
> + get_color()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member via a subclass pointer, for which
> there's
> + a public accessor in the base class.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t3 : public t2 {};
> +
> +int test (t3 *ptr_3)
> +{
> + return ptr_3->colour; // { dg-error "'class t3' has no member
> named 'colour'; did you mean 'int t2::m_color'\\? \\(accessible via
> 'int t2::get_color\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_3->colour;
> + ^~~~~~
> + get_color()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a protected member, for which there's isn't a
> public
> + accessor.
> +
> + We expect no fix-it hint; instead a message identifying where the
> + data member was declared. */
> +
> +class t4
> +{
> +protected:
> + int m_color; // { dg-message "declared protected here" }
> +};
> +
> +int test (t4 *ptr_4)
> +{
> + return ptr_4->colour; // { dg-error "'class t4' has no member
> named 'colour'; did you mean 'int t4::m_color'\\? \\(not accessible
> from this context\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_4->colour;
> + ^~~~~~
> + { dg-end-multiline-output "" } */
> + /* { dg-begin-multiline-output "" }
> + int m_color;
> + ^~~~~~~
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member, for which the accessor is also
> private.
> +
> + We expect no fix-it hint; instead a message identifying where the
> + data member was declared. */
> +
> +class t5
> +{
> + int get_color () const { return m_color; }
> + int m_color; // { dg-message "declared private here" }
> +};
> +
> +int test (t5 *ptr_5)
> +{
> + return ptr_5->colour; // { dg-error "'class t5' has no member
> named 'colour'; did you mean 'int t5::m_color'\\? \\(not accessible
> from this context\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_5->colour;
> + ^~~~~~
> + { dg-end-multiline-output "" } */
> + /* { dg-begin-multiline-output "" }
> + int m_color;
> + ^~~~~~~
> + { dg-end-multiline-output "" } */
> +}
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)
2018-07-27 15:00 ` [PING] " David Malcolm
@ 2018-09-21 21:38 ` David Malcolm
2018-09-24 17:11 ` Martin Sebor
2018-10-11 18:36 ` Jason Merrill
0 siblings, 2 replies; 6+ messages in thread
From: David Malcolm @ 2018-09-21 21:38 UTC (permalink / raw)
To: gcc-patches; +Cc: Jason Merrill, David Malcolm
This is v2 of the patch; I managed to bit-rot my own patch due to my
fix for r264335, which tightened up the "is this meaningful" threshold
on edit distances when finding spelling correction candidates.
The only change in this version is to rename various things in
the testcase so that they continue to be suggested
("colour" vs "m_color" are no longer near enough, so I renamed
"colour" to "m_colour").
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?
Blurb from v1:
PR c++/84993 identifies a problem with our suggestions for
misspelled member names in the C++ FE for the case where the
member is private.
For example, given:
class foo
{
public:
double get_ratio() const { return m_ratio; }
private:
double m_ratio;
};
void test(foo *ptr)
{
if (ptr->ratio >= 0.5)
;// etc
}
...we currently emit this suggestion:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean 'm_ratio'?
if (ptr->ratio >= 0.5)
^~~~~
m_ratio
...but if the user follows this suggestion, they get:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'double foo::m_ratio' is private within this context
if (ptr->m_ratio >= 0.5)
^~~~~~~
<source>:7:10: note: declared private here
double m_ratio;
^~~~~~~
<source>:12:12: note: field 'double foo::m_ratio' can be accessed via 'double foo::get_ratio() const'
if (ptr->m_ratio >= 0.5)
^~~~~~~
get_ratio()
It feels wrong to be emitting a fix-it hint that doesn't compile, so this
patch adds the accessor fix-it hint logic to this case, so that we directly
offer a valid suggestion:
<source>: In function 'void test(foo*)':
<source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean
'double foo::m_ratio'? (accessible via 'double foo::get_ratio() const')
if (ptr->ratio >= 0.5)
^~~~~
get_ratio()
gcc/cp/ChangeLog:
PR c++/84993
* call.c (enforce_access): Move diagnostics to...
(complain_about_access): ...this new function.
* cp-tree.h (class access_failure_info): Rename split out field
"m_field_decl" into "m_decl" and "m_diag_decl".
(access_failure_info::record_access_failure): Add tree param.
(access_failure_info::was_inaccessible_p): New accessor.
(access_failure_info::get_decl): New accessor.
(access_failure_info::get_diag_decl): New accessor.
(access_failure_info::get_any_accessor): New member function.
(access_failure_info::add_fixit_hint): New static member function.
(complain_about_access): New decl.
* typeck.c (access_failure_info::record_access_failure): Update
for change to fields.
(access_failure_info::maybe_suggest_accessor): Split out into...
(access_failure_info::get_any_accessor): ...this new function...
(access_failure_info::add_fixit_hint): ...and this new function.
(finish_class_member_access_expr): Split out "has no member named"
error-handling into...
(complain_about_unrecognized_member): ...this new function, and
check that the guessed name is accessible along the access path.
Only provide a spell-correction fix-it hint if it is; otherwise,
attempt to issue an accessor fix-it hint.
gcc/testsuite/ChangeLog:
PR c++/84993
* g++.dg/torture/accessor-fixits-9.C: New test.
---
gcc/cp/call.c | 64 ++++++----
gcc/cp/cp-tree.h | 17 ++-
gcc/cp/typeck.c | 150 +++++++++++++++++------
gcc/testsuite/g++.dg/torture/accessor-fixits-9.C | 119 ++++++++++++++++++
4 files changed, 282 insertions(+), 68 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 69503ca..445dde8 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6512,6 +6512,38 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
return error_mark_node;
}
+/* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
+ in the diagnostics.
+
+ If ISSUE_ERROR is true, then issue an error about the
+ access, followed by a note showing the declaration.
+ Otherwise, just show the note. */
+
+void
+complain_about_access (tree decl, tree diag_decl, bool issue_error)
+{
+ if (TREE_PRIVATE (decl))
+ {
+ if (issue_error)
+ error ("%q#D is private within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl),
+ "declared private here");
+ }
+ else if (TREE_PROTECTED (decl))
+ {
+ if (issue_error)
+ error ("%q#D is protected within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl),
+ "declared protected here");
+ }
+ else
+ {
+ if (issue_error)
+ error ("%q#D is inaccessible within this context", diag_decl);
+ inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
+ }
+}
+
/* If the current scope isn't allowed to access DECL along
BASETYPE_PATH, give an error. The most derived class in
BASETYPE_PATH is the one used to qualify DECL. DIAG_DECL is
@@ -6536,34 +6568,12 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
if (!accessible_p (basetype_path, decl, true))
{
+ if (flag_new_inheriting_ctors)
+ diag_decl = strip_inheriting_ctors (diag_decl);
if (complain & tf_error)
- {
- if (flag_new_inheriting_ctors)
- diag_decl = strip_inheriting_ctors (diag_decl);
- if (TREE_PRIVATE (decl))
- {
- error ("%q#D is private within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl),
- "declared private here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- else if (TREE_PROTECTED (decl))
- {
- error ("%q#D is protected within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl),
- "declared protected here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- else
- {
- error ("%q#D is inaccessible within this context", diag_decl);
- inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
- if (afi)
- afi->record_access_failure (basetype_path, diag_decl);
- }
- }
+ complain_about_access (decl, diag_decl, true);
+ if (afi)
+ afi->record_access_failure (basetype_path, decl, diag_decl);
return false;
}
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 6cd6e5f..6c12c5f 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6101,18 +6101,27 @@ extern void complain_about_bad_argument (location_t arg_loc,
class access_failure_info
{
public:
- access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
- m_field_decl (NULL_TREE) {}
+ access_failure_info () : m_was_inaccessible (false),
+ m_basetype_path (NULL_TREE),
+ m_decl (NULL_TREE), m_diag_decl (NULL_TREE) {}
- void record_access_failure (tree basetype_path, tree field_decl);
+ void record_access_failure (tree basetype_path, tree decl, tree diag_decl);
+
+ bool was_inaccessible_p () const { return m_was_inaccessible; }
+ tree get_decl () const { return m_decl; }
+ tree get_diag_decl () const { return m_diag_decl; }
+ tree get_any_accessor (bool const_p) const;
void maybe_suggest_accessor (bool const_p) const;
+ static void add_fixit_hint (rich_location *richloc, tree accessor);
private:
bool m_was_inaccessible;
tree m_basetype_path;
- tree m_field_decl;
+ tree m_decl;
+ tree m_diag_decl;
};
+extern void complain_about_access (tree, tree, bool);
extern bool enforce_access (tree, tree, tree,
tsubst_flags_t,
access_failure_info *afi = NULL);
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index e993220..a404877 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2708,43 +2708,138 @@ check_template_keyword (tree decl)
}
/* Record that an access failure occurred on BASETYPE_PATH attempting
- to access FIELD_DECL. */
+ to access DECL, where DIAG_DECL should be used for diagnostics. */
void
access_failure_info::record_access_failure (tree basetype_path,
- tree field_decl)
+ tree decl, tree diag_decl)
{
m_was_inaccessible = true;
m_basetype_path = basetype_path;
- m_field_decl = field_decl;
+ m_decl = decl;
+ m_diag_decl = diag_decl;
}
/* If an access failure was recorded, then attempt to locate an
- accessor function for the pertinent field, and if one is
- available, add a note and fix-it hint suggesting using it. */
+ accessor function for the pertinent field.
+ Otherwise, return NULL_TREE. */
-void
-access_failure_info::maybe_suggest_accessor (bool const_p) const
+tree
+access_failure_info::get_any_accessor (bool const_p) const
{
- if (!m_was_inaccessible)
- return;
+ if (!was_inaccessible_p ())
+ return NULL_TREE;
tree accessor
- = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
+ = locate_field_accessor (m_basetype_path, m_diag_decl, const_p);
if (!accessor)
- return;
+ return NULL_TREE;
/* The accessor must itself be accessible for it to be a reasonable
suggestion. */
if (!accessible_p (m_basetype_path, accessor, true))
- return;
+ return NULL_TREE;
- rich_location richloc (line_table, input_location);
+ return accessor;
+}
+
+/* Add a fix-it hint to RICHLOC suggesting the use of ACCESSOR_DECL, by
+ replacing the primary location in RICHLOC with "accessor()". */
+
+void
+access_failure_info::add_fixit_hint (rich_location *richloc,
+ tree accessor_decl)
+{
pretty_printer pp;
- pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
- richloc.add_fixit_replace (pp_formatted_text (&pp));
+ pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor_decl)));
+ richloc->add_fixit_replace (pp_formatted_text (&pp));
+}
+
+/* If an access failure was recorded, then attempt to locate an
+ accessor function for the pertinent field, and if one is
+ available, add a note and fix-it hint suggesting using it. */
+
+void
+access_failure_info::maybe_suggest_accessor (bool const_p) const
+{
+ tree accessor = get_any_accessor (const_p);
+ if (accessor == NULL_TREE)
+ return;
+ rich_location richloc (line_table, input_location);
+ add_fixit_hint (&richloc, accessor);
inform (&richloc, "field %q#D can be accessed via %q#D",
- m_field_decl, accessor);
+ m_diag_decl, accessor);
+}
+
+/* Subroutine of finish_class_member_access_expr.
+ Issue an error about NAME not being a member of ACCESS_PATH (or
+ OBJECT_TYPE), potentially providing a fix-it hint for misspelled
+ names. */
+
+static void
+complain_about_unrecognized_member (tree access_path, tree name,
+ tree object_type)
+{
+ /* Attempt to provide a hint about misspelled names. */
+ tree guessed_id = lookup_member_fuzzy (access_path, name,
+ /*want_type=*/false);
+ if (guessed_id == NULL_TREE)
+ {
+ /* No hint. */
+ error ("%q#T has no member named %qE",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type, name);
+ return;
+ }
+
+ location_t bogus_component_loc = input_location;
+ gcc_rich_location rich_loc (bogus_component_loc);
+
+ /* Check that the guessed name is accessible along access_path. */
+ access_failure_info afi;
+ lookup_member (access_path, guessed_id, /*protect=*/1,
+ /*want_type=*/false, /*complain=*/false,
+ &afi);
+ if (afi.was_inaccessible_p ())
+ {
+ tree accessor = afi.get_any_accessor (TYPE_READONLY (object_type));
+ if (accessor)
+ {
+ /* The guessed name isn't directly accessible, but can be accessed
+ via an accessor member function. */
+ afi.add_fixit_hint (&rich_loc, accessor);
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %q#D? (accessible via %q#D)",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, afi.get_diag_decl (), accessor);
+ }
+ else
+ {
+ /* The guessed name isn't directly accessible, and no accessor
+ member function could be found. */
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %q#D? (not accessible from this context)",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, afi.get_diag_decl ());
+ complain_about_access (afi.get_decl (), afi.get_diag_decl (), false);
+ }
+ }
+ else
+ {
+ /* The guessed name is directly accessible; suggest it. */
+ rich_loc.add_fixit_misspelled_id (bogus_component_loc,
+ guessed_id);
+ error_at (&rich_loc,
+ "%q#T has no member named %qE;"
+ " did you mean %qE?",
+ TREE_CODE (access_path) == TREE_BINFO
+ ? TREE_TYPE (access_path) : object_type,
+ name, guessed_id);
+ }
}
/* This function is called by the parser to process a class member
@@ -2940,27 +3035,8 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
/* Try again at instantiation time. */
goto dependent;
if (complain & tf_error)
- {
- tree guessed_id = lookup_member_fuzzy (access_path, name,
- /*want_type=*/false);
- if (guessed_id)
- {
- location_t bogus_component_loc = input_location;
- gcc_rich_location rich_loc (bogus_component_loc);
- rich_loc.add_fixit_misspelled_id (bogus_component_loc,
- guessed_id);
- error_at (&rich_loc,
- "%q#T has no member named %qE;"
- " did you mean %qE?",
- TREE_CODE (access_path) == TREE_BINFO
- ? TREE_TYPE (access_path) : object_type,
- name, guessed_id);
- }
- else
- error ("%q#T has no member named %qE",
- TREE_CODE (access_path) == TREE_BINFO
- ? TREE_TYPE (access_path) : object_type, name);
- }
+ complain_about_unrecognized_member (access_path, name,
+ object_type);
return error_mark_node;
}
if (member == error_mark_node)
diff --git a/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
new file mode 100644
index 0000000..d9e77ba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
@@ -0,0 +1,119 @@
+// PR c++/84993
+// { dg-options "-fdiagnostics-show-caret" }
+
+/* Misspelling (by omitting a leading "m_") of a private member for which
+ there's a public accessor.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t1
+{
+public:
+ int get_ratio () const { return m_ratio; }
+
+private:
+ int m_ratio;
+};
+
+int test (t1 *ptr_1)
+{
+ return ptr_1->ratio; // { dg-error "'class t1' has no member named 'ratio'; did you mean 'int t1::m_ratio'\\? \\(accessible via 'int t1::get_ratio\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_1->ratio;
+ ^~~~~
+ get_ratio()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member for which there's a public accessor.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t2
+{
+public:
+ int get_color () const { return m_color; }
+
+private:
+ int m_color;
+};
+
+int test (t2 *ptr_2)
+{
+ return ptr_2->m_colour; // { dg-error "'class t2' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_2->m_colour;
+ ^~~~~~~~
+ get_color()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member via a subclass pointer, for which there's
+ a public accessor in the base class.
+
+ We expect a fix-it hint suggesting the accessor. */
+
+class t3 : public t2 {};
+
+int test (t3 *ptr_3)
+{
+ return ptr_3->m_colour; // { dg-error "'class t3' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_3->m_colour;
+ ^~~~~~~~
+ get_color()
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a protected member, for which there's isn't a public
+ accessor.
+
+ We expect no fix-it hint; instead a message identifying where the
+ data member was declared. */
+
+class t4
+{
+protected:
+ int m_color; // { dg-message "declared protected here" }
+};
+
+int test (t4 *ptr_4)
+{
+ return ptr_4->m_colour; // { dg-error "'class t4' has no member named 'm_colour'; did you mean 'int t4::m_color'\\? \\(not accessible from this context\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_4->m_colour;
+ ^~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ int m_color;
+ ^~~~~~~
+ { dg-end-multiline-output "" } */
+}
+
+
+/* Misspelling of a private member, for which the accessor is also private.
+
+ We expect no fix-it hint; instead a message identifying where the
+ data member was declared. */
+
+class t5
+{
+ int get_color () const { return m_color; }
+ int m_color; // { dg-message "declared private here" }
+};
+
+int test (t5 *ptr_5)
+{
+ return ptr_5->m_colour; // { dg-error "'class t5' has no member named 'm_colour'; did you mean 'int t5::m_color'\\? \\(not accessible from this context\\)" }
+ /* { dg-begin-multiline-output "" }
+ return ptr_5->m_colour;
+ ^~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ int m_color;
+ ^~~~~~~
+ { dg-end-multiline-output "" } */
+}
--
1.8.5.3
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)
2018-09-21 21:38 ` [PATCH] v2: " David Malcolm
@ 2018-09-24 17:11 ` Martin Sebor
2018-10-09 15:09 ` David Malcolm
2018-10-11 18:36 ` Jason Merrill
1 sibling, 1 reply; 6+ messages in thread
From: Martin Sebor @ 2018-09-24 17:11 UTC (permalink / raw)
To: David Malcolm, gcc-patches; +Cc: Jason Merrill
On 09/21/2018 04:09 PM, David Malcolm wrote:
> This is v2 of the patch; I managed to bit-rot my own patch due to my
> fix for r264335, which tightened up the "is this meaningful" threshold
> on edit distances when finding spelling correction candidates.
>
> The only change in this version is to rename various things in
> the testcase so that they continue to be suggested
> ("colour" vs "m_color" are no longer near enough, so I renamed
> "colour" to "m_colour").
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> Blurb from v1:
>
> PR c++/84993 identifies a problem with our suggestions for
> misspelled member names in the C++ FE for the case where the
> member is private.
>
> For example, given:
>
> class foo
> {
> public:
> double get_ratio() const { return m_ratio; }
>
> private:
> double m_ratio;
> };
>
> void test(foo *ptr)
> {
> if (ptr->ratio >= 0.5)
> ;// etc
> }
>
> ...we currently emit this suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean 'm_ratio'?
> if (ptr->ratio >= 0.5)
> ^~~~~
> m_ratio
>
> ...but if the user follows this suggestion, they get:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'double foo::m_ratio' is private within this context
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> <source>:7:10: note: declared private here
> double m_ratio;
> ^~~~~~~
> <source>:12:12: note: field 'double foo::m_ratio' can be accessed via 'double foo::get_ratio() const'
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> get_ratio()
>
> It feels wrong to be emitting a fix-it hint that doesn't compile, so this
> patch adds the accessor fix-it hint logic to this case, so that we directly
> offer a valid suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean
> 'double foo::m_ratio'? (accessible via 'double foo::get_ratio() const')
> if (ptr->ratio >= 0.5)
> ^~~~~
> get_ratio()
I wonder if suggesting the inaccessible member is at all helpful
if it cannot be used. Would mentioning only the accessor be
a better approach?
Also, to echo a comment made by someone else in a bug I don't
remember, rather than phrasing the error in the form of
a question ("did you mean x?") it might be better to either
state what we know:
error: 'class foo' has no member named 'ratio'; the nearest
match is 'double foo::m_ratio' (accessible via 'double
foo::get_ratio() const')
or offer a suggestion:
error: 'class foo' has no member named 'ratio'; suggest
using 'double foo::get_ratio() const' instead
A different approach altogether to these spelling messages that
occurs to me but that you may have already considered and rejected
would be to do what GCC does for errors due to ambiguous overloads:
i.e., enumerate the available candidates. This would work well in
cases when multiple members are a close match. It would also make
it possible to explain, for each candidate member, whether it's
accessible.
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)
2018-09-24 17:11 ` Martin Sebor
@ 2018-10-09 15:09 ` David Malcolm
0 siblings, 0 replies; 6+ messages in thread
From: David Malcolm @ 2018-10-09 15:09 UTC (permalink / raw)
To: Martin Sebor, gcc-patches; +Cc: Jason Merrill
On Mon, 2018-09-24 at 10:56 -0600, Martin Sebor wrote:
> On 09/21/2018 04:09 PM, David Malcolm wrote:
> > This is v2 of the patch; I managed to bit-rot my own patch due to
> > my
> > fix for r264335, which tightened up the "is this meaningful"
> > threshold
> > on edit distances when finding spelling correction candidates.
> >
> > The only change in this version is to rename various things in
> > the testcase so that they continue to be suggested
> > ("colour" vs "m_color" are no longer near enough, so I renamed
> > "colour" to "m_colour").
> >
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> >
> > OK for trunk?
> >
> > Blurb from v1:
> >
> > PR c++/84993 identifies a problem with our suggestions for
> > misspelled member names in the C++ FE for the case where the
> > member is private.
> >
> > For example, given:
> >
> > class foo
> > {
> > public:
> > double get_ratio() const { return m_ratio; }
> >
> > private:
> > double m_ratio;
> > };
> >
> > void test(foo *ptr)
> > {
> > if (ptr->ratio >= 0.5)
> > ;// etc
> > }
> >
> > ...we currently emit this suggestion:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean 'm_ratio'?
> > if (ptr->ratio >= 0.5)
> > ^~~~~
> > m_ratio
> >
> > ...but if the user follows this suggestion, they get:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'double foo::m_ratio' is private within this
> > context
> > if (ptr->m_ratio >= 0.5)
> > ^~~~~~~
> > <source>:7:10: note: declared private here
> > double m_ratio;
> > ^~~~~~~
> > <source>:12:12: note: field 'double foo::m_ratio' can be accessed
> > via 'double foo::get_ratio() const'
> > if (ptr->m_ratio >= 0.5)
> > ^~~~~~~
> > get_ratio()
> >
> > It feels wrong to be emitting a fix-it hint that doesn't compile,
> > so this
> > patch adds the accessor fix-it hint logic to this case, so that we
> > directly
> > offer a valid suggestion:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean
> > 'double foo::m_ratio'? (accessible via 'double foo::get_ratio()
> > const')
> > if (ptr->ratio >= 0.5)
> > ^~~~~
> > get_ratio()
>
> I wonder if suggesting the inaccessible member is at all helpful
> if it cannot be used.
Of the two members "m_ratio" and "get_ratio", the winning candidate
based on edit-distance lookup was the inaccessible member; the accessor
is "further away" in terms of edit distance. I think it's useful to
the user to identify both, as this patch does.
> Would mentioning only the accessor be
> a better approach?
I think it's more helpful to the user to be explicit about what the
compiler is "thinking", and mention both. Consider the case where the
accessor has a wildy different name to the misspelled member:
class foo {
public:
int get_pertinent_data () { return m_val; }
private:
int m_val;
};
int test (foo *f)
{
return f->val;
}
The patch emits:
t.c: In function ‘int test(foo*)’:
t.c:11:14: error: ‘class foo’ has no member named ‘val’; did you mean
‘int foo::m_val’? (accessible via ‘int foo::get_pertinent_data()’)
11 | return f->val;
| ^~~
| get_pertinent_data()
which I think is clear and helpful.
> Also, to echo a comment made by someone else in a bug I don't
> remember, rather than phrasing the error in the form of
> a question ("did you mean x?") it might be better to either
> state what we know:
>
> error: 'class foo' has no member named 'ratio'; the nearest
> match is 'double foo::m_ratio' (accessible via 'double
> foo::get_ratio() const')
>
> or offer a suggestion:
>
> error: 'class foo' has no member named 'ratio'; suggest
> using 'double foo::get_ratio() const' instead
I think you're referring to PR 84890, though that bug was about this
kind of message:
note: ‘INT_MAX’ is defined in header ‘<limits.h>’; did you forget to
‘#include <limits.h>’?
PR 84890 combines several concerns:
(a) is the message too verbose? (initial comment)
(b) is the message potentially annoying to the user? (comment 5)
(c) is it better to phrase the message as a suggestion, rather than as
a question? (comment 8)
Taking these in turn:
(a) may apply to the missing include message, but I think the "did you
mean 'foo'" is short and clear
(b) both styles of message refer to the user as "you", which has a risk
of making things personal. I think it's fine for the "did you mean"
case. I think there's an argument that the "did you forget" could be
annoying - but that's not a concern for this patch.
(c) of the various wordings:
(c.1) "did you mean 'foo'?" (as in the patch, and the status quo for
the rest of the C and C++ FE)
(c.2) "suggest using 'foo' instead" doesn't seem grammatically
correct to me, in that it sounds like the user is being asked to
suggest something, rather than to change their code. I think there's
an implicit "I" in there - "I suggest" - but after abbreviation it
seems clunky to me.
(c.3) "suggestion: use 'foo'" might work, but (c.1) seems much
simpler to me
So I prefer the wording in the patch. If we're going to change the
wording across the compiler, I'd prefer to do that in a separate patch.
> A different approach altogether to these spelling messages that
> occurs to me but that you may have already considered and rejected
> would be to do what GCC does for errors due to ambiguous overloads:
> i.e., enumerate the available candidates. This would work well in
> cases when multiple members are a close match. It would also make
> it possible to explain, for each candidate member, whether it's
> accessible.
I hadn't thought of that approach, but my hunch is that there's likely
to only be one good suggestion: this use-case is trying to handle the
case where the user is trying to access a field of a class, and can't
quite remember the spelling, or what the accessor is.
FWIW I actually have another patch I'm about to post which updates
those available candidates suggestions (it special-cases the common
case where there's just one candidate: it uses the "did you mean %qs?"
wording, and the user gets a fix-it hint inline, and two less "note"
diagnostics)
Dave
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)
2018-09-21 21:38 ` [PATCH] v2: " David Malcolm
2018-09-24 17:11 ` Martin Sebor
@ 2018-10-11 18:36 ` Jason Merrill
1 sibling, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2018-10-11 18:36 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches List
OK.
On Fri, Sep 21, 2018 at 5:22 PM David Malcolm <dmalcolm@redhat.com> wrote:
>
> This is v2 of the patch; I managed to bit-rot my own patch due to my
> fix for r264335, which tightened up the "is this meaningful" threshold
> on edit distances when finding spelling correction candidates.
>
> The only change in this version is to rename various things in
> the testcase so that they continue to be suggested
> ("colour" vs "m_color" are no longer near enough, so I renamed
> "colour" to "m_colour").
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> Blurb from v1:
>
> PR c++/84993 identifies a problem with our suggestions for
> misspelled member names in the C++ FE for the case where the
> member is private.
>
> For example, given:
>
> class foo
> {
> public:
> double get_ratio() const { return m_ratio; }
>
> private:
> double m_ratio;
> };
>
> void test(foo *ptr)
> {
> if (ptr->ratio >= 0.5)
> ;// etc
> }
>
> ...we currently emit this suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean 'm_ratio'?
> if (ptr->ratio >= 0.5)
> ^~~~~
> m_ratio
>
> ...but if the user follows this suggestion, they get:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'double foo::m_ratio' is private within this context
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> <source>:7:10: note: declared private here
> double m_ratio;
> ^~~~~~~
> <source>:12:12: note: field 'double foo::m_ratio' can be accessed via 'double foo::get_ratio() const'
> if (ptr->m_ratio >= 0.5)
> ^~~~~~~
> get_ratio()
>
> It feels wrong to be emitting a fix-it hint that doesn't compile, so this
> patch adds the accessor fix-it hint logic to this case, so that we directly
> offer a valid suggestion:
>
> <source>: In function 'void test(foo*)':
> <source>:12:12: error: 'class foo' has no member named 'ratio'; did you mean
> 'double foo::m_ratio'? (accessible via 'double foo::get_ratio() const')
> if (ptr->ratio >= 0.5)
> ^~~~~
> get_ratio()
>
> gcc/cp/ChangeLog:
> PR c++/84993
> * call.c (enforce_access): Move diagnostics to...
> (complain_about_access): ...this new function.
> * cp-tree.h (class access_failure_info): Rename split out field
> "m_field_decl" into "m_decl" and "m_diag_decl".
> (access_failure_info::record_access_failure): Add tree param.
> (access_failure_info::was_inaccessible_p): New accessor.
> (access_failure_info::get_decl): New accessor.
> (access_failure_info::get_diag_decl): New accessor.
> (access_failure_info::get_any_accessor): New member function.
> (access_failure_info::add_fixit_hint): New static member function.
> (complain_about_access): New decl.
> * typeck.c (access_failure_info::record_access_failure): Update
> for change to fields.
> (access_failure_info::maybe_suggest_accessor): Split out into...
> (access_failure_info::get_any_accessor): ...this new function...
> (access_failure_info::add_fixit_hint): ...and this new function.
> (finish_class_member_access_expr): Split out "has no member named"
> error-handling into...
> (complain_about_unrecognized_member): ...this new function, and
> check that the guessed name is accessible along the access path.
> Only provide a spell-correction fix-it hint if it is; otherwise,
> attempt to issue an accessor fix-it hint.
>
> gcc/testsuite/ChangeLog:
> PR c++/84993
> * g++.dg/torture/accessor-fixits-9.C: New test.
> ---
> gcc/cp/call.c | 64 ++++++----
> gcc/cp/cp-tree.h | 17 ++-
> gcc/cp/typeck.c | 150 +++++++++++++++++------
> gcc/testsuite/g++.dg/torture/accessor-fixits-9.C | 119 ++++++++++++++++++
> 4 files changed, 282 insertions(+), 68 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 69503ca..445dde8 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -6512,6 +6512,38 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
> return error_mark_node;
> }
>
> +/* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
> + in the diagnostics.
> +
> + If ISSUE_ERROR is true, then issue an error about the
> + access, followed by a note showing the declaration.
> + Otherwise, just show the note. */
> +
> +void
> +complain_about_access (tree decl, tree diag_decl, bool issue_error)
> +{
> + if (TREE_PRIVATE (decl))
> + {
> + if (issue_error)
> + error ("%q#D is private within this context", diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl),
> + "declared private here");
> + }
> + else if (TREE_PROTECTED (decl))
> + {
> + if (issue_error)
> + error ("%q#D is protected within this context", diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl),
> + "declared protected here");
> + }
> + else
> + {
> + if (issue_error)
> + error ("%q#D is inaccessible within this context", diag_decl);
> + inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
> + }
> +}
> +
> /* If the current scope isn't allowed to access DECL along
> BASETYPE_PATH, give an error. The most derived class in
> BASETYPE_PATH is the one used to qualify DECL. DIAG_DECL is
> @@ -6536,34 +6568,12 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
>
> if (!accessible_p (basetype_path, decl, true))
> {
> + if (flag_new_inheriting_ctors)
> + diag_decl = strip_inheriting_ctors (diag_decl);
> if (complain & tf_error)
> - {
> - if (flag_new_inheriting_ctors)
> - diag_decl = strip_inheriting_ctors (diag_decl);
> - if (TREE_PRIVATE (decl))
> - {
> - error ("%q#D is private within this context", diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl),
> - "declared private here");
> - if (afi)
> - afi->record_access_failure (basetype_path, diag_decl);
> - }
> - else if (TREE_PROTECTED (decl))
> - {
> - error ("%q#D is protected within this context", diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl),
> - "declared protected here");
> - if (afi)
> - afi->record_access_failure (basetype_path, diag_decl);
> - }
> - else
> - {
> - error ("%q#D is inaccessible within this context", diag_decl);
> - inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
> - if (afi)
> - afi->record_access_failure (basetype_path, diag_decl);
> - }
> - }
> + complain_about_access (decl, diag_decl, true);
> + if (afi)
> + afi->record_access_failure (basetype_path, decl, diag_decl);
> return false;
> }
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 6cd6e5f..6c12c5f 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6101,18 +6101,27 @@ extern void complain_about_bad_argument (location_t arg_loc,
> class access_failure_info
> {
> public:
> - access_failure_info () : m_was_inaccessible (false), m_basetype_path (NULL_TREE),
> - m_field_decl (NULL_TREE) {}
> + access_failure_info () : m_was_inaccessible (false),
> + m_basetype_path (NULL_TREE),
> + m_decl (NULL_TREE), m_diag_decl (NULL_TREE) {}
>
> - void record_access_failure (tree basetype_path, tree field_decl);
> + void record_access_failure (tree basetype_path, tree decl, tree diag_decl);
> +
> + bool was_inaccessible_p () const { return m_was_inaccessible; }
> + tree get_decl () const { return m_decl; }
> + tree get_diag_decl () const { return m_diag_decl; }
> + tree get_any_accessor (bool const_p) const;
> void maybe_suggest_accessor (bool const_p) const;
> + static void add_fixit_hint (rich_location *richloc, tree accessor);
>
> private:
> bool m_was_inaccessible;
> tree m_basetype_path;
> - tree m_field_decl;
> + tree m_decl;
> + tree m_diag_decl;
> };
>
> +extern void complain_about_access (tree, tree, bool);
> extern bool enforce_access (tree, tree, tree,
> tsubst_flags_t,
> access_failure_info *afi = NULL);
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index e993220..a404877 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -2708,43 +2708,138 @@ check_template_keyword (tree decl)
> }
>
> /* Record that an access failure occurred on BASETYPE_PATH attempting
> - to access FIELD_DECL. */
> + to access DECL, where DIAG_DECL should be used for diagnostics. */
>
> void
> access_failure_info::record_access_failure (tree basetype_path,
> - tree field_decl)
> + tree decl, tree diag_decl)
> {
> m_was_inaccessible = true;
> m_basetype_path = basetype_path;
> - m_field_decl = field_decl;
> + m_decl = decl;
> + m_diag_decl = diag_decl;
> }
>
> /* If an access failure was recorded, then attempt to locate an
> - accessor function for the pertinent field, and if one is
> - available, add a note and fix-it hint suggesting using it. */
> + accessor function for the pertinent field.
> + Otherwise, return NULL_TREE. */
>
> -void
> -access_failure_info::maybe_suggest_accessor (bool const_p) const
> +tree
> +access_failure_info::get_any_accessor (bool const_p) const
> {
> - if (!m_was_inaccessible)
> - return;
> + if (!was_inaccessible_p ())
> + return NULL_TREE;
>
> tree accessor
> - = locate_field_accessor (m_basetype_path, m_field_decl, const_p);
> + = locate_field_accessor (m_basetype_path, m_diag_decl, const_p);
> if (!accessor)
> - return;
> + return NULL_TREE;
>
> /* The accessor must itself be accessible for it to be a reasonable
> suggestion. */
> if (!accessible_p (m_basetype_path, accessor, true))
> - return;
> + return NULL_TREE;
>
> - rich_location richloc (line_table, input_location);
> + return accessor;
> +}
> +
> +/* Add a fix-it hint to RICHLOC suggesting the use of ACCESSOR_DECL, by
> + replacing the primary location in RICHLOC with "accessor()". */
> +
> +void
> +access_failure_info::add_fixit_hint (rich_location *richloc,
> + tree accessor_decl)
> +{
> pretty_printer pp;
> - pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor)));
> - richloc.add_fixit_replace (pp_formatted_text (&pp));
> + pp_printf (&pp, "%s()", IDENTIFIER_POINTER (DECL_NAME (accessor_decl)));
> + richloc->add_fixit_replace (pp_formatted_text (&pp));
> +}
> +
> +/* If an access failure was recorded, then attempt to locate an
> + accessor function for the pertinent field, and if one is
> + available, add a note and fix-it hint suggesting using it. */
> +
> +void
> +access_failure_info::maybe_suggest_accessor (bool const_p) const
> +{
> + tree accessor = get_any_accessor (const_p);
> + if (accessor == NULL_TREE)
> + return;
> + rich_location richloc (line_table, input_location);
> + add_fixit_hint (&richloc, accessor);
> inform (&richloc, "field %q#D can be accessed via %q#D",
> - m_field_decl, accessor);
> + m_diag_decl, accessor);
> +}
> +
> +/* Subroutine of finish_class_member_access_expr.
> + Issue an error about NAME not being a member of ACCESS_PATH (or
> + OBJECT_TYPE), potentially providing a fix-it hint for misspelled
> + names. */
> +
> +static void
> +complain_about_unrecognized_member (tree access_path, tree name,
> + tree object_type)
> +{
> + /* Attempt to provide a hint about misspelled names. */
> + tree guessed_id = lookup_member_fuzzy (access_path, name,
> + /*want_type=*/false);
> + if (guessed_id == NULL_TREE)
> + {
> + /* No hint. */
> + error ("%q#T has no member named %qE",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type, name);
> + return;
> + }
> +
> + location_t bogus_component_loc = input_location;
> + gcc_rich_location rich_loc (bogus_component_loc);
> +
> + /* Check that the guessed name is accessible along access_path. */
> + access_failure_info afi;
> + lookup_member (access_path, guessed_id, /*protect=*/1,
> + /*want_type=*/false, /*complain=*/false,
> + &afi);
> + if (afi.was_inaccessible_p ())
> + {
> + tree accessor = afi.get_any_accessor (TYPE_READONLY (object_type));
> + if (accessor)
> + {
> + /* The guessed name isn't directly accessible, but can be accessed
> + via an accessor member function. */
> + afi.add_fixit_hint (&rich_loc, accessor);
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %q#D? (accessible via %q#D)",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, afi.get_diag_decl (), accessor);
> + }
> + else
> + {
> + /* The guessed name isn't directly accessible, and no accessor
> + member function could be found. */
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %q#D? (not accessible from this context)",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, afi.get_diag_decl ());
> + complain_about_access (afi.get_decl (), afi.get_diag_decl (), false);
> + }
> + }
> + else
> + {
> + /* The guessed name is directly accessible; suggest it. */
> + rich_loc.add_fixit_misspelled_id (bogus_component_loc,
> + guessed_id);
> + error_at (&rich_loc,
> + "%q#T has no member named %qE;"
> + " did you mean %qE?",
> + TREE_CODE (access_path) == TREE_BINFO
> + ? TREE_TYPE (access_path) : object_type,
> + name, guessed_id);
> + }
> }
>
> /* This function is called by the parser to process a class member
> @@ -2940,27 +3035,8 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
> /* Try again at instantiation time. */
> goto dependent;
> if (complain & tf_error)
> - {
> - tree guessed_id = lookup_member_fuzzy (access_path, name,
> - /*want_type=*/false);
> - if (guessed_id)
> - {
> - location_t bogus_component_loc = input_location;
> - gcc_rich_location rich_loc (bogus_component_loc);
> - rich_loc.add_fixit_misspelled_id (bogus_component_loc,
> - guessed_id);
> - error_at (&rich_loc,
> - "%q#T has no member named %qE;"
> - " did you mean %qE?",
> - TREE_CODE (access_path) == TREE_BINFO
> - ? TREE_TYPE (access_path) : object_type,
> - name, guessed_id);
> - }
> - else
> - error ("%q#T has no member named %qE",
> - TREE_CODE (access_path) == TREE_BINFO
> - ? TREE_TYPE (access_path) : object_type, name);
> - }
> + complain_about_unrecognized_member (access_path, name,
> + object_type);
> return error_mark_node;
> }
> if (member == error_mark_node)
> diff --git a/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
> new file mode 100644
> index 0000000..d9e77ba
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/accessor-fixits-9.C
> @@ -0,0 +1,119 @@
> +// PR c++/84993
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +/* Misspelling (by omitting a leading "m_") of a private member for which
> + there's a public accessor.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t1
> +{
> +public:
> + int get_ratio () const { return m_ratio; }
> +
> +private:
> + int m_ratio;
> +};
> +
> +int test (t1 *ptr_1)
> +{
> + return ptr_1->ratio; // { dg-error "'class t1' has no member named 'ratio'; did you mean 'int t1::m_ratio'\\? \\(accessible via 'int t1::get_ratio\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_1->ratio;
> + ^~~~~
> + get_ratio()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member for which there's a public accessor.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t2
> +{
> +public:
> + int get_color () const { return m_color; }
> +
> +private:
> + int m_color;
> +};
> +
> +int test (t2 *ptr_2)
> +{
> + return ptr_2->m_colour; // { dg-error "'class t2' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_2->m_colour;
> + ^~~~~~~~
> + get_color()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member via a subclass pointer, for which there's
> + a public accessor in the base class.
> +
> + We expect a fix-it hint suggesting the accessor. */
> +
> +class t3 : public t2 {};
> +
> +int test (t3 *ptr_3)
> +{
> + return ptr_3->m_colour; // { dg-error "'class t3' has no member named 'm_colour'; did you mean 'int t2::m_color'\\? \\(accessible via 'int t2::get_color\\(\\) const'\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_3->m_colour;
> + ^~~~~~~~
> + get_color()
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a protected member, for which there's isn't a public
> + accessor.
> +
> + We expect no fix-it hint; instead a message identifying where the
> + data member was declared. */
> +
> +class t4
> +{
> +protected:
> + int m_color; // { dg-message "declared protected here" }
> +};
> +
> +int test (t4 *ptr_4)
> +{
> + return ptr_4->m_colour; // { dg-error "'class t4' has no member named 'm_colour'; did you mean 'int t4::m_color'\\? \\(not accessible from this context\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_4->m_colour;
> + ^~~~~~~~
> + { dg-end-multiline-output "" } */
> + /* { dg-begin-multiline-output "" }
> + int m_color;
> + ^~~~~~~
> + { dg-end-multiline-output "" } */
> +}
> +
> +
> +/* Misspelling of a private member, for which the accessor is also private.
> +
> + We expect no fix-it hint; instead a message identifying where the
> + data member was declared. */
> +
> +class t5
> +{
> + int get_color () const { return m_color; }
> + int m_color; // { dg-message "declared private here" }
> +};
> +
> +int test (t5 *ptr_5)
> +{
> + return ptr_5->m_colour; // { dg-error "'class t5' has no member named 'm_colour'; did you mean 'int t5::m_color'\\? \\(not accessible from this context\\)" }
> + /* { dg-begin-multiline-output "" }
> + return ptr_5->m_colour;
> + ^~~~~~~~
> + { dg-end-multiline-output "" } */
> + /* { dg-begin-multiline-output "" }
> + int m_color;
> + ^~~~~~~
> + { dg-end-multiline-output "" } */
> +}
> --
> 1.8.5.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-11 18:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12 1:58 [PATCH] C++: suggestions for misspelled private members (PR c++/84993) David Malcolm
2018-07-27 15:00 ` [PING] " David Malcolm
2018-09-21 21:38 ` [PATCH] v2: " David Malcolm
2018-09-24 17:11 ` Martin Sebor
2018-10-09 15:09 ` David Malcolm
2018-10-11 18:36 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).