FWD » Core Development » Base Language

Complete the support for the following classes:

• openedge.core.collections.list (partial/stubs)
• openedge.core.collections.stringcollection (partial/stubs)
• openedge.core.memptr (full/partial)
• openedge.core.string (none/none)
• openedge.core.system.argumenterror (none/none)
• openedge.core.timestamp (none/none)
• openedge.net.http.credentials (full/stubs)
• openedge.net.http.ihttpclientlibrary (partial/partial)
• openedge.net.serverconnection.clientsocket (full/stubs)
• openedge.net.uri (full/partial)
• progress.json.objectmodel.jsonarray (full/partial)
• progress.lang.class (partial/partial)
• progress.lang.parameterlist (full/basic)
• progress.lang.proerror (full/basic)
• progress.reflect.method (full/stubs)

Support should be added in task branch 6129b.

Greg Shah wrote:

Support should be added in task branch 6129b.

What is the target OE version?

OE 11.7.x is in use right now. They can't move to OE 12 without facing serious issues which have not yet been resolved.

Greg Shah wrote:

OE 11.7.x is in use right now. They can't move to OE 12 without facing serious issues which have not yet been resolved.

Greg, should I go ahead with the plan of using PROVERSION for the 4GL OO code base? I hope we can use the same skeleton even if properties/methods were only added in the newest OE versions or we can use PROVERSION with &IF preprocessor in that case.

Yes, please use PROVERSION and preprocessing to conditionally implement logic by OE version.

Greg Shah wrote:

Yes, please use PROVERSION and preprocessing to conditionally implement logic by OE version.

Is this the right repo for skeleton - https://proj.goldencode.com/downloads/skeletons/repo?

I'm trying to use preprocessor directives for added/changed methods in those provided classes, there are classes that are added later only and were not present in 11.7, should I put all class body in preprocessor for them? So it looks like the class is still there only there is no body?

On the other hand there are difference in functionality sometimes so there we will probably need to use the proversion function so runtime not compile time since there will only be one FWD implementation is it?

Greg Shah wrote:

Yes, please use PROVERSION and preprocessing to conditionally implement logic by OE version.

Greg, we do not have all OE versions available to know exactly what was changed in each version - I have now the skeleton classes generated for 11.7 and 12.2. I can start from 11.7 as baseline and use preprocessor using the major version number as differentiator, something like this:

&if entry(1, proversion, '.') = '12' &then
message 'oe12' view-as alert-box.
&else
message 'other' view-as alert-box.
&endif

Now, this only has effect at compile time - for FWD will be when the conversion is executed and that normally works fine with 4GL code that is actually converted but afraid the skeleton code doesn't fall into that category so I'm not sure using preprocessor statements in skeleton even makes sense but maybe I'm missing something.

I was looking at the differences between generated skeleton classes for OE11.7 and OE12.2 and while new members added to classes could be somehow implemented using conditional processing in OO implementation (would probably have to be done at runtime) there are some cases that I don't see how we could accommodate with only one 'version independent' implementation. For instance the inheritance chain changed in `ABLUnit` packages (`Model` and `Reflection`) where existing classes now extends other base classes or implement new interfaces. Say we somehow have a 'least common multiple' in FWD implementation so we have all members available in all OE versions (supported) and we manage using runtime version check to mimic the same functionality as in the target OE version, there will still be issues with reflection/type-of/cast because of this potential change in the inheritance.

Right now I don't think `polluting` the implementation with conditional code to support different OE version in one code base is the best idea, why don't you go with the same approach as PSC on this one and have it as an add-on (really separate project out of FWD core) and implement OE versions as needed using separate branches? Once a OE version ships the branch is sealed anyway, one can still 'upgrade' to another (newer) version proven this one don't use some new version specific additions that makes it incompatible with the previous OE core.

Please let me know which way you like us to proceed, just though to mention that (again) although I know you said you don't like version specific implementation.

11.7 and 12.2 is fine.

Now, this only has effect at compile time - for FWD will be when the conversion is executed and that normally works fine with 4GL code that is actually converted but afraid the skeleton code doesn't fall into that category so I'm not sure using preprocessor statements in skeleton even makes sense but maybe I'm missing something.

In FWD terms, the preprocessor affects parsing and the skeletons are ONLY used for parsing. The idea here is to use the preprocessor to represent the API properly between versions so we can properly parse an application that is dependent upon the API in a specific OE version.

For instance the inheritance chain changed in `ABLUnit` packages (`Model` and `Reflection`) where existing classes now extends other base classes or implement new interfaces. Say we somehow have a 'least common multiple' in FWD implementation so we have all members available in all OE versions (supported) and we manage using runtime version check to mimic the same functionality as in the target OE version, there will still be issues with reflection/type-of/cast because of this potential change in the inheritance.

I understand that PSC has made poor decisions on things and then broken the API contract over time to take a different approach. We will have to take each case as it comes.

Right now, we are primarily focused on 11.7 because the customers using these features are on that version. The 12.x stuff does not matter as much (or at all), yet. I'm sure that someday (maybe soon), it will matter when there are more 12.x customers.

Right now I don't think `polluting` the implementation with conditional code to support different OE version in one code base is the best idea, why don't you go with the same approach as PSC on this one and have it as an add-on (really separate project out of FWD core) and implement OE versions as needed using separate branches?

I prefer not to have different FWD branches for different OE versions. Do we really need to support 12.x right now?

Greg Shah wrote:

I prefer not to have different FWD branches for different OE versions.

Understood, then only one version can do it but imho it worth nothing for FWD to mimic multiple versions of OE for those OO classes provided by PSC.

Do we really need to support 12.x right now?

If there are no customers using it then most probably no. We still have OE11.7 so we can work with that version but as far as I remember the implementation was updated to 12.2, I see Vladimir already moved the assertions to 12.6 so will try to see how far can we get using preprocessors but we can then keep OE11.7 as main target and OE12.2 'extensions/fixes' are only nice to have?

we can then keep OE11.7 as main target and OE12.2 'extensions/fixes' are only nice to have?

Correct. If there are incompatibilities, please keep a careful list of the things we will have to rework later.

We do have the "luxury" of detecting problems at conversion time and mapping the resulting code completely differently at runtime. This may indeed be a problem for the casting case and it is nasty because it means we have to duplicate some amount of the class hierarchy, but it can be done. We will defer this problem until later.

Greg, is there any update on method/ctor resolution during conversion in regard with Java erasure issue caused by the `object` parameters/return type? For ctor's this doesn't seem to work at all and for methods the order of methods in the 4GL source code seems to be important so I would think updating the skeleton might need a full application code reconversion.

I've made a small change in the conversion rule for handling poly as method return type so I could get the generated skeleton to convert but for ctor erasure issue I remember you said something about reworking the whole thing at some point so thought to check with you first.

is there any update on method/ctor resolution during conversion in regard with Java erasure issue caused by the `object` parameters/return type? For ctor's this doesn't seem to work at all and for methods the order of methods in the 4GL source code seems to be important so I would think updating the skeleton might need a full application code reconversion.

Are you working off of trunk rev 14484?

Greg Shah wrote:

Are you working off of trunk rev 14484?

Conversion was ran with trunk #14483, working on 6129b with changes.

For ctor's this doesn't seem to work at all

6129b which is now in trunk revision 14484 does have all my previous work on method overloading. C'tor processing is still an issue.

for methods the order of methods in the 4GL source code seems to be important so I would think updating the skeleton might need a full application code reconversion

Yes, this order is important. We probably should eliminate this dependency but it will require a different algorithm for name/signature conflict resolution.

Greg Shah wrote:

6129b which is now in trunk revision 14484 does have all my previous work on method overloading. C'tor processing is still an issue.
Yes, this order is important. We probably should eliminate this dependency but it will require a different algorithm for name/signature conflict resolution.

Was thinking about this, the only issue right now is in JsonArray ctor that can takes either an array of JsonObject or JsonArray so the erasure on object must be solved somehow... but, I imagine in real life application code this could be a common scenario as well.

I'm sure this might have been considered before, early mornings thoughts only looks good at times, but thought to mention it just in case... the basic idea would be to generate (during conversion) simple wrappers for OO classes that only extends object (which can be made abstract) for a specific PLO type and then use those wrappers in ctor/method signature during conversion. Of course this will require the same handling for when a variable/property of given type is defined/instantiated but it will probably alleviate (if not completely fix) the whole mapping processing during conversion.

public class JsonArrayRef extends object<JsonArray> { ... }

public class JsonObjectRef extends object<JsonObject> { ... }

...
public void __progress_json_objectmodel_JsonArray_constructor__(final JsonObjectRef[] _objectRef) { ... }
public void __progress_json_objectmodel_JsonArray_constructor__(final JsonArrayRef[] _objectRef) { ... }

Marian: Please summarize the current level of conversion support for all of the classes #6410-1. We need all of these to be finished (conversion support with runtime stubs) by the end of April 2023.

Greg Shah wrote:

Marian: Please summarize the current level of conversion support for all of the classes #6410-1. We need all of these to be finished (conversion support with runtime stubs) by the end of April 2023.

Greg, will pick that up tomorrow and should be done by end of next week for sure, just have to check if I've lost something after the server crash :(

That ctor erasure issue remains for JsonArray, guess my proposals doesn't make much sense so not sure how to get around this one right now.

Branch 6410a has been branched from trunk 14552. I've pushed it to xfer (/opt/fwd/6410a).

7199c rev 14559 contains the implementation for OpenEdge.Core.System.ArgumentError. Required by #7199

When converting skeleton classes there seems to be some issue with classes that implements interfaces with properties, only if the interface is part of `OESDK`, if the interface is also part of application code the conversion works fine.

This works:

interface convert.ITest:
   define public property Connected as logical no-undo
      public get.
end.

class convert.TestImpl implements convert.ITest:
   define public property Connected as logical no-undo
      public get.
end.

This doesn't convert, the skeleton interface (`ServerConnection`) has the property Connected:

class convert.TestImpl implements OpenEdge.Core.ServerConnection.IServerConnection:
   define public property Connected as logical no-undo
      public get.
end.

When converting this it gives the following error and it fails to convert:

Failure in file './convert/TestImpl.cls':
com.goldencode.ast.AstException: Error processing ./convert/TestImpl.cls
    at com.goldencode.p2j.uast.AstGenerator.processFile(AstGenerator.java:1025)
    at com.goldencode.p2j.uast.ScanDriver.lambda$0(ScanDriver.java:398)
    at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:434)
    at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:263)
    at com.goldencode.p2j.convert.TransformDriver.runScanDriver(TransformDriver.java:390)
    at com.goldencode.p2j.convert.TransformDriver.front(TransformDriver.java:255)
    at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:986)
    at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1284)
    at testcases.Convert.main(Convert.java:22)
Caused by: java.lang.NullPointerException: null value for annotation 'set-javaname':define [DEFINE_PROPERTY]:1546188226574 @2:4
   public [KW_PUBLIC]:1546188226576 @2:11
   Connected [SYMBOL]:1546188226580 @2:27
   as [KW_AS]:1546188226582 @2:37
      logical [KW_LOGICAL]:1546188226584 @2:40
   no-undo [KW_NO_UNDO]:1546188226586 @2:48
   get [KW_GET]:1546188226588 @3:14
      public [KW_PUBLIC]:1546188226594 @3:7

    at com.goldencode.ast.XmlFilePlugin.writeSingleAnnotation(XmlFilePlugin.java:1154)
    at com.goldencode.ast.XmlFilePlugin.writeAnnotations(XmlFilePlugin.java:1115)

For what is worth, changing the name of property in class solves the issue.

Constantin Asofiei wrote:

7199c rev 14559 contains the implementation for OpenEdge.Core.System.ArgumentError. Required by #7199

Can that be pushed in 6410a on xfer?

Marian Edu wrote:

Constantin Asofiei wrote:

7199c rev 14559 contains the implementation for OpenEdge.Core.System.ArgumentError. Required by #7199

Can that be pushed in 6410a on xfer?

Done in 6410a/14553. Please review.

Regarding the conversion issue with the interface property defined in a skeleton class: this needs to be fixed in FWD. Please test this change in ClassDefinition.registerPropertyMethod line 820:

      else if (override && (javaname = lookupOverrideName(legacySig, override, true)) != null )
      {
         // first time in (for an override); register only if the parent defines this accessor
         addJavaMethodName(javaname, jparms, legacySig);

         node.putAnnotation(key, javaname);
      }

Constantin Asofiei wrote:

Done in 6410a/14553. Please review.

Got it, thanks, will see if there is anything missing.

Regarding the conversion issue with the interface property defined in a skeleton class: this needs to be fixed in FWD. Please test this change in ClassDefinition.registerPropertyMethod line 820:
[...]

Yes, that works, thanks.

Marian, what skeleton revision did you use for the skeleton patch? I can't apply it cleanly. I've pushed skeleton rev 116 to xfer - please create the patch on this version.

Marian, something else to note: I'm looking at the patch, and I don't understand something: the Java method name for some OO skeleton methods were renamed. What reason was for this? We need to 'freeze' this for existing skeletons implemented or stubbed in FWD. FWD takes care of using whatever Java method name is at the implementation, automatically, for a OE method call.

Also, 6410a branch is on xfer - do you still have problems using bzr on xfersrv01? If not, please use 6410a for the changes.

Constantin Asofiei wrote:

Marian, what skeleton revision did you use for the skeleton patch? I can't apply it cleanly. I've pushed skeleton rev 116 to xfer - please create the patch on this version.

I think whatever was on xfer when I did the patch, I will create a new patch after updating from xfer.

Constantin Asofiei wrote:

Marian, something else to note: I'm looking at the patch, and I don't understand something: the Java method name for some OO skeleton methods were renamed. What reason was for this? We need to 'freeze' this for existing skeletons implemented or stubbed in FWD. FWD takes care of using whatever Java method name is at the implementation, automatically, for a OE method call.

Understood, I can rollback those changes, wasn't sure if we must change the code to match the new output from current conversion or not... it does look like the new one is smarter though :)

Also, 6410a branch is on xfer - do you still have problems using bzr on xfersrv01? If not, please use 6410a for the changes.

I was using 6410a from xfer, are you saying this one was updated in between?

Marian Edu wrote:

Also, 6410a branch is on xfer - do you still have problems using bzr on xfersrv01? If not, please use 6410a for the changes.

I was using 6410a from xfer, are you saying this one was updated in between?

I mean instead of posting patches, commit directly to 6410a on xfer.

Constantin Asofiei wrote:

7199c rev 14559 contains the implementation for OpenEdge.Core.System.ArgumentError. Required by #7199

7199c was merged to trunk rev 14610 and archived

Constantin Asofiei wrote:

Marian Edu wrote:

Also, 6410a branch is on xfer - do you still have problems using bzr on xfersrv01? If not, please use 6410a for the changes.

I was using 6410a from xfer, are you saying this one was updated in between?

I mean instead of posting patches, commit directly to 6410a on xfer.

Completely forgot about this, reverted method signature changes and pushed rev #14554 of 6410a on xfer.

Constantin: Have you reviewed 6410a? I'd like to get it into trunk.

Marian: We urgently need the runtime for Progress.Lang.Class:getMethods() (#7460). How quickly can that be ready?

Marian, please upload a skeleton patch using the latest skeleton project from xfer (rev 116).

Greg, I've taken a look at 6410a, and the main issue is changing the return type for i.e. LegacyClass.invoke, from BaseDataType to Object. I understand this is to support extent return values, but this is not that simple. The ripples go beyond just changing the return type. For example, this code:

def var lc as progress.lang.class.
def var l as oo.foo.
l = new oo.foo().
l:p1 = "abc".

lc = get-class("oo.foo").

function func0 returns int (input v1 as char).
   message "func0" v1.
end.

func0(lc:getPropertyValue(l, "p1")).
func0(lc:invoke(l, "m1")).

now (with trunk) gets converted to:

         func0(new character(lc.ref().getPropertyValue(l, new character("p1"))));
         func0(new character(lc.ref().invoke(l, new character("m1"))));

Currently, as getPropertyValue returns Object, this line will not compile in Java. If we change invoke to return also Object, func0(new character(lc.ref().invoke(l, new character("m1")))); will no longer compile, also. I can't say for sure which customers will be affected, but if we take this 6410a version as is, we can't merge to trunk until we conversion test all apps.

What do you suggest?

We've discussed this POLY issue with extent in #5729. But I can't find if we made a dedicated task for making POLY work with extents.

I can clean up 6410a to merge it to trunk, but I need to see the latest skeleton patch, to have a better view of the changes (as they are linked) and to determine if apps would need a full reconversion or not.

Constantin Asofiei wrote:

Marian, please upload a skeleton patch using the latest skeleton project from xfer (rev 116).

Constantin, I did update skeleton to rev #116 and the skeleton diff look exactly the same as the one I've uploaded before. The content was generated from 4GL using reflection, the methods were ordered alphabetically so that's why it might looks like there are more changes than what you've expected? Imho there should be some generator to create the skeleton based on certain OE version and then use that as-is instead of manually adding methods/properties... it's a pain in the *** to find out what was changed :(

I think the generator previously ordered the methods/properties as defined in the source code, only at some point PSC decided to use include files for quite a few of those classes and the order completely changed, a simple diff using text compare is next to useless hence the change to use the alphabetic order.

Did reverted back to use BDT instead of Object for `invoke` method, some fixes for `setParameter` in `ParameterList` to accommodate the 2 parameters overload - committed rev #14555 of 6410a on xfer, please review.

Let me know if the `getMethods` implementation should be done on this branch or another one.

I think that was meant to be getQualifiedJavaName, not getQualifiedName.

Constantin Asofiei wrote:

I think that was meant to be getQualifiedJavaName, not getQualifiedName.

That did the trick, thanks Constantin.

One question here, I'm looking at the 'invoke' method and that was implemented straight into the class (LegacyClass) although it might make more sense to have that logic moved in method/ctor... should I do that change or leave it where it is and callback from method into the class by passing the method's name (like I don't already have the method)?

Thanks

Marian Edu wrote:

One question here, I'm looking at the 'invoke' method and that was implemented straight into the class (LegacyClass) although it might make more sense to have that logic moved in method/ctor... should I do that change or leave it where it is and callback from method into the class by passing the method's name (like I don't already have the method)?

The problem here is that LegacyClass.invoke resolves the target at each invocation, depending on the arguments. With the Legacy4GLMethod, this is already resolved (FWD must call the converted Java method associated with this exact legacy method). There is ControlFlowOps.invokeLegacyMethod(InternalEntry, which I think needs to be called in case of Legacy4GLMethod.invoke - and the InternalEntry needs to be resolved using the parameter types defined at the Legacy4GLMethod.

Constantin Asofiei wrote:

The problem here is that LegacyClass.invoke resolves the target at each invocation, depending on the arguments.

Not sure about this but I expect some (more like the same) method resolution to be performed when using GetMethod on the Class.

With the Legacy4GLMethod, this is already resolved (FWD must call the converted Java method associated with this exact legacy method). There is ControlFlowOps.invokeLegacyMethod(InternalEntry, which I think needs to be called in case of Legacy4GLMethod.invoke - and the InternalEntry needs to be resolved using the parameter types defined at the Legacy4GLMethod.

Yes, that was my concern as well... if we just pass the parameters to the class's invoke it might be a different method that is being executed :(

Marian Edu wrote:

Constantin Asofiei wrote:

The problem here is that LegacyClass.invoke resolves the target at each invocation, depending on the arguments.

Not sure about this but I expect some (more like the same) method resolution to be performed when using GetMethod on the Class.

If I understand correctly, leave the resolution logic at LegacyClass.getMethod, and you can cache the InternalEntry at Legacy4GLMethod. So we don't resolve the target on each invoke.

Constantin Asofiei wrote:

Marian Edu wrote:

Constantin Asofiei wrote:

The problem here is that LegacyClass.invoke resolves the target at each invocation, depending on the arguments.

Not sure about this but I expect some (more like the same) method resolution to be performed when using GetMethod on the Class.

If I understand correctly, leave the resolution logic at LegacyClass.getMethod, and you can cache the InternalEntry at Legacy4GLMethod. So we don't resolve the target on each invoke.

Sounds like a plan.

Greg, I have this patch for an issue with Progress.Lang.Class:invoke - we need to re-throw the structured exception. Should I include it in 6410a or in a different branch? As this needs to be in 7156b.

--- old/src/com/goldencode/p2j/oo/lang/LegacyClass.java
+++ new/src/com/goldencode/p2j/oo/lang/LegacyClass.java
@@ -967,6 +967,10 @@
             }
          }
       }
+      catch (LegacyErrorException lex)
+      {
+         throw lex;
+      }
       catch (ErrorConditionException ece)
       {
          ErrorManager.setSilent(silent);

Do you think we are ready to merge 6410a to trunk?

Greg Shah wrote:

Do you think we are ready to merge 6410a to trunk?

No. Method:invoke and Class:getMethod(s) are not implemented.

OK, then let's get this patch into trunk using a different branch.

From Marian Edu:

think I've stumbled upon the same problem before but can't remember exactly the use case nor the solution, or it might be just a wrong recollection... I need to have some 'internal' instances created for relection - method, parameter - to which the ctor is private and I need only to instantiate internally and pass some java objects as input so probably a simple Java ctor like for LegacyMethod is to be used, problem is how to make those internal instance to be recognised as 'valid object'. I this ObjectOps's 'newInstanceInternal' does the magic needed for this but not sure, it feels wrong to have 'getMethodInstance' and 'getParameterInstance' in ObjectOps... is there another (better) way?

Marian, can these be created outside of FWD runtime (i.e. by the application?) If these can only be created by FWD runtime, then you can implement _BaseObject_.isTracked to return false, and they will always be valid, without any reference tracking. I assume there are no destructors to be executed anyway, correct? The point here is to carefully manage these instances as singleton (maybe cache them), as I assume the state can't be changed for these.

And if the above is true, to create these you can add a Legacy4GLMethod.newInstance(), bypass all legacy execute/constructor (or just invoke them manually). The fields most likely are no-undo, so use TypeFactory instead of UndoableFactory.

The other part we need to check is how Progress.Lang.Object:next-siblign works for a Progress.Reflect.Method instance (same for enums, I don't recall testing these). But this may be another issue which can be solved via ObjectOps.registerObject().

Constantin Asofiei wrote:

Marian, can these be created outside of FWD runtime (i.e. by the application?) If these can only be created by FWD runtime, then you can implement _BaseObject_.isTracked to return false, and they will always be valid, without any reference tracking. I assume there are no destructors to be executed anyway, correct? The point here is to carefully manage these instances as singleton (maybe cache them), as I assume the state can't be changed for these.

Thanks, the 4GL ctor is private so this should do.

The other part we need to check is how Progress.Lang.Object:next-siblign works for a Progress.Reflect.Method instance (same for enums, I don't recall testing these). But this may be another issue which can be solved via ObjectOps.registerObject().

Did a quick check and no 'reflection' instances shows in the session objects although those are 'valid-objects' - in fact not even the class instance shows in the list so I guess we do not have this problem, 4GL treats those as 'internal' as well.

Please check also what Progress.Reflect.Method:next-sibling returns - is it always unknown? If yes, maybe this (and others in BaseObject) need to return unknown if isTracked returns false.

Constantin Asofiei wrote:

Please check also what Progress.Reflect.Method:next-sibling returns - is it always unknown? If yes, maybe this (and others in BaseObject) need to return unknown if isTracked returns false.

For all 'internal' objects the valid-object on the instance itself returns true, both Next-Sibling/Prev-Sibling returns unknown for those and none of them shows in the list of objects in the current session.

Marian Edu wrote:

Constantin Asofiei wrote:

Please check also what Progress.Reflect.Method:next-sibling returns - is it always unknown? If yes, maybe this (and others in BaseObject) need to return unknown if isTracked returns false.

For all 'internal' objects the valid-object on the instance itself returns true, both Next-Sibling/Prev-Sibling returns unknown for those and none of them shows in the list of objects in the current session.

Thanks, so we need what I mentioned before, we need to fix BaseObject.getNextSibling and similar to return unknown for untracked widget instance.

Marian Edu wrote:

Constantin Asofiei wrote:

Please check also what Progress.Reflect.Method:next-sibling returns - is it always unknown? If yes, maybe this (and others in BaseObject) need to return unknown if isTracked returns false.

For all 'internal' objects the valid-object on the instance itself returns true, both Next-Sibling/Prev-Sibling returns unknown for those and none of them shows in the list of objects in the current session.

So, I guess getNextSibling/getPrevSibling methods in BaseObject needs to be updated to return unknown if object isn't tracked.

Constantin Asofiei wrote:

Thanks, so we need what I mentioned before, we need to fix BaseObject.getNextSibling and similar to return unknown for untracked widget.

Right, will do that in 6410 ok?

Marian Edu wrote:

Constantin Asofiei wrote:

Thanks, so we need what I mentioned before, we need to fix BaseObject.getNextSibling and similar to return unknown for untracked widget.

Right, will do that in 6410 ok?

Great, thanks.

Constantin: Please review.

We need to get our OO support to the 100% mark sooner rather than later. Otherwise we will be constantly scrambling to deal with things that present as bugs but are really just missing features. So my initial idea is that we should go ahead and finish all of pending features for getMethods.

6410a was rebased from trunk rev 14747 - new rev 14777.

I'm putting this into conversion testing of some projects.

Branch 6410a was merged to trunk rev 14751 and archived.

Marian: please let us know when we can create branch 6410b.

Constantin Asofiei wrote:

Branch 6410a was merged to trunk rev 14751 and archived.

Marian: please let us know when we can create branch 6410b.

Constantin, if you want me to close the remaining issues then please go ahead and make the new branch and will pick it up from there. I will go over the unit tests and update the ones related to methods.

Thanks

In addition to the remaining items in getMethods(), what work is left from #6410-1?

Greg Shah wrote:

In addition to the remaining items in getMethods(), what work is left from #6410-1?

• openedge.core.collections.list (stubs)
• openedge.core.collections.stringcollection (stubs)
• openedge.core.memptr (full) - resource support level needs to be updated
• openedge.core.string (full) - resource support level needs to be updated
• openedge.core.system.argumenterror (full)
• openedge.core.timestamp (stubs)
• openedge.net.http.credentials (full) - resource support level needs to be updated
• openedge.net.http.ihttpclientlibrary (full) - resource support level needs to be updated
• openedge.net.serverconnection.clientsocket (stubs)
• openedge.net.uri (full) - resource support level needs to be updated
• progress.json.objectmodel.jsonarray (full) - resource support level needs to be updated
• progress.lang.class (partial) - missing ctor's, properties, variables, events, invoke needs some attention
• progress.lang.parameterlist (full) - resource support level needs to be updated
• progress.lang.proerror (full) - the callstack is only needed if enabled for session (SESSION:ERROR-STACK-TRACE), think there was a discussion about it in ABLUnit thread
• progress.reflect.method (partial) - imho the invoke logic should be moved here from LegacyClass, some issues with dataset-handle, table-handle parameters, those are annotated as handle during conversion - we can either fix it there or need to check the java method parameter type.

Please let me know when a new branch is available for this on xfer and what needs to be done beside finishing the method/invoke support.

Created task branch 6410b from trunk rev 14756.

The branch is available on xfer.

Constantin Asofiei wrote:

Created task branch 6410b from trunk rev 14756.

The branch is available on xfer.

Thanks, just to confirm I was able to pull it down.

Related to the parameter annotation for dataset-handle/table-handle is it ok to leave those annotated as simply handle or it should get fixed in conversion? Right now we only use the annotations for building the parameter 'signature', qualified also seems to be all lowercase for whatever reason :(

what needs to be done beside finishing the method/invoke support.

Please do implement all the remaining items as documented in #6410-72.

Marian, I think we need 'invokeStandalone' to be emitted when the returned value is not read and 'invoke' in all other cases - which will force the method to have something to return, otherwise raise the 15304 error. Note that LegacyClass.invokeStandalone already exists - look in annotations/method_defs.rules at line 960:

         <rule>ref.isAnnotation("qualified") and
               getReferenceNoteString(ref.id, "qualified").equalsIgnoreCase("Progress.Lang.Class")
            <action>copy.putAnnotation("javaname", "invokeStandalone")</action>
         </rule>
      </rule>
   </walk-rules>

Marian, please let me know when you have everything committed to 6410b, so I can get it from xfer, to rebase it.

Constantin Asofiei wrote:

Marian, please let me know when you have everything committed to 6410b, so I can get it from xfer, to rebase it.

Constantin, you can go ahead and rebase it and i will merge afterwards anyway, i have a local git repo so won’t lose anything…

6410b is now on trunk rev 14790 and was pushed to xfer.

I'd like to get the next phase of #6410 support into code review if it is far enough along.

Constantin Asofiei wrote:

Marian, I have this patch to properly report datasethandle/tablehandle vs dataset/table - please review. I plan to integrate it in 7156b. Do you have work on 6410b? If so, please integrate it in that, otherwise I'll use another branch.

Hi Constantin, thought you modified the conversion rules or something to be more specific about the handle in the signature... I've taken almost the exact path as you meanwhile, I will push some of current changes in bazaar today.

I'm trying to fight a case when invalid parameters are being sent to invoke, the error message contains the caller method/procedure and I think because of the block added to invoke this is 'polluting' the call-stack, in FWD the caller is reported as invoke while in 4GL it is the original caller where invoke was used... I can drop the function block there but then you said there might be issues with error handling, I've couldn't come up with any solution so far :(

Constantin Asofiei wrote:

Hmm... this may mean that OpenEdge does not report the internal classes in the callstack. Can you get some other cases where an internal OpenEdge class call fails, and check the callstack?

Nope, seems to only be something specific to the invoke method - although might well be other 'internal' methods of classes from the Progress namespace.

Marian Edu wrote:

... because of the block added to invoke this is 'polluting' the call-stack

Was this block added in 6410b? I don't see any BlockManager API in trunk.

Constantin Asofiei wrote:

Marian Edu wrote:

... because of the block added to invoke this is 'polluting' the call-stack

Was this block added in 6410b? I don't see any BlockManager API in trunk.

I did added that as per your code review, the invoke method in LegacyClass had/has no block - for LegacyMethod I've tried to add one, it does kinda work but has that nasty side-effect. I understand why is behaving like it does but as said 4GL seems to not add this call to invoke into the call-stack, regular class methods call works fine.

Constantin Asofiei wrote:

Marian, see another patch - please integrate it also in 6410b.

Had to manually apply that, for Call there seems to be a few revisions missing - you've sent `014`, what was in 6410b was up to `011`.

I see these commits on xfer/6410b:

-----------------------------------------------------------
revno: 14792
committer: Marian Edu<marian.edu@acorn.ro>
branch nick: 6410b
timestamp: Wed 2023-11-08 14:09:44 +0200
message:
  add no-error option for create dataset/datasource
------------------------------------------------------------
revno: 14791
committer: Marian Edu<marian.edu@acorn.ro>
branch nick: 6410b
timestamp: Wed 2023-11-08 14:05:34 +0200
message:
  import call params patch

If you have nothing else to commit, I'll rebase.

Constantin Asofiei wrote:

If you have nothing else to commit, I'll rebase.

Give me 10-15 mins please to commit some more...

Marian Edu wrote:

Constantin Asofiei wrote:

If you have nothing else to commit, I'll rebase.

Give me 10-15 mins please to commit some more...

OK, let me know when it's done.

Constantin Asofiei wrote:

OK, let me know when it's done.

All done what I've could get 'proper' enough... rev #14794. Let me know when done with the rebase and will pull it down afterwards. Thanks

Starting rebase now.

6410b rev 14794 was rebased from trunk rev 14824 - new rev 14828.

Marian, please remind me of your issues - I plan to look at them over the next week. Also, point me to any test representative for these issues. And make sure all relevant changes are committed to 6410b before you leave today.

6410b rev 14828 was rebased from trunk rev 14856 - new rev 14860

What is the status of this work? Are we close to being able to merge it?

Marian: we will get #6410-81 and #6410-84 patches to trunk via #8238. We need to rebase 6410b, please let me know when is a good time (all is committed to xfer).

Constantin Asofiei wrote:

Marian: we will get #6410-81 and #6410-84 patches to trunk via #8238. We need to rebase 6410b, please let me know when is a good time (all is committed to xfer).

Constantin, please go ahead and rebase that and will pick it up and merge on our side as needed. Thanks

Constantin, are there any changes in trunk pertaining conversion especially for classes extending/implementing skeleton classes? We kept getting conversion issues that we had to manually fix (after ignoring the conversion error). I've added some sample source code in `tests/convert` folder in testcases (rev #1566) that should show the issue we had with it, I do have a local patch for it in case it wasn't already reported/fixed. Basically the changes are in @SymbolResolver for resolving getter/setter for extent properties in builtin classes ('bulk') and in ClassDefinition to avoid NPE (null setter java method if interface/base class only have a getter defined), this is also only for when implementing/extending built-in classes. Please let me know if this is fixed already and you can merge it in 6410 or we keep the changes and push them in 6410.

Thanks.

Some notes from Marian:

We had constantly issues with conversion when classes extended/implemented built-in classes (skeleton), I've made a note for this on the #6410-100 and I will try to see if this was already solved in trunk otherwise we keep the current local patch - otherwise on each conversion we had to manually fix the converted code and that was anything but fun :(

I expect we will close #6410 this week, we are working a bit more on collection/list/iterators tests trying to figure out how the implementation actually looks like - the issue here is that the 4GL implementation is based on temp-table (AbstractTTCollection) and both Collection and List have constructors that can take a temp-table handle as parameter. Normally in FWD implementation we do not use temp-table, there is some support for this on AbstractTTCollection and Collection but we really didn't test that part. It's probably useless to implement that as I don't expect those to be extended in the first place but the possibility is there so god knows what developers can do... my preference would be to simply remove those constructors so we easily find out if used in client's code base and maybe only implement that part when/if needed, Greg?

It's probably useless to implement that as I don't expect those to be extended in the first place but the possibility is there so god knows what developers can do... my preference would be to simply remove those constructors so we easily find out if used in client's code base and maybe only implement that part when/if needed

Agreed. Please add some comments in the class javadoc about this "intentional limitation" where we are avoiding any features that directly map to the temp-table approach used in the 4GL.

The code in `p2j.oo` almost exclusively use `returnNormal` and expect the execution to end there instead of the `storeReturnValue` paired with a Java `return` as generated by conversion lately. Is `returnNormal` even used outside of the built-in classes? Wouldn't just change the `returnWorker` to throw the `ReturnUnwindException` always unless `STORE` option used?

Marian Edu wrote:

The code in `p2j.oo` almost exclusively use `returnNormal` and expect the execution to end there instead of the `storeReturnValue` paired with a Java `return` as generated by conversion lately. Is `returnNormal` even used outside of the built-in classes? Wouldn't just change the `returnWorker` to throw the `ReturnUnwindException` always unless `STORE` option used?

FWD in BlockManager now has 'safety' code to allow returnNormal from p2j.oo code to work as before the storeReturnValue changes. But, if you write new Java code in p2j.oo, use storeReturnValue(); return; but only if this actually exits the function block. I mean, you can't use this pattern from a REPEAT block.

Constantin, I'm trying to rerun all tests for method invoke and parameter list but those fails to convert now because of the method lookup, there seems to be no real support for 'poly' parameters in there... both 'exact' and 'flex mode' match are using ParameterKey's 'typeEquivalent' and the 'Object' simply does not equal any of the other data types we're using there :)

I see there is a new property 'dynamicPoly' that is not really used, thought to set that to true if type is 'bdt' ('Object') and mode is INPUT but it just feels wrong to let the ParameterKey if it's type equivalent, imho that depends on how it is used - it is from the caller or callee, it has no knowledge of that.

Unless that was already fixed in another branch that we can merge back in 6410 I'm thinking to change the method lookup in ClassDefinition, I could use the 'dynamicPoly' property but I'm not sure I got its meaning right... should that be true for input BDT/Object parameter or what is it used for exactly?

We have tests for `ParameterList` that are failing for extent parameters used as output/input-output. As it is now there is no way to make this work since the conversion is passing the array as input to the `setParameter` method so I don't think we can turn that into an `OutputExtentParameter`/`InputOutputExtentParameter`. Imho this can only be solved during conversion like a special case for `setParameter` method when the mode is output/input-output and value is an extent, thoughts?

Marian Edu wrote:

We have tests for `ParameterList` that are failing for extent parameters used as output/input-output. As it is now there is no way to make this work since the conversion is passing the array as input to the `setParameter` method so I don't think we can turn that into an `OutputExtentParameter`/`InputOutputExtentParameter`. Imho this can only be solved during conversion like a special case for `setParameter` method when the mode is output/input-output and value is an extent, thoughts?

This gets tricky if you pass an extent field via ::. But yes, this would need to get solved via conversion, but you don't know which would be used, OutputExtentParameter or InputOutputExtentParameter (as the mode can be a variable, not literal), so we need something different.