Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993)

[Jason Merrill <jason at redhat dot com>]

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
>