PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

  Dear Fortranners,

when debugging the testcase, I noticed that a coarray declaration in
a COMMON statement wrongly set the dimension attribute instead of the
codimension.  As a consequence, subsequent checks that catch this
invalid situation would not trigger.

I see two possible solutions:

- in gfc_match_common, replace

	  /* Deal with an optional array specification after the
	     symbol name.  */
	  m = gfc_match_array_spec (&as, true, true);

  by

  m = gfc_match_array_spec (&as, true, false);

  which in turn would lead to a syntax error.  Interestingly, the Intel
  compiler also takes this route and gives a syntax error.

- check the resulting as->corank and emit an error as in the attached
  patch.

The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?

Thanks,
Harald
  

*PING*

Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
> Dear Fortranners,
>
> when debugging the testcase, I noticed that a coarray declaration in
> a COMMON statement wrongly set the dimension attribute instead of the
> codimension.  As a consequence, subsequent checks that catch this
> invalid situation would not trigger.
>
> I see two possible solutions:
>
> - in gfc_match_common, replace
>
> 	  /* Deal with an optional array specification after the
> 	     symbol name.  */
> 	  m = gfc_match_array_spec (&as, true, true);
>
>    by
>
>    m = gfc_match_array_spec (&as, true, false);
>
>    which in turn would lead to a syntax error.  Interestingly, the Intel
>    compiler also takes this route and gives a syntax error.
>
> - check the resulting as->corank and emit an error as in the attached
>    patch.
>
> The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?
>
> Thanks,
> Harald
>
  

On Wed, 3 Nov 2021 21:00:41 +0100
Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote:

> *PING*
> 
> Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
> > Dear Fortranners,
> > 
> > when debugging the testcase, I noticed that a coarray declaration in
> > a COMMON statement wrongly set the dimension attribute instead of the
> > codimension.  As a consequence, subsequent checks that catch this
> > invalid situation would not trigger.
> > 
> > I see two possible solutions:
> > 
> > - in gfc_match_common, replace
> > 
> > 	  /* Deal with an optional array specification after the
> > 	     symbol name.  */
> > 	  m = gfc_match_array_spec (&as, true, true);

If coarrays are generally forbidden in commons then..
> > 
> >    by
> > 
> >    m = gfc_match_array_spec (&as, true, false);

.. this sounds right to me.

> > 
> >    which in turn would lead to a syntax error.  Interestingly, the Intel
> >    compiler also takes this route and gives a syntax error.
> > 
> > - check the resulting as->corank and emit an error as in the attached
> >    patch.

If we want to be more helpful than a mere syntax error (and we
should be) then yes.
Otherwise we'd have to explicitly
@@ -5275,9 +5275,19 @@ gfc_match_common (void)
 
 	  /* Deal with an optional array specification after the
 	     symbol name.  */
-	  m = gfc_match_array_spec (&as, true, true);
+	  m = gfc_match_array_spec (&as, true, false);
 	  if (m == MATCH_ERROR)
 	    goto cleanup;
+	  if (m == MATCH_NO)
+	    {
+	      /* See if it is a coarray and diagnose it nicely.  */
+	      if (gfc_match_array_spec (&as, false, true) == MATCH_YES)
+		{
+		  gfc_error ("Symbol %qs in COMMON at %C cannot be a "
+			     "coarray", sym->name);
+		  goto cleanup;
+		}
+	    }

where your patch works equally well and is more concise.
Maybe you want to add a test for the double-colon variant too?
   common /c2/ y[:] ! { dg-error "cannot be a coarray" }

A type with a coarray seems to require to be allocatable so is
rejected (properly, but not mentioning the coarray) with
Error: Derived type variable ‘comm_ty1’ in COMMON at (1) has an ultimate component that is allocatable

When reading gfc_match_array_spec i thought that it might have been cleaner
to split the coarray handling out to a separate gfc_match_coarray_spec but
that's what we have.
> > 
> > The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?

LGTM but i cannot approve it.
Thanks for the patch!
  

Hi Bernhard,

Am 04.11.21 um 10:06 schrieb Bernhard Reutner-Fischer via Fortran:
> On Wed, 3 Nov 2021 21:00:41 +0100
> Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote:
>
>> *PING*
>>
>> Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
>>> Dear Fortranners,
>>>
>>> when debugging the testcase, I noticed that a coarray declaration in
>>> a COMMON statement wrongly set the dimension attribute instead of the
>>> codimension.  As a consequence, subsequent checks that catch this
>>> invalid situation would not trigger.
>>>
>>> I see two possible solutions:
>>>
>>> - in gfc_match_common, replace
>>>
>>> 	  /* Deal with an optional array specification after the
>>> 	     symbol name.  */
>>> 	  m = gfc_match_array_spec (&as, true, true);
>
> If coarrays are generally forbidden in commons then..

F2018:

(R874) A common-block-object shall not be a dummy argument, a function
result, an allocatable variable, a derived-type object with an ultimate
component that is allocatable, a procedure pointer, an automatic data
object, a variable with the BIND attribute, an unlimited polymorphic
pointer, or a coarray.

>>>
>>>     by
>>>
>>>     m = gfc_match_array_spec (&as, true, false);
>
> .. this sounds right to me.
>
>>>
>>>     which in turn would lead to a syntax error.  Interestingly, the Intel
>>>     compiler also takes this route and gives a syntax error.
>>>
>>> - check the resulting as->corank and emit an error as in the attached
>>>     patch.
>
> If we want to be more helpful than a mere syntax error (and we
> should be) then yes.
> Otherwise we'd have to explicitly
> @@ -5275,9 +5275,19 @@ gfc_match_common (void)
>
>   	  /* Deal with an optional array specification after the
>   	     symbol name.  */
> -	  m = gfc_match_array_spec (&as, true, true);
> +	  m = gfc_match_array_spec (&as, true, false);
>   	  if (m == MATCH_ERROR)
>   	    goto cleanup;
> +	  if (m == MATCH_NO)
> +	    {
> +	      /* See if it is a coarray and diagnose it nicely.  */

I think you would need to add

	      gfc_array_spec *as;

to avoid clobbering the correct "as" as it is needed later.

> +	      if (gfc_match_array_spec (&as, false, true) == MATCH_YES)
> +		{
> +		  gfc_error ("Symbol %qs in COMMON at %C cannot be a "
> +			     "coarray", sym->name);
> +		  goto cleanup;
> +		}
> +	    }
>
> where your patch works equally well and is more concise.
> Maybe you want to add a test for the double-colon variant too?
>     common /c2/ y[:] ! { dg-error "cannot be a coarray" }

Well, that one is already rejected, although with a different error
message.

I am not sure whether to add that case.  In fact, there are
issues with not always rejecting things like y[:] in declarations
as the last dimension must be "*" or "lbound:*".  If checking
gets improved in this direction, we would have to adjust the error
message.  So adding this variant now does not buy us much.

> A type with a coarray seems to require to be allocatable so is
> rejected (properly, but not mentioning the coarray) with
> Error: Derived type variable ‘comm_ty1’ in COMMON at (1) has an ultimate component that is allocatable

If multiple errors show up, which ones are most important or
must show up?

If you ask me, it is more important to get correct results from
correct input than optimal error messages on wrong code.

I guess users may have an opinion different from mine...

> When reading gfc_match_array_spec i thought that it might have been cleaner
> to split the coarray handling out to a separate gfc_match_coarray_spec but
> that's what we have.
>>>
>>> The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?
>
> LGTM but i cannot approve it.
> Thanks for the patch!
>

Thanks for your comments so far.

Let's see what others think.

Harald
  

Le 04/11/2021 à 20:49, Harald Anlauf via Fortran a écrit :
> Let's see what others think.
> 
OK, thanks.

Mikael