FWD » Core Development » Base Language

I've encountered an issue with assigning arrays. Consider the following 4GL code:

    DEF VAR rkeys AS CHAR EXTENT NO-UNDO.
    DEF VAR a AS CHARACTER EXTENT 3 INITIAL ["one","two","three"] NO-UNDO.
    rkeys = a.

      character[] rkeys = TypeFactory.characterExtent();
      character[] a = TypeFactory.characterExtent(3, "one", "two", "three");

      externalProcedure(GetKeysTest.this, new Block((Body) () -> 
      {
         rkeys = (character[]) assignMulti(rkeys, a);
      }));

which does not compile as contains modification of a variable inside lambda expression (Local variable rkeys defined in an enclosing scope must be final or effectively final).
See #4124-113m #4124-114, #4124-115, #4124-116, #4124-117

Indeterminate arrays are profoundly broken in the current FWD trunk. This task and #4123 show two very simple cases that should work but do not.

My initial thought is that we will need to use the ref[0] dirty trick here to resolve this since we cannot promote vars to solve the issue (promotion breaks recursive function/internal proc/method usage). It is cleaner to have a simple "holder" class but then we must use methods to assign() and get() the actual instance. Thus the conversion would need to be a bit more complex and the converted code would be more verbose.

Constantin: Do you have any other solutions in mind? I'm also really surprised we haven't needed to fix this yet.

Greg, the ref[0] approach was added, but only for OUTPUT parameters (I really don't recall why I did it only for this specific case). So, something like this converts fine:

procedure proc0.
   def output param v as int extent.
   def var v2 as int extent 3.
   v = v2.
end.

   public void proc0(final OutputExtentParameter<integer> extv)
   {
      integer[] v[] = 
      {
         UndoableFactory.initOutput(extv)
      };
      integer[] v2 = UndoableFactory.integerExtent(3);

      internalProcedure(new Block((Body) () -> 
      {
         assignMulti(v[0], v2);
      }
   }

We can extend this to work with any other case.

OK. Can you please add that change to 4207a?

Greg Shah wrote:

OK. Can you please add that change to 4207a?

OK, will do.

• use a special wrapper (so that we can make even the dynamic extent vars 'mutable', maybe limited to shared cases)
• keep track of all Java Field instances from where this dynamic extent shared var is referenced (until its extent is fixed), and use reflection to update these fields.

• getter which returns the extent var (without an index argument)
• setter which has as argument a single extent parameter, without index

The above need to be generated only when the property doesn't have an explicit body for the getter and setter.

I have a conversion fix for the INPUT and normal variable, but I'm working also on getting rid of the promotion of the variables/parameters scoped to an internal procedure/function. My solution at this time is to use an anon class instance, instead of the defined ExtentExpr instances - this will keep the scope to the defining top-level block.

The conversion fixes are in 4207a rev 11360. It also moves the ExtentExpr to anon classes, to get rid of the parameter promotion (which is definitely incorrect).

Ah, and I just realized something - for variables scoped to the external program, we should just promote them, and avoid the overhead of the two-dimensional array.

• shared variable problem in #4160-8 (we need to keep track of the Java class fields and 'update' them when the shared dynamic extent var gets its size set).
• OE class properties conversion issues in #4160-9
• a bug in ArrayAssigner - when is defined, a dynamic extent var gets registered with the previous scope, and not the next (to be opened) scope. I thought the change would be simple, but there is lots of usage from p2j.oo code, where some APIs (which define an extent param) are called from within the top-level body, instead of being called before it gets executed.

Constantin Asofiei wrote:

Ah, and I just realized something - for variables scoped to the external program, we should just promote them, and avoid the overhead of the two-dimensional array.

This is in rev 4207a rev 11361.

a bug in ArrayAssigner - when is defined, a dynamic extent var gets registered with the previous scope, and not the next (to be opened) scope. I thought the change would be simple, but there is lots of usage from p2j.oo code, where some APIs (which define an extent param) are called from within the top-level body, instead of being called before it gets executed.

Constantin: Please provide enough detail on this so that Roger can go find all the p2j.oo code and fix those cases.

use a special wrapper (so that we can make even the dynamic extent vars 'mutable', maybe limited to shared cases)

I think this is the easiest approach to understand and implement. I'd like to avoid the reflection approach.

I think there is a regression. See testcase below. In fact, I just ran 4207a against @uast/accum_scope*.p" and there were regressions that didn't happen in trunk.

def var i as int.
form i label "Total" with frame f1.
repeat i = 1 to 10 with frame f1:
   display i.
      accum i (total).
end.
display accum total i @ i with frame f1.
message "Done.".

The resultant code snippet shows a scope issue for i:

   public void execute()
   {
      integer i = UndoableFactory.integer();

      externalProcedure(AccumScope.this, new Block((Body) () ->
      {
         f1Frame.openScope();

         ToClause toClause0 = new ToClause(i, 1, 10);

         repeatTo("loopLabel0", toClause0, new Block((Init) () ->
         {
            accumTotal0.reset();
         },
         (Body) () ->
         {

Roger Borrello wrote:

I think there is a regression. See testcase below. In fact, I just ran 4207a against @uast/accum_scope*.p" and there were regressions that didn't happen in trunk.

[...]

The resultant code snippet shows a scope issue for i:

[...]

Sorry... took the wrong snippet, since it didn't show the issue.

public class AccumScope
{
   AccumScopeF1 f1Frame = GenericFrame.createFrame(AccumScopeF1.class, "f1");

   TotalAccumulator accumTotal0 = new TotalAccumulator(i);

   /**
    * External procedure (converted to Java from the 4GL source code
    * in accum_scope.p).
    */
   public void execute()
   {
      integer i = UndoableFactory.integer();

      externalProcedure(AccumScope.this, new Block((Body) () ->
      {
         f1Frame.openScope();

         ToClause toClause0 = new ToClause(i, 1, 10);

Roger Borrello wrote:

Roger Borrello wrote:

I think there is a regression. See testcase below. In fact, I just ran 4207a against @uast/accum_scope*.p" and there were regressions that didn't happen in trunk.

OK, I've backed out a change related to promoting vars used in accumulate expressions. I need to think a little more why the accumulator needs to be promoted.

Constantin Asofiei wrote:

Roger Borrello wrote:

Roger Borrello wrote:

I think there is a regression. See testcase below. In fact, I just ran 4207a against @uast/accum_scope*.p" and there were regressions that didn't happen in trunk.

OK, I've backed out a change related to promoting vars used in accumulate expressions. I need to think a little more why the accumulator needs to be promoted.

There is still another regression. uast/accum/accum_count_as_var.p has an error in annotations:

     [java] ./uast/accum/accum_count_as_var.p
     [java] Elapsed job time:  00:00:01.356
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] throwException("Unmatched expression in ACCUM function", this)
     [java] ^  { Unmatched expression in ACCUM function [KW_COUNT id <12884902021> 14:25] }
     [java] ---------------------------
     [java] ERROR:
     [java] com.goldencode.p2j.pattern.TreeWalkException: ERROR!  Active Rule:
     [java] -----------------------
     [java]       RULE REPORT      
     [java] -----------------------
     [java] Rule Type :   WALK
     [java] Source AST:  [ count ] BLOCK/STATEMENT/KW_MSG/CONTENT_ARRAY/EXPRESSION/FUNC_POLY/AGGREGATE/KW_COUNT/ @14:25 {12884902021}
     [java] Copy AST  :  [ count ] BLOCK/STATEMENT/KW_MSG/CONTENT_ARRAY/EXPRESSION/FUNC_POLY/AGGREGATE/KW_COUNT/ @14:25 {12884902021}
     [java] Condition :  throwException("Unmatched expression in ACCUM function", this)
     [java] Loop      :  false
     [java] --- END RULE REPORT ---

Below is the testcase:

/* count and total both exhibit this behavior, average, min, max, and all */
/* sub- forms do NOT act as variables */

def var i as int.

repeat i = 1 to 10:
   accum "first" (count).
   if i modulo 2 = 0 then accum "second" (count).
   if false then accum "third" (count).
end.

message "1st =" accum count "first" 
        "; 2nd =" accum count "second" 
        "; 3rd =" accum count "third" 
        "; implicit =" count.

repeat i = 1 to 5:
   accum "fourth" (count).
   if i modulo 2 = 0 then accum "fifth" (count).
end.

message "4th =" accum count "fourth" 
        "; 5th =" accum count "fifth" 
        "; implicit =" count.

repeat i = 1 to 60:
   accum "sixth" (count).
end.

message "6th =" accum count "sixth" 
        "; implicit =" count.

The above is from trunk: if false then accum "third" (count). is marked as unreachable, which is not correct. Any accum statement must be converted.

Greg Shah wrote:

a bug in ArrayAssigner - when is defined, a dynamic extent var gets registered with the previous scope, and not the next (to be opened) scope. I thought the change would be simple, but there is lots of usage from p2j.oo code, where some APIs (which define an extent param) are called from within the top-level body, instead of being called before it gets executed.

Constantin: Please provide enough detail on this so that Roger can go find all the p2j.oo code and fix those cases.

The main issue is ArrayAssigner$WorkArea.register; this should look like this:

      public void register(BaseDataType[] array, boolean pending)
      {
         if (pending)
         {
            pendingDynamicArrays.add(array);
         }
         else 
         {
            // register only to one scope
            if (!isRegistered(array))
            {
               dynamicArrays.peek().add(array);
            }
         }
      }

      /** Set of pending dynamic arrays. */
      private Set<BaseDataType[]> pendingDynamicArrays = Sets.newIdentityHashSet();

         Set<BaseDataType[]> dynarr = Sets.newIdentityHashSet();
         dynarr.addAll(pendingDynamicArrays);

         dynamicArrays.push(dynarr);

When called from registerDynamicArray, this must have the pending argument set to true. This assuming that registerDynamicArray is always called before the block scopeStart. But, this code AbstractExtentParameter.initParameter is being executed from inside top-level body, from this location:

HttpHeaderCollection.getAll(OutputExtentParameter<object<? extends HttpHeader>>)

I think this is the only case where registerDynamicArray can't be used as 'pending', and the code needs to be changed to be executed in the top-level block. The other cases I noticed are related to declaring an undoable var (via UndoableFactory) inside the top-level block; again, this is another part which is not OK, as the registration will be done with the next block.

Constantin Asofiei wrote:

The above is from trunk: if false then accum "third" (count). is marked as unreachable, which is not correct. Any accum statement must be converted.

Does this mean the testcase is invalid? Or is there a bug? I was using 4207a, not trunk.

Roger Borrello wrote:

Does this mean the testcase is invalid? Or is there a bug? I was using 4207a, not trunk.

I meant the bug is in trunk, too.

Constantin Asofiei wrote:

Greg Shah wrote:

a bug in ArrayAssigner - when is defined, a dynamic extent var gets registered with the previous scope, and not the next (to be opened) scope. I thought the change would be simple, but there is lots of usage from p2j.oo code, where some APIs (which define an extent param) are called from within the top-level body, instead of being called before it gets executed.

Constantin: Please provide enough detail on this so that Roger can go find all the p2j.oo code and fix those cases.

The main issue is ArrayAssigner$WorkArea.register; this should look like this:
[...]
where pendingDynamicArrays is defined in WorkArea:
[...]
and gets used in scopeStart, like this:
[...]

When called from registerDynamicArray, this must have the pending argument set to true. This assuming that registerDynamicArray is always called before the block scopeStart. But, this code AbstractExtentParameter.initParameter is being executed from inside top-level body, from this location:
[...]

I think this is the only case where registerDynamicArray can't be used as 'pending', and the code needs to be changed to be executed in the top-level block. The other cases I noticed are related to declaring an undoable var (via UndoableFactory) inside the top-level block; again, this is another part which is not OK, as the registration will be done with the next block.

That was not enough detail to get me going. I believe Greg thought it would be an exercise in automation, in order to make a large set of edits. Is that the case?

Roger Borrello wrote:

That was not enough detail to get me going. I believe Greg thought it would be an exercise in automation, in order to make a large set of edits. Is that the case?

This is not a matter to automate the edits, but a matter of how to 'backtrace' all callers of a certain code. I haven't done any automation beside renaming/moving APIs; OTOH, for an offtopic reading, there is the https://github.com/INRIA/spoon project which can be used for Java source-code refactoring.

Constantin Asofiei wrote:

The conversion fixes are in 4207a rev 11360. It also moves the ExtentExpr to anon classes, to get rid of the parameter promotion (which is definitely incorrect).

Ah, and I just realized something - for variables scoped to the external program, we should just promote them, and avoid the overhead of the two-dimensional array.

What is outstanding:

• shared variable problem in #4160-8 (we need to keep track of the Java class fields and 'update' them when the shared dynamic extent var gets its size set).
• OE class properties conversion issues in #4160-9
• a bug in ArrayAssigner - when is defined, a dynamic extent var gets registered with the previous scope, and not the next (to be opened) scope. I thought the change would be simple, but there is lots of usage from p2j.oo code, where some APIs (which define an extent param) are called from within the top-level body, instead of being called before it gets executed.

Can you summarize the state of these 3 items? I'd like to help where I can.