FWD » Core Development » Base Language

Please check this 4GL code into the testcases project and post the name here.

Please check the uast/scoping/local-variable-in-query.p.

• most of affected files moved local variables from class scope to anonymous Block of execute() method scope.
• there are some files that were defining a duplicate for same variable, using the init() to set the 'external': ExpAddco.this.ipinCustNum = ipinCustNum;. These were completely removed. This is a good thing, as they were not used in the method at all.
• if two or more functions had the same name of parameters and they were promoted, the RefN suffix was appended to make them different. Since they are not promoted any more, they can keep their original name (of course, in java dialect). The second good thing.
• there is one regression in conversion: the AircraftProgram failed to compile because curAircraft is used in the EmbeddedAssignmentExpr0 class that is defined at the 'top' level in the public class, where the variable is not visible. After moving manually the inner class to anonymous Block of execute() method scope (there is an inner class Validation0 at same level), the Majic compiled without any warnings.

I think that is the correct location/scope of the inner EmbeddedAssignmentExpr0 class (like the inner Validation classes). I'm checking now why it was 'promoted' like the normal variables.

Ovidiu Maxiniuc wrote:

I think that is the correct location/scope of the inner EmbeddedAssignmentExpr0 class (like the inner Validation classes). I'm checking now why it was 'promoted' like the normal variables.

In my recent changes, where I moved record buffers from the top level to the more specific scopes, I tried to minimize the change by only moving those constructs which referenced the buffers (validation expressions, client where expressions, query substitution expressions). I did not make any changes to embedded assignment expressions.

That's exactly what I should do in this regard, too: to move the embedded assignment expressions to a more specific scope. I looked into trpl code and I saw that the embedded_assignment_inner_class_def seems to be attached to a getSectionAnchor(java.cs_inner_classes). However, it is not the most specific section.
And while checking the expressions.rule I saw that some other inner classes might be linked to same anchor, including: HandleExpr, PutExpr, ExportExpr. I need to test these too, to be sure.

See get_top_level_section_anchor in common-progress.rules. I use this as a substitute for getSectionAnchor.

Eric Faulhaber wrote:

See get_top_level_section_anchor in common-progress.rules. I use this as a substitute for getSectionAnchor.

Thanks, it looks promising. It has fixed the location for my testcases.

Code review 0521a:

Looks good to me in terms of the persistence-related changes, though I did notice a typo in the header for expressions.rules: "Attaced" instead of "Attached".

Greg, please also review/comment, particularly in light of the effect on HandleExpr, PutExpr, ExportExpr, and EmbeddedAssignmentExpr.

I tested om_upd20150521a.zip with the current project. After conversion, there were a number of compile errors (actually, seems to be the same one repeated many times). The problem is that the HandleExprN inner class definitions are now scoped to the external procedure, but there are still instance variables of the top level class which reference these inner classes and try to construct instances of them.

There were 100 of these errors in total, which means there may be other errors (and categories of error) hiding behind these.

The build log and a sampling of the failing files are posted at #1868, note 56.

Please ignore my om_upd20150522a.zip. It brings nothing new, I zipped the wrong files. Sorry.

Do you have a new version that I should use for the code review?

• the dynamic-expr evaluations classes remain attached to the old location, I.e. the pBeginOutput procedure block, so they are not visible to the scope they are used. I dislike the idea of always sent them to global scope based on a particular annotation like (form-header), but link the HeaderExpr with the registerHeader construct so they share the same scope.
• is the FORM HEADER contains a local variable (to pBeginOutput), the reference to it will 'leak' to external procedure, where it is not visible, in fact. I think we should 'promote' it to solve the visibility issue. I don't know if such case exits in the customer server code, I just found this while building my testcases.

Code review 0527b:

The changes look correct to me, but I am not expert in this area of the code. Greg will be reviewing this today. What makes this "not the ideal solution" you wanted?

The good news is that I reconverted the customer server project with P2J rev. 10870 plus this update and tested the result, and this turns out to be the mother of all high-leverage fixes. It fixed over 4,000 unit tests! Nice one!

Please get this regression tested (conversion and runtime) and checked in as soon as possible, unless something needs attention after Greg's review.

Code Review om_upd20150528a.zip

1. Are the form header changes are necessary to fix this bug? If I understand correctly, the actual query-related promotion changes are as simple as just removing this code from scope_promotion.rules:

               ancestor(prog.client_where, -1)       or
               ancestor(prog.client_where_subst, -1) or
               ancestor(prog.query_subst, -1)        or

There seem to be no other non-query changes in that file. All the other changes are only to formatting or comments.

2. This code in ui_statements.rules is very fragile:

         <!-- in the case that the HEADER was declared in a procedure, we need to check if the
              bogus peer the node was 'promoted' to external procedure -->
         <rule>copy.getAncestor(-1, prog.procedure) != null
            <!-- TODO: check if same constructs can appear in functions & triggers -->
            <action>ref2 = osref.getAncestor(-1, java.static_method_call)</action>
            <while>ref2 != null
               <rule>ref2.getText().equals("externalProcedure")
                  <action>copy.putAnnotation("promoteHeaderItems", true)</action>
                  <action>ref2 = null</action>

                  <action on="false">
                     ref2 = ref2.parent.getAncestor(-1, java.static_method_call)
                  </action>
               </rule>
            </while>
         </rule>

This code might break anytime we modify the JAST structure, token types and so forth. Why can't we determine the exact place in the conversion rules where we decide to change the attachment point? The place a rule there to leave behind an annotation. This detection would operate on the Progress ASTs and thus it would be much less fragile.

3. It seems that this code in ui_statements.rules is just dead code?

      <rule>evalLib("variables") and type != prog.sys_handle and isNote("refid")

         <action>ref2 = copy.getAncestor(-1, prog.statement)</action>
         <rule>ref2 != null and ref2.isAnnotation("promoteHeaderItems")
            <!-- look for the declaration of the variable -->
            <action>ref2 = getAst(getNoteLong("refid"))</action>

            <rule>!(ref2.isAnnotation("promote"))
               <!-- promote the variable definition associated with this variable reference -->
               <!-- TODO: unfortunately this si too late, variables were already defined-->
               <action>ref2.putAnnotation("promote", true)</action>
            </rule>
         </rule>
      </rule>

Greg Shah wrote:

Code Review om_upd20150528a.zip
1. Are the form header changes are necessary to fix this bug? If I understand correctly, the actual query-related promotion changes are as simple as just removing this code from scope_promotion.rules:
[...]
There seem to be no other non-query changes in that file. All the other changes are only to formatting or comments.

The FORM HEADER will generate HeaderExpr inner classes that were places in the public class section. Since the local variables that they returned are now local (only in the scope they are used), the HeaderExpr won't be able to access them.

2. This code in ui_statements.rules is very fragile:
[...]
This code might break anytime we modify the JAST structure, token types and so forth. Why can't we determine the exact place in the conversion rules where we decide to change the attachment point? The place a rule there to leave behind an annotation. This detection would operate on the Progress ASTs and thus it would be much less fragile.

That's the place where the we decide to create the attachment point. Please see above line where the java.bogus peer AST is created. What I am trying to do the the while loop is to verify if the attachment was done under the external procedure for a FRAME HEADER declared in an internal one in P4GL code. In this case the promoteHeaderItems annotation is left behind to let the items from header that they need to be promoted.
The when attaching the inner classes definitions the promoteHeaderItems annotation is checked in expressions.rules and the HeaderExpr are attached to correct scope.

3. It seems that this code in ui_statements.rules is just dead code?
[...]

This is true. As you probably have seen when converting the testcase I mentioned in note 22, the variable is not promoted because this is processed too late. I considered it too risky to change the order of variable_definition after the ui_statements.
I decided to keep it at least as documentation. The good part is that there are no such cases in either of the two projects.

OK, I've dug into this code more deeply. I think I understand better what you are trying to do.

I still believe that the changes in ui_statements.rules are not correct.

In the while loop we are trying to determine if the header inner class needs promotion because the registration is being done in the external procedure. I believe you are wrong about the fact of where we determine that the registration is to be moved. By the time the bogus node is created, the "decision" is already known. We can make this code less fragile and more clear by adding processing at annotations time. The location of the registration is determined by the existence and location of the FRAME_ALLOC node. That is the frame-specific node that causes the open scope. This code in ui_statements.rules "finds" that node:

         <action>osref = copy.getImmediateChild(prog.kw_form, null)</action>
         <action>osref = getAst(#(java.lang.Long) osref.getAnnotation("frame-id"))</action>

After this osref is either null or it is the frame_alloc node. This node is artificial and is created in annotations/frame_scoping.rules. Anytime after that, we can determine the decision that will be made.

If that node exists under the external procedure BUT the current form header node is inside an internal procedure, then we put the promoteHeaderItems annotation into the form_header node. I think this code would do it:

<rule>
   type == prog.statement            and 
   downPath("KW_FORM/KW_HEADER")     and
   descendant(prog.content_array, 1) and
   copy.getAncestor(-1, prog.procedure) != null)

   <action>osref = copy.getImmediateChild(prog.kw_form, null)</action>
   <action>osref = getAst(#(java.lang.Long) osref.getAnnotation("frame-id"))</action>

   <rule>osref != null and osref.getAncestor(-1, prog.procedure) == null
      <action>copy.putAnnotation("promoteHeaderItems", true)</action>
   </rule>

</rule>

This won't be fragile. It is (in my opinion), easier to understand.

If you then do this in a new annotations rule set that executes AFTER frame_scoping.rules but BEFORE scope_promotion.rules, then you can change scope_promotion.rules to check for the existence of promoteHeaderItems and cause proper variable promotion in the normal way. This would eliminate the need for the second code block in ui_statements.rules (the code that is "dead" today). Again, I think this is a better approach.

What do you think? Did I miss something or misunderstand something? If not, then ui_statements.rules would not be changed at all and the changes would be placed back in annotations.

Code Review om_upd20150605a.zip

I'm good with this approach. You can check it in if it passes testing.

Update 0605a did well with the full group of search unit tests.

The update passed the regression testing, was committed to bzr as revno 10874 and it was distributed by mail.