Re: [PATCH] c++: fix wrong fixit hints for misspelled typedef [PR77565]

[Jason Merrill <jason@redhat.com>]

On 9/17/21 13:31, Michel Morin wrote:
> On Fri, Sep 17, 2021 at 3:23 AM Jason Merrill <jason@redhat.com> wrote:
>>
>> On 9/16/21 11:50, Michel Morin wrote:
>>> On Thu, Sep 16, 2021 at 5:44 AM Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 9/14/21 04:29, Michel Morin via Gcc-patches wrote:
>>>>> On Tue, Sep 14, 2021 at 7:14 AM David Malcolm <dmalcolm@redhat.com> wrote:
>>>>>>
>>>>>> On Tue, 2021-09-14 at 03:35 +0900, Michel Morin via Gcc-patches wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> PR77565 reports that, with the code `typdef int Int;`, GCC emits
>>>>>>> "did you mean 'typeof'?" instead of "did you mean 'typedef'?".
>>>>>>>
>>>>>>> This happens because the typo corrector determines that `typeof` is a
>>>>>>> candidate for suggestion (through
>>>>>>> `cp_keyword_starts_decl_specifier_p`),
>>>>>>> but `typedef` is not.
>>>>>>>
>>>>>>> This patch fixes the issue by adding `typedef` as a candidate. The
>>>>>>> patch
>>>>>>> additionally adds the `inline` specifier and cv-specifiers as a
>>>>>>> candidate.
>>>>>>> Here is a patch (tests `make check-gcc` pass on darwin):
>>>>>>
>>>>>> Thanks for this patch (and for reporting the bug in the first place).
>>>>>>
>>>>>> I notice that, as well as being used for fix-it hints by
>>>>>> lookup_name_fuzzy (indirectly via suggest_rid_p),
>>>>>> cp_keyword_starts_decl_specifier_p is also used by
>>>>>> cp_lexer_next_token_is_decl_specifier_keyword, which is used by
>>>>>> cp_parser_lambda_declarator_opt and cp_parser_constructor_declarator_p.
>>>>>
>>>>> Ah, you're right! Thank you for pointing this out.
>>>>> I failed to grep those functions somehow.
>>>>>
>>>>> One thing that confuses me is that cp_keyword_starts_decl_specifier_p
>>>>> misses many keywords that can start decl-specifiers (e.g.
>>>>> typedef/inline/cv-qual and friend/explicit/virtual).
>>>>> So let's wait C++ frontend maintainers ;)
>>>>
>>>> That is strange.  Let's add all the rest of them as well.
>>>
>>> Done. Thanks for your help!
>>>
>>> One more thing — cp_keyword_starts_decl_specifier_p includes RID_ATTRIBUTE
>>> (from the beginning; see https://gcc.gnu.org/PR28261 ), but attributes are
>>> not decl-specifiers. Would it be reasonable to remove this?
>>
>> It looks like the place that PR28261 used
>> cp_lexer_next_token_is_decl_specifier_keyword specifically exempts
>> attributes:
>>
>>>            && (!cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
>>>                /* GNU attributes can actually appear both at the start of
>>>                   a parameter and parenthesized declarator.
>>>                   S (__attribute__((unused)) int);
>>>                   is a constructor, but
>>>                   S (__attribute__((unused)) foo) (int);
>>>                   is a function declaration.  */
>>>                || (cp_parser_allow_gnu_extensions_p (parser)
>>>                    && cp_next_tokens_can_be_gnu_attribute_p (parser)))
>>
>> So yes, let's remove RID_ATTRIBUTE and the || clause there.  I'd keep
>> the comment, but move it to go with the test for C++11 attributes below.
> 
> Done. No regressions introduced.
> 
>>> One more thing — cp_keyword_starts_decl_specifier_p includes RID_ATTRIBUTE
>>> (from the beginning; see https://gcc.gnu.org/PR28261 ), but attributes are
>>> not decl-specifiers.
> 
> Oh, this is wrong. I thought that, since C++11 attributes are not a
> decl-specifier, neither are GNU attributes. But the comment just before
> cp_parser_decl_specifier_seq says that GNU attributes are considered as a
> decl-specifier. So I'm not confident about the removal of RID_ATTRIBUTE in
> cp_keyword_starts_decl_specifier_p...

GNU attributes can appear in lots of places, and the only two callers of 
cp_parser_next_token_is_decl_specifier_keyword don't want to treat 
attributes accordingly.  Let's go with both your patches, and also 
remove the consequently-unnecessary attributes check in 
cp_parser_lambda_declarator_opt:

>   if (cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
>       && !cp_next_tokens_can_be_gnu_attribute_p (parser))

OK with that change.

> I've split the patch into two. The first one is for adding missing keywords to
> fix PR77565 and the second one is for removing the "attribute" keyword.
> Here is the second patch (if this is not applied, that's no problem ;) )
> 
> ======================================================
> c++: adjust the handling of RID_ATTRIBUTE.
> 
> gcc/cp/ChangeLog:
> 
> * parser.c (cp_keyword_starts_decl_specifier_p): Do not
> handle RID_ATTRIBUTE.
> (cp_parser_constructor_declarator_p): Remove now-redundant
> checks.
> 
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 40308d0d33f..d184a3aca7e 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -1062,7 +1062,6 @@ cp_keyword_starts_decl_specifier_p (enum rid keyword)
>       case RID_TYPEDEF:
>       case RID_INLINE:
>         /* GNU extensions.  */
> -    case RID_ATTRIBUTE:
>       case RID_TYPEOF:
>         /* C++11 extensions.  */
>       case RID_DECLTYPE:
> @@ -30798,23 +30797,22 @@ cp_parser_constructor_declarator_p
> (cp_parser *parser, cp_parser_flags flags,
>      /* A parameter declaration begins with a decl-specifier,
>         which is either the "attribute" keyword, a storage class
>         specifier, or (usually) a type-specifier.  */
> -   && (!cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
> -       /* GNU attributes can actually appear both at the start of
> - a parameter and parenthesized declarator.
> - S (__attribute__((unused)) int);
> - is a constructor, but
> - S (__attribute__((unused)) foo) (int);
> - is a function declaration.  */
> -       || (cp_parser_allow_gnu_extensions_p (parser)
> -   && cp_next_tokens_can_be_gnu_attribute_p (parser)))
> -   /* A parameter declaration can also begin with [[attribute]].  */
> +   && !cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
> +   /* GNU attributes can actually appear both at the start of
> +      a parameter and parenthesized declarator.
> +      S (__attribute__((unused)) int);
> +      is a constructor, but
> +      S (__attribute__((unused)) foo) (int);
> +      is a function declaration. [[attribute]] can appear in the
> +      first form too, but not in the second form.  */
>      && !cp_next_tokens_can_be_std_attribute_p (parser))
>    {
>      tree type;
>      tree pushed_scope = NULL_TREE;
>      unsigned saved_num_template_parameter_lists;
> 
> -   if (cp_next_tokens_can_be_gnu_attribute_p (parser))
> +   if (cp_parser_allow_gnu_extensions_p (parser)
> +       && cp_next_tokens_can_be_gnu_attribute_p (parser))
>        {
>          unsigned int n = cp_parser_skip_gnu_attributes_opt (parser, 1);
>          while (--n)
> ======================================================
> 
> Regards,
> Michel
> 
> 
>>> Both patches (with and without removal of RID_ATTRIBUTE) attached.
>>> No regressions on x86_64-apple-darwin.
>>>
>>> Regards,
>>> Michel
>>>
>>>
>>>
>>>>>> So I'm not sure if this fix is exactly correct - hopefully one of the
>>>>>> C++ frontend maintainers can chime in.  If
>>>>>> cp_keyword_starts_decl_specifier_p isn't quite the right place for
>>>>>> this, the fix could probably go in suggest_rid_p instead, which *is*
>>>>>> specific to spelling corrections.
>>>>>>
>>>>>> Hope this is constructive; thanks again for the patch
>>>>>> Dave
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> ============================================
>>>>>>> c++: add typo corrections for typedef/inline/cv-qual [PR77565]
>>>>>>>
>>>>>>> PR c++/77565
>>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>> * parser.c (cp_keyword_starts_decl_specifier_p): Handle
>>>>>>> typedef/inline specifiers and cv-qualifiers.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> * g++.dg/spellcheck-typenames.C: Add tests for decl-specs.
>>>>>>>
>>>>>>> --- a/gcc/cp/parser.c
>>>>>>> +++ b/gcc/cp/parser.c
>>>>>>> @@ -1051,6 +1051,12 @@ cp_keyword_starts_decl_specifier_p (enum rid
>>>>>>> keyword)
>>>>>>>         case RID_FLOAT:
>>>>>>>         case RID_DOUBLE:
>>>>>>>         case RID_VOID:
>>>>>>> +      /* CV qualifiers.  */
>>>>>>> +    case RID_CONST:
>>>>>>> +    case RID_VOLATILE:
>>>>>>> +      /* typedef/inline specifiers.  */
>>>>>>> +    case RID_TYPEDEF:
>>>>>>> +    case RID_INLINE:
>>>>>>>           /* GNU extensions.  */
>>>>>>>         case RID_ATTRIBUTE:
>>>>>>>         case RID_TYPEOF:
>>>>>>> --- a/gcc/testsuite/g++.dg/spellcheck-typenames.C
>>>>>>> +++ b/gcc/testsuite/g++.dg/spellcheck-typenames.C
>>>>>>> @@ -76,3 +76,38 @@ singed char ch; // { dg-error "1: 'singed' does
>>>>>>> not
>>>>>>> name a type; did you mean 's
>>>>>>>      ^~~~~~
>>>>>>>      signed
>>>>>>>        { dg-end-multiline-output "" } */
>>>>>>> +
>>>>>>> +typdef int my_int; // { dg-error "1: 'typdef' does not name a type;
>>>>>>> did you mean 'typedef'?" }
>>>>>>> +/* { dg-begin-multiline-output "" }
>>>>>>> + typdef int my_int;
>>>>>>> + ^~~~~~
>>>>>>> + typedef
>>>>>>> +   { dg-end-multiline-output "" } */
>>>>>>> +
>>>>>>> +inlien int inline_func(); // { dg-error "1: 'inlien' does not name a
>>>>>>> type; did you mean 'inline'?" }
>>>>>>> +/* { dg-begin-multiline-output "" }
>>>>>>> + inlien int inline_func();
>>>>>>> + ^~~~~~
>>>>>>> + inline
>>>>>>> +   { dg-end-multiline-output "" } */
>>>>>>> +
>>>>>>> +coonst int ci = 0; // { dg-error "1: 'coonst' does not name a type;
>>>>>>> did you mean 'const'?" }
>>>>>>> +/* { dg-begin-multiline-output "" }
>>>>>>> + coonst int ci = 0;
>>>>>>> + ^~~~~~
>>>>>>> + const
>>>>>>> +   { dg-end-multiline-output "" } */
>>>>>>> +
>>>>>>> +voltil int vi; // { dg-error "1: 'voltil' does not name a type; did
>>>>>>> you mean 'volatile'?" }
>>>>>>> +/* { dg-begin-multiline-output "" }
>>>>>>> + voltil int vi;
>>>>>>> + ^~~~~~
>>>>>>> + volatile
>>>>>>> +   { dg-end-multiline-output "" } */
>>>>>>> +
>>>>>>> +statik int si; // { dg-error "1: 'statik' does not name a type; did
>>>>>>> you mean 'static'?" }
>>>>>>> +/* { dg-begin-multiline-output "" }
>>>>>>> + statik int si;
>>>>>>> + ^~~~~~
>>>>>>> + static
>>>>>>> +   { dg-end-multiline-output "" } */
>>>>>>> ============================================
>>>>>>>
>>>>>>> --
>>>>>>> Regards,
>>>>>>> Michel
>>>>>>
>>>>>>
>>>>>
>>>>
>>