From patchwork Fri Sep 21 22:09:34 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 973436 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-486170-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="lq4QS4oQ"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42H64m2Tg9z9sC7 for ; Sat, 22 Sep 2018 07:22:29 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; q=dns; s= default; b=ZUu9/lcXkG3hF3p0YColHHwBr8rj3CaLqcGAoX0cktshoS53shq3b eV9EsyULmYdepYscDD6QZnuqR4IZDCfl9iE5mDZsLlvmFcuPSMxR2yNPyVLyRJRo 4u52cLiZXaGPE9jM94fomofqRP89rItlrr57lsShMWNlwUzOFzeef4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; s= default; bh=G10hsnApEdFyLAI+remISZDiUi0=; b=lq4QS4oQYp2QmRjksywq WVcHSOHVnumGNHpXW/ikD7AXKCs5VA/cnznplm4uQgQ3FzDhPDy0D1i1mUH5bJK/ aUo9byFD0Q3J/LMX/H8dKJPPeC8CwwnbXfFXRSXLdg8qDaYKeRkbi7ffp2hyB/Ip 97UQefe7tln/Q3LqeGRYvZQ= Received: (qmail 86477 invoked by alias); 21 Sep 2018 21:22:21 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 86463 invoked by uid 89); 21 Sep 2018 21:22:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=near, colour, UD:typeck.c, typeck.c X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Sep 2018 21:22:17 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2E6E28110D for ; Fri, 21 Sep 2018 21:22:16 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-33.phx2.redhat.com [10.3.112.33]) by smtp.corp.redhat.com (Postfix) with ESMTP id C69097D939; Fri, 21 Sep 2018 21:22:14 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: Jason Merrill , David Malcolm Subject: [PATCH] v2: C++: suggestions for misspelled private members (PR c++/84993) Date: Fri, 21 Sep 2018 18:09:34 -0400 Message-Id: <1537567774-8310-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1532703610.22345.109.camel@redhat.com> References: <1532703610.22345.109.camel@redhat.com> X-IsSubscribed: yes 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 "" } */ +}