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

On Thu, May 23, 2024 at 05:11:39PM -0400, Jason Merrill wrote:
> 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.
> 

True.

FWIW I just used the same wording as in 'cp_parser_import_declaration';
my understanding is it's because you can disambiguate by adding a
newline after the 'module' itself:

  int module;
  int i =
    module
    ;

is OK.  But I'll update this message to be clearer.

> 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.
> 

How about this then? Bootstrapped and regtested on x86_64-pc-linux-gnu.

-- >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.
	(cp_parser_import_declaration): Sync error message with that in
	cp_parser_error_1.

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.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/parser.cc                          | 26 ++++++++++++++++++++---
 gcc/testsuite/g++.dg/modules/mod-decl-1.C |  8 ++++---
 gcc/testsuite/g++.dg/modules/mod-decl-6.C | 11 ++++++++++
 gcc/testsuite/g++.dg/modules/mod-decl-7.C | 11 ++++++++++
 gcc/testsuite/g++.dg/modules/mod-decl-8.C | 14 ++++++++++++
 5 files changed, 64 insertions(+), 6 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

On 5/23/24 19:58, Nathaniel Shead wrote:
> On Thu, May 23, 2024 at 05:11:39PM -0400, Jason Merrill wrote:
>> 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.
>>
> 
> True.
> 
> FWIW I just used the same wording as in 'cp_parser_import_declaration';
> my understanding is it's because you can disambiguate by adding a
> newline after the 'module' itself:
> 
>    int module;
>    int i =
>      module
>      ;
> 
> is OK.  But I'll update this message to be clearer.
> 
>> 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.
>>
> 
> How about this then? Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK, thanks.

> -- >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.
> 	(cp_parser_import_declaration): Sync error message with that in
> 	cp_parser_error_1.
> 
> 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.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/parser.cc                          | 26 ++++++++++++++++++++---
>   gcc/testsuite/g++.dg/modules/mod-decl-1.C |  8 ++++---
>   gcc/testsuite/g++.dg/modules/mod-decl-6.C | 11 ++++++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-7.C | 11 ++++++++++
>   gcc/testsuite/g++.dg/modules/mod-decl-8.C | 14 ++++++++++++
>   5 files changed, 64 insertions(+), 6 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
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 476ddc0d63a..779625144db 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -3230,6 +3230,19 @@ cp_parser_error_1 (cp_parser* parser, const char* gmsgid,
>         return;
>       }
>   
> +  if (cp_token_is_module_directive (token))
> +    {
> +      auto_diagnostic_group d;
> +      error_at (token->location, "unexpected module directive");
> +      if (token->keyword != RID__EXPORT)
> +	inform (token->location, "perhaps insert a line break after"
> +		" %qs, or other disambiguation, to prevent this being"
> +		" considered a module control-line",
> +		(token->keyword == RID__MODULE) ? "module" : "import");
> +      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 +15148,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))
>       {
> @@ -15217,9 +15237,9 @@ cp_parser_import_declaration (cp_parser *parser, module_parse mp_state,
>         error_at (token->location, "post-module-declaration"
>   		" imports must be contiguous");
>       note_lexer:
> -      inform (token->location, "perhaps insert a line break, or other"
> -	      " disambiguation, to prevent this being considered a"
> -	      " module control-line");
> +      inform (token->location, "perhaps insert a line break after"
> +	      " %<import%>, or other disambiguation, to prevent this"
> +	      " being considered a module control-line");
>       skip_eol:
>         cp_parser_skip_to_pragma_eol (parser, token);
>       }
> diff --git a/gcc/testsuite/g++.dg/modules/mod-decl-1.C b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> index 23d34483dd7..df398b3ef50 100644
> --- a/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-1.C
> @@ -10,17 +10,19 @@ 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" }
> +  // { dg-message "line break after .module." "" { target *-*-* } .-1 }
>   };
>   
>   void Bink ()
>   {
> -  module fifth; // { dg-error "expected" }
> +  module fifth; // { dg-error "unexpected module directive" }
> +  // { dg-message "line break after .module." "" { target *-*-* } .-1 }
>   }
>   
>   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..d25a1938044
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-7.C
> @@ -0,0 +1,11 @@
> +// PR c++/115200
> +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> +// { dg-module-cmi !M }
> +
> +module;
> +
> +void foo() {
> +
> +export module M;  // { dg-error "unexpected module directive" }
> +
> +}
> 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..9f5f1de544a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-decl-8.C
> @@ -0,0 +1,14 @@
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi M }
> +
> +export module M;
> +
> +struct module {};
> +struct import {};
> +
> +void foo() {
> +  module
> +    x;  // OK
> +  import
> +    y;  // OK
> +}