FWD » Bugs

In cases when there are more than one function with an output or input-output parameter, like in:

function foo returns char (input-output d as char).
   return ?.
end.

function bar returns char (output k as int).
   return ?.
end.

The output or input-output parameters for any subsequent function are leaked to the first function, like in (notice the k parameter leaked to the bar function):

   public character foo(final character d)
   {
      return characterFunction(new Block()
      {
         public void init()
         {
            TransactionManager.deregister(new Undoable[]
            {
               d,
               k
            });
         }

         public void body()
         {
            returnNormal(new character());
         }
      });
   }

   public character bar(final integer k)
   {
      return characterFunction(new Block()
      {
         public void init()
         {
            k.assign(new integer());
         }

         public void body()
         {
            returnNormal(new character());
         }
      });
   }

After investigations I localized the issue when converting .p.ast to .jast (this is where the variables start leaking).
Checking the rules/convert/variable_definitions.rules file at about lines 1040 to 1061 you can see that dereg_id "deregister" node is created only once because of the dereg_id == -1 constraint.
I believe the solution would be to reset the dereg_i variable after each function definition.

OK, I think you are getting close. But I am not sure you have gotten the root cause identified yet. Consider this:

   <descent-rules>

      <!-- is this a top-level scope (internal/external procedure, function
           or trigger) -->
      <rule>evalLib("isTopLevel", copy)

         <!-- save off the previous scope's data -->
         <action>addScope("toplvl")</action>

         ...snip...

         <!-- save off de-registration ID values and init a new one for
              this scope -->
         <action>addDictionaryLong("toplvl", "dereg", dereg_id)</action>
         <action>dereg_id = #(long) (-1)</action>

If isTopLevel() is working properly, then every time we walk down (this is in <decent-rules>) into a top level node (like FUNCTION -> BLOCK), then we would save off our current dereg_id (push it on a stack in the dictionary) and reset the dereg_id to -1. That should be doing the job for us. Either it is not doing the job or it is not always doing the job. You can find isTopLevel() in rules/include/common-progress.rules.

There is a small problem there. The ast looks something like:

<ast text="function" type="FUNCTION" ...>
   <ast text="function" type="KW_FUNCT" ...>
      <ast text="(" type="LPARENS">
         <ast text="parameter" type="PARAMETER"/>
      </ast>
   </ast>
   <ast text="block" type="BLOCK">
      ...
   </ast>

As you can see, the parameters are processed BEFORE the <BLOCK>, and isTopLevel() is defined to take into consideration the <BLOCK> node, not the FUNCTION node. So the dereg_id is set at first param of the first function and then at each successive function block it is saved and then restored. This leads to the fact that when processing any parameters for future defined functions will encounter a dereg_id != -1 while during the processing of the BLOCK node dereg_id == -1.

1. Reset the dereg_id on ASCENT rules of topLevel blocks and don't keep any stack management. I think this is not needed as functions, procedures and triggers cannot overlap nor can be imbricated / contained one inside another, but i'm not sure about parameters for the main block / program.
2. Modify the stack management for dereg_id to save / restore it's value on descent from FUNCTION instead of BLOCK.

Good. You have found the root cause.

The first solution will not work in all cases. The reason the stack management is there is that top level blocks CAN be nested. For example, a trigger can be inside an internal procedure. Functions, internal procs, triggers are by nature all nested inside an external procedure.

The second solution will work. Add a new function that is similar to isTopLevel(), but which processes on the proper node. Internal procedures and triggers should be safe already because internal procs have define parameter statements nested in the block itself and triggers don't have parameters. Then modify the ascent and descent rules to change from isTopLevel() to your new function.

Here is the implementation. I could not fully replace the isTopLevel with the new function, as toplvl scope was used for storing also other data.

1. Added a new function in rules/include/common-progress.rules:
786: added:

      <!-- detect only true top level procedure function and triggers,
           not their inner block -->
      <function name="isTopLevelNotBlock">
         <parameter name="target" type="com.goldencode.ast.Aast" />
         <variable  name="ttype"  type="java.lang.Integer" />
         <return    name="top"    type="java.lang.Boolean" />

         <rule>top = false</rule>

         <rule>target.parent == null
            <action>top = true</action>

            <rule on="false">
                  target.type == prog.procedure or
                  target.type == prog.function  or
                  target.type == prog.trigger_block
               <action>top = true</action>
            </rule>
         </rule>
      </function>

2. And in rules/convert/variable_references.rules:
1126: added:

      <rule>evalLib("isTopLevelNotBlock", copy)
            <action>
               dereg_id = lookupDictionaryLong("toplvlnblock", "dereg")
            </action>
            <action>deleteScope("toplvlnblock")</action>
      </rule>

1143: removed 4 lines:

         <!-- restore de-registration ID -->
         <action>
            dereg_id = lookupDictionaryLong("toplvl", "dereg")
         </action>

1168: added:

      <rule>evalLib("isTopLevelNotBlock", copy)
            <action>addScope("toplvlnblock")</action>
            <action>
               addDictionaryLong("toplvlnblock", "dereg", dereg_id)
            </action>
            <action>dereg_id = #(long) (-1)</action>
      </rule>

1208: removed 4 lines:

         <!-- save off de-registration ID values and init a new one for
              this scope -->
         <action>addDictionaryLong("toplvl", "dereg", dereg_id)</action>
         <action>dereg_id = #(long) (-1)</action>

On my test cases the conversion runs fine.

Here is my feedback:

1. An update zip must be build with the full project pathing for every file.

Archive:  om_upd20120814a.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  2012-08-14 08:47   convert/
    54817  2012-08-14 08:47   convert/variable_definitions.rules
        0  2012-06-07 08:21   include/
   259208  2012-08-14 09:34   include/common-progress.rules
---------                     -------
   314025                     4 files

It should be:

p2j/rules/convert/
   p2j/rules/convert/variable_definitions.rules
   p2j/rules/include/
   p2j/rules/include/common-progress.rules

2. The copyright notice needs to be updated in both files.

3. There are typos in the comment in common-progress.rules ("tryue" and "frunction"). This is unexpected since the code as shown in the Redmine issue history (above) does not have the same typos. :)

4. The general approach with isTopLevelNotBlock() needs some modification. I can see it was just a modified version of the original, but the changes make it harder to understand. There are other changes that can be made to simplify it. I also would like a different name since this one is a but confusing for me. I think something like this is easier to understand:

      <!-- detect only true top level procedures, functions and triggers,
           and NOT the associated inner block node -->
      <function name="isTopLevelNode">
         <parameter name="target" type="com.goldencode.ast.Aast" />
         <variable  name="ttype"  type="java.lang.Integer" />
         <return    name="top"    type="java.lang.Boolean" />

         <rule>top = false</rule>
         <rule>ttype = target.type</rule>

         <!-- match an external procedure -->
         <rule>ttype == prog.block and target.parent == null
            <action>top = true</action>
         </rule>

         <!-- match any of the other top level blocks, but match at
              the main node, not at the contained block node -->
         <rule>
            ttype == prog.procedure or
            ttype == prog.function  or
            ttype == prog.trigger_block
            <action>top = true</action>
         </rule>

      </function>

- changed and added comments
- changed the name to more closely match the intent
- leveraged the ttype var to slightly improve performance
- added the test for type == block back into the portion that is meant to match the external procedure (which is a BLOCK node with no parent, a very unique case AND one that was implicit in your previous version, but needs to be more explicit to ensure that readers can make the connection)
- separated out the test of the other top level nodes since being nested is not needed (the code is simpler and easier to read as a result)

5. Make sure that all of your testcases are added to the p2j/testcases/uast/ directory. Also make sure that your testcases include examples that don't trigger the de-registration code and also some which have multiple parameters that need to be de-registered.

6. Fix the above issues and re-test on these testcases.

7. If that passes, submit the change (attach it to a Redmine issue history entry and set the status to Review).

8. If the review does not show any issues, then you need to regression test the conversion change. To regression test a change that only affects the converted code (and has no runtime implications), we do the following:
- we run conversion (using the latest P2J code WITHOUT the proposed change) on the current version of TIMCO's Majic project and we make a copy of the results
- we run conversion again (on the same Majic code), this time using the proposed change
- we compare the new results with the copied results using something like "dirdiff" or "diff" (using recursive mode), the result should only have changes that are expected and correct (if the proposed change doesn't affect Majic, then the results should be identical)
- document the results in Redmine and fix any problems found and re-run conversion using the proposed change until there are no regressions

It all looks good except that in the intervening time, the rules/include/common-progress.rules has been updated in Bazaar. Please merge on top of that version. Then re-zip the update and upload it here. Also, just convert and run your testcase as a simple check on the success of your merging.

I will do a final code review. If OK, we won't redo the regression testing. You'll just get approval to check it in and apply it to staging (Constantin can do that).

You are approved to check it in and to apply it to staging (Constantin can do that). When both are complete, distribute it as an update.

I've applied it to staging (no build or conversion needed).

The testcase still needs to be checked into Bazaar.

Sorry, I forgot to "add" the file before "commit".
The .p file should be already in the new testcases repository now.