[PING for gcc 8] Re: [PATCH] Fix spelling suggestions for reserved words (PR c++/80177)

[David Malcolm]

Ping for gcc 8.

On Fri, 2017-03-31 at 12:41 -0400, David Malcolm wrote:
> As noted in the PR, the C++ frontend currently offers a poor
> suggestion for this misspelling:
> 
> a.C: In function ‘void f()’:
> a.C:3:3: error: ‘static_assertion’ was not declared in this scope
>    static_assertion (1 == 0, "1 == 0");
>    ^~~~~~~~~~~~~~~~
> a.C:3:3: note: suggested alternative: ‘__cpp_static_assert’
>    static_assertion (1 == 0, "1 == 0");
>    ^~~~~~~~~~~~~~~~
>    __cpp_static_assert
> 
> when it ought to offer "static_assert" as a suggestion instead.
> 
> The root causes are two issues within lookup_name_fuzzy
> (called here with FUZZY_LOOKUP_NAME):
> 
> (a) If it finds a good enough match in the preprocessor it will
>     return the best match *before* considering reserved words,
>     rather than picking the closest match overall.
> 
>     The fix is to have merge all the results into one best_match
>     instance, and pick the overall winner.  However, given that
>     some candidates are identifiers (trees), and others are cpp
>     macros, the best_match instance's candidate type needs to
>     be converted from tree to const char *.  This has some minor
>     knock-on effects within name-lookup.c.  Sadly it means some
>     extra calls to strlen (one per candidate), but this will be
>     purely when error-handling.
> 
> (b) It rejects "static_assert" here:
> 
>       4998    if (!cp_keyword_starts_decl_specifier_p (resword->rid))
>       4999      continue;
> 
>     as "static_assert" doesn't start a decl specifier.
> 
>     The fix is to only apply this rejection criterion if we're
> looking
>     for typenames, rather than for names in general.
> 
> This patch addresses both issues and adds test coverage.
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> Adds 7 PASS and 1 UNSUPPORTED (for -std=c++98) to g++.sum
> 
> OK for next stage 1?
> 
> gcc/cp/ChangeLog:
> 	PR c++/80177
> 	* name-lookup.c (suggest_alternative_in_explicit_scope):
> Convert
> 	candidate type of bm from tree to const char *.
> 	(consider_binding_level): Likewise.
> 	(lookup_name_fuzzy): Likewise, using this to merge the best
> 	result from the preprocessor into bm, rather than immediately
> 	returning, so that better matches from reserved words can
> "win".
> 	Guard the rejection of keywords that don't start decl
> -specifiers
> 	so it only happens for FUZZY_LOOKUP_TYPENAME.
> 
> gcc/testsuite/ChangeLog:
> 	PR c++/80177
> 	* g++.dg/spellcheck-pr80177.C: New test case.
> ---
>  gcc/cp/name-lookup.c                      | 37 +++++++++++++--------
> ----------
>  gcc/testsuite/g++.dg/spellcheck-pr80177.C |  7 ++++++
>  2 files changed, 23 insertions(+), 21 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/spellcheck-pr80177.C
> 
> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
> index 994f7f0..16ec0a1 100644
> --- a/gcc/cp/name-lookup.c
> +++ b/gcc/cp/name-lookup.c
> @@ -48,7 +48,8 @@ static bool lookup_using_namespace (tree, struct
> scope_binding *, tree,
>  				    tree, int);
>  static bool qualified_lookup_using_namespace (tree, tree,
>  					      struct scope_binding
> *, int);
> -static void consider_binding_level (tree name, best_match <tree,
> tree> &bm,
> +static void consider_binding_level (tree name,
> +				    best_match <tree, const char *>
> &bm,
>  				    cp_binding_level *lvl,
>  				    bool look_within_fields,
>  				    enum lookup_name_fuzzy_kind
> kind);
> @@ -4550,14 +4551,13 @@ suggest_alternative_in_explicit_scope
> (location_t location, tree name,
>  
>    cp_binding_level *level = NAMESPACE_LEVEL (scope);
>  
> -  best_match <tree, tree> bm (name);
> +  best_match <tree, const char *> bm (name);
>    consider_binding_level (name, bm, level, false,
> FUZZY_LOOKUP_NAME);
>  
>    /* See if we have a good suggesion for the user.  */
> -  tree best_id = bm.get_best_meaningful_candidate ();
> -  if (best_id)
> +  const char *fuzzy_name = bm.get_best_meaningful_candidate ();
> +  if (fuzzy_name)
>      {
> -      const char *fuzzy_name = IDENTIFIER_POINTER (best_id);
>        gcc_rich_location richloc (location);
>        richloc.add_fixit_replace (fuzzy_name);
>        inform_at_rich_loc (&richloc, "suggested alternative: %qs",
> @@ -4797,7 +4797,7 @@ qualified_lookup_using_namespace (tree name,
> tree scope,
>     Traverse binding level LVL, looking for good name matches for
> NAME
>     (and BM).  */
>  static void
> -consider_binding_level (tree name, best_match <tree, tree> &bm,
> +consider_binding_level (tree name, best_match <tree, const char *>
> &bm,
>  			cp_binding_level *lvl, bool
> look_within_fields,
>  			enum lookup_name_fuzzy_kind kind)
>  {
> @@ -4809,7 +4809,7 @@ consider_binding_level (tree name, best_match
> <tree, tree> &bm,
>  	tree best_matching_field
>  	  = lookup_member_fuzzy (type, name, want_type_p);
>  	if (best_matching_field)
> -	  bm.consider (best_matching_field);
> +	  bm.consider (IDENTIFIER_POINTER (best_matching_field));
>        }
>  
>    for (tree t = lvl->names; t; t = TREE_CHAIN (t))
> @@ -4838,7 +4838,7 @@ consider_binding_level (tree name, best_match
> <tree, tree> &bm,
>        if (tree name = DECL_NAME (d))
>  	/* Ignore internal names with spaces in them.  */
>  	if (!strchr (IDENTIFIER_POINTER (name), ' '))
> -	  bm.consider (DECL_NAME (d));
> +	  bm.consider (IDENTIFIER_POINTER (name));
>      }
>  }
>  
> @@ -4851,7 +4851,7 @@ lookup_name_fuzzy (tree name, enum
> lookup_name_fuzzy_kind kind)
>  {
>    gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
>  
> -  best_match <tree, tree> bm (name);
> +  best_match <tree, const char *> bm (name);
>  
>    cp_binding_level *lvl;
>    for (lvl = scope_chain->class_bindings; lvl; lvl = lvl
> ->level_chain)
> @@ -4874,9 +4874,9 @@ lookup_name_fuzzy (tree name, enum
> lookup_name_fuzzy_kind kind)
>       the identifiers already checked.  */
>    best_macro_match bmm (name, bm.get_best_distance (), parse_in);
>    cpp_hashnode *best_macro = bmm.get_best_meaningful_candidate ();
> -  /* If a macro is the closest so far to NAME, suggest it.  */
> +  /* If a macro is the closest so far to NAME, consider it.  */
>    if (best_macro)
> -    return (const char *)best_macro->ident.str;
> +    bm.consider ((const char *)best_macro->ident.str);
>  
>    /* Try the "starts_decl_specifier_p" keywords to detect
>       "singed" vs "signed" typos.  */
> @@ -4884,8 +4884,9 @@ lookup_name_fuzzy (tree name, enum
> lookup_name_fuzzy_kind kind)
>      {
>        const c_common_resword *resword = &c_common_reswords[i];
>  
> -      if (!cp_keyword_starts_decl_specifier_p (resword->rid))
> -	continue;
> +      if (kind == FUZZY_LOOKUP_TYPENAME)
> +	if (!cp_keyword_starts_decl_specifier_p (resword->rid))
> +	  continue;
>  
>        tree resword_identifier = ridpointers [resword->rid];
>        if (!resword_identifier)
> @@ -4897,16 +4898,10 @@ lookup_name_fuzzy (tree name, enum
> lookup_name_fuzzy_kind kind)
>        if (!C_IS_RESERVED_WORD (resword_identifier))
>  	continue;
>  
> -      bm.consider (resword_identifier);
> +      bm.consider (IDENTIFIER_POINTER (resword_identifier));
>      }
>  
> -  /* See if we have a good suggesion for the user.  */
> -  tree best_id = bm.get_best_meaningful_candidate ();
> -  if (best_id)
> -    return IDENTIFIER_POINTER (best_id);
> -
> -  /* No meaningful suggestion available.  */
> -  return NULL;
> +  return bm.get_best_meaningful_candidate ();
>  }
>  
>  /* Subroutine of outer_binding.
> diff --git a/gcc/testsuite/g++.dg/spellcheck-pr80177.C
> b/gcc/testsuite/g++.dg/spellcheck-pr80177.C
> new file mode 100644
> index 0000000..2ff24e8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/spellcheck-pr80177.C
> @@ -0,0 +1,7 @@
> +// { dg-do compile { target c++11 } }
> +
> +void pr80177 ()
> +{
> +  static_assertion (1 == 0, "1 == 0"); // { dg-error "3:
> 'static_assertion' was not declared in this scope" }
> +  // { dg-message "3: suggested alternative: 'static_assert'" "" {
> target *-*-* } .-1 }
> +}