OK.
On Fri, Sep 21, 2018 at 5:22 PM David Malcolm <dmalc...@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
>
