C++: suggestions for misspelled private members (PR c++/84993)

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

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