| Message ID | c2e6c9d8-2074-de56-9da3-0d4fa3207714@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Fix PR 44960, accepts invalid reference on function | expand |
On Thu, Jan 16, 2020 at 11:34:43PM +0100, Thomas Koenig wrote: > diff --git a/gcc/testsuite/gfortran.dg/function_reference_1.f90 b/gcc/testsuite/gfortran.dg/function_reference_1.f90 > new file mode 100644 > index 00000000000..78c92a6f20d > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/function_reference_1.f90 > @@ -0,0 +1,11 @@ > +! { dg-do compile } > +! PR 44960 - this was erroneusly accepted. > +! Original test case by Daniel Franke. > + > +type t > + integer :: a > +end type t > +type(t) :: foo > +print *, foo(1)%a ! { dg-error "Function call can not contain a reference" } > +end > + I do not understand this error message, and find it to be confusing. foo(1)%a looks like an invalid array reference. That is, foo is scalar and foo(1) is an array element. PS: s/can not/cannot
Hello Thomas, hi all, On 1/16/20 11:34 PM, Thomas Koenig wrote: > Anyway, the fix is rather straightforward. We cannot have a reference > on a function. If this slipped through on parsing, let's issue the > error during resolution. > > Regression-tested. OK for trunk? I think I am fine with the check itself – but … > + if (expr->ref) > + { > + gfc_error ("Function call can not contain a reference at %L", I have issues with the wording of the error message. First, when reading the error message without context, I thought it is about something regarding the arguments ("contain") or maybe the reference of a function iself. It is clear that we do not want to have anything after the function-reference (→R1521). And we do lack a good wording for that what one can (but shan't) put after the function-reference as it can be quite a lot (cf. →R901): a following '%' to attempt to create a structure-component or complex-part-component access or "(" to get a substring or array-element/section access or '[' to get something coindexed. Not that I like the wording in particular, but we use quite often "Unexpected junk" in the parser. Hence: "Unexpected junk after function reference at %L" would work. Or maybe clearer: char ref_char = '%'; // REF_INQUIRY or REF_COMPONENT if (expr->ref->type == REF_ARRAY || expr->ref->type == REF_SUBSTRING) ref_char = ((expr->ref->type == REF_ARRAY && !expr->ref->u.ar.dimen) '[' : '('; … ("Unexpected %qc after function reference at %L", ref_char, …); Although, I am not sure the '[' (coindex) can occur. Cheers, Tobias
On 1/17/20 4:49 AM, Steve Kargl wrote: > On Thu, Jan 16, 2020 at 11:34:43PM +0100, Thomas Koenig wrote: >> +type(t) :: foo >> +print *, foo(1)%a ! { dg-error "Function call can not contain a reference" } > I do not understand this error message, and find it to be confusing. > foo(1)%a looks like an invalid array reference. That is, foo is scalar > and foo(1) is an array element. Well, we simply do not know whether "external" or "dimension" has been forgotten. As "external" can also be determined by the use, we end up regarding it as function reference… Another example: character(len=4):: str print *, str(1)(1:4) end Maybe a more helpful error message is: "Unexpected junk after function reference or missing dimension declaration for %s", sym->name) (Or instead of "junk" the fancier variant of my previous email.) Cheers, Tobias
On Fri, Jan 17, 2020 at 09:29:33AM +0100, Tobias Burnus wrote: > On 1/17/20 4:49 AM, Steve Kargl wrote: > > On Thu, Jan 16, 2020 at 11:34:43PM +0100, Thomas Koenig wrote: > >> +type(t) :: foo > >> +print *, foo(1)%a ! { dg-error "Function call can not contain a reference" } > > I do not understand this error message, and find it to be confusing. > > foo(1)%a looks like an invalid array reference. That is, foo is scalar > > and foo(1) is an array element. > > Well, we simply do not know whether "external" or "dimension" has been > forgotten. As "external" can also be determined by the use, we end up > regarding it as function reference… > > Another example: > > character(len=4):: str > print *, str(1)(1:4) > end > > Maybe a more helpful error message is: "Unexpected junk after function > reference or missing dimension declaration for %s", sym->name) > > (Or instead of "junk" the fancier variant of my previous email.) > Gfortran probably should not try to guess what the user thought s/he wanted. The generic "Syntax error" would seem to apply here. To me, foo(1)%a looks much more like an array reference rather than a function reference.
Am 17.01.20 um 15:42 schrieb Steve Kargl: > Gfortran probably should not try to guess what the user > thought s/he wanted. The generic "Syntax error" would > seem to apply here. To me, foo(1)%a looks much more like > an array reference rather than a function reference. OK, so here's a patch which does just that. The error message low looks like function_reference_1.f90:9:8: 9 | print *, foo(1)%a ! { dg-error "Syntax error" } | 1 Error: Syntax error in expression at (1) The location information is a bit off, but in the absence of location information for the reference (which we do not collect), I think this is the best I can do. So, OK for trunk (with the old ChangeLog)? Regards Thomas
On Fri, Jan 17, 2020 at 11:20:06PM +0100, Thomas Koenig wrote: > Am 17.01.20 um 15:42 schrieb Steve Kargl: > > Gfortran probably should not try to guess what the user > > thought s/he wanted. The generic "Syntax error" would > > seem to apply here. To me, foo(1)%a looks much more like > > an array reference rather than a function reference. > > OK, so here's a patch which does just that. > > The error message low looks like > > function_reference_1.f90:9:8: > > 9 | print *, foo(1)%a ! { dg-error "Syntax error" } > | 1 > Error: Syntax error in expression at (1) > > The location information is a bit off, but in the absence of location > information for the reference (which we do not collect), I think this > is the best I can do. > > So, OK for trunk (with the old ChangeLog)? > It's fine with me. May want to give Tobias a chance to comment. > + if (expr->ref) > + { > + gfc_error ("Syntax error in expression at %L", &expr->where); I assume that %C puts the locus at the end of the line. I haven't spent to much time trying to understand expressions in an output IO list, but as you state, it seems that gfortran loose the locus.
On 1/17/20 11:20 PM, Thomas Koenig wrote: > function_reference_1.f90:9:8: > > 9 | print *, foo(1)%a ! { dg-error "Syntax error" } > | 1 > Error: Syntax error in expression at (1) The error message is not helpful at all. I was recently struggling to understand why/where some code was failing with syntax error – and it took me a while to find it. And with this message, I had also to find out what did go wrong. How about: ("Unexpected junk after %s at %L", expr->n.symtree->sym->name, &expr->where)? – or "Unexpected junk in reference to %s at %L"? Or deviating from your current error messages: ""Inconsistent use of %s at %L" (Side note: I think we have the general problem that expr->where does not start after the white space, which can be slightly confusing.) Cheers, Tobias
Am 18.01.20 um 12:44 schrieb Tobias Burnus: >> function_reference_1.f90:9:8: >> >> 9 | print *, foo(1)%a ! { dg-error "Syntax error" } >> | 1 >> Error: Syntax error in expression at (1) > > The error message is not helpful at all. Well, yes. It is no better than what we currently have for the slightly different test case type t integer :: a end type t type(t) :: foo external foo print *, foo(1)%a end which is a.f90:6:16: 6 | print *, foo(1)%a | 1 Error: Syntax error in PRINT statement at (1) (but at least that points to the right place). I can see if I can also replace this with something more useful (have to find the place where this is issued first, though). Regards Thomas
Hello world, I found an ommission in primary.c which prevented issuing a more specific error instead of "syntax error" for the case when a function was declared in an EXTERNAL statement, and I have now gone for the "Unexpected junk after foo" variant. Regression-tested. OK for trunk? Regards Thomas 2020-01-18 Thomas König <tkoenig@gcc.gnu.org> PR fortran/44960 * primary.c (gfc_match_rvalue): Break after setting MATCH_ERROR. * resolve.c (resolve_function): Issue error when a function call contains a reference. 2020-01-18 Thomas König <tkoenig@gcc.gnu.org> PR fortran/44960 * gfortran.dg/function_reference_1.f90: New test.
Hello Thomas, looks good to me. Nice catch the missing "break". Tobias PS: I think it does not exercise a differ code path, but instead of derived types, using a character string works as well. The following is accepted with the unmodified trunk: character(len=4):: str print *, str(1)(1:4) end On 1/18/20 7:04 PM, Thomas Koenig wrote: > Hello world, > > I found an ommission in primary.c which prevented issuing a > more specific error instead of "syntax error" for the case > when a function was declared in an EXTERNAL statement, > and I have now gone for the "Unexpected junk after foo" > variant. > > Regression-tested. OK for trunk? > > Regards > > Thomas > > 2020-01-18 Thomas König <tkoenig@gcc.gnu.org> > > PR fortran/44960 > * primary.c (gfc_match_rvalue): Break after setting MATCH_ERROR. > * resolve.c (resolve_function): Issue error when a > function call contains a reference. > > 2020-01-18 Thomas König <tkoenig@gcc.gnu.org> > > PR fortran/44960 > * gfortran.dg/function_reference_1.f90: New test. > > >
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index 6f2a4c4d65a..1525c00ea4c 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -3129,6 +3129,13 @@ resolve_function (gfc_expr *expr) || sym->intmod_sym_id == GFC_ISYM_CAF_SEND)) return true; + if (expr->ref) + { + gfc_error ("Function call can not contain a reference at %L", + &expr->where); + return false; + } + if (sym && sym->attr.intrinsic && !gfc_resolve_intrinsic (sym, &expr->where)) return false; diff --git a/gcc/testsuite/gfortran.dg/function_reference_1.f90 b/gcc/testsuite/gfortran.dg/function_reference_1.f90 new file mode 100644 index 00000000000..78c92a6f20d --- /dev/null +++ b/gcc/testsuite/gfortran.dg/function_reference_1.f90 @@ -0,0 +1,11 @@ +! { dg-do compile } +! PR 44960 - this was erroneusly accepted. +! Original test case by Daniel Franke. + +type t + integer :: a +end type t +type(t) :: foo +print *, foo(1)%a ! { dg-error "Function call can not contain a reference" } +end +