Message ID | 1511365007-22684-1-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
Series | C++: avoid most reserved words as misspelling suggestions (PR c++/81610 and PR c++/80567) | expand |
Ping: https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html On Wed, 2017-11-22 at 10:36 -0500, David Malcolm wrote: > lookup_name_fuzzy can offer some reserved words as suggestions for > misspelled words, helping with "singed"/"signed" typos. > > PR c++/81610 and PR c++/80567 report problems where the C++ frontend > suggested "if", "for" and "else" as corrections for misspelled > variable > names. > > The root cause is that in r247233 > ("Fix spelling suggestions for reserved words (PR c++/80177)") > I loosened the conditions on these reserved words, adding this > condition: > if (kind == FUZZY_LOOKUP_TYPENAME) > to the logic for rejecting words that don't start decl-specifiers, to > allow for "static_assert" to be offered. > > This is too loose a condition: we don't want to suggest *any* > reserved word > when we're in a context where we don't know we expect a typename. > > For the kinds of error-recover situations where we're suggesting > spelling corrections we don't have much contextual information, so it > seems prudent to be stricter about which reserved words we offer > as spelling suggestions; I don't think it makes sense for us to > suggest e.g. "for". > > This patch implements that by effectively reinstating the old logic, > but special-casing RID_STATIC_ASSERT, moving the logic to a new > subroutine (in case we want to allow for other special-cases). > > I attempted to add suggestions for the various RID_*CAST, to cope > with e.g. "reinterptet_cast" (I can never type that correctly on the > first try), but the following '<' token confuses the error-recovery > enough that the suggestion code isn't triggered. > > Hence this more minimal fix. > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/cp/ChangeLog: > PR c++/81610 > PR c++/80567 > * name-lookup.c (suggest_rid_p): New function. > (lookup_name_fuzzy): Replace enum-rid-filtering logic with call > to > suggest_rid_p. > > gcc/testsuite/ChangeLog: > PR c++/81610 > PR c++/80567 > * g++.dg/spellcheck-reswords.C: New test case. > * g++.dg/spellcheck-stdlib.C: Remove xfail from dg-bogus > suggestion of "if". > --- > gcc/cp/name-lookup.c | 31 > +++++++++++++++++++++++++++--- > gcc/testsuite/g++.dg/spellcheck-reswords.C | 11 +++++++++++ > gcc/testsuite/g++.dg/spellcheck-stdlib.C | 2 +- > 3 files changed, 40 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/spellcheck-reswords.C > > diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c > index 7c363b0..a96be46 100644 > --- a/gcc/cp/name-lookup.c > +++ b/gcc/cp/name-lookup.c > @@ -5671,6 +5671,32 @@ class macro_use_before_def : public > deferred_diagnostic > cpp_hashnode *m_macro; > }; > > +/* Determine if it can ever make sense to offer RID as a suggestion > for > + a misspelling. > + > + Subroutine of lookup_name_fuzzy. */ > + > +static bool > +suggest_rid_p (enum rid rid) > +{ > + switch (rid) > + { > + /* Support suggesting function-like keywords. */ > + case RID_STATIC_ASSERT: > + return true; > + > + default: > + /* Support suggesting the various decl-specifier words, to > handle > + e.g. "singed" vs "signed" typos. */ > + if (cp_keyword_starts_decl_specifier_p (rid)) > + return true; > + > + /* Otherwise, don't offer it. This avoids suggesting e.g. > "if" > + and "do" for short misspellings, which are likely to lead > to > + nonsensical results. */ > + return false; > + } > +} > > /* Search for near-matches for NAME within the current bindings, and > within > macro names, returning the best match as a const char *, or NULL > if > @@ -5735,9 +5761,8 @@ lookup_name_fuzzy (tree name, enum > lookup_name_fuzzy_kind kind, location_t loc) > { > const c_common_resword *resword = &c_common_reswords[i]; > > - if (kind == FUZZY_LOOKUP_TYPENAME) > - if (!cp_keyword_starts_decl_specifier_p (resword->rid)) > - continue; > + if (!suggest_rid_p (resword->rid)) > + continue; > > tree resword_identifier = ridpointers [resword->rid]; > if (!resword_identifier) > diff --git a/gcc/testsuite/g++.dg/spellcheck-reswords.C > b/gcc/testsuite/g++.dg/spellcheck-reswords.C > new file mode 100644 > index 0000000..db6104b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/spellcheck-reswords.C > @@ -0,0 +1,11 @@ > +void pr81610 (void *p) > +{ > + forget (p); // { dg-error "not declared" } > + // { dg-bogus "'for'" "" { target *-*-*} .-1 } > +} > + > +void pr80567 (void *p) > +{ > + memset (p, 0, 4); // { dg-error "not declared" } > + // { dg-bogus "'else'" "" { target *-*-*} .-1 } > +} > diff --git a/gcc/testsuite/g++.dg/spellcheck-stdlib.C > b/gcc/testsuite/g++.dg/spellcheck-stdlib.C > index 6e6ab1d..c7a6626 100644 > --- a/gcc/testsuite/g++.dg/spellcheck-stdlib.C > +++ b/gcc/testsuite/g++.dg/spellcheck-stdlib.C > @@ -16,7 +16,7 @@ void test_cstdio (void) > FILE *f; // { dg-error "'FILE' was not declared in this scope" } > // { dg-message "'FILE' is defined in header '<cstdio>'; did you > forget to '#include <cstdio>'?" "" { target *-*-* } .-1 } > // { dg-error "'f' was not declared in this scope" "" { target *- > *-* } .-2 } > - // { dg-bogus "suggested alternative: 'if'" "PR c++/80567" { xfail > *-*-* } .-3 } > + // { dg-bogus "suggested alternative: 'if'" "PR c++/80567" { > target *-*-* } .-3 } > > char buf[BUFSIZ]; // { dg-error "'BUFSIZ' was not declared" } > // { dg-message "'BUFSIZ' is defined in header '<cstdio>'; did you > forget to '#include <cstdio>'?" "" { target *-*-* } .-1 }
On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat.com> wrote: > PR c++/81610 and PR c++/80567 report problems where the C++ frontend > suggested "if", "for" and "else" as corrections for misspelled variable > names. Hmm, what about cases where people are actually misspelling keywords? Don't we want to handle that? fi (true) { } retrun 42; In the PRs you mention, the actual identifiers are 1) missing includes, which we should check first, and 2) pretty far from the suggested keywords. Jason
On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: > On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat.com> > wrote: Original post: https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html > > PR c++/81610 and PR c++/80567 report problems where the C++ > > frontend > > suggested "if", "for" and "else" as corrections for misspelled > > variable > > names. I've now marked these PRs as regressions: the nonsensical suggestions are only offered by trunk, not by gcc 7 and earlier. > Hmm, what about cases where people are actually misspelling keywords? > Don't we want to handle that? > > fi (true) { } > retrun 42; I'd prefer not to. gcc 7 and earlier don't attempt to correct the spelling of the "fi" and "retrun" above. trunk currently does offer "return" as a suggestion, but it was by accident, and I'm wary of attempting to support these corrections: is "fi" meant to be an "if", or a function call that's missing its decl, or a name lookup issue? ...etc > In the PRs you mention, the actual identifiers are 1) missing > includes, which we should check first, and 2) pretty far from the > suggested keywords. The C++ FE is missing a suggestion about which #include to use for "memset", but I'd prefer to treat that as a follow-up patch (and probably for next stage 1). In the meantime, is this patch OK for trunk? (as a regression fix) Thanks Dave
Ping On Fri, 2018-01-26 at 13:12 -0500, David Malcolm wrote: > On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: > > On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat.co > > m> > > wrote: > > Original post: > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html > > > > PR c++/81610 and PR c++/80567 report problems where the C++ > > > frontend > > > suggested "if", "for" and "else" as corrections for misspelled > > > variable > > > names. > > I've now marked these PRs as regressions: the nonsensical suggestions > are only offered by trunk, not by gcc 7 and earlier. > > > Hmm, what about cases where people are actually misspelling > > keywords? > > Don't we want to handle that? > > > > fi (true) { } > > retrun 42; > > I'd prefer not to. > > gcc 7 and earlier don't attempt to correct the spelling of the "fi" > and > "retrun" above. > > trunk currently does offer "return" as a suggestion, but it was by > accident, and I'm wary of attempting to support these corrections: is > "fi" meant to be an "if", or a function call that's missing its decl, > or a name lookup issue? ...etc > > > In the PRs you mention, the actual identifiers are 1) missing > > includes, which we should check first, and 2) pretty far from the > > suggested keywords. > > The C++ FE is missing a suggestion about which #include to use for > "memset", but I'd prefer to treat that as a follow-up patch (and > probably for next stage 1). > > In the meantime, is this patch OK for trunk? (as a regression fix) > > Thanks > Dave
On Fri, Jan 26, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: >> On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat.com> >> wrote: > > Original post: > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html > >> > PR c++/81610 and PR c++/80567 report problems where the C++ >> > frontend >> > suggested "if", "for" and "else" as corrections for misspelled >> > variable >> > names. > > I've now marked these PRs as regressions: the nonsensical suggestions > are only offered by trunk, not by gcc 7 and earlier. > >> Hmm, what about cases where people are actually misspelling keywords? >> Don't we want to handle that? >> >> fi (true) { } >> retrun 42; > > I'd prefer not to. > > gcc 7 and earlier don't attempt to correct the spelling of the "fi" and > "retrun" above. > > trunk currently does offer "return" as a suggestion, but it was by > accident, and I'm wary of attempting to support these corrections: is > "fi" meant to be an "if", or a function call that's missing its decl, > or a name lookup issue? ...etc > >> In the PRs you mention, the actual identifiers are 1) missing >> includes, which we should check first, and 2) pretty far from the >> suggested keywords. > > The C++ FE is missing a suggestion about which #include to use for > "memset", but I'd prefer to treat that as a follow-up patch (and > probably for next stage 1). > > In the meantime, is this patch OK for trunk? (as a regression fix) Yes. Jason
On Wed, 2018-02-07 at 12:21 -0500, Jason Merrill wrote: > On Fri, Jan 26, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.com> > wrote: > > On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: > > > On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat. > > > com> > > > wrote: > > > > Original post: > > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html > > > > > > PR c++/81610 and PR c++/80567 report problems where the C++ > > > > frontend > > > > suggested "if", "for" and "else" as corrections for misspelled > > > > variable > > > > names. > > > > I've now marked these PRs as regressions: the nonsensical > > suggestions > > are only offered by trunk, not by gcc 7 and earlier. > > > > > Hmm, what about cases where people are actually misspelling > > > keywords? > > > Don't we want to handle that? > > > > > > fi (true) { } > > > retrun 42; > > > > I'd prefer not to. > > > > gcc 7 and earlier don't attempt to correct the spelling of the "fi" > > and > > "retrun" above. > > > > trunk currently does offer "return" as a suggestion, but it was by > > accident, and I'm wary of attempting to support these corrections: > > is > > "fi" meant to be an "if", or a function call that's missing its > > decl, > > or a name lookup issue? ...etc > > > > > In the PRs you mention, the actual identifiers are 1) missing > > > includes, which we should check first, and 2) pretty far from the > > > suggested keywords. > > > > The C++ FE is missing a suggestion about which #include to use for > > "memset", but I'd prefer to treat that as a follow-up patch (and > > probably for next stage 1). > > > > In the meantime, is this patch OK for trunk? (as a regression fix) > > Yes. Thanks; committed (r257456). FWIW, I've filed PR c++/84269 so I remember to fix the missing suggestion for "memset" (in gcc 9 stage1). Dave
On Wed, Feb 7, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Wed, 2018-02-07 at 12:21 -0500, Jason Merrill wrote: >> On Fri, Jan 26, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.com> >> wrote: >> > On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: >> > > On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@redhat. >> > > com> >> > > wrote: >> > >> > Original post: >> > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html >> > >> > > > PR c++/81610 and PR c++/80567 report problems where the C++ >> > > > frontend >> > > > suggested "if", "for" and "else" as corrections for misspelled >> > > > variable >> > > > names. >> > >> > I've now marked these PRs as regressions: the nonsensical >> > suggestions >> > are only offered by trunk, not by gcc 7 and earlier. >> > >> > > Hmm, what about cases where people are actually misspelling >> > > keywords? >> > > Don't we want to handle that? >> > > >> > > fi (true) { } >> > > retrun 42; >> > >> > I'd prefer not to. >> > >> > gcc 7 and earlier don't attempt to correct the spelling of the "fi" >> > and >> > "retrun" above. >> > >> > trunk currently does offer "return" as a suggestion, but it was by >> > accident, and I'm wary of attempting to support these corrections: >> > is >> > "fi" meant to be an "if", or a function call that's missing its >> > decl, >> > or a name lookup issue? ...etc >> > >> > > In the PRs you mention, the actual identifiers are 1) missing >> > > includes, which we should check first, and 2) pretty far from the >> > > suggested keywords. >> > >> > The C++ FE is missing a suggestion about which #include to use for >> > "memset", but I'd prefer to treat that as a follow-up patch (and >> > probably for next stage 1). >> > >> > In the meantime, is this patch OK for trunk? (as a regression fix) >> >> Yes. > > Thanks; committed (r257456). > > FWIW, I've filed PR c++/84269 so I remember to fix the missing > suggestion for "memset" (in gcc 9 stage1). Did you have a reaction to my comment about the suggested keyword being pretty far from the actual identifier? Do we want to lower the cutoff for suggestions at all? Jason
On Wed, 2018-02-07 at 13:22 -0500, Jason Merrill wrote: > On Wed, Feb 7, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.com> > wrote: > > On Wed, 2018-02-07 at 12:21 -0500, Jason Merrill wrote: > > > On Fri, Jan 26, 2018 at 1:12 PM, David Malcolm <dmalcolm@redhat.c > > > om> > > > wrote: > > > > On Mon, 2017-12-11 at 17:24 -0500, Jason Merrill wrote: > > > > > On Wed, Nov 22, 2017 at 10:36 AM, David Malcolm <dmalcolm@red > > > > > hat. > > > > > com> > > > > > wrote: > > > > > > > > Original post: > > > > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02048.html > > > > > > > > > > PR c++/81610 and PR c++/80567 report problems where the C++ > > > > > > frontend > > > > > > suggested "if", "for" and "else" as corrections for > > > > > > misspelled > > > > > > variable > > > > > > names. > > > > > > > > I've now marked these PRs as regressions: the nonsensical > > > > suggestions > > > > are only offered by trunk, not by gcc 7 and earlier. > > > > > > > > > Hmm, what about cases where people are actually misspelling > > > > > keywords? > > > > > Don't we want to handle that? > > > > > > > > > > fi (true) { } > > > > > retrun 42; > > > > > > > > I'd prefer not to. > > > > > > > > gcc 7 and earlier don't attempt to correct the spelling of the > > > > "fi" > > > > and > > > > "retrun" above. > > > > > > > > trunk currently does offer "return" as a suggestion, but it was > > > > by > > > > accident, and I'm wary of attempting to support these > > > > corrections: > > > > is > > > > "fi" meant to be an "if", or a function call that's missing its > > > > decl, > > > > or a name lookup issue? ...etc > > > > > > > > > In the PRs you mention, the actual identifiers are 1) missing > > > > > includes, which we should check first, and 2) pretty far from > > > > > the > > > > > suggested keywords. > > > > > > > > The C++ FE is missing a suggestion about which #include to use > > > > for > > > > "memset", but I'd prefer to treat that as a follow-up patch > > > > (and > > > > probably for next stage 1). > > > > > > > > In the meantime, is this patch OK for trunk? (as a regression > > > > fix) > > > > > > Yes. > > > > Thanks; committed (r257456). > > > > FWIW, I've filed PR c++/84269 so I remember to fix the missing > > suggestion for "memset" (in gcc 9 stage1). > > Did you have a reaction to my comment about the suggested keyword > being pretty far from the actual identifier? Do we want to lower the > cutoff for suggestions at all? I've played around with tweaking how the cutoff works, [1] in response to e.g. PR c/82967 ('"did you mean" suggestions are way too suggestive'), but I've not yet come up with a version I prefer to the current implementation.
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 7c363b0..a96be46 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -5671,6 +5671,32 @@ class macro_use_before_def : public deferred_diagnostic cpp_hashnode *m_macro; }; +/* Determine if it can ever make sense to offer RID as a suggestion for + a misspelling. + + Subroutine of lookup_name_fuzzy. */ + +static bool +suggest_rid_p (enum rid rid) +{ + switch (rid) + { + /* Support suggesting function-like keywords. */ + case RID_STATIC_ASSERT: + return true; + + default: + /* Support suggesting the various decl-specifier words, to handle + e.g. "singed" vs "signed" typos. */ + if (cp_keyword_starts_decl_specifier_p (rid)) + return true; + + /* Otherwise, don't offer it. This avoids suggesting e.g. "if" + and "do" for short misspellings, which are likely to lead to + nonsensical results. */ + return false; + } +} /* Search for near-matches for NAME within the current bindings, and within macro names, returning the best match as a const char *, or NULL if @@ -5735,9 +5761,8 @@ lookup_name_fuzzy (tree name, enum lookup_name_fuzzy_kind kind, location_t loc) { const c_common_resword *resword = &c_common_reswords[i]; - if (kind == FUZZY_LOOKUP_TYPENAME) - if (!cp_keyword_starts_decl_specifier_p (resword->rid)) - continue; + if (!suggest_rid_p (resword->rid)) + continue; tree resword_identifier = ridpointers [resword->rid]; if (!resword_identifier) diff --git a/gcc/testsuite/g++.dg/spellcheck-reswords.C b/gcc/testsuite/g++.dg/spellcheck-reswords.C new file mode 100644 index 0000000..db6104b --- /dev/null +++ b/gcc/testsuite/g++.dg/spellcheck-reswords.C @@ -0,0 +1,11 @@ +void pr81610 (void *p) +{ + forget (p); // { dg-error "not declared" } + // { dg-bogus "'for'" "" { target *-*-*} .-1 } +} + +void pr80567 (void *p) +{ + memset (p, 0, 4); // { dg-error "not declared" } + // { dg-bogus "'else'" "" { target *-*-*} .-1 } +} diff --git a/gcc/testsuite/g++.dg/spellcheck-stdlib.C b/gcc/testsuite/g++.dg/spellcheck-stdlib.C index 6e6ab1d..c7a6626 100644 --- a/gcc/testsuite/g++.dg/spellcheck-stdlib.C +++ b/gcc/testsuite/g++.dg/spellcheck-stdlib.C @@ -16,7 +16,7 @@ void test_cstdio (void) FILE *f; // { dg-error "'FILE' was not declared in this scope" } // { dg-message "'FILE' is defined in header '<cstdio>'; did you forget to '#include <cstdio>'?" "" { target *-*-* } .-1 } // { dg-error "'f' was not declared in this scope" "" { target *-*-* } .-2 } - // { dg-bogus "suggested alternative: 'if'" "PR c++/80567" { xfail *-*-* } .-3 } + // { dg-bogus "suggested alternative: 'if'" "PR c++/80567" { target *-*-* } .-3 } char buf[BUFSIZ]; // { dg-error "'BUFSIZ' was not declared" } // { dg-message "'BUFSIZ' is defined in header '<cstdio>'; did you forget to '#include <cstdio>'?" "" { target *-*-* } .-1 }