Hi Harald,
I agree that the gfc_match_array_ref() is not needed for error recovery or
helps anyhow. I therefore removed it. I left the new error message in, because
in the case that c = x(:)(2:5) is in a subroutine, like in pr102532.f90, then I
get no error. Neither without return MATCH_ERROR nor with it. Therefore I think
the gfc_error_now() can help.
I have bootstrapped and regtested the modified patch again on
x86_64-pc-linux-gnu / Fedora 39 before committing it as:
gcc-15-4171-g0ad2c76bea2
Thanks for the review and the fruitful discussion. I hope that this solution
will fix the bug and also improve gfortran's user experience a tiny little bit.
Thanks again and regards,
Andre
On Mon, 7 Oct 2024 22:58:23 +0200
Harald Anlauf <anl...@gmx.de> wrote:
> Hi Andre,
>
> On 10/7/24 11:04, Andre Vehreschild wrote:
> > Hi Harald,
> >
> > thank you for your input. I still have some small nits to discuss to make
> > everyone happy. Therefore:
> >
> >> 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.
> >
> > Me either, but when I remove the gfc_error_now() and only do
> >
> >> if (gfc_peek_ascii_char () == '(')
> >> return MATCH_ERROR;
> >
> > as you proposed, then no error is given for:
> >
> > character(:), allocatable :: x[:]
> > character(:), allocatable :: c
> > c = x(:)(2:5)
> >
> > I.e. nothing at all.
>
> hmmm, without the hunk in question I do get:
>
> 4 | c = x(:)(2:5)
> | 1
> Error: Unclassifiable statement at (1)
>
>
> which is the same when doing a return MATCH_ERROR;
>
> When I simply use:
>
> if (gfc_peek_ascii_char () == '(')
> {
> gfc_error ("Unexpected array/substring ref at %C");
> return MATCH_ERROR;
> }
>
> this already generates:
>
> 4 | c = x(:)(2:5)
> | 1
> Error: Unexpected array/substring ref at (1)
>
>
> > Therefore at the moment I prefer to stick to the initial> solution
> with the gfc_error_now, which not only gives an error in the
> > associate, but also when one just does an array/substring-ref outside of
> > parentheses. And I like the new error message, because I consider it more
> > helpful than just a syntax error or the invalid association target message.
> > What do you think?
>
> The motivation for my asking is based on the following naive thinking
> (assuming that x is of type character):
>
> x(:)(2:5) ! could be a rank mismatch when x is an array
> x[1](:)(2:5) ! is always a syntax error
> x(:)[1](2:5) ! could by diagnosed as a rank mismatch
>
> That is of course wishful thinking on my side. No compiler
> matches this completely, and diagnosing a syntax error is
> certainly acceptable behavior. (Some other brand shows funny
> diagnostics coming likely from the resolution phase).
>
> >> Is there a reason that you do not check the return value of
> >> gfc_match_array_ref?
> >
> > What am I to do with the result? We are in an error case independent of the
> > result of gfc_match_array_ref. The intention of using that routine here was
> > to digest the unexpected input and allow for (easier|better) error
> > recovery.
>
> Do you have an example that shows the use of gfc_match_array_ref here?
> Commenting it out doesn't seem to make a difference in the error case
> here, unless I missed something.
>
> > May> be I should just put a comment on it, to make it more clear. Or
> is there
> > another way to help the parser recover from an error?
>
> Well, I am not the expert to answer that. Without gfc_error_now,
> we're more likely seeing errors coming from the parsing of the
> associate, and here I would point to Paul as the one with the most
> experience. I would hope that the parsing of associate would see
> if an error was issued for the associate target and allow that error
> to be emitted.
>
> > Sorry for the additional round. But this error has been around for so long,
> > that it doesn't matter, if we need another day to come up with a solution.
>
> Indeed! :-)
>
> > Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
>
> I am fine with your solution. Diagnostics can be improved later
> any time...
>
> > Regards,
> > Andre
>
> Thanks for your patience!
>
> Harald
>
> >
> >> 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 <anl...@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
> >>>
> >>
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
