From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27084 invoked by alias); 11 Oct 2018 18:32:54 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26727 invoked by uid 89); 11 Oct 2018 18:32:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=ratio, sk:accesso, near, qualify X-HELO: mail-ot1-f65.google.com Received: from mail-ot1-f65.google.com (HELO mail-ot1-f65.google.com) (209.85.210.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 11 Oct 2018 18:32:51 +0000 Received: by mail-ot1-f65.google.com with SMTP id 14so5911819oth.2 for ; Thu, 11 Oct 2018 11:32:50 -0700 (PDT) MIME-Version: 1.0 References: <1532703610.22345.109.camel@redhat.com> <1537567774-8310-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1537567774-8310-1-git-send-email-dmalcolm@redhat.com> From: Jason Merrill Date: Thu, 11 Oct 2018 18:36:00 -0000 Message-ID: Subject: Re: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993) To: David Malcolm Cc: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00687.txt.bz2 OK. On Fri, Sep 21, 2018 at 5:22 PM David Malcolm 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: > > : 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. > --- > 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 >