c++/modules: Improve errors for bad module-directives [PR115200]

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

-- >8 --

This fixes an ICE when a module directive is not given at global scope.
Although not explicitly mentioned, it seems implied from [basic.link] p1
and [module.global.frag] that a module-declaration must appear at the
global scope after preprocessing.  Apart from this the patch also
slightly improves the errors given when accidentally using a module
control-line in other situations where it is not expected.

	PR c++/115200

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_error_1): Special-case unexpected module
	directives for better diagnostics.
	(cp_parser_module_declaration): Check that the module
	declaration is at global scope.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/mod-decl-1.C: Update error messages.
	* g++.dg/modules/mod-decl-6.C: New test.
	* g++.dg/modules/mod-decl-7.C: New test.
	* g++.dg/modules/mod-decl-8.C: New test.
	* g++.dg/modules/mod-decl-8.h: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/parser.cc                          | 32 +++++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/mod-decl-1.C |  6 ++---
 gcc/testsuite/g++.dg/modules/mod-decl-6.C | 11 ++++++++
 gcc/testsuite/g++.dg/modules/mod-decl-7.C | 12 +++++++++
 gcc/testsuite/g++.dg/modules/mod-decl-8.C |  9 +++++++
 gcc/testsuite/g++.dg/modules/mod-decl-8.h |  4 +++
 6 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-6.C
 create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-7.C
 create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-8.C
 create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-8.h

On 5/23/24 10:54, Nathaniel Shead wrote:
> Bootstrapped and regtested (so far just modules.exp and dg.exp) on
> x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> 
> -- >8 --
> 
> This fixes an ICE when a module directive is not given at global scope.
> Although not explicitly mentioned, it seems implied from [basic.link] p1
> and [module.global.frag] that a module-declaration must appear at the
> global scope after preprocessing.  Apart from this the patch also
> slightly improves the errors given when accidentally using a module
> control-line in other situations where it is not expected.

This could also come up with something like

int module;
int i =
   module; // error, unexpected module directive

Adding a line break seems like confusing advice for this problem; 
rather, they need to remove the line break before 'module'.  And 
possibly add it in somewhere else, but the problem is that 'module' is 
the first token on the line.  And if I put that in a namespace,

namespace A {
   int module;
   int i =
     module; // error, unexpected module directive
}

the problem is the same, but we get a different diagnostic.

I think I'd leave the "must be at global scope" diagnostic to 
cp_parser_module_declaration, and assume that if we see a module keyword 
at function scope it wasn't intended to be a module directive.

> 	PR c++/115200
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.cc (cp_parser_error_1): Special-case unexpected module
> 	directives for better diagnostics.
> 	(cp_parser_module_declaration): Check that the module
> 	declaration is at global scope.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/mod-decl-1.C: Update error messages.
> 	* g++.dg/modules/mod-decl-6.C: New test.
> 	* g++.dg/modules/mod-decl-7.C: New test.
> 	* g++.dg/modules/mod-decl-8.C: New test.
> 	* g++.dg/modules/mod-decl-8.h: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/parser.cc                          | 32 +++++++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-1.C |  6 ++---
>   gcc/testsuite/g++.dg/modules/mod-decl-6.C | 11 ++++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-7.C | 12 +++++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-8.C |  9 +++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-8.h |  4 +++
>   6 files changed, 71 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-6.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-7.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-8.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/mod-decl-8.h
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 476ddc0d63a..1c0543ba154 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -3230,6 +3230,31 @@ cp_parser_error_1 (cp_parser* parser, const char* gmsgid,
>         return;
>       }
>   
> +  if (cp_token_is_module_directive (token))
> +    {
> +      cp_token *next = (token->keyword == RID__EXPORT
> +			? cp_lexer_peek_nth_token (parser->lexer, 2) : token);
> +
> +      auto_diagnostic_group d;
> +      error_at (token->location, "unexpected module directive");
> +      tree scope = current_scope ();
> +      if (next->keyword == RID__MODULE
> +	  && token->main_source_p
> +	  && scope != global_namespace)
> +	{
> +	  /* Nicer error for unterminated scopes in GMF includes.  */
> +	  inform (token->location,
> +		  "module-declaration must be at global scope");
> +	  inform (location_of (scope), "scope opened here");
> +	}
> +      else
> +	inform (token->location, "perhaps insert a line break, or other"
> +		" disambiguation, to prevent this being considered a"
> +		" module control-line");
> +      cp_parser_skip_to_pragma_eol (parser, token);
> +      return;
> +    }
> +
>     /* If this is actually a conflict marker, report it as such.  */
>     if (token->type == CPP_LSHIFT
>         || token->type == CPP_RSHIFT
> @@ -15135,12 +15160,19 @@ cp_parser_module_declaration (cp_parser *parser, module_parse mp_state,
>     parser->lexer->in_pragma = true;
>     cp_token *token = cp_lexer_consume_token (parser->lexer);
>   
> +  tree scope = current_scope ();
>     if (flag_header_unit)
>       {
>         error_at (token->location,
>   		"module-declaration not permitted in header-unit");
>         goto skip_eol;
>       }
> +  else if (scope != global_namespace)
> +    {
> +      error_at (token->location, "module-declaration must be at global scope");
> +      inform (DECL_SOURCE_LOCATION (scope), "scope opened here");
> +      goto skip_eol;
> +    }
>     else if (mp_state == MP_FIRST && !exporting
>         && cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
>       {
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-1.C b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> index 23d34483dd7..84fa31c7024 100644
> --- a/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> @@ -10,17 +10,17 @@ module foo.second; // { dg-error "only permitted as" }
>   
>   namespace Foo
>   {
> -module third;  // { dg-error "only permitted as" }
> +module third;  // { dg-error "must be at global scope" }
>   }
>   
>   struct Baz
>   {
> -  module forth; // { dg-error "expected" }
> +  module forth; // { dg-error "unexpected module directive" }
>   };
>   
>   void Bink ()
>   {
> -  module fifth; // { dg-error "expected" }
> +  module fifth; // { dg-error "unexpected module directive" }
>   }
>   
>   module a.; // { dg-error "only permitted as" }
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-6.C b/gcc/testsuite/g++.dg/modules/mod-decl-6.C
> new file mode 100644
> index 00000000000..5fb342455e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-6.C
> @@ -0,0 +1,11 @@
> +// PR c++/115200
> +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> +// { dg-module-cmi !M }
> +
> +module;
> +
> +namespace ns {  // { dg-message "scope opened here" }
> +
> +export module M;  // { dg-error "global scope" }
> +
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-7.C b/gcc/testsuite/g++.dg/modules/mod-decl-7.C
> new file mode 100644
> index 00000000000..87eec0d2697
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-7.C
> @@ -0,0 +1,12 @@
> +// PR c++/115200
> +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> +// { dg-module-cmi !M }
> +
> +module;
> +
> +void foo() {  // { dg-message "scope opened here" }
> +
> +export module M;  // { dg-error "unexpected module directive" }
> +		  // { dg-message "global scope" "" { target *-*-* } .-1 }
> +
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-8.C b/gcc/testsuite/g++.dg/modules/mod-decl-8.C
> new file mode 100644
> index 00000000000..50e125ce1bc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-8.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-prune-output "not writing module" }
> +
> +module;
> +
> +#include "mod-decl-8.h"
> +// { dg-error "module control-line|unexpected module directive" "" { target *-*-* } 0 }
> +
> +export module M;
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-8.h b/gcc/testsuite/g++.dg/modules/mod-decl-8.h
> new file mode 100644
> index 00000000000..8ce03f04399
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-8.h
> @@ -0,0 +1,4 @@
> +struct module { int r; };
> +void foo() {
> +  module x;
> +}