From 03f6d32edb50546f1a123e848ae98a70a747b0c7 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Thu, 11 Oct 2018 19:03:33 +0000 Subject: [PATCH] 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: : In function 'void test(foo*)': :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: : In function 'void test(foo*)': :12:12: error: 'double foo::m_ratio' is private within this context if (ptr->m_ratio >= 0.5) ^~~~~~~ :7:10: note: declared private here double m_ratio; ^~~~~~~ :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: : In function 'void test(foo*)': :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. From-SVN: r265056 --- gcc/cp/ChangeLog | 26 +++ gcc/cp/call.c | 64 ++++---- gcc/cp/cp-tree.h | 17 +- gcc/cp/typeck.c | 150 +++++++++++++----- gcc/testsuite/ChangeLog | 5 + .../g++.dg/torture/accessor-fixits-9.C | 119 ++++++++++++++ 6 files changed, 313 insertions(+), 68 deletions(-) create mode 100644 gcc/testsuite/g++.dg/torture/accessor-fixits-9.C diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index da443d9be40..a3d29d75b36 100644 --- a/gcc/cp/ChangeLog +++ b/gcc/cp/ChangeLog @@ -1,3 +1,29 @@ +2018-10-11 David Malcolm + + 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. + 2018-10-11 Nathan Sidwell * parser.c (cp_parser_translation_unit): Return void. Don't fail diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 747f837019d..0baf26e4346 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -6514,6 +6514,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 @@ -6538,34 +6570,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 efbdad83966..26ded3a9214 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6103,18 +6103,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 8f819592533..c921096cb31 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/ChangeLog b/gcc/testsuite/ChangeLog index df8f70c671e..a1ba7da09b8 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2018-10-11 David Malcolm + + PR c++/84993 + * g++.dg/torture/accessor-fixits-9.C: New test. + 2018-10-11 Nathan Sidwell * g++.dg/parse/close-brace.C: New. 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 00000000000..d9e77ba2d3b --- /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 "" } */ +} -- 2.30.2