[Fortran,PR51815,v1] Fix parsing of substring refs in coarrays.

Hi all,

this rather old PR reported a parsing bug, when a coarray'ed character substring
ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In this case the
parser confused the substring ref with an array-ref, because an array_spec was
present. This patch fixes this by requesting only coarray parsing from
gfc_match_array_ref when no regular dimension is present. The patch is not
involved when an array of coarray'ed strings is parsed (that worked
beforehand).

I had to fix the dg-error clauses in the testcase pr102532 because now the error
of having to many refs is detected by the parsing stage and no longer by the
resolve stage. It has become a simple syntax error. I hope this is ok.

Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?

Regards,
	Andre
--
Andre Vehreschild * Email: vehre ad gmx dot de

Hi Andre,

Am 01.10.24 um 09:43 schrieb Andre Vehreschild:
> Hi all,
>
> this rather old PR reported a parsing bug, when a coarray'ed character substring
> ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In this case the
> parser confused the substring ref with an array-ref, because an array_spec was
> present. This patch fixes this by requesting only coarray parsing from
> gfc_match_array_ref when no regular dimension is present. The patch is not
> involved when an array of coarray'ed strings is parsed (that worked
> beforehand).

while the patch addresses the issue mentioned in the PR,

> I had to fix the dg-error clauses in the testcase pr102532 because now the error
> of having to many refs is detected by the parsing stage and no longer by the
> resolve stage. It has become a simple syntax error. I hope this is ok.

I find the error messages now less helpful to users: before the patch
we got "Rank mismatch in array reference", which was more suitable
than the newer version with more or less confusing syntax errors.

I assume you tried to find a better solution - but Intel and NAG
also give syntax errors - so basically I am fine with the patch.

You may want to wait for a second opinion.  If nobody else responds
within the next 2 days, you may proceed nevertheless.

Thanks,
Harald

> Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
>
> Regards,
> 	Andre
> --
> Andre Vehreschild * Email: vehre ad gmx dot de

Hi Harald,

we could do something like this:

diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc
index d73d5eaed84..5000906f5f2 100644
--- a/gcc/fortran/primary.cc
+++ b/gcc/fortran/primary.cc
@@ -2823,6 +2823,16 @@ check_substring:
          if (substring)
            primary->ts.u.cl = NULL;

+         if (gfc_peek_ascii_char () == '(')
+           {
+             gfc_array_ref arr_ref;
+             gfc_array_spec *as
+               = sym->ts.type == BT_CLASS ? CLASS_DATA (sym)->as : sym->as;
+             gfc_match_array_ref (&arr_ref, as, 0, 0);
+
+             gfc_error_now ("Unexpected array/substring ref at %C");
+             return MATCH_ERROR;
+           }
          break;

        case MATCH_NO:

It would at least give a better hint. Attached is the patch that adds this to
the previous one.

Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Is this ok?

Regards and thanks for the review,
	Andre

On Tue, 1 Oct 2024 23:31:11 +0200
Harald Anlauf <anlauf@gmx.de> wrote:

> Hi Andre,
>
> Am 01.10.24 um 09:43 schrieb Andre Vehreschild:
> > Hi all,
> >
> > this rather old PR reported a parsing bug, when a coarray'ed character
> > substring ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In
> > this case the parser confused the substring ref with an array-ref, because
> > an array_spec was present. This patch fixes this by requesting only coarray
> > parsing from gfc_match_array_ref when no regular dimension is present. The
> > patch is not involved when an array of coarray'ed strings is parsed (that
> > worked beforehand).
>
> while the patch addresses the issue mentioned in the PR,
>
> > I had to fix the dg-error clauses in the testcase pr102532 because now the
> > error of having to many refs is detected by the parsing stage and no longer
> > by the resolve stage. It has become a simple syntax error. I hope this is
> > ok.
>
> I find the error messages now less helpful to users: before the patch
> we got "Rank mismatch in array reference", which was more suitable
> than the newer version with more or less confusing syntax errors.
>
> I assume you tried to find a better solution - but Intel and NAG
> also give syntax errors - so basically I am fine with the patch.
>
> You may want to wait for a second opinion.  If nobody else responds
> within the next 2 days, you may proceed nevertheless.
>
> Thanks,
> Harald
>
> > Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
> >
> > Regards,
> > 	Andre
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>


--
Andre Vehreschild * Email: vehre ad gmx dot de

Hi Andre,

Am 02.10.24 um 10:49 schrieb Andre Vehreschild:
> Hi Harald,
> 
> we could do something like this:
> 
> diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc
> index d73d5eaed84..5000906f5f2 100644
> --- a/gcc/fortran/primary.cc
> +++ b/gcc/fortran/primary.cc
> @@ -2823,6 +2823,16 @@ check_substring:
>            if (substring)
>              primary->ts.u.cl = NULL;
> 
> +         if (gfc_peek_ascii_char () == '(')
> +           {
> +             gfc_array_ref arr_ref;
> +             gfc_array_spec *as
> +               = sym->ts.type == BT_CLASS ? CLASS_DATA (sym)->as : sym->as;
> +             gfc_match_array_ref (&arr_ref, as, 0, 0);
> +
> +             gfc_error_now ("Unexpected array/substring ref at %C");
> +             return MATCH_ERROR;
> +           }
>            break;
> 
>          case MATCH_NO:
> 
> It would at least give a better hint. Attached is the patch that adds this to
> the previous one.

this seems to go into the right direction - except that I am not a
great fan of gfc_error_now, as that tries to paper over deficiencies
in error recovery.

Is there a reason that you do not check the return value of
gfc_match_array_ref?  Apart from the gfc_error_now, the above
behaves essentially the same a a simple

	  if (gfc_peek_ascii_char () == '(')
	    return MATCH_ERROR;

for the testcase at hand.

Indeed your suggestion (or the shortened version above) improves
the diagnostics ("user experience") also for this variant:

subroutine foo
    character(:), allocatable :: x[:]
    character(:), dimension(:), allocatable :: c[:]
    type t
       character(:), allocatable :: x[:]
       character(:), dimension(:), allocatable :: c[:]
    end type t
    type(t) :: z
    associate (y => x(:)(2:))
    end associate
    associate (a => c(:)(:)(2:))
    end associate
    associate (y => z%x(:)(2:))
    end associate
    associate (a => z%c(:)(:)(2:))
    end associate
end

with several error messages of the kind

Error: Invalid association target at (1)

or

Error: Rank mismatch in array reference at (1) (1/0)

looking less technical than a parsing error.
I think this is as good as it can be.

So OK from my side with either your additional patch or my
shortened version.

Thanks for the patch!

Harald


> Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Is this ok?
> 
> Regards and thanks for the review,
> 	Andre
> 
> On Tue, 1 Oct 2024 23:31:11 +0200
> Harald Anlauf <anlauf@gmx.de> wrote:
> 
>> Hi Andre,
>>
>> Am 01.10.24 um 09:43 schrieb Andre Vehreschild:
>>> Hi all,
>>>
>>> this rather old PR reported a parsing bug, when a coarray'ed character
>>> substring ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In
>>> this case the parser confused the substring ref with an array-ref, because
>>> an array_spec was present. This patch fixes this by requesting only coarray
>>> parsing from gfc_match_array_ref when no regular dimension is present. The
>>> patch is not involved when an array of coarray'ed strings is parsed (that
>>> worked beforehand).
>>
>> while the patch addresses the issue mentioned in the PR,
>>
>>> I had to fix the dg-error clauses in the testcase pr102532 because now the
>>> error of having to many refs is detected by the parsing stage and no longer
>>> by the resolve stage. It has become a simple syntax error. I hope this is
>>> ok.
>>
>> I find the error messages now less helpful to users: before the patch
>> we got "Rank mismatch in array reference", which was more suitable
>> than the newer version with more or less confusing syntax errors.
>>
>> I assume you tried to find a better solution - but Intel and NAG
>> also give syntax errors - so basically I am fine with the patch.
>>
>> You may want to wait for a second opinion.  If nobody else responds
>> within the next 2 days, you may proceed nevertheless.
>>
>> Thanks,
>> Harald
>>
>>> Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
>>>
>>> Regards,
>>> 	Andre
>>> --
>>> Andre Vehreschild * Email: vehre ad gmx dot de
>>
> 
> 
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
>