c++/modules: Improve diagnostic when redeclaring builtin in module [PR102345]

This is just a small improvement to a diagnostic.  I thought about also
adding an inform to suggest something like "standard library headers
should be included in the GMF" or somesuch, but I'm not sure that'll be
especially valuable and may be confusing if this particular error is
caused some other way.

Bootstrapped and regtested (so far just modules.exp) on
x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?

-- >8 --

If a user mistakenly includes a standard library header within the
module purview, they currently get a confusing "declaration conflicts
with builtin" error.  This patch updates the message to include "in
module", to help guide the user towards the likely cause.

gcc/cp/ChangeLog:

	* module.cc (module_may_redeclare): Update error message.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/enum-12.C: Test for updated error.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                       | 9 ++++++++-
 gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

On 5/24/24 11:20, Nathaniel Shead wrote:
> This is just a small improvement to a diagnostic.  I thought about also
> adding an inform to suggest something like "standard library headers
> should be included in the GMF" or somesuch, but I'm not sure that'll be
> especially valuable and may be confusing if this particular error is
> caused some other way.
> 
> Bootstrapped and regtested (so far just modules.exp) on
> x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> 
> -- >8 --
> 
> If a user mistakenly includes a standard library header within the
> module purview, they currently get a confusing "declaration conflicts
> with builtin" error.  This patch updates the message to include "in
> module", to help guide the user towards the likely cause.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (module_may_redeclare): Update error message.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/enum-12.C: Test for updated error.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                       | 9 ++++++++-
>   gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 6cd7d9e0b93..60cf3ebc468 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -19140,7 +19140,14 @@ module_may_redeclare (tree olddecl, tree newdecl)
>     decl = newdecl ? newdecl : olddecl;
>     location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
>     if (DECL_IS_UNDECLARED_BUILTIN (olddecl))
> -    error_at (loc, "declaration %qD conflicts with builtin", decl);
> +    {
> +      if (newdecl_attached_p)
> +	error_at (loc, "declaring %qD in module %qs conflicts with builtin",

Maybe "with builtin in global module"?

> +		  decl, new_mod->get_flatname ());
> +      else
> +	error_at (loc, "declaring %qD in global module conflicts with builtin",

I'm not sure mentioning the global module adds anything in this case?

Jason

On Fri, May 24, 2024 at 11:24:38AM -0400, Jason Merrill wrote:
> On 5/24/24 11:20, Nathaniel Shead wrote:
> > This is just a small improvement to a diagnostic.  I thought about also
> > adding an inform to suggest something like "standard library headers
> > should be included in the GMF" or somesuch, but I'm not sure that'll be
> > especially valuable and may be confusing if this particular error is
> > caused some other way.
> > 
> > Bootstrapped and regtested (so far just modules.exp) on
> > x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> > 
> > -- >8 --
> > 
> > If a user mistakenly includes a standard library header within the
> > module purview, they currently get a confusing "declaration conflicts
> > with builtin" error.  This patch updates the message to include "in
> > module", to help guide the user towards the likely cause.
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* module.cc (module_may_redeclare): Update error message.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/enum-12.C: Test for updated error.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >   gcc/cp/module.cc                       | 9 ++++++++-
> >   gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
> >   2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 6cd7d9e0b93..60cf3ebc468 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -19140,7 +19140,14 @@ module_may_redeclare (tree olddecl, tree newdecl)
> >     decl = newdecl ? newdecl : olddecl;
> >     location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
> >     if (DECL_IS_UNDECLARED_BUILTIN (olddecl))
> > -    error_at (loc, "declaration %qD conflicts with builtin", decl);
> > +    {
> > +      if (newdecl_attached_p)
> > +	error_at (loc, "declaring %qD in module %qs conflicts with builtin",
> 
> Maybe "with builtin in global module"?

Yup, that's even clearer, thanks.

> 
> > +		  decl, new_mod->get_flatname ());
> > +      else
> > +	error_at (loc, "declaring %qD in global module conflicts with builtin",
> 
> I'm not sure mentioning the global module adds anything in this case?
> 

I'd done it for consistency with the other cases in case we ever somehow
reached this code path, but happy to remove since this should always be
a problem regardless.

So maybe something like this?

-- >8 --

If a user mistakenly includes a standard library header within the
module purview, they currently get a confusing "declaration conflicts
with builtin" error.  This patch updates the message to include "in
module", to help guide the user towards the likely cause.

gcc/cp/ChangeLog:

	* module.cc (module_may_redeclare): Update error message.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/enum-12.C: Test for updated error.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                       | 8 +++++++-
 gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 6cd7d9e0b93..3f8f84bb9fd 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -19140,7 +19140,13 @@ module_may_redeclare (tree olddecl, tree newdecl)
   decl = newdecl ? newdecl : olddecl;
   location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
   if (DECL_IS_UNDECLARED_BUILTIN (olddecl))
-    error_at (loc, "declaration %qD conflicts with builtin", decl);
+    {
+      if (newdecl_attached_p)
+	error_at (loc, "declaring %qD in module %qs conflicts with builtin "
+		  "in global module", decl, new_mod->get_flatname ());
+      else
+	error_at (loc, "declaration %qD conflicts with builtin", decl);
+    }
   else if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P (old_inner))
     {
       auto_diagnostic_group d;
diff --git a/gcc/testsuite/g++.dg/modules/enum-12.C b/gcc/testsuite/g++.dg/modules/enum-12.C
index 064f220dedf..cf8f445e076 100644
--- a/gcc/testsuite/g++.dg/modules/enum-12.C
+++ b/gcc/testsuite/g++.dg/modules/enum-12.C
@@ -4,7 +4,7 @@
 
 export module foo;
 namespace std {
-  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "conflicts with builtin" }
+  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "in module .foo. conflicts with builtin" }
 }
 
 // { dg-prune-output "not writing module" }

On 5/24/24 11:32, Nathaniel Shead wrote:
> On Fri, May 24, 2024 at 11:24:38AM -0400, Jason Merrill wrote:
>> On 5/24/24 11:20, Nathaniel Shead wrote:
>>> This is just a small improvement to a diagnostic.  I thought about also
>>> adding an inform to suggest something like "standard library headers
>>> should be included in the GMF" or somesuch, but I'm not sure that'll be
>>> especially valuable and may be confusing if this particular error is
>>> caused some other way.
>>>
>>> Bootstrapped and regtested (so far just modules.exp) on
>>> x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
>>>
>>> -- >8 --
>>>
>>> If a user mistakenly includes a standard library header within the
>>> module purview, they currently get a confusing "declaration conflicts
>>> with builtin" error.  This patch updates the message to include "in
>>> module", to help guide the user towards the likely cause.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* module.cc (module_may_redeclare): Update error message.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/modules/enum-12.C: Test for updated error.
>>>
>>> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
>>> ---
>>>    gcc/cp/module.cc                       | 9 ++++++++-
>>>    gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
>>>    2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>> index 6cd7d9e0b93..60cf3ebc468 100644
>>> --- a/gcc/cp/module.cc
>>> +++ b/gcc/cp/module.cc
>>> @@ -19140,7 +19140,14 @@ module_may_redeclare (tree olddecl, tree newdecl)
>>>      decl = newdecl ? newdecl : olddecl;
>>>      location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
>>>      if (DECL_IS_UNDECLARED_BUILTIN (olddecl))
>>> -    error_at (loc, "declaration %qD conflicts with builtin", decl);
>>> +    {
>>> +      if (newdecl_attached_p)
>>> +	error_at (loc, "declaring %qD in module %qs conflicts with builtin",
>>
>> Maybe "with builtin in global module"?
> 
> Yup, that's even clearer, thanks.
> 
>>
>>> +		  decl, new_mod->get_flatname ());
>>> +      else
>>> +	error_at (loc, "declaring %qD in global module conflicts with builtin",
>>
>> I'm not sure mentioning the global module adds anything in this case?
>>
> 
> I'd done it for consistency with the other cases in case we ever somehow
> reached this code path, but happy to remove since this should always be
> a problem regardless.
> 
> So maybe something like this?

OK.

> -- >8 --
> 
> If a user mistakenly includes a standard library header within the
> module purview, they currently get a confusing "declaration conflicts
> with builtin" error.  This patch updates the message to include "in
> module", to help guide the user towards the likely cause.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (module_may_redeclare): Update error message.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/enum-12.C: Test for updated error.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                       | 8 +++++++-
>   gcc/testsuite/g++.dg/modules/enum-12.C | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 6cd7d9e0b93..3f8f84bb9fd 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -19140,7 +19140,13 @@ module_may_redeclare (tree olddecl, tree newdecl)
>     decl = newdecl ? newdecl : olddecl;
>     location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
>     if (DECL_IS_UNDECLARED_BUILTIN (olddecl))
> -    error_at (loc, "declaration %qD conflicts with builtin", decl);
> +    {
> +      if (newdecl_attached_p)
> +	error_at (loc, "declaring %qD in module %qs conflicts with builtin "
> +		  "in global module", decl, new_mod->get_flatname ());
> +      else
> +	error_at (loc, "declaration %qD conflicts with builtin", decl);
> +    }
>     else if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P (old_inner))
>       {
>         auto_diagnostic_group d;
> diff --git a/gcc/testsuite/g++.dg/modules/enum-12.C b/gcc/testsuite/g++.dg/modules/enum-12.C
> index 064f220dedf..cf8f445e076 100644
> --- a/gcc/testsuite/g++.dg/modules/enum-12.C
> +++ b/gcc/testsuite/g++.dg/modules/enum-12.C
> @@ -4,7 +4,7 @@
>   
>   export module foo;
>   namespace std {
> -  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "conflicts with builtin" }
> +  enum class align_val_t : decltype(sizeof(int)) {};  // { dg-error "in module .foo. conflicts with builtin" }
>   }
>   
>   // { dg-prune-output "not writing module" }