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 "" } */
> +}
