[Patch, Fortran] (Coarray) Fix constraint checks for LOCK_TYPE

[Tobias Burnus]

Mikael,

first, thanks for carefully reading the patch!

On 08/03/2011 01:43 PM, Mikael Morin wrote:
> > PS: It somehow took me quite some time to understand "subcomponent" even
> > though the standard is rather clear about it. 
> Is it? It seems I haven't understood the constraint as you did.

It's like the famous saying: "Unix is user friendly, it is just picky
about its friends."

Regarding this part of the standard, I only feel acquainted but not
being a real friend ... I think it is clearly stated in the standard,
but I continue to struggle to apply it correctly.

> So basically, one looks at the components of a structure, and the components 
> of all the non-allocatable non-pointer derived type components (and so on 
> recursively...).

That's also my reading.

> Among those components, if one has type LOCK_TYPE and is not a coarray, then the enclosing variable shall be a coarray (which seems to mean that all 
> variables of this type have to be a coarray).

Yes, I think that's what the constraint requires at the end: In
LOCK(expr) the "expr" needs to be a coarray - or coindexed (such that it
were a coarray without the coindex).

> Though variables in the general case can be components, I don't think it is 
> the case here as only named variables are involved here.
> Does that sound right?

The first part of the sentence sounds wrong: A component itself is not a
variable. I think you mean a "structure-component" - and for "var%comp"
both "var%comp" and "var" are variables and I belief both "var%comp" and
"var" are named.

"R602 variable is designator or expr"
"R601 designator is object-name or ... or structure-component"
"R613 structure-component is data-ref"
"R611 data-ref is part-ref [ % part-ref ]"
"R612 part-ref is part-name [ ( section-subscript-list ) ] [
image-selector ]"


> >        /* Looking for lock_type components.  */
> > -      if (c->attr.lock_comp
> > -         || (sym->ts.type == BT_DERIVED
> > +      if ((c->ts.type == BT_DERIVED
> >               && c->ts.u.derived->from_intmod == INTMOD_ISO_FORTRAN_ENV
> > -             && c->ts.u.derived->intmod_sym_id == ISOFORTRAN_LOCK_TYPE))
> > -       sym->attr.lock_comp = 1;
> > +             && c->ts.u.derived->intmod_sym_id == ISOFORTRAN_LOCK_TYPE)
> > +         || (c->ts.type == BT_CLASS && c->attr.class_ok
> > +             && CLASS_DATA (c)->ts.u.derived->from_intmod
> > +                == INTMOD_ISO_FORTRAN_ENV
> > +             && CLASS_DATA (c)->ts.u.derived->intmod_sym_id
> > +                == ISOFORTRAN_LOCK_TYPE))
> > +       {
> > +         if (pointer)
> > +           gfc_error ("Pointer component %s at %L of LOCK_TYPE must be a "
> > +                      "coarray", c->name, &c->loc);
> ... this is wrong as the constraint should be on the variable, not on the 
> components of its type.

I think you are right: The derived-type declaration itself is valid
according to the standard - one just may not use it for variable
declaration.

The question is: Should we bother? Is there some genuine interest in
supporting valid but unusable type declarations? The problem is that
diagnosing the problem can get rather difficult. For instance:

type t
type(lock_type) :: C
end type
type t2
type(t), allocatable :: B
end type t2
type t3
type(t2) :: D
end type t3

is valid - however, it is invalid to use:
type(t) :: x[*], y

However, how to write then the error message? "Error: Invalid
declaration at (1) as constraint C642 is violated" is probably not very
helpful, but should one really re-resolve the derived type? There are
also many possible solutions:

"B" should be a coarray, "C" could be also allocatable and a coarray, or
"B" could be not allocatable and "D" or "y" could be a coarray.

Thus, writing a nice and helpful message gets pretty complicated. And,
as written, I do not see a compelling reason for not diagnosing it at
type-declaration time - even if the type is formally correct.


Regarding the check itself - if one assumes that one wants to have an
error, I believe that part is correct. If there is a pointer, it cannot
be valid. Example 1:

type t
type(lock_type), pointer :: lock1
end type t

I cannot write "lock1[:]" as in components, only allocatables are allowed:

"C442 (R436) If a coarray-spec appears, it shall be a
deferred-coshape-spec-list and the component shall have the ALLOCATABLE
attribute."

Turning the variable into a coarray as in
type(t) :: x[*]
does not help: the x%lock1 is not a coarray. "A subobject of a coarray
is a coarray if it does not have any cosubscripts, vector subscripts,
allocatable component selection, or pointer component selection." (Sect.
2.4.7)

On the other hand, if "type t" contains a noncoarray lock_type, one
cannot do use "type(t), pointer :: ptrcomp" as "...%ptrcomp%lock"
wouldn't be a coarray (cf. above) - and if "lock" in "...%ptrcomp%lock"
were a coarray, it would be invalid as: "C444 A data component whose
type has a coarray ultimate component shall be a nonpointer
nonallocatable scalar and shall not be a coarray."


Having said that, I just realized that the following program is not
rejected but it should:

use iso_fortran_env
type t
type(lock_type) :: lock
end type t

type t2
type(t), pointer :: x
end type t2
end


The modified section of the patch would then be:

/* Looking for lock_type components. */
if ((c->ts.type == BT_DERIVED
&& c->ts.u.derived->from_intmod == INTMOD_ISO_FORTRAN_ENV
&& c->ts.u.derived->intmod_sym_id == ISOFORTRAN_LOCK_TYPE)
|| (c->ts.type == BT_CLASS && c->attr.class_ok
&& CLASS_DATA (c)->ts.u.derived->from_intmod
== INTMOD_ISO_FORTRAN_ENV
&& CLASS_DATA (c)->ts.u.derived->intmod_sym_id
== ISOFORTRAN_LOCK_TYPE)
|| (c->ts.type == BT_DERIVED && c->ts.u.derived->attr.lock_comp
&& !allocatable && !pointer))
{
lock_type = 1;
lock_comp = c;
sym->attr.lock_comp = 1;
}

/* F2008, C1302. */

if (pointer && !coarray && (lock_type
|| (c->ts.type == BT_DERIVED
&& c->ts.u.derived->attr.lock_comp)))
gfc_error ("Component %s at %L of LOCK_TYPE or with LOCK_TYPE "
"component is a pointer but not a coarray",
c->name, &c->loc);

[...]
> > +      if (sym->attr.lock_comp && coarray && !lock_type)
> > +       gfc_error ("Component %s at %L of LOCK_TYPE or with LOCK_TYPE has to "
>> > +                  "be a coarray as %s at %L has a codimension",
>> > +                  lock_comp->name, &lock_comp->loc, c->name, &c->loc); 
> Same here (the three of them) the constraint is not on components.

Here, something similar applies:

type t
integer, allocatable :: caf_comp[:]
type(lock_type) :: lock
end type t
type(t) :: x[*]

It is invalid to make "x" a coarray as "t" already has coarray
components - but if I don't make "x" a coarray, x%lock is not a coarray,
which is invalid.

Thus, I believe there is no way that this error is printed for a type
declaration which can be used to create a valid named variable.

>> > +      || (!gfc_is_coarray (code->expr1) && !gfc_is_coindexed (code-
>> >expr1)))
> This part is quite OK.
> I have the feeling that !gfc_is_coindexed is superfluous once one has 
> !gfc_is_coarray.
> I'm also not sure that the removal of `coarray' in the error message is 
> needed as C1302 forces LOCK_TYPE entities to be coarrays or subobjects of 
> coarrays.

The LOCK_TYPE named object needs to be a coarray - the entity you pass
to LOCK does not have to be a coarray. Example:

type(lock_type), type :: lock[*]
LOCK(lock) ! (1)
LOCK(lock[1]) ! (2)

In (1), the argument is a scalar coarray of lock_type. But in (2) one
does not have a coarray but just a coindexed scalar of lock_type.

As "lock[1]" is not a coarray, I believe removing "coarray" from the
error message makes sense as does checking for both gfc_is_coarray and
gfc_is_coindexed.

> Looks like it's not sufficient.
> One can have a non-coarray LOCK_TYPE component and an other component that is 
> a coarray.

That's the reason for all the checks in parse.c. I make use of the fact
that one cannot have more than one coarray - if a component is a
coarray, the variable cannot be a coarray and vice versa. I think I have
covered all cases there, but I might be wrong (as I already have been
for the pointer, cf. above).

> > @@ -0,0 +1,7 @@
> > +! { dg-do compile }
> > +! { dg-options "-fcoarray=single" }
> > +!
> > +!
> > +! LOCK/LOCK_TYPE checks 
> > +!
> > +
> This one was passing already, wasn't it? ;-)

Hopefully!

Tobias