C++: fix ordering of missing std #include suggestion (PR c++/81514)

PR c++/81514 reports a problem where
  g++.dg/lookup/missing-std-include-2.C
fails on Solaris, offering the suggestion:

  error: 'string' is not a member of 'std'
  note: suggested alternative: 'sprintf'

instead of the expected:

  error: 'string' is not a member of 'std'
  note: 'std::string' is defined in header '<string>'; did you forget to '#include <string>'?

This is after a:
  #include <stdio.h>

suggest_alternative_in_explicit_scope currently works in two phases:

(a) it attempts to look for misspellings within the explicitly-given
    namespace and suggests the best it finds

(b) failing that, it then looks for well-known "std::"
    names and suggests a missing header

This now seems the wrong way round to me; if the user has
typed "std::string", a missing #include <string> seems more helpful
as a suggestion than attempting to look for misspellings.

This patch reverses the ordering of (a) and (b) above, so that
missing header hints for well-known std:: names are offered first,
only then falling back to misspelling hints.

The problem doesn't show up on my x86_64-pc-linux-gnu box, as
the pertinent part of the #include <stdio.h> appears to be
equivalent to:

  extern int sprintf (char *dst, const char *format, ...);
  namespace std
  {
    using ::sprintf;
  }

The "std::sprintf" thus examined within consider_binding_level
is the same tree node as ::sprintf, and is rejected by:

      /* Skip anticipated decls of builtin functions.  */
      if (TREE_CODE (d) == FUNCTION_DECL
          && DECL_BUILT_IN (d)
          && DECL_ANTICIPATED (d))
        continue;

and so the name "sprintf" is never considered as a spell-correction
for std::"string".

Hence we're not issuing spelling corrections for aliases
within a namespace for builtins from the global namespace;
these are pre-created by cxx_builtin_function, which has:

4397	  /* All builtins that don't begin with an '_' should additionally
4398	     go in the 'std' namespace.  */
4399	  if (name[0] != '_')
4400	    {
4401	      tree decl2 = copy_node(decl);
4402	      push_namespace (std_identifier);
4403	      builtin_function_1 (decl2, std_node, false);
4404	      pop_namespace ();
4405	    }

I'm not sure why Solaris' decl of std::sprintf doesn't hit the
reject path above.

I was able to reproduce the behavior seen on Solaris on my Fedora
box by using this:

  namespace std
  {
    extern int sprintf (char *dst, const char *format, ...);
  }

which isn't rejected by the "Skip anticipated decls of builtin
functions" test above, and hence sprintf is erroneously offered
 as a suggestion.

The patch reworks the test case to work in the above way,
to trigger the problem on Linux, and then fixes it by
changing the order that the suggestions are tried in
name-lookup.c.  It introduces an "empty.h" since the testcase
is also to verify that we suggest a good location for new #include
directives relative to pre-existing #include directives.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/cp/ChangeLog:
	PR c++/81514
	* name-lookup.c (maybe_suggest_missing_header): Convert return
	type from void to bool; return true iff a suggestion was offered.
	(suggest_alternative_in_explicit_scope): Move call to
	maybe_suggest_missing_header to before use of best_match, and
	return true if the former offers a suggestion.

gcc/testsuite/ChangeLog:
	PR c++/81514
	* g++.dg/lookup/empty.h: New file.
	* g++.dg/lookup/missing-std-include-2.C: Replace include of
	stdio.h with empty.h and a declaration of a "std::sprintf" not based
	on a built-in.
---
 gcc/cp/name-lookup.c                               | 39 +++++++++++-----------
 gcc/testsuite/g++.dg/lookup/empty.h                |  1 +
 .../g++.dg/lookup/missing-std-include-2.C          | 11 ++++--
 3 files changed, 29 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/lookup/empty.h

> I'm not sure why Solaris' decl of std::sprintf doesn't hit the
> reject path above.
>
> I was able to reproduce the behavior seen on Solaris on my Fedora
> box by using this:
>
>   namespace std
>   {
>     extern int sprintf (char *dst, const char *format, ...);
>   }

This is how C library symbols were intended to be declared
in the C++ <cxxx> headers (plus extern "C" that GCC doesn't
implement).  Very few systems went to the trouble to make
those changes.  Solaris was one of them.

Martin

Ping

On Thu, 2017-07-27 at 17:36 -0400, David Malcolm wrote:
> PR c++/81514 reports a problem where
>   g++.dg/lookup/missing-std-include-2.C
> fails on Solaris, offering the suggestion:
> 
>   error: 'string' is not a member of 'std'
>   note: suggested alternative: 'sprintf'
> 
> instead of the expected:
> 
>   error: 'string' is not a member of 'std'
>   note: 'std::string' is defined in header '<string>'; did you forget
> to '#include <string>'?
> 
> This is after a:
>   #include <stdio.h>
> 
> suggest_alternative_in_explicit_scope currently works in two phases:
> 
> (a) it attempts to look for misspellings within the explicitly-given
>     namespace and suggests the best it finds
> 
> (b) failing that, it then looks for well-known "std::"
>     names and suggests a missing header
> 
> This now seems the wrong way round to me; if the user has
> typed "std::string", a missing #include <string> seems more helpful
> as a suggestion than attempting to look for misspellings.
> 
> This patch reverses the ordering of (a) and (b) above, so that
> missing header hints for well-known std:: names are offered first,
> only then falling back to misspelling hints.
> 
> The problem doesn't show up on my x86_64-pc-linux-gnu box, as
> the pertinent part of the #include <stdio.h> appears to be
> equivalent to:
> 
>   extern int sprintf (char *dst, const char *format, ...);
>   namespace std
>   {
>     using ::sprintf;
>   }
> 
> The "std::sprintf" thus examined within consider_binding_level
> is the same tree node as ::sprintf, and is rejected by:
> 
>       /* Skip anticipated decls of builtin functions.  */
>       if (TREE_CODE (d) == FUNCTION_DECL
>           && DECL_BUILT_IN (d)
>           && DECL_ANTICIPATED (d))
>         continue;
> 
> and so the name "sprintf" is never considered as a spell-correction
> for std::"string".
> 
> Hence we're not issuing spelling corrections for aliases
> within a namespace for builtins from the global namespace;
> these are pre-created by cxx_builtin_function, which has:
> 
> 4397	  /* All builtins that don't begin with an '_' should
> additionally
> 4398	     go in the 'std' namespace.  */
> 4399	  if (name[0] != '_')
> 4400	    {
> 4401	      tree decl2 = copy_node(decl);
> 4402	      push_namespace (std_identifier);
> 4403	      builtin_function_1 (decl2, std_node, false);
> 4404	      pop_namespace ();
> 4405	    }
> 
> I'm not sure why Solaris' decl of std::sprintf doesn't hit the
> reject path above.
> 
> I was able to reproduce the behavior seen on Solaris on my Fedora
> box by using this:
> 
>   namespace std
>   {
>     extern int sprintf (char *dst, const char *format, ...);
>   }
> 
> which isn't rejected by the "Skip anticipated decls of builtin
> functions" test above, and hence sprintf is erroneously offered
>  as a suggestion.
> 
> The patch reworks the test case to work in the above way,
> to trigger the problem on Linux, and then fixes it by
> changing the order that the suggestions are tried in
> name-lookup.c.  It introduces an "empty.h" since the testcase
> is also to verify that we suggest a good location for new #include
> directives relative to pre-existing #include directives.
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/cp/ChangeLog:
> 	PR c++/81514
> 	* name-lookup.c (maybe_suggest_missing_header): Convert return
> 	type from void to bool; return true iff a suggestion was
> offered.
> 	(suggest_alternative_in_explicit_scope): Move call to
> 	maybe_suggest_missing_header to before use of best_match, and
> 	return true if the former offers a suggestion.
> 
> gcc/testsuite/ChangeLog:
> 	PR c++/81514
> 	* g++.dg/lookup/empty.h: New file.
> 	* g++.dg/lookup/missing-std-include-2.C: Replace include of
> 	stdio.h with empty.h and a declaration of a "std::sprintf" not
> based
> 	on a built-in.
> ---
>  gcc/cp/name-lookup.c                               | 39 +++++++++++-
> ----------
>  gcc/testsuite/g++.dg/lookup/empty.h                |  1 +
>  .../g++.dg/lookup/missing-std-include-2.C          | 11 ++++--
>  3 files changed, 29 insertions(+), 22 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/lookup/empty.h
> 
> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
> index cd7428a..49c4dea 100644
> --- a/gcc/cp/name-lookup.c
> +++ b/gcc/cp/name-lookup.c
> @@ -4838,34 +4838,34 @@ get_std_name_hint (const char *name)
>    return NULL;
>  }
>  
> -/* Subroutine of suggest_alternative_in_explicit_scope, for use when
> we have no
> -   suggestions to offer.
> -   If SCOPE is the "std" namespace, then suggest pertinent header
> -   files for NAME.  */
> +/* If SCOPE is the "std" namespace, then suggest pertinent header
> +   files for NAME at LOCATION.
> +   Return true iff a suggestion was offered.  */
>  
> -static void
> +static bool
>  maybe_suggest_missing_header (location_t location, tree name, tree
> scope)
>  {
>    if (scope == NULL_TREE)
> -    return;
> +    return false;
>    if (TREE_CODE (scope) != NAMESPACE_DECL)
> -    return;
> +    return false;
>    /* We only offer suggestions for the "std" namespace.  */
>    if (scope != std_node)
> -    return;
> +    return false;
>    gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
>  
>    const char *name_str = IDENTIFIER_POINTER (name);
>    const char *header_hint = get_std_name_hint (name_str);
> -  if (header_hint)
> -    {
> -      gcc_rich_location richloc (location);
> -      maybe_add_include_fixit (&richloc, header_hint);
> -      inform_at_rich_loc (&richloc,
> -			  "%<std::%s%> is defined in header %qs;"
> -			  " did you forget to %<#include %s%>?",
> -			  name_str, header_hint, header_hint);
> -    }
> +  if (!header_hint)
> +    return false;
> +
> +  gcc_rich_location richloc (location);
> +  maybe_add_include_fixit (&richloc, header_hint);
> +  inform_at_rich_loc (&richloc,
> +		      "%<std::%s%> is defined in header %qs;"
> +		      " did you forget to %<#include %s%>?",
> +		      name_str, header_hint, header_hint);
> +  return true;
>  }
>  
>  /* Look for alternatives for NAME, an IDENTIFIER_NODE for which name
> @@ -4880,6 +4880,9 @@ suggest_alternative_in_explicit_scope
> (location_t location, tree name,
>    /* Resolve any namespace aliases.  */
>    scope = ORIGINAL_NAMESPACE (scope);
>  
> +  if (maybe_suggest_missing_header (location, name, scope))
> +    return true;
> +
>    cp_binding_level *level = NAMESPACE_LEVEL (scope);
>  
>    best_match <tree, const char *> bm (name);
> @@ -4895,8 +4898,6 @@ suggest_alternative_in_explicit_scope
> (location_t location, tree name,
>  			  fuzzy_name);
>        return true;
>      }
> -  else
> -    maybe_suggest_missing_header (location, name, scope);
>  
>    return false;
>  }
> diff --git a/gcc/testsuite/g++.dg/lookup/empty.h
> b/gcc/testsuite/g++.dg/lookup/empty.h
> new file mode 100644
> index 0000000..a057418
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lookup/empty.h
> @@ -0,0 +1 @@
> +/* empty file for use by missing-std-include-2.C.  */
> diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
> b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
> index ae918f8..51c604a 100644
> --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
> +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
> @@ -6,7 +6,12 @@
>  /* This is padding (to avoid the generated patch containing DejaGnu
>     directives).  */
>  
> -#include <stdio.h>
> +#include "empty.h"
> +
> +namespace std
> +{
> +  extern int sprintf (char *dst, const char *format, ...);
> +};
>  
>  void test (void)
>  {
> @@ -45,11 +50,11 @@ void test_2 (void)
>  @@ -7,6 +7,8 @@
>      directives).  */
>   
> - #include <stdio.h>
> + #include "empty.h"
>  +#include <string>
>  +#include <iostream>
>   
> - void test (void)
> + namespace std
>   {
>  { dg-end-multiline-output "" }
>  #endif

OK.

On Thu, Aug 17, 2017 at 7:50 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Ping
>
> On Thu, 2017-07-27 at 17:36 -0400, David Malcolm wrote:
>> PR c++/81514 reports a problem where
>>   g++.dg/lookup/missing-std-include-2.C
>> fails on Solaris, offering the suggestion:
>>
>>   error: 'string' is not a member of 'std'
>>   note: suggested alternative: 'sprintf'
>>
>> instead of the expected:
>>
>>   error: 'string' is not a member of 'std'
>>   note: 'std::string' is defined in header '<string>'; did you forget
>> to '#include <string>'?
>>
>> This is after a:
>>   #include <stdio.h>
>>
>> suggest_alternative_in_explicit_scope currently works in two phases:
>>
>> (a) it attempts to look for misspellings within the explicitly-given
>>     namespace and suggests the best it finds
>>
>> (b) failing that, it then looks for well-known "std::"
>>     names and suggests a missing header
>>
>> This now seems the wrong way round to me; if the user has
>> typed "std::string", a missing #include <string> seems more helpful
>> as a suggestion than attempting to look for misspellings.
>>
>> This patch reverses the ordering of (a) and (b) above, so that
>> missing header hints for well-known std:: names are offered first,
>> only then falling back to misspelling hints.
>>
>> The problem doesn't show up on my x86_64-pc-linux-gnu box, as
>> the pertinent part of the #include <stdio.h> appears to be
>> equivalent to:
>>
>>   extern int sprintf (char *dst, const char *format, ...);
>>   namespace std
>>   {
>>     using ::sprintf;
>>   }
>>
>> The "std::sprintf" thus examined within consider_binding_level
>> is the same tree node as ::sprintf, and is rejected by:
>>
>>       /* Skip anticipated decls of builtin functions.  */
>>       if (TREE_CODE (d) == FUNCTION_DECL
>>           && DECL_BUILT_IN (d)
>>           && DECL_ANTICIPATED (d))
>>         continue;
>>
>> and so the name "sprintf" is never considered as a spell-correction
>> for std::"string".
>>
>> Hence we're not issuing spelling corrections for aliases
>> within a namespace for builtins from the global namespace;
>> these are pre-created by cxx_builtin_function, which has:
>>
>> 4397    /* All builtins that don't begin with an '_' should
>> additionally
>> 4398       go in the 'std' namespace.  */
>> 4399    if (name[0] != '_')
>> 4400      {
>> 4401        tree decl2 = copy_node(decl);
>> 4402        push_namespace (std_identifier);
>> 4403        builtin_function_1 (decl2, std_node, false);
>> 4404        pop_namespace ();
>> 4405      }
>>
>> I'm not sure why Solaris' decl of std::sprintf doesn't hit the
>> reject path above.
>>
>> I was able to reproduce the behavior seen on Solaris on my Fedora
>> box by using this:
>>
>>   namespace std
>>   {
>>     extern int sprintf (char *dst, const char *format, ...);
>>   }
>>
>> which isn't rejected by the "Skip anticipated decls of builtin
>> functions" test above, and hence sprintf is erroneously offered
>>  as a suggestion.
>>
>> The patch reworks the test case to work in the above way,
>> to trigger the problem on Linux, and then fixes it by
>> changing the order that the suggestions are tried in
>> name-lookup.c.  It introduces an "empty.h" since the testcase
>> is also to verify that we suggest a good location for new #include
>> directives relative to pre-existing #include directives.
>>
>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>>
>> OK for trunk?
>>
>> gcc/cp/ChangeLog:
>>       PR c++/81514
>>       * name-lookup.c (maybe_suggest_missing_header): Convert return
>>       type from void to bool; return true iff a suggestion was
>> offered.
>>       (suggest_alternative_in_explicit_scope): Move call to
>>       maybe_suggest_missing_header to before use of best_match, and
>>       return true if the former offers a suggestion.
>>
>> gcc/testsuite/ChangeLog:
>>       PR c++/81514
>>       * g++.dg/lookup/empty.h: New file.
>>       * g++.dg/lookup/missing-std-include-2.C: Replace include of
>>       stdio.h with empty.h and a declaration of a "std::sprintf" not
>> based
>>       on a built-in.
>> ---
>>  gcc/cp/name-lookup.c                               | 39 +++++++++++-
>> ----------
>>  gcc/testsuite/g++.dg/lookup/empty.h                |  1 +
>>  .../g++.dg/lookup/missing-std-include-2.C          | 11 ++++--
>>  3 files changed, 29 insertions(+), 22 deletions(-)
>>  create mode 100644 gcc/testsuite/g++.dg/lookup/empty.h
>>
>> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
>> index cd7428a..49c4dea 100644
>> --- a/gcc/cp/name-lookup.c
>> +++ b/gcc/cp/name-lookup.c
>> @@ -4838,34 +4838,34 @@ get_std_name_hint (const char *name)
>>    return NULL;
>>  }
>>
>> -/* Subroutine of suggest_alternative_in_explicit_scope, for use when
>> we have no
>> -   suggestions to offer.
>> -   If SCOPE is the "std" namespace, then suggest pertinent header
>> -   files for NAME.  */
>> +/* If SCOPE is the "std" namespace, then suggest pertinent header
>> +   files for NAME at LOCATION.
>> +   Return true iff a suggestion was offered.  */
>>
>> -static void
>> +static bool
>>  maybe_suggest_missing_header (location_t location, tree name, tree
>> scope)
>>  {
>>    if (scope == NULL_TREE)
>> -    return;
>> +    return false;
>>    if (TREE_CODE (scope) != NAMESPACE_DECL)
>> -    return;
>> +    return false;
>>    /* We only offer suggestions for the "std" namespace.  */
>>    if (scope != std_node)
>> -    return;
>> +    return false;
>>    gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
>>
>>    const char *name_str = IDENTIFIER_POINTER (name);
>>    const char *header_hint = get_std_name_hint (name_str);
>> -  if (header_hint)
>> -    {
>> -      gcc_rich_location richloc (location);
>> -      maybe_add_include_fixit (&richloc, header_hint);
>> -      inform_at_rich_loc (&richloc,
>> -                       "%<std::%s%> is defined in header %qs;"
>> -                       " did you forget to %<#include %s%>?",
>> -                       name_str, header_hint, header_hint);
>> -    }
>> +  if (!header_hint)
>> +    return false;
>> +
>> +  gcc_rich_location richloc (location);
>> +  maybe_add_include_fixit (&richloc, header_hint);
>> +  inform_at_rich_loc (&richloc,
>> +                   "%<std::%s%> is defined in header %qs;"
>> +                   " did you forget to %<#include %s%>?",
>> +                   name_str, header_hint, header_hint);
>> +  return true;
>>  }
>>
>>  /* Look for alternatives for NAME, an IDENTIFIER_NODE for which name
>> @@ -4880,6 +4880,9 @@ suggest_alternative_in_explicit_scope
>> (location_t location, tree name,
>>    /* Resolve any namespace aliases.  */
>>    scope = ORIGINAL_NAMESPACE (scope);
>>
>> +  if (maybe_suggest_missing_header (location, name, scope))
>> +    return true;
>> +
>>    cp_binding_level *level = NAMESPACE_LEVEL (scope);
>>
>>    best_match <tree, const char *> bm (name);
>> @@ -4895,8 +4898,6 @@ suggest_alternative_in_explicit_scope
>> (location_t location, tree name,
>>                         fuzzy_name);
>>        return true;
>>      }
>> -  else
>> -    maybe_suggest_missing_header (location, name, scope);
>>
>>    return false;
>>  }
>> diff --git a/gcc/testsuite/g++.dg/lookup/empty.h
>> b/gcc/testsuite/g++.dg/lookup/empty.h
>> new file mode 100644
>> index 0000000..a057418
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/lookup/empty.h
>> @@ -0,0 +1 @@
>> +/* empty file for use by missing-std-include-2.C.  */
>> diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
>> b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
>> index ae918f8..51c604a 100644
>> --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
>> +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C
>> @@ -6,7 +6,12 @@
>>  /* This is padding (to avoid the generated patch containing DejaGnu
>>     directives).  */
>>
>> -#include <stdio.h>
>> +#include "empty.h"
>> +
>> +namespace std
>> +{
>> +  extern int sprintf (char *dst, const char *format, ...);
>> +};
>>
>>  void test (void)
>>  {
>> @@ -45,11 +50,11 @@ void test_2 (void)
>>  @@ -7,6 +7,8 @@
>>      directives).  */
>>
>> - #include <stdio.h>
>> + #include "empty.h"
>>  +#include <string>
>>  +#include <iostream>
>>
>> - void test (void)
>> + namespace std
>>   {
>>  { dg-end-multiline-output "" }
>>  #endif