[patch, fortran] Fix PR 44960, accepts invalid reference on function

[patch, fortran] Fix PR 44960, accepts invalid reference on function

[Thomas Koenig]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Hello world,

here is my first patch made from the git world.  It certainly took
enough time to work out how to to this, but I think I have it
figured out now...

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?

Regards

	Thomas

2020-01-16  Thomas König  <tkoenig@gcc.gnu.org>

	PR fortran/44960
	* resolve.c (resolve_function): Issue error when a
	function call contains a reference.

2020-01-16  Thomas König  <tkoenig@gcc.gnu.org>

	PR fortran/44960
	* gfortran.dg/function_reference_1.f90: New test.

[-- Attachment #2: p1.diff --]
[-- Type: text/x-patch, Size: 1030 bytes --]

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
+

[-- Attachment #3: function_reference_1.f90 --]
[-- Type: text/x-fortran, Size: 233 bytes --]

! { 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

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Steve Kargl]

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

-- 
Steve

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Tobias Burnus]

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

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Tobias Burnus]

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

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Steve Kargl]

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.

-- 
Steve

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Thomas Koenig]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

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

[-- Attachment #2: p2.diff --]
[-- Type: text/x-patch, Size: 982 bytes --]

diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 6f2a4c4d65a..a846677b770 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -3129,6 +3129,12 @@ resolve_function (gfc_expr *expr)
 	  || sym->intmod_sym_id == GFC_ISYM_CAF_SEND))
     return true;
 
+  if (expr->ref)
+    {
+      gfc_error ("Syntax error in expression 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..1b7f4809c5c
--- /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 "Syntax error" }
+end
+

[-- Attachment #3: function_reference_1.f90 --]
[-- Type: text/x-fortran, Size: 204 bytes --]

! { 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 "Syntax error" }
end

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Steve Kargl]

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.

-- 
Steve

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Tobias Burnus]

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

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Thomas Koenig]

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

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Thomas Koenig]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

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.




[-- Attachment #2: p3.diff --]
[-- Type: text/x-patch, Size: 1890 bytes --]

diff --git a/gcc/fortran/primary.c b/gcc/fortran/primary.c
index 07b8ac08ba2..bd50827bb15 100644
--- a/gcc/fortran/primary.c
+++ b/gcc/fortran/primary.c
@@ -3661,6 +3661,7 @@ gfc_match_rvalue (gfc_expr **result)
 	  gfc_error ("The leftmost part-ref in a data-ref cannot be a "
 		     "function reference at %C");
 	  m = MATCH_ERROR;
+	  break;
 	}
 
       m = MATCH_YES;
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 6f2a4c4d65a..697afadb378 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 ("Unexpected junk after %qs at %L", expr->symtree->n.sym->name,
+		 &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..be634c9dd4b
--- /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 "Unexpected junk" }
+end
+
diff --git a/gcc/testsuite/gfortran.dg/function_reference_2.f90 b/gcc/testsuite/gfortran.dg/function_reference_2.f90
new file mode 100644
index 00000000000..375c58bb6d2
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/function_reference_2.f90
@@ -0,0 +1,10 @@
+! { dg-do compile }
+! PR 44960 - improve the error message
+program main
+  type t
+  integer :: a
+end type t
+type(t) :: foo
+external foo
+i = foo(1)%1 ! { dg-error "leftmost part-ref in a data-ref cannot be a function reference" }
+end

[-- Attachment #3: function_reference_2.f90 --]
[-- Type: text/x-fortran, Size: 232 bytes --]

! { dg-do compile }
! PR 44960 - improve the error message
program main
  type t
  integer :: a
end type t
type(t) :: foo
external foo
i = foo(1)%1 ! { dg-error "leftmost part-ref in a data-ref cannot be a function reference" }
end

[-- Attachment #4: function_reference_1.f90 --]
[-- Type: text/x-fortran, Size: 207 bytes --]

! { 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 "Unexpected junk" }
end

Re: [patch, fortran] Fix PR 44960, accepts invalid reference on function

[Tobias Burnus]

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