FWD » Core Development » Base Language

Implement complete support for both OO 4GL and structured error handling.

Object Oriented (OO) 4GL

• CLASS (only USE-WIDGET-POOL and SERIALIZABLE are not yet implemented)
• INTERFACE
• USING (still need to add support for unqualified references)
• CONSTRUCTOR (runtime support for static constructors is still needed)
• DESTRUCTOR
• DEFINE PROPERTY, including getters and setters
• METHOD (parameter mode enhancements still needed for overload matching)
• data members including instantiation, scoping and life cycle
• variable/parameter definitions AS CLASS or LIKE VAR_CLASS or LIKE FIELD_CLASS
• FIELD_CLASS support
• VALID-OBJECT()
• CAST() and DYNAMIC-CAST()
• TYPE-OF()
• DYNAMIC-INVOKE()
• instantiation of objects via the NEW() function, NEW statement, DYNAMIC-NEW or Progress.Lang.Object:New()
• DELETE
• THIS-OBJECT (language statement and as a reference to the current class instance)
• SUPER (language statement)
• SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT (along with normal traversal using NEXT-SIBLING, PREV-SIBLING)
• dereferencing objects including both method calls, data member access and chaining
  data members including variables, temp-tables
• objects as shared variables and array elements (dynamic or static usage)
• prodatasets data members including chaining and proper scoping
• DEFINE EVENT and CLASS event support (needs runtime implementation)
• ENUMs and bitwise AND, OR, NOT (implemented in #4349)

Structured Error Handling

• CATCH (does this need to be moved to a delegated model in the runtime? also, the built-in 4GL error classes probably need to inherit from RuntimeException)
• FINALLY
• ON event THROW (needs runtime)
• UNDO THROW (needs runtime)
• ROUTINE-LEVEL and BLOCK-LEVEL statements (needs runtime)
• block option STOP-AFTER (needs runtime)

Some comments from people at PSC:

1. Methods act like internal procedures when there is no return value and act like user-defined functions when there is a return value.
2. In the 4GL object inheritance is actually implemented under the covers using persistent procedures. They mentioned that this was the cause of a severe performance issue they have when classes have a deeply nested inheritance hierarchy.

Both of these things should be tested to confirm our understanding.

In #3308-252, Constantin found that the OO 4GL supports "duck typing".

Some of the built-in classes provided by OpenEdge are not documented in the 4GL reference PDF. This online doc is a place to look for such classes:

https://documentation.progress.com/output/oehttpclient/oe117/

Comments from a customer 4GL developer:

• ABL garbage collection is based on reference counting.
• The ABL (at least at one time, if not now) had some pretty bad garbage collection issues:
  • The CAST function could somehow affect their reference counting in a negative way. The developer did not mention if this incremented or decremented the count.
  • TEMP-TABLE fields of type FIELD_CLASS were not considered in the reference counts leading to objects going away before all references were gone.

Class Members Access Summary

Notes:

• "Data Member" column
  • Y means that this resource is considered by Progress to be a data member (something that stored state).
  • N means this is a "class-scoped handle resource".
  • An asterisk means this is a first class OO feature that is similar to a data member.
• ENUMs have not been tested yet.
• DEFINE IMAGE needs to be checked on Windows to see if the quirk exists there.
• No data members except for VARIABLEs can be public. All of them can otherwise be either protected or private but they default to private.
• All class scoped handle based resources can only be accessed as private (no protected or public) and the default is the same as private.
• EVENT and PROPERTY resources default to public and can be set to any access mode explicitly.
• I could find no resource with which the access modifiers can be used when defining inside a method (The access modifiers PUBLIC, PROTECTED, PRIVATE, and STATIC are not valid inside the body of a METHOD, FUNCTION, PROCEDURE or PROPERTY. (12890) compile error).

I'm posting our duck typing example here so that readers can see the full context of the discussion.

duck_typing.sh

PROPATH=".,oo1" pro -p duck-typing.p

duck_typing.p

def var a as Foo.

a = new Foo().
a:msg().

propath=".,oo2".

a = new Foo().

a:msg().

oo1/Foo.cls

class Foo:

    method public void msg():
      message "oo1".
    end method.

end class.

oo2/Foo.cls

class Foo:

    method public void msg():
      message "oo2".
    end method.

end class.

This example shows that the 4GL provides completely dynamic class loading based on runtime state. Two completely unrelated classes that happen to have methods of the same name and signature (msg()) will load and work with no complaints just by modifying the propath at runtime. This doesn't just affect the class loading, it affects the type of the object reference (in this case the var named a). The type is considered Foo but what does Foo mean when multiple different Foo implementations can be assigned without complaint?

If a had been saved to some other variable, field or member, then references to that Foo class would be active at the same time as references to the second Foo class.

In Java, class loading is controlled by 2 things:

• The dependencies resolved during compilation (e.g. by javac). The compiler will find these dependencies and resolve them at compilation time by searching for the related classes (and their contained members/methods) using the classpath that exists during compilation.
• The classpath and class loaders in the JVM at runtime. When a class is loaded at runtime it is found using the classpath which is something defined at the JVM startup. It cannot be changed after startup nor can the core classloader (the "system classloader"). It is possible to implement a custom classloader that also is used and this classloader would be able to be more dynamic than just processing a fixed classpath. For some good discussion on this, please see How do you change the CLASSPATH within Java?. FWD in fact, does provide its own class loader to allow jars to be added/removed/replaced at runtime without restarting the JVM process. However, this is very different from the 4GL approach. Assuming (as is the case in this example), that the fully qualified name of each of the classes is the same Foo, in Java one cannot have multiple instances of the same Foo class loaded at the same time from different locations.

Class loading is not a solution for this duck typing problem.

Before we consider approaches to deal with this, I think we need to answer the following questions:

1. Is this duck typing problem also present in the class hierarchy? For example, if Bar is a child class of Foo, can you instantiate multiple instances of Bar with different Foo parent classes just by changing the propath between instantiation attempts?

2. To what degree does a method signature matter in this duck typed world? What aspects of the signature are checked at compile time and what parts are only checked at runtime when a method is executed? In other words: how dynamic is the method invocation and how weak is the compile-time typing?

3. How much of the CAST() processing is happening at runtime as compared to compile-time. The 4GL docs for CAST() mention this:

At compile time, ABL verifies that the specified object type is within the class hierarchy of the specified object reference. At run time, the AVM checks the validity of the cast operation. Therefore, if you access a class member on the cast object reference that exists for the cast data type, but the referenced object at run time does not actually define the accessed class member, the AVM raises ERROR at run time.

This should not be possible in a strongly typed (compile-time typing) system.

The strange thing is that the documentation for DYNAMIC-CAST() contradicts the above comment:

You can also use the CAST function to perform all casting operations at compile time. The primary reason for using the DYNAMIC-CAST function is to cast object references based on run-time conditions that determine the object type to use for the cast.

Constantin: Please work on some testcases to explore these questions. Feel free to add other critical related questions here which I've missed.

4. About static constructors. 4GL states:

STATIC specifies a static constructor that executes exactly once in an ABL session the first time you reference a class type that defines this constructor in its class hierarchy. You cannot invoke a static constructor in any other way or at any other time. You can define only one static constructor for a given class. If you do not define a static constructor, ABL defines a default static constructor to initialize the static members of a class.

When switching the PROPATH, and a different class file exists, is the static constructor invoked again (if this class was already loaded)? This might depend on how 4GL keeps the loaded class registry - by class name or something else. Also, what happens with the static members - do they get initialized?

A conversion issue - the limitations introduced by duck typing I think will not allow us to use Java inheritance, when emulating the 4GL inheritance.

the limitations introduced by duck typing I think will not allow us to use Java inheritance, when emulating the 4GL inheritance.

This is my fear and is why I added question 1.

Greg Shah wrote:

the limitations introduced by duck typing I think will not allow us to use Java inheritance, when emulating the 4GL inheritance.

This is my fear and is why I added question 1.

This is confirmed by a testcase - we can't use Java inheritance, as the super-classes are resolved using the runtime PROPATH.

And a quick note about the static c'tors - looks like they are invoked only once, the first time this class (resolved by its name) is loaded. If PROPATH changes and a different file defines the same class, the static c'tor will not be invoked. Anyway, more analysis is required for static members.

Use string(THIS-OBJECT) (or string(ooVar)) to determine the handle ID (this is suffixed to the class name, like oo.Bar_1001); but I think THIS-OBJECT will show you (at the ID) only the object, and not its super-classes - these might be hidden via the super-procedure chains, as compiler prohibits THIS-PROCEDURE (and other system-handle procedures) being executed from within a class code.

Using construct like this before and after the PROPATH changes, shows the same ID (like Progress.Lang.Class_1002):

def var cls1 as Progress.Lang.Class.
cls1 = Progress.Lang.Class:GetClass("oo.Foo").
message string(cls1).

This (and a test on a static var) shows that the class is loaded only once, no matter if the propath changes, as the var reports the same value, before and after the propath change. I think 4GL creates a persistent copy of this program and associates it at runtime with all the static member access.

I was thinking we could do a mode where, if a project is not using duck-typing, we rely on Java inheritance for conversion and runtime.

But using the POLY arguments at the method calls, makes this impossible - the compiler checks the full signature, unless we have a POLY via DB_REF_NON_STATIC reference (h::f1). In this case, the runtime does the argument validation - the compiler just checks the number of arguments. Although for OO cases, the POLY value conversion at runtime looks more strict than for RUN statement.

More, when using method overloading, you can have a call like v:m1(h::f1), where the class has a m1(int) and a m1(char) - the runtime will take care of invoking the proper overloaded method.

So, I don't think we can even emit Java calls for the method invocations (and using some dynamic proxy to determine the program where the target resides) - we will need to rely on dynamic invocation, like we do for RUN statements. This is mostly because the Java compiler will not be able to choose to the proper overloaded method,

I guess this answers question 2.

I was thinking we could do a mode where, if a project is not using duck-typing, we rely on Java inheritance for conversion and runtime.

Yes, I'm thinking about this too. I'm working on a report for analytics that will detect any duplicate fully qualified object definitions in the project.

My initial thought is that this should be rare. It is certainly a bad practice and will lead to strange results (as you've noted with the statics). I'm inclined to force such things to be replaced rather than supporting them since the result would be significantly worse just to support a really bad design decision of Progress. At a minimum, this would allow direct member access in the parent hierarchy, even if the methods had to be dynamically dispatched.

I'm not too surprised about the dynamic nature of method calls. On the other hand, using POLY also seems a somewhat rare case. Is there any reason that a method call with non-POLY arguments can't be directly dispatched?

Another issue with using Java inheritance: in Java, you can't reduce the visibility of a member, just upgrade it. In 4GL, you can have a public method in a base class and a protected version in the sub-class.

LE: this was tested incorrectly, 4GL has the same compile-time check as Java.

Also in Java you can't hide an instance method via a static method, but you can in 4GL.

LE: this was tested incorrectly, 4GL has the same compile-time check as Java.

The idea is: if we use Java inheritance, we need to make sure that the OO features used in the project are fully compatible with Java.

And there is this "gem" (from the 4GL docs):

"On a state-reset AppServer, a given AppServer agent begins a fresh ABL session with each new client connection. Thus, each client connection to a state-reset AppServer re-initializes the static members of each class that is referenced."

if we use Java inheritance, we need to make sure that the OO features used in the project are fully compatible with Java.

Agreed. We need to maintain a list of these items.

Greg Shah wrote:

Is there any reason that a method call with non-POLY arguments can't be directly dispatched?

You mean without using a dynamic proxy? The problem I can think of is the EXTENT parameters - there is some validation done at runtime, for dynamic extent cases.

Also, what about the 4GL's DESTRUCTOR - do we rely on Java garbage collection?

in Java you can't hide an instance method via a static method, but you can in 4GL.

Did you find this via testing? The docs suggest the opposite:

When overloading methods, note that instance and static methods overload each other. In other words, the scope (presence or absence of the STATIC keyword) is not counted in distinguishing one overloaded method from another. For this reason, you cannot define an instance method and a static method in the same class hierarchy that both have the same signature. Such a combination causes ABL to raise a compile-time error.

You mean without using a dynamic proxy? The problem I can think of is the EXTENT parameters - there is some validation done at runtime, for dynamic extent cases.

We can exclude that case too. Again, it seems a more rare case.

Also, what about the 4GL's DESTRUCTOR - do we rely on Java garbage collection?

I think so. You aren't supposed to be able to execute it directly and it only gets executed when garbage collected. This seems exactly the same as the Java finalizer concept. I think we can map to that unless there is some unexpected behavior we find during testing.

Constantin Asofiei wrote:

Another issue with using Java inheritance: in Java, you can't reduce the visibility of a member, just upgrade it. In 4GL, you can have a public method in a base class and a protected version in the sub-class.

LE: this was tested incorrectly, 4GL has the same compile-time check as Java.

Greg Shah wrote:

in Java you can't hide an instance method via a static method, but you can in 4GL.

Did you find this via testing? The docs suggest the opposite:

Had duck-typing active and wrong class on the propath, so 4GL has same compile-time checks as Java for the member visibility and method hiding.

Greg Shah wrote:

I think so. You aren't supposed to be able to execute it directly and it only gets executed when garbage collected. This seems exactly the same as the Java finalizer concept. I think we can map to that unless there is some unexpected behavior we find during testing.

JVM specs don't guarantee which tread will execute the finalize() method - and if the DESTRUCTOR contains 4GL logic, this needs to be executed on the Conversation thread (at least). More, the destructor will be called when DELETE OBJECT is executed, and it can contain even UI statements.

Are destructors used in current projects?

JVM specs don't guarantee which tread will execute the finalize() method - and if the DESTRUCTOR contains 4GL logic, this needs to be executed on the Conversation thread (at least).

Good point. We could implement a finalizer that generates an event that can be responded to on the conversation thread, but I'm not sure that is needed.

We will have to be tracking object instances anyway. For example, SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT allows one to walk the entire set of objects that are currently instantiated. This is insane of course, any business system of reasonable complexity will have massive numbers of objects created at any one point in time. Still, when the object would be removed from that list I guess we can also handle the destructor processing.

On the other hand, there is this:

The object references maintained by the FIRST-OBJECT attribute and the NEXT-SIBLING property do not count as references for garbage collection. That is, if a class instance is referenced only on the session object chain, it is available for automatic garbage collection.

As long as we ensure the destructor is invoked at the right time, I guess this does not matter.

Are destructors used in current projects?

Yes.

Leaving duck typing aside, and looking through the JVM specs, I can't find other issues at this time.

This comment related to CAST I think was in mention of the duck-typing behavior.

Therefore, if you access a class member on the cast object reference that exists for the cast data type, but the referenced object at run time does not actually define the accessed class member,

The difference with Java casting is that this is done at runtime, and not compile time, in 4GL; a code like:

def var x as oo.Foo.
def var y as oo.Bar.

x = new oo.Foo().
y = cast(x, "oo.Bar").

where oo.Bar extends oo.Foo will fail at runtime with a Invalid cast from oo.Foo to oo.Bar. (12869).

LE: (GES) I could not recreate this runtime failure with the above sample in 11.6. Instead I get a compile failure Incompatible data types in expression or assignment. (223). I do see a runtime warning (error-status:error is false but you get a message/error-status:get-message(1) of the 12869 error. But I can only duplicate this using DYNAMIC-ENUM() with 2 different enum classes.

Also, if the x = new oo.Foo(). line is removed and x is unknown, runtime will not show any error.

Good results so far. Please check in your testcases to testcases/uast/oo/.

Next questions:

5. Please consider instantiation of objects via the NEW() function, NEW statement, DYNAMIC-NEW and Progress.Lang.Object:New(). Can we use Java .class instances for the NEW() function/NEW statement? The constructor invocation order and rules about invocation of super() seem to match Java (based on the docs). Considering that we have to maintain a chain of instantiated objects AND we need to duplicate error behavior, I think it is not safe to use the Java new operator directly. But if we can hard code .class references, that is a benefit.

6. What are the rules for destructor invocation? The documentation suggests it can occur from DELETE OBJECT, garbage collecton and when there is a failure during constructor invocation. Is this correct? Can you think of any reason we actually need the finalizer or can we drive everything off an explicit or implicit DELETE?

7. What facilities will we need in ControlFlowOps for method invocation?

8. We need to implement a replacement for the Progress.* and OpenEdge.* built-in classes. I was planning a hand-coded Java implementation though I am open to the alternative (implement our own version of those as 4GL classes which we actually convert). What are your opinions on this and where do we keep the classes?

Branch 3750a revision 11302 includes these changes:

• Added reports for constructors, destructors and duplicate fully qualified OO class/interface names.
• Added changes to map the filename to a class definition so that any reference can be tied back to the exact class that was found during parsing.

I'm working on the resolution of the converted Java class name information (simple class name, package name and fully qualified class name) for a given original source filename. In rev 11302, every CLASS_NAME, VAR_CLASS or other class reference is now has a source-file annotation with the original file name of the class being referenced.

My idea is that at parsing time, we have resolved the 4GL fully qualified names to some actual unambiguous source file. By mapping to that file's specific fully qualified Java class name, then everything else will be unambiguous. Do you see any flaws in my logic or unexpected results? I am assuming that the duck typing won't work, which is OK.

Greg Shah wrote:

My idea is that at parsing time, we have resolved the 4GL fully qualified names to some actual unambiguous source file. By mapping to that file's specific fully qualified Java class name, then everything else will be unambiguous. Do you see any flaws in my logic or unexpected results? I am assuming that the duck typing won't work, which is OK.

Stuff which uses 4GL-style class names may not work if we don't keep this mapping in an external file; for example, doing Progress.Lang.Class:GetClass("oo.Foo"). - of oo.Foo is in file abl/prj1/oo/Foo.cls, and converted Java file name is src/prj1/oo/Foo.java - we will need to know (at runtime) that oo.Foo in 4GL maps to prj1.oo.Foo in FWD. This is possible if you have the prj1/ entry on PROPATH.

I've been assuming that we will need the conversion name mapping anyway for things like DYNAMIC-NEW. I was thinking it would go into the name_map.xml with our other program name mappings.

I think we can calculate this if I just add the fully qualified OO name to the class-mapping node as an ooname attribute. SourceNameMapper can handle the calculation from there.

• how do you plan to emit the 4GL c'tors? As a Java constructor, right?
• destructor methods are called for each class in this object's hierarchy - this is because in 4GL objects are built up from multiple persistently-ran external programs, with some weird super-procedure management.
• so, if instantiation fails during c'tor invocation, we need to know which classes have initialized for this object (in the hierarchy) - so destructors will be called only for these.
• and more, we will not be able to emit the destructors as Java instance methods - as these break the Java's method override paradigm. Consider oo.Bar having a destructor and instantiation fails at oo.Bar class - only oo.Foo super-class destructor will be executed, and (unless I'm missing something) we can't explicitly execute this in Java using the oo.Bar reference. They will need to be static methods, having as single argument an instance of this type - and all instance references will need to be qualified with this argument.

We might be able to hide the 'super class initialization' in the FWD block management (i.e. we receive the class name as an argument to BlockManager.constructor API), so we know which c'tors have executed at time of a failure.

Also, something else related - how are OO variables (i.e. def var c as oo.Bar.) converted in FWD? Will these be converted to Java's oo.Bar class equivalent, or they will be wrapped in a kind-of handle equivalent, dynamic proxy or something else? The main problem here is the reference counting - we will not be able to count these if we don't explicitly track the assignment. Aspects might work, but I'm not sure of the performance.

Considering that we have to maintain a chain of instantiated objects ...

I'm not sure this is problematic, considering we will need our own implementation of Progress.Lang.Object - as this will be the superclass for all 4GL-style classes, we can hook this chain maintenance (and anything else) in the implicit c'tor of Progress.Lang.Object.

AND we need to duplicate error behavior,

Yes, I think this is what blocks us - we need to know which super-class has initialized and this requires to know when the object has finished initialization (so that we can clean this tracking). So the .class is OK, as long as we emit the proper name.

6. What are the rules for destructor invocation? The documentation suggests it can occur

• from @DELETE OBJECT - this is correct
• garbage collection - I couldn't duplicate this (as the garbage collector can't be easily forced to trigger, I guess)
• there is a failure during constructor invocation - see above for the destructor peculiarities in super-classes.

Can you think of any reason we actually need the finalizer or can we drive everything off an explicit or implicit DELETE?

See above, the Java finalizer can't help.

7. What facilities will we need in ControlFlowOps for method invocation?

Do you see some reason not to use direct Java-style method calls? Special APIs would be required for duck-typing but otherwise, if we rely on Java inheritance, no special APIs would be needed.

8. We need to implement a replacement for the Progress.* and OpenEdge.* built-in classes. I was planning a hand-coded Java implementation though

I'd go with a hand-coded Java implementation, as we could skip the 4GL-style top level blocks for them and we can easier fine-tune the performance for these. The advantage is any runtime changes will not require a reconversion of these files - just a FWD deploy.

Constantin Asofiei wrote:

• how do you plan to emit the 4GL c'tors? As a Java constructor, right?

And I just realized something - if we need ANY kind of block management in the constructor, we can not rely on Java constructors to initialize the object. If there is an explicit 4GL super() constructor call or this-object() constructor call, this must be the first statement in the Java constructor, too. And we can't do this if we emit this inside a BlockManager API.

So constructors will need to be initialization-like Java methods. And conversion needs to take care to properly emit the implicit constructor call, if there is no explicit constructor call.

Something about widget definitions in a class (like DEF BUTTON btn) - in documentation there is a statement like 'Class definition files can define static visual handle-based objects (widgets) as PRIVATE data members.' - but note that these are not static members, but instance members.

And USE-WIDGET-POOL - is this currently used?

Constantin, are there any limitations as to how a field of data type class can be used in a WHERE clause? For instance, can you do something like the following?

find first tt where tt.obj.foo = "bar".

If so, how is tt.obj.foo dereferenced? Is this the equivalent of a user-defined function, which is resolved once, before the FIND is executed (like a query substitution parameter)? Or is the dereferencing of the foo member of tt.obj done for each record in the temp-table, until one is found which matches "bar"?

Before we go further down this path of inquiry, can you first determine with test cases whether the above syntax (and similar forms, like calling OO methods from a class field object reference) is possible?

Eric Faulhaber wrote:

If so, how is tt.obj.foo dereferenced? Is this the equivalent of a user-defined function, which is resolved once, before the FIND is executed (like a query substitution parameter)?

This one. The expression is evaluated before the query is executed.

In a temp-table, a class field can only have the Progress.Lang.Object type - so direct property references is not possible. You can invoke Progress.Lang.Object methods directly, like where tt.obj:toString() = "x".

You can use cast to cast the reference to another type and access its members directly, like cast(tt.obj, "oo.Bar"):foo = "bar" - but if the tt buffer has no record, then no error is shown/thrown/etc - the condition just evaluates to false and terminates all other processing, even if the cast is followed by something like or true - so we need to be careful with our HQL where preprocessing.

Direct field reference like where tt.obj = ? or where tt.obj <> ? does work.

But there is something interesting with how delete object tt.obj works: the tt.obj field is not unknown, but valid-object(tt.obj) will be false. Assigning the tt.obj to unknown works.

Greg, we need to determine how we handle the same stuff with unknown object references, for normal var case. 4GL gives a Invalid handle. Not initialized or points to a deleted object. (3135) when trying to access a member from an unknown object - this shows that we can't assign the ref to Java null, we will need to hide the object state behind a dynamic proxy.

And USE-WIDGET-POOL - is this currently used?

Sorry, it took some time to run the search across both projects. Yes, it is used in a single location.

Greg, we need to determine how we handle the same stuff with unknown object references, for normal var case. 4GL gives a Invalid handle. Not initialized or points to a deleted object. (3135) when trying to access a member from an unknown object - this shows that we can't assign the ref to Java null, we will need to hide the object state behind a dynamic proxy.

Questions:

• I assume that statics are not affected, only instance members. The 4GL doesn't allow accessing statics from an instance and so there is no way to dereference a static in an unknown var. Correct?
• Does this behavior extend to all data members, properties as well as instance methods?
• If this extends to non-method members, does the same behavior occur on both read and write?

I do not want to implement "shadow" interfaces for every class. This wouldn't solve the problem for members anyway and it would make a massive mess. Libraries like CGLIB can proxy without interfaces, though it only works for non-final instance methods.

There is no solution for data members because the calling byte code directly dereferences fields without invoking any byte code in the class that contains the field. A "solution" might be to provide a kind of bean interface for get/set of each non-method instance member. I don't want to use a true bean approach, which could lead to many naming conflicts since methods and data members/properties are in different namespaces. For a data member foo of type footype we could implement _foo() for read and _foo(footype) for write. Then all accesses would be indirect and proxyable. I don't like this idea but it could be done.

I wonder if something lower level like asm could allow us to proxy even the final methods. The issue is that a common approach is to implement the proxy as a subclass, so they naturally can't handle the final methods.

Greg Shah wrote:

• I assume that statics are not affected, only instance members. The 4GL doesn't allow accessing statics from an instance and so there is no way to dereference a static in an unknown var. Correct?

Correct. Errors raised in the static constructor are ignored. Also, a static class can't be unloaded from memory easily (delete is a no-op for a Progress.Lang.Class instance). There is a way to unload it and force it to re-initialize if you change its source-code (and thus it gets recompiled), but this wouldn't be a production-level feature.

• Does this behavior extend to all data members, properties as well as instance methods?

Yes.

• If this extends to non-method members, does the same behavior occur on both read and write?

Yes.

Greg Shah wrote:

And USE-WIDGET-POOL - is this currently used?

Sorry, it took some time to run the search across both projects. Yes, it is used in a single location.

Documentation states:

Directs the AVM to create an unnamed widget pool that is scoped to the class hierarchy of each instance of the class. When specified, all dynamic handle-based objects that are created in the class by and for instance class members are created by default in this unnamed widget pool.

This suggests that only resources created and saved into instance members are added to this pool. From some testing, I think this is some kind of compile-time setting which augments any CREATE statement within the class so that the resource is added to the instance pool. This is because using a CREATE with a class member outside of the class, doesn't add it to this pool.

If the class contains static members, this option directs the AVM to create a separate unnamed widget pool that is scoped to the class for all dynamic handle-based objects created for or by these static class members in the session.

Considering that a class can't be unloaded, this unnamed widget pool can never be deleted - I assume this is some kind of isolation of these dynamic resources. Same, I think this is some compile-time augmentation for CREATE statements using static members.

Same, I think this is some compile-time augmentation for CREATE statements using static members.

Is there a different widget pool for each instance or is there a single widget pool for the class? For the instance case, we can create it in the "constructor worker" method. For the class case, in a static intializer.

How do we associate that widget pool with that instance or class?

What do we do to associate the handle based resource creation with the instance/class widget pool? If it is class based, can we treat this as a kind of implicitly named widget pool that has the same name as the class?

Greg Shah wrote:

Is there a different widget pool for each instance or is there a single widget pool for the class?

My assumption is that this is per-instance.

For the instance case, we can create it in the "constructor worker" method. For the class case, in a static intializer.

We don't emit explicit code for this - we do it in the constructor's (instance or static) associated BDT API, from within FWD runtime, right?

How do we associate that widget pool with that instance or class?

• get a specific name for this pool (maybe via some WidgetPool.nextPoolName() API)
• we save this inside the object (or at the class), in some field added within Progress.Lang.Object
• for CREATE statements (which match the rules to be augmented), we convert it as if the IN WIDGET-POOL clause exists.

What do we do to associate the handle based resource creation with the instance/class widget pool? If it is class based, can we treat this as a kind of implicitly named widget pool that has the same name as the class?

The converted CREATE would match something like CREATE <res> IN WIDGET-POOL this-object:fwd-instance-widget-pool(), or CREATE <res> IN WIDGET-POOL Progress.Lang.Object:fwd-static-widget-pool("classname"). The idea here is to ensure the widget pool names are unique, and can't collide with anything else the user might have created. We could use a special prefix or random UUIDs.

Anyway, I need more tests for this.

3750a revision 11306 is the first phase of conversion support for OO 4GL. This revision can properly convert empty class and interface definitions, including abstract, final and inherits. It doesn't handle implements, nor use-widget-pool or serializable. The extends clause is fully qualified at this time, but that will be fixed shortly. The result compiles but since it is empty, it doesn't do anything yet. The replacement for Progress.Lang.Object is com.goldencode.p2j.util.BaseObject (at least for now).

In order to handle the conversion of OO references, we must calculate the Java package and classname for every OO program before the annotations phase starts. The calculation is done in the annotations/naming.rules and this ruleset has been "pushed back" into a new "Annotations Prep" phase of the conversion. This phase MUST come AFTER m0 because the naming rules depend upon p2o name lookups. This is why we could not handle this issue with "Early Annotations". See annotations/annotations_prep.xml.

3750a revision 11307 adds support for IMPLEMENTS.

For the instance case, we can create it in the "constructor worker" method. For the class case, in a static intializer.

We don't emit explicit code for this - we do it in the constructor's (instance or static) associated BDT API, from within FWD runtime, right?

I'm talking about the creation of the widget pool itself, not the registration of handle resources to be contained in it. I assumed the converted class would need an equivalent to the CREATE-WIDGET-POOL statement. The best place for an instance-level widget pool seems to be the constructor worker.

Your plan is good.

random UUIDs

Yes, this is the way.

Greg Shah wrote:

I'm talking about the creation of the widget pool itself, not the registration of handle resources to be contained in it. I assumed the converted class would need an equivalent to the CREATE-WIDGET-POOL statement. The best place for an instance-level widget pool seems to be the constructor worker.

Yes, I meant BlockManager instead of BDT.

I think we need to create the pool persistent.

The 4GL documentation for FIELD_CLASS states that it is only possible to make it Progress.Lang.Object. It turns out the documentation is correct:

def var f as oo.Foo.

/* Use the 'Progress.Lang.Object' datatype for temp-table column 'obj' instead of a specific class type 'oo.Foo' which is not supported. (12864) */
/* def temp-table tt field num as int field obj as oo.Foo. */
def temp-table tt field num as int field obj as Progress.Lang.Object.

f = new oo.Foo().

create tt.
tt.num = 30.
tt.obj = f.

/* must cast inside where or you get this: */
/* Could not locate element 'bogus' in class 'Progress.Lang.Object'. (12927) */
/* find first tt where not tt.obj:bogus(). */

find first tt where cast(tt.obj, oo.Foo):description() ne ?.

/* must cast here else you get: */
/* ** Incompatible data types in expression or assignment. (223) */
/* f = tt.obj. */
f = cast(tt.obj, oo.Foo).

/* must cast before use outside of where, else you get a 12927 error */
/* oo.Foo_1046 whatever oo.Foo_1046 whatever */
message tt.obj:toString() cast(tt.obj, oo.Foo):description() f:toString() f:description().

It seems like a bad limitation but it is easier for us.

I've been writing some testcases to explore unknown value, assignment and undo. In all of these areas, the behavior is not compatible with a direct Java reference approach.

• Unknown Value
  • All variables initialize to unknown value. There is no way to initialize with a new() as part of the variable definition.
  • When a reference is set to unknown, de-referencing it generates the error 3135 as discussed #3751-37. We could use CGLIB dynamic subclass generation for non-final methods but it doesn't help with final methods or with field access.
• In order to support DELETE and SESSION:FIRST-OBJECT/LAST-OBJECT chaining, we must manage the construction process and mostly will need some form of reference counting to know how many "live" references there are in converted code.
• Assignment and Parameter Passing
  • Both of these require our runtime to handle behavior that cannot be easily emitted into the business logic (nor would we want to do so if we could).
  • The runtime must have delegated access to the instances and for that reason we need a mutable wrapper instead of a reference.
• UNDO
  • When you assign a new instance reference to an undoable VAR_CLASS reference, the normal transaction semantics of UNDO are honored.
  • This means that the reference assignment will be UNDOne with rollback.

All of the above suggest the cleanest approach is to implement a wrapper for the VAR_CLASS object reference. This wrapper (com.goldencode.p2j.util.ObjectRef?) would be a BDT (and Undoable) and would thus be able to be managed as any other variable. It will naturally support all of the above behaviors. The idea is that we pass the wrapper class for everything except when we need to invoke methods or access members. We would call a obj() method to unwrap the actual object instance.

I'm thinking we use generics to allow that obj() method to return the correct type. Thus a variable definition might look something like this:

ObjectRef<My4GLOOClass> varname = UndoableFactory.objectRef()

If we need to pass the class to the objectRef() for the runtime to have access, we can do so.

It also fully solves the error 3135/unknown value dereference issue because we can handle that in the obj() unwrapper method.

I don't see a better way to do this. Hiding the BDT stuff in BaseObject just doesn't solve all the issues. We need a mutable wrapper to be able to manage all the behavior compatibly.

Does anyone have any thoughts, concerns or comments?

Eric suggests using object instead of ObjectRef. It is shorter and is in the same kind of lowercase format as the other concrete wrapper implementations. I'll go with this.

Along with this, I'll name the de-referencing method ref() instead of obj().

I'm working on var/parm defs right now. I've just checked in 3750a rev 11311 which is an early version of the object wrapper.

Constantin, would you please look at the following question:

• Is there any reason to believe that the OO 4GL object instances are a kind of handle (other than the error message 3135)? We know that you can delete these and walk the chain of them. I'm trying to figure out if object should subclass from handle.

3750a rev 11312 provides variable and parameter definition support for AS CLASS, LIKE VAR_CLASS and LIKE FIELD_CLASS. Some more testing is needed, but the basics should be there.

As with the previously implemented features, all class names are fully qualified at this time. This is temporary.

Greg Shah wrote:

• Is there any reason to believe that the OO 4GL object instances are a kind of handle (other than the error message 3135)? We know that you can delete these and walk the chain of them. I'm trying to figure out if object should subclass from handle.

Although it has some common behaviour (like resource ID which can be seen in OO's toString, and valid), they are not interchangeable. At this time I'm more comfortable keeping them separate.

Under the cover, 4GL might use some kind of special handle-like structure which has an external program resource and decorates around it to make it look like an object. But I don't think that can help us.

Is anyone working on the conversion of FIELD_CLASS right now?

I'm not.

Constantin ?

Greg Shah wrote:

Is anyone working on the conversion of FIELD_CLASS right now?

I'm not.

Constantin ?

No, I'm still working on the buffer scoping issue.

I'm working on getting data members to emit properly. Then I am going to take method definitions.

Constantin: After the buffer issue, can you please implement the following:

• constructors
• destructors
• the 4 forms of NEW
• DELETE
• SESSION:FIRST-OBJECT/LAST-OBJECT and traversal via the Progress.Lang.Object methods nextSibling() and prevSibling().

3750a rev 11313 provides support for variables as data members in classes or interfaces.

Triggers can be defined inside a class as shown by this example:

def var t as TriggerInClassDef.
t = new TriggerInClassDef().

t:edit().

class TriggerInClassDef:

   on "a" anywhere
   do:
      message "trigger fired".
   end.

   method public void x():
      def var txt as char.
      update txt.
   end.
end.

The trigger works as you would expect.

I also did confirm that internal procedures and user defined functions cannot be defined inside a class definition.

class broken:

   function forbidden returns int ():
      return 0.
   end.

   procedure verboten:
   end.

end.

The compiler generates "Procedures and user-defined functions may not be defined in a class or interface file. (12910)".

Since methods can't be in the same compilation unit with functions or procedures, there cannot be any naming conflicts between methods and the other top level blocks. Triggers aren't named so there is no conflict there either.

Of course, methods can be overloaded and so we cannot and must not disambiguate duplicate method names. Considering the above, this should not be a problem. The thing I don't understand is what problem we were solving with getUniqueJavaMethodName (see annotations.xml). This is used to ensure we create unique procedure names and to ensure unique user defined function names. For example, the following are NOT ALLOWED:

function one returns int ():
return 0.
end.

function one returns int ():
return 14.
end.

procedure two:
end.

procedure two:
end.

function three returns int ():
   return 0.
end.

procedure three:
end.

Functions even share the same namespace as internal procedures.

Why do we need to disambiguate duplicate proc or func names?

Greg Shah wrote:

Functions even share the same namespace as internal procedures.

Why do we need to disambiguate duplicate proc or func names?

During conversion, a foo-a function name and a foo_a procedure name will get converted to the same java method name, fooA - so we need to track collisions not in the legacy name, but in the converted java name, as our naming convertor may produce same Java name for two different legacy names.

Greg Shah wrote:

I'm working on getting data members to emit properly. Then I am going to take method definitions.

Constantin: After the buffer issue, can you please implement the following:

• constructors
• destructors
• the 4 forms of NEW
• DELETE
• SESSION:FIRST-OBJECT/LAST-OBJECT and traversal via the Progress.Lang.Object methods nextSibling() and prevSibling().

• per note #3751-33, the constructors will not be Java constructors, but object initialization methods.
• per note #3751-32 - destructors can be instance methods, if we name them something like p1_p2_Bar_destructor (i.e. fully qualified class name followed by destructor). We may want to use the same approach for constructors, too.

the constructors will not be Java constructors, but object initialization methods

Yes, this is my understanding too.

destructors can be instance methods, if we name them something like p1_p2_Bar_destructor

This seems reasonable. One question: why is the package part needed? I'd rather decorate it with __Bar_destructor__.

Greg Shah wrote:

This seems reasonable. One question: why is the package part needed? I'd rather decorate it with __Bar_destructor__.

The idea is to avoid collisions - you can have same class name but in different packages, and one being a super-class of the other.

Do we already have a OO ops helper class? For new, delete, etc.

If not, I'll add ObjectOps.

Is Progress.Lang.Object skeleton required to be in the conversion list? I get this for a def var o as Progress.Lang.Object. statement:

EXPRESSION EXECUTION ERROR:
---------------------------
persist()
^  { null value for annotation 'full-java-class':def [DEFINE_VARIABLE]:12884901891 @1:1
   o [SYMBOL]:12884901895 @1:9
   as [KW_AS]:12884901897 @1:11
      Progress.Lang.Object [CLASS_NAME]:12884901899 @1:14
 }

Constantin Asofiei wrote:

Do we already have a OO ops helper class? For new, delete, etc.

If not, I'll add ObjectOps.

Nothing yet, please do add it.

Constantin Asofiei wrote:

Is Progress.Lang.Object skeleton required to be in the conversion list? I get this for a def var o as Progress.Lang.Object. statement:
[...]

Nevermind, I see the dependency on ./skeleton/oo4gl/Progress/Lang/Object.cls - I had the skeleton in p2j.cfg.xml to a different parent folder.

I'll fix the annotations_prep.rules to use the p2j.cfg.xml oo-skeleton-path folder.

Constantin Asofiei wrote:

Is Progress.Lang.Object skeleton required to be in the conversion list? I get this for a def var o as Progress.Lang.Object. statement:
[...]

No it should not be in the list. Any references are mapped to BaseObject. See annotations/annotations_prep.xml and search for BaseObject.

Can we automate the annotations_prep.rules step of registering the built-in classes?

   Recursive level 2 parse of class: ../../../skeleton/oo4gl/Progress/Lang/Object.cls
   Recursive level 3 parse of class: ../../../skeleton/oo4gl/Progress/Lang/Class.cls
   Recursive level 4 parse of class: ../../../skeleton/oo4gl/Progress/Lang/ParameterList.cls

We already know the referenced built-in 4GL skeleton classes during parsing - we can use this set and we just need to determine a convention for the class name.

This will explode at some point, and I assume we can't use the progress root package name. I'd just get rid of this package and use something like com.goldencode.p2j.util.oo.lang.Object for Progress.Lang.Object (i.e. we keep the class names, remove the root package and place them in com.goldencode.p2j.util.oo.).

Otherwise, we should at least get rid of abends related to this, otherwise it will be cumbersome to determine which built-in classes need to be registered here.

Constantin Asofiei wrote:

Can we automate the annotations_prep.rules step of registering the built-in classes?
[...]

We already know the referenced built-in 4GL skeleton classes during parsing - we can use this set and we just need to determine a convention for the class name.

This is a good idea.

This will explode at some point, and I assume we can't use the progress root package name. I'd just get rid of this package and use something like com.goldencode.p2j.util.oo.lang.Object for Progress.Lang.Object (i.e. we keep the class names, remove the root package and place them in com.goldencode.p2j.util.oo.).

Yes, this is why I was previously asking about ideas on how to handle the built-in classes. I like your idea here.

Otherwise, we should at least get rid of abends related to this, otherwise it will be cumbersome to determine which built-in classes need to be registered here.

Also a good idea.

Please go ahead with your recommended changes.

3750a rev 11316 is some intermediate changes for method definitions. They are not working yet, but I don't want our parallel work to diverge too much. We need to be checking in frequently enough that we can avoid major rework or conflicts.

Constantin Asofiei wrote:

I'd just get rid of this package and use something like com.goldencode.p2j.util.oo.lang.Object for Progress.Lang.Object (i.e. we keep the class names, remove the root package and place them in com.goldencode.p2j.util.oo.).

A minor thing, but... is there a reason this needs to be a sub-package of util? How about dropping util from the package and going with com.goldencode.p2j.oo?

Eric Faulhaber wrote:

A minor thing, but... is there a reason this needs to be a sub-package of util? How about dropping util from the package and going with com.goldencode.p2j.oo?

p2j.oo is good.

OO method calls and attribute access is not there yet, correct?

Also, I assume only stubs will be added for i.e. delete, nextSibling(), etc

For normal expressions processing (object and class dereferencing), we are planning to use a direct method invocation approach. The only case where we have a need for indirect invocation of a METHOD_DEF is DYNAMIC_INVOKE() which is a FUNC_POLY that is similar to DYNAMIC-FUNCTION() except it doesn't have the IN handle syntax.

Can you think of any other case for indirect invocation?

I'm excluding constructors and destructors from this list since those are always indirectly referenced and must be call-backs from the runtime.

Based on the DYNAMIC-INVOKE() case, we still need all the name map information, including the parameter analysis/signatures. I'm trying to make changes to collect_parameters right now.

Constantin Asofiei wrote:

Eric Faulhaber wrote:

A minor thing, but... is there a reason this needs to be a sub-package of util? How about dropping util from the package and going with com.goldencode.p2j.oo?

p2j.oo is good.

Agreed.

Also, I assume only stubs will be added for i.e. delete, nextSibling(), etc

Yes, this is OK.

Do you still want to use BaseObject name for Progress.Lang.Object class?

• added DELETE OBJECT obj, SESSION:LAST/FIRST-OBJECT conversion support.
• reworked builtin class registration to automatically pickup all builtin skeleton classes.

Constantin Asofiei wrote:

Do you still want to use BaseObject name for Progress.Lang.Object class?

I wanted to avoid Object. Do you have an alternative in mind?

Greg Shah wrote:

Constantin Asofiei wrote:

Do you still want to use BaseObject name for Progress.Lang.Object class?

I wanted to avoid Object.

Yes, I realized the possible confusion, too.

Do you have an alternative in mind?

I'm keeping BaseObject for now; we should hardcode this name in some TRPL constant, to be used across rules - it will be easier to manage.

Also, I think Progress.Lang.Class should be LegacyClass or something.

Greg Shah wrote:

Can you think of any other case for indirect invocation?

4GL has reflection support via Progress.Reflect.Method and others.

I'm adding these to my list:

VALID-OBJECT()
CAST() and DYNAMIC-CAST()
TYPE-OF()

Plus CLASS_NAME literals.

I think Progress.Lang.Class should be LegacyClass or something.

I like it.

CLASS_NAME literals will emit as string literals with the legacy qualified class name or Java full qualified name?

Constantin Asofiei wrote:

CLASS_NAME literals will emit as string literals with the legacy qualified class name or Java full qualified name?

I think in most cases we can emit the actual Java class reference. For example, as a static qualifier it should not be a string.

In what cases are these string literals?

Greg Shah wrote:

Constantin Asofiei wrote:

CLASS_NAME literals will emit as string literals with the legacy qualified class name or Java full qualified name?

I think in most cases we can emit the actual Java class reference. For example, as a static qualifier it should not be a string.

In what cases are these string literals?

I was having duck typing in mind. For now it works with p1.p2.Bar.class for NEW, CAST, etc.

Constantin Asofiei wrote:

Greg Shah wrote:

Can you think of any other case for indirect invocation?

4GL has reflection support via Progress.Reflect.Method and others.

And for DYNAMIC-CAST( object-reference , expression ). - this requires the mapping of legacy class names to qualified class names, at runtime.

We are excluding support for duck typing as a permanent restriction, so we don't need strings there.

DYNAMIC-CAST should not have a CLASS_NAME parameter since the 2nd parm is a character type. I agree we will need the legacy to Java class name mappings for the lookup there.

CAST allows a construct like cast(o, p1.O1):m1()..

This needs to be converted to something like:

ObjectOps.cast(o, com.goldencode.testcases.p1.O1.class).<com.goldencode.testcases.p1.O1>cast().m1();

The idea here is that ObjectOps.cast returns a object. OTOH, it can be emitted like ObjectOps.cast(o, com.goldencode.testcases.p1.O1.class).m1(); if its signature is public static <V> V cast(object o, Class<V> cls).

Only CAST allows this kind of chaining. NEW and DYNAMIC-CAST does not allow it.

OTOH, it can be emitted like ObjectOps.cast(o, com.goldencode.testcases.p1.O1.class).m1(); if its signature is public static <V> V cast(object o, Class<V> cls).

This is much better.

However, I agree that CAST needs to return an object.

But why not emit this for chaining: ObjectOps.cast(o, com.goldencode.testcases.p1.O1.class).ref().m1();?

Greg Shah wrote:

However, I agree that CAST needs to return an object.

But why not emit this for chaining: ObjectOps.cast(o, com.goldencode.testcases.p1.O1.class).ref().m1();?

Ha, its working. I tried it before but didn't get the right generic signature; this one works: public static <V extends BaseObject> object<V> cast(object o, Class<V> cls).

• Added CAST, DYNAMIC-CAST, DYNAMIC-NEW, NEW, TYPE-OF, VALID-OBJECT.
• Emit .class Java reference for certain CLASS_NAME nodes.

For Progress.Lang.Object:New() - this requires the OO method call infrastructure first.

I'm starting the constructors/destructors next.

What was the purpose to add isTopLevelOrOO instead of just changing isTopLevel to handle OO blocks, too?

isTopLevel excludes the class or interface body but isTopLevelOrOO does not.

Revision 11319 adds more enablement for method defs (and some enablement for the other types like constructor, destructor and prop getter/setter). It is still not done. I'm working on changes throughout the convert/ rules now, this is the last set of known areas to change.

I hope I'm not missing anything, but I suspect there still may be some parts of fixups or annotations which will need some tweaks.

I guess I should have said: we are deliberately excluding those in isTopLevel because the places where it is being used already exclude the external procedure case.

Greg Shah wrote:

isTopLevel excludes the class or interface body but isTopLevelOrOO does not.

Revision 11319 adds more enablement for method defs (and some enablement for the other types like constructor, destructor and prop getter/setter). It is still not done. I'm working on changes throughout the convert/ rules now, this is the last set of known areas to change.

Most of this enablement is required for constructors/destructors, too. Plus, as they have lots in common with methods and getters/setters, I wonder if this shouldn't be worked at the same time - otherwise, I will need to chase all method-related rules (for example collect_parameters.rules, variable_definitions.rules, etc) and expand the logic to handle the constructor and destructor blocks, too.

I'm about to have an implementation of methods pretty soon (at least in classes). At that time we can figure out the next steps for constructors and destructors.

Rev 11320 has my latest changes, but it is not done yet. I'm actually at the point of emitting the method.

In the interim, can you please look at data members that are not vars (buffers, frames...) in classes? I've checked in my recent testcases, oo/ResourceHolder.cls is a good one to try (except the methods must be commented out right now).

Greg Shah wrote:

In the interim, can you please look at data members that are not vars (buffers, frames...) in classes?

Yes, I'm looking at these.

A part with resource data members is code like frame.openScope() - this needs to be emitted I think in 'legacy default constructor', which for us will be a default initialization method; this will always be called during the object setup, and it will take care of initializing the object's implicit state. This would be invoked just before the initialization method (associated with the legacy constructor) will be invoked.

Rev 11321 adds more method def support. It is almost working now. Method parameters are broken. I'm working on it.

Revision 11322 provides fully functional method definition support.

Constantin: Everything I've been working on is checked in. Some thoughts:

1. To work on the non-var members, I recommend you look at the rev 11322 changes for variable_definitions.rules. Because the class and interface defs don't have an external procedure, the controlid and instvarid values don't exist. I had to "fake out" some of the location code in that ruleset. I suspect this may affect other resources too.

2. To work on the constructors and destructors, search on prog.method_def. This will show places where I had to make changes. Many of these locations already take constructors and destructors into account, but others do not.

I'm working on what you mentioned in #3751-106. Note that the vars defined within a method are not scoped to it - I'll fix this.

• implicit initialization c'tor - our 'execute' method with just the frame/buffer openScope, etc. this will be the first one execute before the actual legacy c'tor.
• local vars must be defined inside the method/c'tor block, and not at the class
• c'tors - automatically emit a super c'tor invocation if no explicit one (super or this-object) - this will be the first statement inside the constructor body.
• c'tors - automatically emit an empty no-param c'tor if one doesn't exist.

Revision 11326 provides fixes to make interface definitions work properly.

Ovidiu: Is the FIELD_CLASS support complete? Will the DMOs compile?

Constantin: What are you working on right now? I think it is time to handle conversion of OBJECT_INVOCATION including chaining and wrapping/unwrapping as needed.

I was thinking of starting work on properties.

We will also need to handle EVENTS, THIS-OBJECT, SUPER, USING and structured error handling.

Greg Shah wrote:

Constantin: What are you working on right now? I think it is time to handle conversion of OBJECT_INVOCATION including chaining and wrapping/unwrapping as needed.

I'm finishing #3751-109, will take OBJECT_INVOCATION next.

I'm taking SUPER and THIS-OBJECT, too.

Greg Shah wrote:

Revision 11326 provides fixes to make interface definitions work properly.
Ovidiu: Is the FIELD_CLASS support complete? Will the DMOs compile?

I cannot grant the completeness. This is because I only fixed the issues that surfaced while converting the POC during M0. I am also not sure about the correctness. I think that object type is correct, but not sure about the default ? value. I looked into the ABL Reference, but I could not find the answer.
Yes: the POC sources were successfully processed by convert.middle.list target and the generated DMOs did compile, with my changes from r11323/3750a.

I'm responding to #3750-69 here so that all the FIELD_CLASS information is in one place. See #3751-48 for some useful background.

TRPL required some minor updates related to the new FIELD_CLASS which were committed as r11324. Please review;

I did review the changes and they seemed fine, though my impression was that there was more needed to finish the support even for DMOs. Of course, we'll need to add the conversion of field references in business logic.

One thing to consider is that our object wrapper is a generic. When one considers that FIELD_CLASS is always of type Progress.Lang.Object, any FIELD_CLASS member or return type of object should really be object<BaseObject>. This would be the case for DMOs and for the business logic conversion.

but not sure about the default ? value

Object instances default to unknown value so this is consistent. Consider that FIELD_CLASS is always Progress.Lang.Object so there is no way to automatically instantiate a more specific type. There is no alternative but to default to unknown.

Constantin Asofiei wrote:

A part with resource data members is code like frame.openScope() - this needs to be emitted I think in 'legacy default constructor', which for us will be a default initialization method; this will always be called during the object setup, and it will take care of initializing the object's implicit state. This would be invoked just before the initialization method (associated with the legacy constructor) will be invoked.

Regarding this: considering that 4GL emulates classes with external programs ran persistent, I think we can consider that scoping behaviour for frames, buffers, etc emulates that. More, the resource lifetime follows the life of the object (persistently ran external program), too. So, I think is OK to consider that part of object creation there will be a call to a __p1_Bar_execute__ method, which emulates an empty external program, with only the scoping initialization code (and maybe triggers and other stuff).

part of object creation there will be a call to a p1_Bar_execute method, which emulates an empty external program, with only the scoping initialization code (and maybe triggers and other stuff).

This makes sense. Having that emit as part of base_structure.xml and convert/control_flow.rules will also make it easier for various member generation code to work naturally. For example, I had to make up for the lack of an execute() method in classes by "faking" some initial values in convert/variable_definitions.rules.

And yes, the triggers would emit there too.

Greg Shah wrote:

For example, I had to make up for the lack of an execute() method in classes by "faking" some initial values in convert/variable_definitions.rules.

This is still needed - we can't scope anything within this execute method; every 4GL class member must be 'promoted' and defined as a Java instance field.

How do we treat the temp-table defined at an interface?

I'm not removing the promotion, the thing here is that our var defs rules were dependent upon some values initializing at the root block node which did not happen in classes/interfaces. If we now have an execute method, then those values will now be initialized and things will work more naturally. The faking out for vars shouldn't be needed. They will still emit as members, it is just that without that init the conversion abended in some cases which led to the "faking" being needed.

If I understand it correctly, it should be the same as we define now except we don't need any buffers. The define statement itself should emit as a DMO and won't appear in the Java interface, just like it doesn't appear in the external proc today. I think the buffer creation/init does need to be suppressed.

The interface will still need to import the dmo package. This will allow method parameters to reference the temp-table safely.

Greg Shah wrote:

If I understand it correctly, it should be the same as we define now except we don't need any buffers.

The buffer is needed - I think we need to bring it to any implementing class.

If I understand it correctly, it should be the same as we define now except we don't need any buffers.

The buffer is needed - I think we need to bring it to any implementing class.

I don't think so. In the 4GL, the implementing class also will have the same (or compatible) define temp-table statement. So our normal processing will handle that naturally. I can't see why the interface needs it to define methods.

• finishes the constructor/destructor support
• implicit initialization c'tor - our 'execute' method with just the frame/buffer openScope, etc. this will be the first one execute before the actual legacy c'tor.
• local vars must be defined inside the method/c'tor block, and not at the class
• c'tors - automatically emit a super c'tor invocation if no explicit one (super or this-object) - this will be the first statement inside the constructor body.
• c'tors - automatically emit an empty no-param c'tor if one doesn't exist.
• class resource member definition. The access modifiers are fixed only for STREAM - haven't touched the other ones yet.
• super() and this-object() constructor call statements
• buffer parameters and class members

I'll take the OBJECT_INVOCATION next (including the SUPER and THIS-OBJECT dereferencing).

Greg Shah wrote:

One thing to consider is that our object wrapper is a generic. When one considers that FIELD_CLASS is always of type Progress.Lang.Object, any FIELD_CLASS member or return type of object should really be object<BaseObject>. This would be the case for DMOs and for the business logic conversion.

I added the support for making the field generic. Now the code is generated as follows:

import com.goldencode.p2j.oo.lang.*; // optional include
private final object<BaseObject> obj; // the member
public Tt0_1_1Impl() { obj = new object<BaseObject>(); } // dmo c'tor
public object<BaseObject> getObj() { return (new object<BaseObject>(obj)); } // field getter
public void setObj(object<BaseObject> obj) { this.obj.assign(obj); } // field setter

Note:
I see only a single case of such usage in customer POC code.

Committed revision 11328. Please review.

Code Review Task branch 3750a Revision 11327

It is good.

In line 705 of buffer_definitions.rules, there is this code <rule>this.getAncestor(-1).downPath("INTERFACE_DEF"). It was inserted around something that was already marked on="false" (see line 706 <rule on="false">ttnoundo). I think line 705 also needs to be on="false".

Code Review Task branch 3750a Revision 11328

Nice work! I think you have the M0 part in a good place. Do you feel comfortable trying to implement the FIELD_CLASS in the conversion rules?

Greg Shah wrote:

Code Review Task branch 3750a Revision 11328

Nice work! I think you have the M0 part in a good place.

Thank you. I am doing now the full POC M0 reconversion just to be on safe side.

Do you feel comfortable trying to implement the FIELD_CLASS in the conversion rules?

I will do it tomorrow.

I will do it tomorrow.

Thank you!

1. compute a mapping of all classes and their defined methods/attributes/etc, to the converted java name.
2. use this global mapping to determine the converted javaname for a method/attribute access.

I'm not working on it. I'm writing up my plan for properties right now. As you will see shortly, I've got my hands full there.

I agree your approach makes sense. I had to do this for the class names but didn't get to the rest of the members. Hopefully the approach can be extended.

Greg Shah wrote:

I agree your approach makes sense. I had to do this for the class names but didn't get to the rest of the members. Hopefully the approach can be extended.

OK, I'll extend it. Just making sure we don't work on the same things.

The following testcases show the behavior of properties (except for the access control options):

class oo.PropertyHolder:

   def private var flag2-setter-called as int init 0.
   def private var flag2-shadow        as log init false.
   def private var no-read             as log init false.
   def private var no-write            as log init false.

   define property flag1 as log init false
      get.
      set.

   define property flag2 as log init false
      get:
         message "getter 1 flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
         if not no-write then
         do:
            /* protect from infinite recursion */
            no-read = true.
            flag2 = true.
            no-read = false.
         end.
         message "getter 2 flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
         return false.
      end.
      set(input new-flag as log):
         if no-read then
            message "setter 0 flag2 = UNKNOWN (no-read is on); setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
         else
         do:
            /* protect from infinite recursion */
            no-write = true.
            message "setter 1 flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
            no-write = false.
         end.

         flag2 = new-flag.
         flag2-setter-called = flag2-setter-called + 1.
         flag2-shadow = new-flag.

         if no-read then
            message "setter 2 flag2 = UNKNOWN (no-read is on); setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
         else
         do:
            /* protect from infinite recursion */
            no-write = true.
            message "setter 3 flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
            no-write = false.
         end.
      end.

   define property flag3 as log init false
      get:
         message "getter with no return".
      end.
      set.

   /*
      A single call to test() on a freshly instantiated instance yields this output:
      getter 1 flag2 = no; setter-called = 0; flag2-shadow = no
      setter 0 flag2 = UNKNOWN (no-read is on); setter-called = 0; flag2-shadow = no
      setter 2 flag2 = UNKNOWN (no-read is on); setter-called = 1; flag2-shadow = yes
      getter 2 flag2 = yes; setter-called = 1; flag2-shadow = yes
      test 1: flag1 = no; flag2 = no; setter-called = 1; flag2-shadow = yes
      getter 1 flag2 = yes; setter-called = 1; flag2-shadow = yes
      getter 2 flag2 = yes; setter-called = 1; flag2-shadow = yes
      setter 1 flag2 = no; setter-called = 1; flag2-shadow = yes
      getter 1 flag2 = yes; setter-called = 2; flag2-shadow = yes
      getter 2 flag2 = yes; setter-called = 2; flag2-shadow = yes
      setter 3 flag2 = no; setter-called = 2; flag2-shadow = yes
      getter 1 flag2 = yes; setter-called = 2; flag2-shadow = yes
      setter 0 flag2 = UNKNOWN (no-read is on); setter-called = 2; flag2-shadow = yes
      setter 2 flag2 = UNKNOWN (no-read is on); setter-called = 3; flag2-shadow = yes
      getter 2 flag2 = yes; setter-called = 3; flag2-shadow = yes
      test 2: flag1 = yes; flag2 = no; setter-called = 3; flag2-shadow = yes
      getter with no return
      test 3: flag3 = ?
   */
   method public void test():
      message "test 1: flag1 = " + string(flag1) + "; flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).
      flag1 = true.
      flag2 = true.
      message "test 2: flag1 = " + string(flag1) + "; flag2 = " + string(flag2) + "; setter-called = " + string(flag2-setter-called) + "; flag2-shadow = " + string(flag2-shadow).

      message "test 3: flag3 = " + if flag3 eq ? then "?" else string(flag3).
   end.

end.

class oo.PropertyHolderExtents:

   define property flag as log init false extent 2
      get.
      set.

   define property flag-indet as log init [false, true] extent
      get.
      set.

   define property flag-indet-2 as log extent
      get.
      set.

   define property nums as int init 0 extent 5
      get(input idx as int):
         return nums[idx].
      end.
      set(input new-num as int, input idx as int64):
         nums[idx] = new-num.
      end.

   define property nums-indet as int extent
      get(input idx as int):
         return nums[idx].
      end.
      set(input new-num as int, input idx as int64):
         nums[idx] = new-num.
      end.

   /*
      A single call to test() on a freshly instantiated instance yields this output:
      test 1: flag[1] = no; flag[2] = yes; size = 2
      test 2: flag-indet[1] = no; flag-indet[2] = yes; size = 2
      test 3.1 size = ?
      test 3.2: flag-indet-2[1] = no; flag-indet-2[2] = yes; flag-indet-2[3] = no; size = 3
      test 4: nums[1] = 1; nums[5] = 5; total = 15
      test 5: nums-indet[1] = 27; nums-indet[2] = 24; nums-indet[3] = 21; size = 3; total = 72
   */
   method public void test():
      def var i as int.
      def var t as int init 0.

      flag[2] = true.
      message "test 1: flag[1] = " + string(flag[1]) + "; flag[2] = " + string(flag[2]) + "; size = " + string(extent(flag)).

      /* an initializer will fix the size of an indeterminate property */
      message "test 2: flag-indet[1] = " + string(flag-indet[1]) + "; flag-indet[2] = " + string(flag-indet[2]) + "; size = " + string(extent(flag-indet)).

      /* accessing the indeterminate array will generate "Invalid runtime use of indeterminate extent.  It must be set to a fixed dimension. (11387)" */
      /* message flag-indet-2[1] extent(flag-indet-2). */
      message "test 3.1 size = " + if extent(flag-indet-2) eq ? then "?" else string(extent(flag-indet-2)).
      extent(flag-indet-2) = 3.
      flag-indet-2[2] = true.
      message "test 3.2: flag-indet-2[1] = " + string(flag-indet-2[1]) + "; flag-indet-2[2] = " + string(flag-indet-2[2]) + "; flag-indet-2[3] = " + string(flag-indet-2[3]) + "; size = " + string(extent(flag-indet-2)).

      do i = 1 to extent(nums).
         nums[i] = i.
         t = t + nums[i].
      end.
      message "test 4: nums[1] = " + string(nums[1]) + "; nums[5] = " + string(nums[5]) + "; total = " + string(t).

      t = 0.
      extent(nums-indet) = 3.
      do i = 1 to extent(nums-indet).
         nums-indet[i] = 30 - (i * 3).
         t = t + nums-indet[i].
      end.
      message "test 5: nums-indet[1] = " + string(nums-indet[1]) + "; nums-indet[2] = " + string(nums-indet[2]) + "; nums-indet[3] = " + string(nums-indet[3]) + "; size = " + string(extent(nums-indet)) + "; total = " + string(t).

   end.

end.

def var prop-holder as oo.PropertyHolder.
def var prop-holder-ext as oo.PropertyHolderExtents.

prop-holder = new oo.PropertyHolder().
prop-holder:test().

prop-holder-ext = new oo.PropertyHolderExtents().
prop-holder-ext:test().

Some things to note:

• The basic design is that there is a kind of "hidden" data member that can only be directly accessed from the associated getter and setter.
  • Only the getter can read the value. If the setter tries to read, it calls the getter.
  • Only the setter can write the value. If the getter tries to write, it calls the setter.
  • All other access (even from within the same class) is only through the getter/setter EXCEPT FOR the EXTENT stmt and function (see below).
  • It is possible to create infinite recursion if the getter writes and the setter reads. The example above has extra code to avoid this case.
• Extents
  • Fully supported, including indeterminate extent.
  • Normal subscript syntax is used for access in both reading and writing.
  • Using an initial clause causes an indeterminate extent prop to become fixed.
  • The EXTENT(property) function returns the size, for indeterminate properties it returns unknown value.
  • The EXTENT(property) = size. statement sets the size and makes the property a fixed extent.
• getters/setters
  • A getter or setter that has no explicit implementation will have a default implementation that just reads or writes the value. This works for extent properties as well as scalar props.
  • If an explicit getter implementation is provided:
    • An extent property getter will have a single INPUT idx AS INT (or INT64) parameter that must also be specified.
    • Otherwise the parameter list is empty or missing.
    • If there is no RETURN stmt in the getter implementation, the getter will implicitly return unknown value.
  • If an explicit setter implementation is provided:
    • The first parameter to the setter is an INPUT var AS type where type is the same as the property type.
    • An extent property setter will have a second parameter INPUT idx AS INT (or INT64) that must also be specified as the subscript index.
• It is possible to define properties as abstract.
• If you have both GET and SET specified, then it is read/write. You can also specify just GET (read-only) or just SET (write only). You must have at least a GET or a SET.
• Access Modes
  • I'm leaving the investigation of access modes until later.
  • What is documented is that the main property access mode is the default for the getter and setter. It is possible to override the access mode on either the GET or the SET (but not both). The idea is that if you need to override both, then you should set a different default access mode on the property.
  • It is reportly possible to override the access mode on an abstract property to be less restrictive.

Implementation Plan

Sadly, the approach is a bit complicated.

• Implement a private member var that is only accessible via the getter/setter. This will be similar to a define var.
• The getter and/or setter will be Java methods with the proper access mode as calculated from the definition.
• If defined abstract or in an interface, the member var will not be emitted, only the getter and/or setter methods will emit. This will provide the same result as an abstract property, avoiding the Java issue that abstract data members don't exist.
• The getters will emit in one of the following possibilities:
  • non-extent default getter (no explicit imple)
  • extent default getter (no explicit imple)
  • non-extent explicit getter
  • extent explicit getter
• The setters will emit in one of the following possibilities:
  • non-extent default setter (no explicit imple)
  • extent default setter (no explicit imple)
  • non-extent explicit setter
  • extent explicit setter
• Extent properties will have two additional methods emitted:
  • lengthOf<property_name>() which will implement the delegated EXTENT() function call using ArrayAssigner
  • resize<property_name>() which will implement the delegated EXTENT stmt using ArrayAssigner
  • I think we may need to make a registration call for the indeterminate extent cases too, probably in the placeholder external procedure.
• Property References
  • Detect that the VAR_ reference is actually a property reference. This is a parse time change to leave behind enough information about the containing class and the property being accessed.
  • Get the converted name (I hope to leverage your work there).
  • Determine the type of access (get, set, extent func, extent stmt).
  • Calculate the method name.
  • Emit the method call (static or instance) instead of a reference. The optional referent and object invocation should emit naturally.
  • The parameter list must be mapped into the rvalue as first parm for setter and for extent props the subscript must be the last parm.

Greg Shah wrote:

• Get the converted name (I hope to leverage your work there).

See the changes in 3750a rev 11329 and 11330 - this tracks the converted method names (property and other member names can be added easily). The found-in-source-file annotation gives you the defining OO source file, and from there you get all the details easily (see the oo_references.rules change).

• In some parts, for example a VAR_CLASS, this is used to save the type of the variable
• in other parts (like ClassDefinition.lookupMethodClassName), I would expect to return the method's parent class - but this returns the qname (if the method returns a OO type).

Maybe I'm not understanding properly the goal of the qualified annotation.

I've managed to go around the qualified annotation, but this still confuses me.

3750a rev 11331 has a mostly working OO method calling.

We need to fix the access modifiers for all class data members (variables, resources, etc) - are you taking care of this?

Tomorrow I'll look into how to access the class variables (and chain them).

An important note: at least for OO, the CB phase depends on some state determined by the F0 phase (class definitions/members/etc). So we can't run front/middle/back separately, we need to run everything at once.

Revision 11332 is the first part of the define property support. It is not done yet.

Constantin Asofiei wrote:

Greg, there is some confusion (at least for me) regarding the qualified annotation.

• In some parts, for example a VAR_CLASS, this is used to save the type of the variable
• in other parts (like ClassDefinition.lookupMethodClassName), I would expect to return the method's parent class - but this returns the qname (if the method returns a OO type).

Maybe I'm not understanding properly the goal of the qualified annotation.

It is always supposed to report the type of the sub-expression node on which it is an annotation. For OO_METH_CLASS, it is the type returned by the method. For VAR_CLASS it is the type of that variable.

It is never meant to report the containing class.

We need to fix the access modifiers for all class data members (variables, resources, etc) - are you taking care of this?

Not yet. I'm still working on the properties.

Greg Shah wrote:

We need to fix the access modifiers for all class data members (variables, resources, etc) - are you taking care of this?

Not yet. I'm still working on the properties.

I've added access modifiers for variables in 3750a rev 11333.

Something else: the names for all class members (properties included) need to be computed in the annotations_prep phase - because otherwise external references to these (from program files, etc) will not work, as we don't know the converted java name.

I'm working on fixing this for variables.

The class var access and chaining works; I need to finish the extent function and subscript dereferencing before committing this.

Greg, references to variables outside of the defining class should not be annotated with 'refid' (and this annotation is wrong now, anyway). Does this look wrong to you?

The refid is unique across the project. It can be used between files without an issue. However, I think to make this work (which has not been done before):

1. When we mark a var reference with a temporary idx at parse time, we need to remember the file if it is not the current file.

2. We need a way to calculate the right refid during fixups when the reference is outside the current file. Our temporary ids are unique within a single file only. For classes only (no other file type can have directly referenced members), we would need to create a mapping to the refid. The map key could be "<file>_<temp_idx>" and the value would be the refid. The refid fixups would have to be done in 2 passes. File-internal references can be handled the same way as today. File-external references would have to be handled in another phase (early annotations?).

3. Later, when we use a refid to obtain the node for a given var def, we need a helper that will load the non-local AST and lookup/return the node.

In this way the rest of our logic can remain the same. This would need to work for all types of var defs and other members where we use refid or the equivalent idea.

OK, I'll work on that next. What showed the refid problem was the extent class var references, outside of the defining class.

I'm cleaning up and committing the current changes (for OO var and method chaining).

Greg Shah wrote:

3. Later, when we use a refid to obtain the node for a given var def, we need a helper that will load the non-local AST and lookup/return the node.

• if we load the the non-local AST each time is needed (and this requires the entire .ast file to be parsed), then the time will increase.
• if we cache the class AST files in memory, there might not be enough to keep them all.

The current phase of class var access and chaining is in 3750a rev 11335. I need to rework this to use the 'refid' annotation, and also remove the dependency on the F0 phase.

I need a bit of help configuring (I guess) my isolation workspace with ABL OO class support. My SymbolResolver cannot find any built-ins.
I copied the skeleton to my root (testcases/uast), and when initPossibleClasses is called I can see all classes gathered in output parameter propathCls which is then saved into SymbolResolver.classDict.
However, later filenames_to_java_classnames.xml is empty. I tracked the getBuiltInClasses() call, but it seems to wa.propathClassDicts which is initialized in SymbolResolver ctor but not populated.

I am running with r11334.

Ovidiu Maxiniuc wrote:

I need a bit of help configuring (I guess) my isolation workspace with ABL OO class support.

Please make sure this is setup correctly in your p2j.cfg.xml: <parameter name="oo-skeleton-path" value="/path/to/skeleton" />

Also, please use rev 11336 of 3750a - this separates the front and back phases to not depend on in-memory state.

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

I need a bit of help configuring (I guess) my isolation workspace with ABL OO class support.

Please make sure this is setup correctly in your p2j.cfg.xml: <parameter name="oo-skeleton-path" value="/path/to/skeleton" />

Yes, I have <parameter name="oo-skeleton-path" value="./skeleton" />.
The path seems fine as the list of classes stored in oo4gl is correct in listFakeoutFiles().

Also, please use rev 11336 of 3750a - this separates the front and back phases to not depend on in-memory state.

Updating now.

3750a rev 11337 fixes refid annotations for OO variables references outside of defining class.

Rev 11338 provides support for property getters and setters. It was a major PITA.

After integrating your recent changes, property conversion is broken again. The classname annotation no longer is present during annotations (it is supposed to be there from annotations/variable_definitions.rules which is now being run in annotations_prep.xml. I fixed a minor issue with inMethod calculation but that did not make a difference (see rev 11339). I suspect this is because we no longer have a var_def annotation which seems to be because we still have a tempidx-file annotation during post-parse-fixups. I don't know what to do to resolve this.

I'm also having an issue with the indeterminate extent properties in oo/PropertyHolderExtents.cls, which do not convert properly. I'm not sure what the conversion should look like there, so I could use some guidance.

Greg Shah wrote:

After integrating your recent changes, property conversion is broken again. The classname annotation no longer is present during annotations (it is supposed to be there from annotations/variable_definitions.rules which is now being run in annotations_prep.xml. I fixed a minor issue with inMethod calculation but that did not make a difference (see rev 11339). I suspect this is because we no longer have a var_def annotation which seems to be because we still have a tempidx-file annotation during post-parse-fixups. I don't know what to do to resolve this.

I've noticed this too, I managed to broke it in my last update, trying to fix another issue. I hope to have it fixed shortly (i.e. refid computed properly from tempidx with or without tempidx-file annotation).

I'm also having an issue with the indeterminate extent properties in oo/PropertyHolderExtents.cls, which do not convert properly. I'm not sure what the conversion should look like there, so I could use some guidance.

I'll take a look.

There is some "weirdness" going on in convert/variable_definitions.rules. There is a location where we create a reference_def Java AST node. This works successfully and we have a non-null refid. We then call ref = getAst(refid); but this fails and ref is null. This affects a few downstream annotations and was causing NPE in those places because we were dereferencing ref directly for putAnnotation(). I put in some logging and NPE protection. When the failure is detected, I use putReferenceNote(refid, , ) which works properly to annotate that node. I haven't figured out why this is happening, but it occurs with class-level member variables (DEF VAR in a CLASS_DEF). See my changes on lines 549 and 655.

I also wrote an alternate version of the access mode calculation that honors the parse time access-mode annotation that is supposed to already be there. This also properly sets the default value depending on the type. So I would like to go with this approach. However, one thing I can't explain is that it works for DEF VAR in a CLASS_DEF but not for DEFINE PROPERTY.

TRPL has a 'var leak' where the var defined in current scope is visible in a function call, in some cases - see hasTopLevelInternalNodeAncestor - this uses ref, but doesn't define it. You need to define it in this function.

Ovidiu Maxiniuc wrote:

Constantin Asofiei wrote:

Also, please use rev 11336 of 3750a - this separates the front and back phases to not depend on in-memory state.

Updating now.

There is not much of a difference. I have both builtin_classes.xml and filenames_to_java_classnames.xml empty. I am looking at the SymbolResolver.getBuilInClasses code. The wa.propathClassDicts is iterated and it has only a single item ".$$$" added in initPossibleClasses. There, only a new, empty ScopedSymbolDictionary is added. Either I am really blind or I am updating from a bad location.

Constantin Asofiei wrote:

TRPL has a 'var leak' where the var defined in current scope is visible in a function call, in some cases - see hasTopLevelInternalNodeAncestor - this uses ref, but doesn't define it. You need to define it in this function.

Great catch! THANK YOU!

The TRPL problem and the tempidx issues are fixed in 11340.

Greg Shah wrote:

I'm also having an issue with the indeterminate extent properties in oo/PropertyHolderExtents.cls, which do not convert properly. I'm not sure what the conversion should look like there, so I could use some guidance.

I think we need getters/setters which access the entire extent variable, not just indexed. You can have something like x = o:flag (where flag is extent).

You can also have something like extent(o:flag) = 2 (if flag is indeterminate extent).

I don't understand why the indexed getter returns an array for flag - shouldn't this return just the element?

My approach for the default getter/setter would be to emit both indexed and not-indexed variants, regardless if the property is dynamic or static extent.

Ovidiu Maxiniuc wrote:

There is not much of a difference. I have both builtin_classes.xml and filenames_to_java_classnames.xml empty.

These files are dependent on what you are trying to convert. If your test is NOT using any custom OO classes, then these files will be empty - as their content will have info for the currently used classes.

So, what are you trying to test?

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

There is not much of a difference. I have both builtin_classes.xml and filenames_to_java_classnames.xml empty.

These files are dependent on what you are trying to convert. If your test is NOT using any custom OO classes, then these files will be empty - as their content will have info for the currently used classes.

So, what are you trying to test?

I commented out all code except:

DEFINE VARIABLE MultiValueArray AS Progress.Json.ObjectModel.JsonArray NO-UNDO.

WARNING: Null annotation (full-java-class) for DEFINE [DEFINE_VARIABLE] @1:1 (2791728742403)
WARNING: Null annotation (simple-java-class) for DEFINE [DEFINE_VARIABLE] @1:1 (2791728742403)
WARNING: Null annotation (containing-package) for DEFINE [DEFINE_VARIABLE] @1:1 (2791728742403)
[...]
persist()
^  { null value for annotation 'full-java-class':DEFINE [DEFINE_VARIABLE]:2791728742403 @1:1
   MultiValueArray [SYMBOL]:2791728742407 @1:17
   AS [KW_AS]:2791728742409 @1:33
      Progress.Json.ObjectModel.JsonArray [CLASS_NAME]:2791728742411 @1:36
   NO-UNDO [KW_NO_UNDO]:2791728742413 @1:72
 }

As I noted in #3751-132, I'm planning to add: lengthOf<property_name>() and resize<property_name>() methods to handle the EXTENT() function and EXTENT statement. I'll have to check the use case for returning the entire array. I didn't think that was possible, but if so then we would need an additional getter (and maybe setter).

I don't know if it is needed right now (probably not).

I don't understand why the indexed getter returns an array for flag - shouldn't this return just the element?

It does for fixed extents but does not in the indeterminate case. I think something downstream is changing how the subscripting code emits.

Greg Shah wrote:

I don't understand why the indexed getter returns an array for flag - shouldn't this return just the element?

It does for fixed extents but does not in the indeterminate case. I think something downstream is changing how the subscripting code emits.

For me, all indexed getters return an array, and not a scalar type. A difference I see in the converted result is for indeterminate case, where the setters are not indexed.

Ovidiu Maxiniuc wrote:

I commented out all code except:
[...]
This fails with:
[...]

You might have caught a bad revision. Please update again, your test works for me.

My previous code was successfully emitting the subscript for the fixed case. I can't check the current code because conversion fails with a stack overflow late in core conversion. The line of code that failed is during processing of a file ./strings_comments_whitespace.p with this:

     [java] -----------------------
     [java]       RULE REPORT      
     [java] -----------------------
     [java] Rule Type :   WALK
     [java] Source AST:  [ block ] BLOCK/ @0:0 {171798691841}
     [java] Copy AST  :  [ block ] BLOCK/ @0:0 {171798691841}
     [java] Condition :  ref = getAst(lastid)
     [java] Loop      :  false
     [java] --- END RULE REPORT ---
     [java] 
     [java] 
     [java] 
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1069)
     [java]     at com.goldencode.p2j.convert.TransformDriver.processTrees(TransformDriver.java:532)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.back(ConversionDriver.java:570)
     [java]     at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:852)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:953)
     [java] Caused by: java.lang.StackOverflowError
     [java]     at java.security.AccessController.doPrivileged(Native Method)
     [java]     at java.net.URLClassLoader.findResource(URLClassLoader.java:569)
     [java]     at java.lang.ClassLoader.getResource(ClassLoader.java:1096)
     [java]     at java.lang.ClassLoader.getResource(ClassLoader.java:1091)
     [java]     at java.net.URLClassLoader.getResourceAsStream(URLClassLoader.java:233)
     [java]     at org.apache.xerces.parsers.SecuritySupport$6.run(Unknown Source)
     [java]     at java.security.AccessController.doPrivileged(Native Method)
     [java]     at org.apache.xerces.parsers.SecuritySupport.getResourceAsStream(Unknown Source)
     [java]     at org.apache.xerces.parsers.ObjectFactory.findJarServiceProvider(Unknown Source)
     [java]     at org.apache.xerces.parsers.ObjectFactory.createObject(Unknown Source)
     [java]     at org.apache.xerces.parsers.ObjectFactory.createObject(Unknown Source)
     [java]     at org.apache.xerces.parsers.DOMParser.<init>(Unknown Source)
     [java]     at org.apache.xerces.parsers.DOMParser.<init>(Unknown Source)
     [java]     at org.apache.xerces.jaxp.DocumentBuilderImpl.<init>(Unknown Source)
     [java]     at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.newDocumentBuilder(Unknown Source)
     [java]     at com.goldencode.util.XmlHelper.parse(XmlHelper.java:574)
     [java]     at com.goldencode.util.XmlHelper.parse(XmlHelper.java:438)
     [java]     at com.goldencode.ast.XmlFilePlugin.loadTree(XmlFilePlugin.java:404)
     [java]     at com.goldencode.ast.AstManager.loadTree(AstManager.java:286)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.parseTree(CommonAstSupport.java:858)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.loadTree(CommonAstSupport.java:875)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.getAst(CommonAstSupport.java:946)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.getAst(CommonAstSupport.java:905)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.getAst(CommonAstSupport.java:947)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.getAst(CommonAstSupport.java:905)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.getAst(CommonAstSupport.java:947)
...
infinite recursion
...

That file has some complex comment cases. We use ref = getAst(lastid) in multiple places, including comments and control flow.

Greg Shah wrote:

My previous code was successfully emitting the subscript for the fixed case. I can't check the current code because conversion fails with a stack overflow late in core conversion. The line of code that failed is during processing of a file ./strings_comments_whitespace.p with this:

[...]

That file has some complex comment cases. We use ref = getAst(lastid) in multiple places, including comments and control flow.

To allow non-local ASTs, I changed getAst(<id>) to always try to load the AST file. I'm looking into it.

Please try cleaning your project first - your registry.xml might be obsolete.

Constantin Asofiei wrote:

Please try cleaning your project first - your registry.xml might be obsolete.

Nevermind, see the fix in 11341.

When I remove that file, the conversion completes. I can see that the PropertyHolderExtents.cls has proper subscripting in the getters and setters except for the flag-indet and flag-indet2 cases. Both of these are logical extents and in this case they both have implicit getters and setters emitted. The nums-indet indeterminate case has an explicit getter/setter implementation and this converts OK (it has the subscripts). The weird thing is that I'm using the same implicit templates for flag which is a fixed extent and for that case the templates work.

Where should I look to see how we handle differences for the indeterminate case?

I see the issue, I'm not handling -1 as the extent annotation. If I see 0 or 1, can I assume it is scalar?

Greg Shah wrote:

I see the issue, I'm not handling -1 as the extent annotation.

Exactly, -1 is for indeterminate extent.

If I see 0 or 1, can I assume it is scalar?

No, treat it as extent.

So if an extent annotation exists at all, it is always an extent, even if set to 0 or 1?

oo.ComplexImplements has regressed recently. It now generates obj and num references even though the class is empty. These are compile failures.

Greg Shah wrote:

So if an extent annotation exists at all, it is always an extent, even if set to 0 or 1?

Actually I just found this comment in variable_initializers.rules:113:

           (extent == 0 is a scalar var
           so ignore that; extent == -1 is an unspecified extent size and
           in that case it is valid to have initializers that fix the
           size, so ignore that)

And I'm pretty sure extent 1 is not scalar (I recall having this conversation when implementing this).

Questions about the new early name conversion approach:

• How do I move the method_def and property getter/setter naming calculations back into annotations_prep?
• What do I do to resolve or create the cross-file refids?
• Can I simply use getAst() on an "external" refid later to access the original defining node?

Revision 11342 fixes the extent getter/setters. I still need to add the other extent helpers. I plan to postpone the whole array accessors at this time, until I have the chance to investigate further. It does seem to break the contract of the property (it doesn't hide the data structure), so I was assuming it wouldn't work. Of course, that has never stopped Progress before.

Greg Shah wrote:

Questions about the new early name conversion approach:

• How do I move the method_def and property getter/setter naming calculations back into annotations_prep?

You mean, to enable non-local access? method_def is in annotations_prep, in naming.rules; same as variable definitions.

The current approach for methods/vars is to rely on ConvertedClassName - this is populated during annotations_prep, saved to disk, and read in annotations phase (it includes methods, variables and extent value for the vars). This can be enhanced to handle the properties. After this, oo_references relies on "found-in-source-file" annotation to determine the correct ConvertedClassName instance, from the fname2jname map, etc. The idea is: I haven't got a chance to check if this can be removed and let it rely solely on refid annotations; but note that refid will not work for builtin class members, so we still need this approach.

Otherwise, if you have the refid, you can get the defining AST and read from that.

• What do I do to resolve or create the cross-file refids?

Look how variables are annotated in Variable.annotateOptions - it adds a tempidx-file annotation, if this is a a class member. After this, post_parse_fixups creates a map of <file>_<tempidx>, <AST-id> pairs (only for class var members, currently), saves to disk, and is read by early_annotations - which is responsible for finishing the refid work.

• Can I simply use getAst() on an "external" refid later to access the original defining node?

Yes. The AST will be loaded automatically.

oo.ComplexImplements has regressed recently. It now generates obj and num references even though the class is empty. These are compile failures.

I'm not getting this, is converted fine for me.

Constantin Asofiei wrote:

You might have caught a bad revision. Please update again, your test works for me.

Well, I found the issue. FWD conversion was working faster by using multiple threads. There were different SymbolResolver$WorkArea-s. So different propathClassDicts.

After disabling multi-threading the .xml-s got populated with classes.

Ovidiu Maxiniuc wrote:

Constantin Asofiei wrote:

You might have caught a bad revision. Please update again, your test works for me.

Well, I found the issue. FWD conversion was working faster by using multiple threads. There were different SymbolResolver$WorkArea-s. So different propathClassDicts.

After disabling multi-threading the .xml-s got populated with classes.

Please create a task with this bug.

I committed the small change related to FIELD_CLASS. Revision 11343.

3750a rev 11345 added ON ... THROW support, 'UNDO, THROW' and BLOCK-LEVEL and ROUTINE-LEVEL stmts.

Now I'm working on the CATCH block. This need to convert to something like:

         doBlock(TransactionType.SUB, "blockLabel0", onPhrase0, new Block(
                  (Init) () ->
         {
            catchError(BaseError.class, (ex) ->
            {
                // the block code here
            });
         },
                  (Body) () -> 
         {
            message("e");
            undoThrow("blockLabel0", e);
         }));

Also, the AST doesn't look right; it is missing a CATCH_BLOCK/BLOCK parent (like we do for ON phrase, where a TRIGGER_BLOCK is generated):

        <ast col="0" id="47244640328" line="0" text="statement" type="STATEMENT">
          <ast col="1" id="47244640329" line="10" text="catch" type="KW_CATCH">
            <annotation datatype="java.lang.Long" key="tempidx" value="286"/>
            <annotation datatype="java.lang.String" key="qualified" value="progress.lang.error"/>
            <annotation datatype="java.lang.String" key="source-file" value="../../../skeleton/oo4gl/Progress/Lang/Error.cls"/>
            <annotation datatype="java.lang.Boolean" key="builtin-cls" value="true"/>
            <annotation datatype="java.lang.Boolean" key="dotnet-cls" value="false"/>
            <annotation datatype="java.lang.String" key="name" value="ex"/>
            <annotation datatype="java.lang.Long" key="type" value="361"/>
            <ast col="7" id="47244640331" line="10" text="ex" type="SYMBOL"/>
            <ast col="10" id="47244640333" line="10" text="as" type="KW_AS">
              <ast col="19" id="47244640337" line="10" text="progress.lang.error" type="CLASS_NAME">
                <annotation datatype="java.lang.String" key="qualified" value="progress.lang.error"/>
                <annotation datatype="java.lang.String" key="source-file" value="../../../skeleton/oo4gl/Progress/Lang/Error.cls"/>
                <annotation datatype="java.lang.Boolean" key="builtin-cls" value="true"/>
                <annotation datatype="java.lang.Boolean" key="dotnet-cls" value="false"/>
                <annotation datatype="java.lang.Long" key="oldtype" value="2637"/>
              </ast>
            </ast>
            <ast col="0" id="47244640340" line="0" text="statement" type="STATEMENT">
              <ast col="3" id="47244640341" line="11" text="message" type="KW_MSG">
                <ast col="0" id="47244640343" line="0" text="" type="CONTENT_ARRAY">
                  <ast col="0" id="47244640344" line="0" text="expression" type="EXPRESSION">
                    <ast col="11" id="47244640345" line="11" text="&quot;something bad happened&quot;" type="STRING">
                      <annotation datatype="java.lang.Boolean" key="is-literal" value="true"/>
                    </ast>
                  </ast>
                </ast>
              </ast>
            </ast>
          </ast>
        </ast>

3750a rev 11347 adds conversion support for the CATCH statement. In the end there was no need for a CATCH_BLOCK/BLOCK, but a INNER_BLOCK/BLOCK - after this, everything emitted naturally, with just some small adjustments.

3750a rev 11348 adds USING conversion support.

I'm taking - DEFINE EVENT and the related features next.

• 4GL
  

  DEFINE PUBLIC EVENT p1 SIGNATURE VOID ( INPUT v1 AS CHARACTER ). 
  p1:Publish("some-value").
  p1:Subscribe(someMethod_Handler).
  p1:Unsubscribe(someMethod_handler).
  

• FWD:
  

  public ClassEvent p1 = new ClassEvent(character.class);
  p1.publish("some-value").
  p1.subscribe(this, "someMethod_Handler).
  p1.unsubscribe(this, "someMethod_Handler).
  

Constantin Asofiei wrote:

The proposed conversion is:

• 4GL
  [...]
• FWD:
  [...]

It is the most close match so the approach is reasonable. As a short term solution, I think that is OK (i.e. to get the code converted today).

Events can be abstract. This "feature" of 4GL event support suggests we need to move to a purely method based approach as we did with properties. This way the interface is maintained and enforced in the same way. So it would be something like:

void publishP1(character v1)
void subscribeP1(<subscriber>, <procedure_or_method>)
void unsubscribeP1(<subscriber>, <procedure_or_method>)

The subscribe and unsubscribe are tricky. The subscriber is optional and can be either a handle or an object instance. The target is either an internal proc or a void method (the transactional equivalent of an internal proc).

In a Java approach, we would prefer to use lambdas. This would require us to generate a functional interface for the signature. Then the subscriber (which can be outside the defining class) would pass that lambda through. But that would only work for the method case.

Since there are only 3 things you can do with an event and all of them are actions, I think this is a good mapping. I haven't seen any code that passes an event as a reference or assigns an event. Can you please confirm this? If indeed, it is not possible to really treat an event as data, then we are safe in moving to the method approach.

We need to confirm this, but the 4GL docs say that the publish can only ever be called from within the same defining class (i.e. that the publish method is always private).

The access mode then really only is used to calculate the subscribe/unsubscribe access modes.

The methods themselves could use a private ClassEvent instance that would be the worker. This instance would have some unique event ID that could be registered in the named events support (assuming the behavior is the same as named events). I guess they are using that "under the covers", just like they re-used persistent procedures.

Of course the static modifier would be honored as well for all methods and for the private instance of ClassEvent.

Greg Shah wrote:

Events can be abstract. This "feature" of 4GL event support suggests we need to move to a purely method based approach as we did with properties. This way the interface is maintained and enforced in the same way. So it would be something like:

I understand, but this is enforced by 4GL at compile time - so the override event must match the abstract version for it to compile.

Since there are only 3 things you can do with an event and all of them are actions, I think this is a good mapping. I haven't seen any code that passes an event as a reference or assigns an event. Can you please confirm this?

Yes, only actions are allowed with an event.

If indeed, it is not possible to really treat an event as data, then we are safe in moving to the method approach.

OK, I'll go this way.

I understand, but this is enforced by 4GL at compile time - so the override event must match the abstract version for it to compile.

I think it would work the same way for the converted code with the method approach. This can be in an interface or abstract class, just like the 4GL.

The only issue is that a class based subscriber might have some level of signature checking at compile time. I'm not sure. A lambda approach for class based subscribers could achieve the same result but that is something we can consider later.

OTOH, a problem with method approach is the signature: the PUBLISH can have POLY arguments, and this complicates things a lot.

Using the p1.publish(some-poly-call, "a", "b"); java approach is easier because we don't need to maintain the signature in the converted code.

I think trying to emulate what 4GL does with the abstract and interface events will give us more headache than is worth, in the long run. More, you can't override a non-abstract event.

The primary benefit I see is that it keeps intact the design approach of the original application. Future development in Java (which implements an interface or extends an abstract class) will retain the existing contract.

A secondary benefit is that there is compile-time checking for the publish in the method approach.

The downside is that any use of POLY parameters will need to be wrapped/casted properly. But there is no ambiguity about the type of the cast, right? So it is just a little bit of extra work to achieve the benefits. Am I forgetting something or mistaken?

In the usage of convert_member_name during annotations_prep, shouldn't we use a different legacy2java map for variables than for methods? These are 2 different namespaces, so there will be collisions but those collisions should not be detected or disambiguated.

I can make the change, but I just want to confirm that I am understanding the approach properly.

There is something else I don't understand about legacy2java. The map is global for the system, but I think it should be class-specific.

Greg Shah wrote:

In the usage of convert_member_name during annotations_prep, shouldn't we use a different legacy2java map for variables than for methods? These are 2 different namespaces, so there will be collisions but those collisions should not be detected or disambiguated.

Yes, we need different legacy2java maps.

There is something else I don't understand about legacy2java. The map is global for the system, but I think it should be class-specific.

Yes, just reset it in a rule-set-level init-rules.

With r11343, the branch 3750a should be able to handle FIELD_CLASS tokens.
Constructs like

DEFINE TEMP-TABLE tt1 NO-UNDO 
   FIELD obj AS Progress.Lang.Object.
CREATE tt1.
ASSIGN tt1.obj = NEW Progress.Lang.Class().

FOR EACH tt1 WHERE obj NE ? AND VALID-OBJECT(tt0.obj):
   DELETE OBJECT tt1.obj NO-ERROR.
   DELETE tt1. 
END.

tt1.create();
tt1.setObj(new object<BaseObject>(ObjectOps.newInstance(com.goldencode.p2j.oo.lang.LegacyClass.class))); // 2nd line

AdaptiveQuery query0 = new AdaptiveQuery();
forEach("loopLabel0", new Block((Init) () -> {
   query0.initialize(tt1, "tt1.obj is not null", () -> tt1.getObj().isValid(), "tt1.id asc");
}, (Body) () -> {
   query0.next();
   if ((tt1.getObj().isValid()).booleanValue()) {
      silent(() -> ObjectOps.delete(tt1.getObj()));
   }

   tt1.deleteRecord();
}));

Yet, think that there is an issue with the generics covariance in the constructor of object<BaseObject>() in the field assignment in 2nd line. I think that the setter in generated dmo should be generated as:

public void setObj(object<? extends BaseObject> obj) { this.obj.assign(obj); }

public void setObj(object<BaseObject> obj) { this.obj.assign(obj); }

With this in place, the assignment in 2nd line should be rewritten as

ttgc.setObj(new object<>(ObjectOps.newInstance(com.goldencode.p2j.oo.lang.LegacyClass.class)));

BTW, I committed a small update that fixes the packages classes with longer paths (r11349) (one or more / were generated in sources file instead of "." separator in package path).

...
tt1.setObj(new object<BaseObject>(ObjectOps.newInstance(com.goldencode.p2j.oo.lang.LegacyClass.class))); // 2nd line
...

I don't think the new object<BaseObject>() is needed here. Shouldn't we just define ObjectOps.newInstance() to return object and then it will work directly?

There is something else I don't understand about legacy2java. The map is global for the system, but I think it should be class-specific.

Yes, just reset it in a rule-set-level init-rules.

When used from naming.rules there is a file-specific map each time, so that is already OK. I think the issue is during the registration of the built-in classes. That map is shared for all of those classes. I will reset it when the class is changed.

Looking closer legacy2java is already reset in a while loop just after a new class is loaded. It is already OK. Sorry for the false alarm.

Greg Shah wrote:

I don't think the new object<BaseObject>() is needed here. Shouldn't we just define ObjectOps.newInstance() to return object and then it will work directly?

Yes, ttgc.setObj(ObjectOps.newInstance(com.goldencode.p2j.oo.lang.LegacyClass.class)); works, but the covariance problem remains. The setters in DMO need to be generated as setObj(object<? extends BaseObject> obj). Otherwise it will not compile.

static mode is not working now - can you look at it, please?

Constantin Asofiei wrote:

static mode is not working now - can you look at it, please?

I guess you mean static mode for naming conversion?

Your previous tests showed that there can be no overlap between static and instance method names (same restriction as in Java). So the same namespace is used.

Greg Shah wrote:

Constantin Asofiei wrote:

static mode is not working now - can you look at it, please?

I guess you mean static mode for naming conversion?

No, static modifiers doesn't emit for variables... I'll look at it.

OTOH, try a method protected abstract - it will not emit properly; please look at this one.

Ovidiu, In the end your per-folder failure can't be solved easily, as the non-local method call is from a different folder, and the call is not annotated with 'refid':

                  <ast col="21" id="21474836560" line="24" text="SetValue" type="OO_METH_VOID">
                      <annotation datatype="java.lang.Long" key="access" value="2007"/>
                      <annotation datatype="java.lang.Boolean" key="static" value="true"/>
                      <annotation datatype="java.lang.String" key="found-in-cls" value="sys.someclass"/>
                      <annotation datatype="java.lang.String" key="found-in-source-file" value="./abl/src/application/src/sys/someclass.cls"/>
                      <annotation datatype="java.lang.Boolean" key="reachable" value="true"/>
                      ...
                  </ast>

Greg: we should annotate all non-local var, property and method access with a 'refid' annotation, so that the current fname2jname approach (Which reads the external filenames_to_java_classnames.xml file, generated from the current list of processed files) is not used for converted files; this will be used only for OO references to builtin class members.

We could add this to the data stored in ConvertedClassName to allow resolving the temp-idx later.

I am deep into changes in ConvertedClassName, annotation_prep.xml and annotations/naming.rules. I want to finish that first.

Greg Shah wrote:

We could add this to the data stored in ConvertedClassName to allow resolving the temp-idx later.

The idea here is that the defining class is NOT included in the current file list, for CB (but was included for F2+M0). And we need to know the i.e. method/var name; having the refid will work.

I am deep into changes in ConvertedClassName, annotation_prep.xml and annotations/naming.rules. I want to finish that first.

OK.

• For interfaces:
  

                void publish_<event-name>(<event-signature>)
                void subscribe_<event-name>([<subscribe-handle>, ]<procedure>)
                void unsubscribe_<event-name>([<subscriber-handle>, ]<procedure>)
                void subscribe_<event-name>(<object>, <method-name>)
                void unsubscribe_<event-name>(<object>, <method-name>)
  

• For abstract:
  

                <access> abstract void publish_<event-name>(<event-signature>);
                <access> abstract void subscribe_<event-name>([<subscribe-handle>, ]<procedure>);
                <access> abstract void unsubscribe_<event-name>([<subscriber>, ]<procedure>);
                <access> abstract void subscribe_<event-name>(<object>, <method-name>);
                <access> abstract void unsubscribe_<event-name>(<object>, <method-name>);
  

• For impl:
  

                <access> void publish_<event-name>(<event-signature>)
                {
                    <event-name>.publish(<arguments>);
                }
                <access> void subscribe_<event-name>([<subscribe-handle>, ]<procedure>)
                {
                    <event-name>.subscribe([<subscribe-handle>, ]<procedure>);
                }
                <access> void unsubscribe_<event-name>([<subscriber-handle>, ]<procedure>);
                {
                    <event-name>.unsubscribe([<subscribe-handle>, ]<procedure>);
                }
                <access> void subscribe_<event-name>(<object>, <method-name>);
                {
                    <event-name>.subscribe(<object>, <method-name>);
                }
                <access> void unsubscribe_<event-name>(<object>, <method-name>);
                {
                    <event-name>.unsubscribe(<object>, <method-name>);
                }
  

The event will be defined like:

   public ClassEvent <event> = new ClassEvent(<signature>);

The idea here is the procedure handle is optional, but when using a method reference in 4GL, we need to emit the defining object instance as first parameter.

Revision 11350 has the following changes:

• Separated variables and methods into separate legacy name maps.
• Moved these maps and the javaname conflict detection back into ConvertedClassName.
• Moved property name conversion back to annotations_prep.
• The method names used for properties are now disambiguated from other methods in the class.
• The resulting property mappings are stored in ConvertedClassName so that the results can be looked up later.

Property references do not yet use this new infrastructure.

The emitted methods for DEFINE EVENT will be:

The publish method when implemented, must be private.

For abstract or interface cases, the publish method should be omitted. It is never part of the external interface for a class so it cannot be put into an external definition of the interface.

public ClassEvent <event> = new ClassEvent(<signature>);

This should be a private member.

Otherwise the plan looks really good.

I don't have a problem relying upon the refid approach for final resolution of the names by references. This is the same way we do it today.

Questions

1. The current refid support for variables (which includes properties) already handles this (through the ooVarTempIdx map shared via XML between post-parse-fixups and early_annotations where the cross-file tempidx resolution happens. I don't think anything else is needed there. Am I wrong?

2. You are suggesting that the method_def and method calls get pushed into this same approach? That is really the work needed here. We would need to:

• at parse time:
  • remember each method's tempidx and tempidx-file (the containing ClassDefinition knows this part)
  • mark each method call with the tempidx and tempidx-file annotations
• at post-parse-fixups, we create an equivalent to ooVarTempIdx for methods
• at early_annotations
  • we read the equivalent to ooVarTempIdx for methods
  • use ooVarTempIdx it to resolve the tempidx and tempidx-file annotations to a refid
• change method call references to use refid to get the javaname

3. Is there anything I am misunderstanding or forgetting?

I can take this once you confirm I know what you're suggesting.

In oo_calls.rules:

      <rule>upPath("ASSIGNMENT/EXPRESSION") and 
            this.type &gt; prog.begin_oo_meth and 
            this.type &lt; prog.end_oo_meth
         <action>
            createPeerAst(java.static_method_call, getNoteString("javaname"), closestPeerId)
         </action>
      </rule> 

This is a "standalone" method call (no qualifier). I think the upPath("ASSIGNMENT/EXPRESSION") is too restrictive. Such a method call might appear anywhere that an expression or sub-expression can occur, just like a FUNC_* call. What are you trying to exclude from this case? Why not just add parent.type != prog.object_invocation?

This code:

         <action>lastid = closestPeerId</action>
         <rule>ref2.type &gt; prog.begin_oo_meth and ref2.type &lt; prog.end_oo_meth
            <action>
               lastid = createJavaAst(java.method_call, 
                                      #(java.lang.String) ref2.getAnnotation("javaname"), 
                                      lastid)
            </action>
            <action>copy.putAnnotation("peerid", lastid)</action>
         </rule>

If the call is static, shouldn't we emit java.static_method_call?

Greg, does this AST look right to you, for a x:e1 access for the class event e1?

                <ast col="7" id="30064771354" line="27" text=":" type="OBJECT_INVOCATION">
                  <ast col="6" id="30064771355" line="27" text="x" type="VAR_CLASS"/>
                  <ast col="10" id="30064771356" line="27" text=":" type="OBJECT_INVOCATION">
                    <ast col="8" id="30064771357" line="27" text="e1" type="CLASS_EVENT"/>
                  </ast>
                </ast>

OBJECT_INVOCATION
   OBJECT_INVOCATION
     VAR_CLASS
   CLASS_EVENT

Greg Shah wrote:

I don't have a problem relying upon the refid approach for final resolution of the names by references. This is the same way we do it today.
...
I can take this once you confirm I know what you're suggesting.

Correct, this is what I am suggesting. This is not a show-stopper if you convert the full project, but can be helpful to have the F2+M0 for the entire project and be able to run CB for a subset of the files. Now this is not working, because of the method's refid limitation (and everything else which can be accessed non-locally - class events, properties, etc).

Greg Shah wrote:

This is a "standalone" method call (no qualifier). I think the upPath("ASSIGNMENT/EXPRESSION") is too restrictive. Such a method call might appear anywhere that an expression or sub-expression can occur, just like a FUNC_* call. What are you trying to exclude from this case? Why not just add parent.type != prog.object_invocation?

I think you are correct, we can just parent.type != prog.object_invocation.

If the call is static, shouldn't we emit java.static_method_call?

This part is a little tricky. In 'brewing' terms, this is not a static call, but a method call - why? Because in this case the reference is the class name - see this code in oo_calls.rules:

      <rule>this.type == prog.class_name and 
            parent.type == prog.object_invocation and 
            this.indexPos == 0
         <action>
            createJavaAst(java.reference, getNoteString("full-java-class"), closestPeerId)
         </action>
      </rule>

If you want it to be a static method call, then you need to emit the qualified class name and the method name together.

The tree looks correct.

The object chaining always matches the leftmost referent first (the primary_expr in chained_object_members). If it is then followed by COLON, we match that, change its type to OBJECT_INVOCATION and make that the root node. This places the initial referent as the first child of the root node. The 2nd operand is matched with downstream_chained_reference and we keep looping so long as a COLON follows.

This is also how we deal with handle based chaining, but we may be rewriting that tree somewhere downstream.

If you want it to be a static method call, then you need to emit the qualified class name and the method name together.

For now, as long as it works I'm OK.

This the refid is needed for non-local events, too - otherwise I can't find the event's javaname to be able to generate the correct method call.

Conclusion: to be complete, anything which can be accessed non-locally requires the refid annotation, for every reference.

Having something like this inside a OO method def:

     def var h as handle.
     def var i as int.
     i = h:next-sibling:prev-sibling:x.     

This is because our parser collects vars local to methods as class members. But even so, assuming the h var is a class member, it really should be annotated with oldtype.

Revision 11352 adds the extra methods for extent properties length-of and the indeterminate resize.

I think properties support only needs the following:

• find a way to make the access mode appear for the actual property member, it must go through a different path than class level define vars, but it is not obvious what that is
• rework the references to the property to emit as method calls

This last part will need the refid support. How do you want to split this up?

I did a quick check for non-local properties and although the refid is there, it fails to lookup the javaname. Have you tested non-local variables? I would expect the same result there.

I started on the method support but the tempidx at the call sites does not match the value in the method_def. The ClassDefinition being used is the right one at lookup time, because the tempidx-file annotation is correct. I will keep working on this, but I'm worried that the getAst() support is not working for non-local refids.

With r11353 the code changes as follows (see previous in note-124 and 193):

import com.goldencode.p2j.oo.lang.*; // optional include
private final object<? extends BaseObject> obj; // the member
public Tt1_1_1Impl() { obj = new object<>(); } // dmo c'tor
public object<? extends BaseObject> getObj() { return (new object<>(obj)); } // field getter
public void setObj(object<? extends BaseObject> obj) { this.obj.assign(obj); } // field setter

tt1.setObj(someObject); // setter usage
tt1.setObj(ObjectOps.newInstance(com.goldencode.p2j.oo.json.objectmodel.JsonArray.class));  // setter usage
someObject.assign(tt1.getObj()); // getter usage

All code is compilable in my testcase.

Greg Shah wrote:

I will keep working on this, but I'm worried that the getAst() support is not working for non-local refids.

It's working, place a breakpoint in CommonAstSupport.getAst:947 - the non-local AST file will be loaded.

So I finally was able to make the local chaining for class events work, too. Now it converts OK - what is not working is non-local event chaining.

See rev 11354 and 11355.

Note that I fixed the static for variables and events... you can't use access annotation for java.reference_def to make it static, you need explicit static=true annotation.

Tomorrow I'll try to solve this:

Non-local references. We have some work to do to enable methods, vars, properties and events for non-local access.

In revision 11357, I've added the rewriting of the references (local and non-local) to properly call the 4 different methods.

There is an issue remaining where the length-of-<prop> method does not emit its FUNC_INT and property reference. This needs to be fixed for the POC. Otherwise it is a very good first version of property support.

Open issues for later:

• test the runtime
• check that a getter with no return does return unknown
• confirm that the overriding of access modes for abstract props
• why isn't the private access mode being honored for the member var that is emitted for the actual backing property?
• handle arbitrary chaining of property references in oo_references
• writing the prop in the getter should map to the setter and vice versa

The getter/setter for properties having as type an OO class must have the proper generic set. Currently they are emitted incorrect:

   public static object getInstance()
   {
      return function("Instance", object.class, new Block((Body) () -> 
      {
         returnNormal(instance);
      }));
   }

   private static void setInstance(final object<null> _var)
   {
      object<null> var = TypeFactory.initInput(_var);

      internalProcedure(new Block((Body) () -> 
      {
         instance.assign(var);
      }));
   }

For a:

    define public static property Instance as class oo.Event2 no-undo
       get ():
          // Instance:v3 = 456.
          return Instance.
       end get.
       private set.

Committed revision 11359.
Fixed covariance of generics for OO methods. Now the signature for

    METHOD STATIC PUBLIC Progress.Json.ObjectModel.JsonArray m5(
                  INPUT m5p1 AS Progress.Lang.Object,
                  OUTPUT m5p2 AS Progress.Lang.Object,
                  INPUT-OUTPUT m5p3 AS Progress.Lang.Object):

   public static object<com.goldencode.p2j.oo.json.objectmodel.JsonArray> m5(
         final object<? extends com.goldencode.p2j.oo.lang.BaseObject> _m5p1, 
         final object<? extends com.goldencode.p2j.oo.lang.BaseObject> m5p2, 
         final object<? extends com.goldencode.p2j.oo.lang.BaseObject> m5p3)
   {
      object<? extends com.goldencode.p2j.oo.lang.BaseObject> m5p1 = TypeFactory.initInput(_m5p1);

Added wrappers for literal parameters in OO methods. (Constantin, I guess you were hunting this yesterday, now the literals should emit wrapped to specific BDT).

LE: an example of usage for the later:

METHOD STATIC PUBLIC CHARACTER m2(m2p1 AS CHARACTER, OUTPUT m2p2 AS LOGICAL):
[...]
k1 = m2("777", k2).

public static character m2(final character _m2p1, final logical m2p2)
[...]
k1.assign(m2(new character("777"), k2));

Constantin Asofiei wrote:

The getter/setter for properties having as type an OO class must have the proper generic set. Currently they are emitted incorrect:
[...]

For a:
[...]
working on it.

As we discussed, I am taking this over.

I'm looking into the conversion abend related to // Instance:v3 = 456. commented line.

Constantin Asofiei wrote:

I'm looking into the conversion abend related to // Instance:v3 = 456. commented line.

This and other issues are fixed in 11358 and 11360.

The generation of setters is fixed. Committed revision 11361.

• is this access to Instance legal in ABL OO?
  

           DEFINE VARIABLE ja1 AS Progress.Lang.Object NO-UNDO.
           Instance = new TestClass().
           ja1 = Instance.
  

• is it normal for the VAR_CLASS Instance from the first assignment above NOT to have an refid? What about an oldtype?

Ovidiu Maxiniuc wrote:

The generation of setters is fixed. Committed revision 11361.

However, I added some code to that from note-222 and I'm having trouble with the conversion. Two questions:

• is this access to Instance legal in ABL OO?

Where is this code located? In a class method/c'tor? If so then yes, local static members can be accessed without qualifier.

• is it normal for the VAR_CLASS Instance from the first assignment above NOT to have an refid? What about an oldtype?

refid is required. oldtype is a problem in the parser - see #3751-215

Are there tempidx annotations left behind in the AST file? refid is computed during post-parse-fixups, from these annotations.

Constantin Asofiei wrote:

Where is this code located? In a class method/c'tor? If so then yes, local static members can be accessed without qualifier.

Yes, they are in body of a (static) method.

• is it normal for the VAR_CLASS Instance from the first assignment above NOT to have an refid? What about an oldtype?

refid is required. oldtype is a problem in the parser - see #3751-215

refid is missing. The conversion stops in assignments.rules while attempting to convert to long/int the oldtype annotation (~line 340).

Are there tempidx annotations left behind in the AST file? refid is computed during post-parse-fixups, from these annotations.

Yes, there are a few. Is this OK?

Ovidiu Maxiniuc wrote:

The conversion stops in assignments.rules while attempting to convert to long/int the oldtype annotation (~line 340).

I'm looking into oldtype, is a parser problem, as I mentioned.

Are there tempidx annotations left behind in the AST file? refid is computed during post-parse-fixups, from these annotations.

Yes, there are a few. Is this OK?

No, is not OK. Look into post-parse-fixups and solve the refid annotation.

Constantin Asofiei wrote:

This and other issues are fixed in 11358 and 11360.

Not purposely, I tested the code generated with 11361. It looks like this:

public final class TestClass
extends BaseObject
{
   public static object<? extends com.goldencode.testcases.testPack.TestClass> instance = TypeFactory.object();
   private integer v3 = TypeFactory.integer();
   [...]
   public static object getInstance()
   {
      return function("Instance", object.class, new Block((Body) () -> 
      {
         instance.ref().v3.assign(new integer(456));
         returnNormal(instance);
      }));
   }

[ant:javac] /home/om/workspaces/3751/p2j/src/com/goldencode/testcases/testPack/TestClass.java:122: error: v3 in TestClass is defined in an inaccessible class or interface
[ant:javac]          instance.ref().v3.assign(new integer(456));
[ant:javac]                        ^

         ((TestClass)instance.ref()).v3.assign(new integer(456));

         TestClass ref = instance.ref();
         ref.v3.assign(new integer(456));

Ovidiu Maxiniuc wrote:

I have 2 alternatives that work:
((F3757)instance.ref()).v3.assign(new integer(456));

I assume F3757 is the TestClass name, right? (i.e. you explicitly cast to the expected type). If so, go ahead with this, but only if the accessed member is private

• Instance = new TestClass(). is not compiling
• if you have a member v and do a Instance:v = 0, it will not work properly, because the getter is public static object getInstance() - this needs the correct generic type, same as the setter.

Please work on these.

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

The conversion stops in assignments.rules while attempting to convert to long/int the oldtype annotation (~line 340).

I'm looking into oldtype, is a parser problem, as I mentioned.

Fixed in rev 11362.

Constantin Asofiei wrote:

Constantin Asofiei wrote:

I'm looking into oldtype, is a parser problem, as I mentioned.

Fixed in rev 11362.

That fixed the tempidx annotations left behind in the AST file, too. Cool!

The CB phase failed with a access-mode problem for class events after ~150 minutes, in the code conversion part.

This is fixed in 11363.

I'm working on the following issues:

1. The unexpected/unrelated var initializers that appear in oo/ComplexInterface.cls for me (but not for Constantin). I'm deep into this and hope to have something soon. It is related to putting the initializers into the depot on a previous file (in my unique conversion file list) and then picking them up in a later file.

2. Fix the length-of case for properties (see #3751-221).

3. Fix the access mode for properties (see #3751-221).

Then I can pick up something else.

We should discuss the plan for non-local buffers, queries and temp-tables.

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

((F3757)instance.ref()).v3.assign(new integer(456));

I assume F3757 is the TestClass name, right? (i.e. you explicitly cast to the expected type). If so, go ahead with this, but only if the accessed member is private

Fixed in 11365.

I will finish Instance:v = 0 issue tomorrow. The type is decided too early and need to be rewritten later.

Revision 11366:

Fix for initializers created and stored in the depot on one program which are then pulled out of the depot and attached in another program. That should never occur. Safety code was added to bypass the separate initializer logic in variable definitions when in an interface or an abstract method/event/property.

Constantin: Could you please review this change? The HERE? comments are the places that I changed to add extra bypassing code. My tests show it to be OK, but if you can see something I'd like to know now.

Greg Shah wrote:

Constantin: Could you please review this change? The HERE? comments are the places that I changed to add extra bypassing code. My tests show it to be OK, but if you can see something I'd like to know now.

Some of it is a little overkill (like prog.define_parameter which is used only for internal procedures, and INIT which AFAIK can't be used in abstract and iface cases), but it doesn't hurt; the other changes look OK.

The code for extent property getters is failing with:

   [javac] ...snip...src/com/goldencode/testcases/oo/PropertyHolderExtents.java:53: error: incompatible types: no instance(s) of type variable(s) T exist so that T[] conforms to logical
    [javac]       return extentFunction("flag", logical.class, 2, new Block((Body) () -> 
    [javac]                            ^
    [javac]   where T is a type-variable:
    [javac]     T extends BaseDataType declared in method <T>extentFunction(String,Class<T>,int,Block)

The converted code:

   public logical getFlag(final int64 _idx)
   {
      int64 idx = TypeFactory.initInput(_idx);

      return extentFunction("flag", logical.class, 2, new Block((Body) () -> 
      {
         returnNormal(subscript(flag, idx));
      }));
   }

I don't see the issue here. Do you see something obvious?

Greg Shah wrote:

The code for extent property getters is failing with:

[...]

The converted code:

[...]

I don't see the issue here. Do you see something obvious?

Isn't extentFunction returning an array? And this is a indexed getter, so you don't need extentFunction.

You're right, that is the issue. Thanks!

Revision 11368 includes fixes for:

• length-of contents in extent properties
• indexed getter return type

There is still a compile error in PropertyHolderExtents which I'm looking at now. It has to do with missing wrapping on the setter's first parm.

I also haven't resolved the access mode for the property member.

There are some issues with incorrect tempidx for non-local var/class-event access, if the definition is in a super-class. Working on it.

1. if a class/object qualifier is NOT provided, look into local definitions (i.e. scoped to methods)
2. if not found and we are in a class, look into the class members
3. if not found, look into local definitions

Constantin Asofiei wrote:

3750a rev 11369 fixes some var/class-event reference problems when the definition is in a super-class. Please review - especially the order of the member lookup, which has these rules:

1. if a class/object qualifier is NOT provided, look into local definitions (i.e. scoped to methods)
2. if not found and we are in a class, look into the class members
3. if not found, look into local definitions

How is 3 different from 1?

Greg, if you manage to decouple the dependency on "filenames_to_java_classnames" (which prohibits CB to be ran on a sub-set of files), that would be great. The major issue is CLASS converted name (required by KW_USING and CLASS_NAME tokens). The idea is, to emit 'refid' for everything which requires a name conversion (and is NOT a builtin class).

Greg Shah wrote:

Constantin Asofiei wrote:

3750a rev 11369 fixes some var/class-event reference problems when the definition is in a super-class. Please review - especially the order of the member lookup, which has these rules:

1. if a class/object qualifier is NOT provided, look into local definitions (i.e. scoped to methods)
2. if not found and we are in a class, look into the class members
3. if not found, look into local definitions

How is 3 different from 1?

• if qualified, look in the specified type
• if not qualified, look into method-level definitions first
• otherwise, look into the class (and super-class) definitions

Looking at how SymbolResolver.lookupWrapped works, I don't think the annotateVariableOptions are fully correct, as lookupWrapped looks in the class scope, too. I'll take a better look tomorrow.

Ovidiu, these legacy classes are used in our project and we need Java equivalents:

./abl/skeleton/oo4gl/Progress/Json/JsonParser.cls
./abl/skeleton/oo4gl/Progress/Json/ObjectModel/JsonArray.cls
./abl/skeleton/oo4gl/Progress/Json/ObjectModel/JsonConstruct.cls
./abl/skeleton/oo4gl/Progress/Json/ObjectModel/JsonDataType.cls
./abl/skeleton/oo4gl/Progress/Json/ObjectModel/JsonObject.cls
./abl/skeleton/oo4gl/Progress/Json/ObjectModel/ObjectModelParser.cls
./abl/skeleton/oo4gl/Progress/Lang/AppError.cls
./abl/skeleton/oo4gl/Progress/Lang/Class.cls
./abl/skeleton/oo4gl/Progress/Lang/Error.cls
./abl/skeleton/oo4gl/Progress/Lang/Object.cls
./abl/skeleton/oo4gl/Progress/Lang/ParameterList.cls
./abl/skeleton/oo4gl/Progress/Lang/ProError.cls

See bellow the list (take from the annotation phase of the CB conversion), on what names the classes, methods, attributes will need to have. If there is a class event or property, these need to follow our conversion model for the access to them. You can try converting these files directly in FWD, and just modify the generated Java sources to include them in the proper FWD package.

Used builtin classes...Used builtin classes...

Responding here instead of #3750 because it is OO related.

if we are in an ASSIGN statement and a property with a custom setter is invoked, how will that address any tx/undo management?

The getters/setters (and the extent length-of/resize methods) are emitted as internal procs (if void return, like setter or resize) or a function (if non-void like getter or length-of). These emit with full block manager/transaction properties so I think we are good to go. I plan to test these but I think this is already very close to correct.

Constantin Asofiei wrote:

Ovidiu, these legacy classes are used in our project and we need Java equivalents:

OK, I'll do it in about 1h, after committing my current work

GES: Solved

Right now I'm still working on the wrapping of method call parms. I can put that on hold to work on other issues, but even my simple testcases show this issue so I think we will hit it in the customer code.

The basic problem is the same as the "chp_wrapper" issue we see with functions or other signature processing. The 4GL allows the following cases to silently convert when there is a mismatch in the signature versus the caller. From convert/user_functions.rules:

            <!-- optionally wrap this non-dynamic func call in another type -->
            <rule on="false">isNote("chp_wrapper")
               <action>chp_wrapper = getNoteString("chp_wrapper")</action>

               <rule>(chp_wrapper == "longchar"   and type == prog.func_char)     or
                     (chp_wrapper == "character"  and type == prog.func_longchar) or

                     (chp_wrapper == "integer"    and type == prog.func_int64)    or
                     (chp_wrapper == "decimal"    and type == prog.func_int)      or
                     (chp_wrapper == "decimal"    and type == prog.func_int64)    or

                     (chp_wrapper == "datetime"   and type == prog.func_date)     or
                     (chp_wrapper == "datetimetz" and type == prog.func_date)     or
                     (chp_wrapper == "datetimetz" and type == prog.func_datetime)
                  <action>
                     lastid = createPeerAst(java.constructor, chp_wrapper, closestPeerId)
                  </action>
               </rule>
            </rule>

We need something like this for method calls. I know it affects the property getters/setters/extent methods. It surely would also affect the event methods.

The big problem here is that we must resolve which of the overloaded methods is the match, in order to determine the target parameter type from the signature. At least they don't support varargs. :)

Unless you see something more urgent, I was going to fix this.

Constantin Asofiei wrote:

Greg, if you manage to decouple the dependency on "filenames_to_java_classnames" (which prohibits CB to be ran on a sub-set of files), that would be great. The major issue is CLASS converted name (required by KW_USING and CLASS_NAME tokens). The idea is, to emit 'refid' for everything which requires a name conversion (and is NOT a builtin class).

I guess this is not needed for today since conversion is completing successfully. We can fix this in the next few weeks. I do agree it is something that must be fixed.

Revision 11371:

Fixes to have access mode and static qualifier emit properly for properties. The backing var for the property is now always forced to be private.

Revision 11376:

Fix for OBJECT_INVOCATION that has a first child of OO_METH_*. This is an unqualified method reference before the first chaining. This affected the case of a local property as a referent because we convert that to an OO_METH_CLASS reference which did not emit. But this was actually a much broader problem.

This resolves #3750-99 and probably other compile failures. At this point I don't know of any other property related issues.

Revision 11380 has this change in oo_calls.rules:

            <action>lastid = createJavaAst(java.lparens, "(", lastid)</action>
            <action>
               lastid = createJavaAst(java.cast, 
                                      #(java.lang.String) 
                                      ref1.getAnnotation("full-java-class"), 
                                      lastid)
            </action>

It parents the ref() unwrapper in a cast. I don't understand why it is needed. The ref method is designed to return the correct type.

It definitely breaks some of my testcases. There are scenarios where the content of the cast is null and there are unbalanced parens. Both issues can surely be addressed, but if we don't need this I'd like to get rid of it. It makes every call in the chain extremely verbose.

Greg Shah wrote:

Revision 11380 has this change in oo_calls.rules:
[...]
It definitely breaks some of my testcases. There are scenarios where the content of the cast is null and there are unbalanced parens. Both issues can surely be addressed, but if we don't need this I'd like to get rid of it. It makes every call in the chain extremely verbose.

It addresses the issue from note-231/232.
ABL code is:

    define public static property Instance as class MyTest no-undo
         get ():
            Instance:v3 = 456.
            return Instance.
         end get.
         private set.

Ovidiu Maxiniuc wrote:

Greg Shah wrote:

Revision 11380 has this change in oo_calls.rules:
[...]
It definitely breaks some of my testcases. There are scenarios where the content of the cast is null and there are unbalanced parens. Both issues can surely be addressed, but if we don't need this I'd like to get rid of it. It makes every call in the chain extremely verbose.

It addresses the issue from note-231/232.
ABL code is:
[...]

The issue is about the private members only. Because the returned type is ? extends TestClass, the compiler must disallow direct access to private members in the result. However, in this case we know that the class is actually the same class and so it is safe to override the compiler's calculation.

Revision 11383:

Only cast ref() calls when the referenced member is private (which is the only case where it is needed). This makes the majority of calls much less verbose.

Constantin suggested this approach in #3751-232.

FYI, this also eliminates the cases where that extra casting broke the converted code so it is good from that perspective too.

Revision 11386 completes the stubs of all necessary built-in classes. I believe this resolves all requires listed in #3751-250.

We have a problem with defining interface-typed variables. For example:

DEFINE VARIABLE ufo9 AS Progress.Lang.Error NO-UNDO.

will be converted to

object<? extends com.goldencode.p2j.oo.lang.LegacyError> ufo9 = TypeFactory.object(com.goldencode.p2j.oo.lang.LegacyError.class);

The problem is, LegacyError is now an interface, unrelated to BaseObject that the object<> wraps. LegacyError maps the Progress.Lang.Error that which is an interface and it seems logical to use interface for LegacyError as well.
To fix this, I am thinking of using abstract class -es that extend BaseObject in Java for ABL interfaces. Do you think it would work?

GES: SOLVED

I'm getting lots of null casts for chains like x:pv2:pv2:pv1., where x is a OO local var (not a class member) and pv2 and pv1 are public members declared in x's type.

This is because var references are annotated with access-mode by the parser, even if the var is defined in a method.

Greg, can you take a look?

Greg, can you take a look?

Yes, I'll look at it.

More, because of the access-mode annotation, the cast is emitted for the public chained members...

Revision 11390 resolves access mode processing for deeply chained/nested member de-references and remove vars defined in methods from being data members. This solved my local recreate for #3751-263.

Revision 11391 fixes multi-assignments for extent object variables and adds support for initialization of indeterminate extent object variables.

Ovidiu Maxiniuc wrote:

We have a problem with defining interface-typed variables. For example:
[...]

will be converted to
[...]

The problem is, LegacyError is now an interface, unrelated to BaseObject that the object<> wraps. LegacyError maps the Progress.Lang.Error that which is an interface and it seems logical to use interface for LegacyError as well.
To fix this, I am thinking of using abstract class -es that extend BaseObject in Java for ABL interfaces. Do you think it would work?

We cannot do this because of the lack of multiple-inheritance.

4GL and Java classes can both implement more than one interface, but only ever one super/parent class. Interfaces are not part of the inheritance hierarchy, so they can be used to mark parts of the inheritance tree as defining a known API/contract. But since they are separate, they don't change the lookup of methods and so forth.

The use of an abstract class as an interface would break this.

However, we can "reverse" the idea. Let's put all of the BaseObject methods (and properties which are methods in our Java implementation) into an interface named _BaseObject_. BaseObject will implement that interface and all generated interfaces will inherit from it. Then we change object to use object<? extends _BaseObject_> and I think it will work.

OM SOLVED
Ovidiu, please work on this one: overloaded methods in 4GL classes must keep their names.

method public void m1(input i as int):
end.
method public void m1(input i as char):
end.

Both must be named m1 in FWD.

OK, mark it as OM WIP

GES: SOLVED

Ovidiu Maxiniuc wrote:

OK, mark it as OM WIP

There is another issue here: the refid annotation (which gets computed from tempidx) must refer the correct method definition. I think the change is not only the method naming, but at the parser, too: the erasure of a method definition is not only the name, but the name, signature pair - and emit tempidx annotations accordingly. Otherwise, you will not be able to link to the proper method, to get (for example) the type of a certain parameter, to wrap it as necessary (if POLY or literal).

There is at least another case of staticField:privateMethodCall("param") which gets converted to staticField.ref().privateMethodCall("param"), without the required casting (to be able to compile it in Java).

GES: SOLVED

New issue: FWD can't compile a OO var declaration having as a type an interface.

CA WIP
New issue: FWD doesn't emit properly static temp tables (and maybe access modifiers).

STILL OPEN
Also, usage of this tt from multiple static methods doesn't have the proper scopes.

Constantin Asofiei wrote:

New issue: FWD can't compile a OO var declaration having as a type an interface.

A discussion on this started in notes 262 & 269 above.

Ovidiu Maxiniuc wrote:

Constantin Asofiei wrote:

New issue: FWD can't compile a OO var declaration having as a type an interface.

A discussion on this started in notes 262 & 269 above.

We can't break the interface/abstract class contract in the converted code.

I think we need to emit the legacy Progress.Lang.Object APIs as an interface (with their implementation as default interface methods), and make this the 'super-object' for everything. If someone wants to instantiate a Progress.Lang.Object, or if a class inherits it, we will hook to the runtime and conversion rules to use the FWD's implementing class.

Do you see a problem with this?

I think I described something similar in #3751-269.

• _BaseObject_ interface
• BaseObject implements _BaseObject_
• all built-in and converted interfaces extend _BaseObject_
• the wrapper is changed to object<? extends _BaseObject_>

With these changes, I believe the issue is resolved and the impact is minimal.

I think we need to emit the legacy Progress.Lang.Object APIs as an interface (with their implementation as default interface methods), and make this the 'super-object' for everything. If someone wants to instantiate a Progress.Lang.Object, or if a class nherits it, we will hook to the runtime and conversion rules to use the FWD's implementing class.

This seems more complicated. Is there an issue with my approach?

Ovidiu: In regard to #3751-270, what is your status? I was going to work on this a few days ago but shifted to work on known issues. I've already thought about this a good bit.

I'm inclined to take this over if you aren't close to a solution. However, I don't want to waste any work you've done.

Greg Shah wrote:

I think we need to emit the legacy Progress.Lang.Object APIs as an interface (with their implementation as default interface methods), and make this the 'super-object' for everything. If someone wants to instantiate a Progress.Lang.Object, or if a class inherits it, we will hook to the runtime and conversion rules to use the FWD's implementing class.

This seems more complicated. Is there an issue with my approach?

Think about this: you will not be able to interchange a legacy interface type with a Progress.Lang.Object in the converted runtime, if we have declarations like:

def var l as progress.lang.error.
def var o as progress.lang.object.

o = l.

If the converted runtime uses BaseObject (Java class) for o and LegacyError (Java interface) for l, then you will not be able to l to o, because 'LegacyError is not a BaseObject'. For this to work, o must have the same type or a super-type of l.

Greg Shah wrote:

Ovidiu: In regard to #3751-270, what is your status? I was going to work on this a few days ago but shifted to work on known issues. I've already thought about this a good bit.
I'm inclined to take this over if you aren't close to a solution. However, I don't want to waste any work you've done.

I am testing the implementation now, in about 30min, depending on the result, I will do the commit.

Constantin Asofiei wrote:

Greg Shah wrote:

I think we need to emit the legacy Progress.Lang.Object APIs as an interface (with their implementation as default interface methods), and make this the 'super-object' for everything. If someone wants to instantiate a Progress.Lang.Object, or if a class inherits it, we will hook to the runtime and conversion rules to use the FWD's implementing class.

This seems more complicated. Is there an issue with my approach?

Think about this: you will not be able to interchange a legacy interface type with a Progress.Lang.Object in the converted runtime, if we have declarations like:
[...]

If the converted runtime uses BaseObject (Java class) for o and LegacyError (Java interface) for l, then you will not be able to l to o, because 'LegacyError is not a BaseObject'. For this to work, o must have the same type or a super-type of l.

Variables of type Progress.Lang.Object would be declared using _BaseObject_.

Constantin Asofiei wrote:

New issue: FWD doesn't emit properly static temp tables (and maybe access modifiers).

Also, usage of this tt from multiple static methods doesn't have the proper scopes.

The temp-table itself is not static or protected... it is just a definition of the table structure which we move out to a DMO.

I suspect what the static or access modes control is access to the default buffer created for that temp-table. This is really the thing that gets used at runtime for access.

Eric: What do you think?

Greg Shah wrote:

I suspect what the static or access modes control is access to the default buffer created for that temp-table. This is really the thing that gets used at runtime for access.

Yes, I meant the default buffer...

Greg, did you add the inMethodDef flag to progress.g recently? Because I previously added inMethod flag... with the same purpose, and inMethodDef I think has something wrong (you have inMethodDef = true; at the end of getter_setter), as it ends up true when trying to add a var definition as class member.

Yes, I added it in 11390. The getter_setter issue is just a copy/paste error. At the end it should be set to false.

I didn't see the inMethod since it was in the caller instead of the pre/post actions of the rule it affected. I also had to pass the flag downstream to handle things differently in the SymbolResolver and Variable. I think we can remove inMethod. Also, the use of inMethod bypasses annotations being set for a define variable inside a method, which I think is incorrect. I just bypassed the addDataMember() in postDefineVar() which I think is more correct.

Constantin Asofiei wrote:

New issue: FWD can't compile a OO var declaration having as a type an interface.

If there are no other objections to my plan, I can go ahead with these changes.

Greg Shah wrote:

Constantin Asofiei wrote:

New issue: FWD can't compile a OO var declaration having as a type an interface.

If there are no other objections to my plan, I can go ahead with these changes.

Yes, go ahead, the plan looks OK.

Greg Shah wrote:

I didn't see the inMethod since it was in the caller instead of the pre/post actions of the rule it affected. I also had to pass the flag downstream to handle things differently in the SymbolResolver and Variable. I think we can remove inMethod. Also, the use of inMethod bypasses annotations being set for a define variable inside a method, which I think is incorrect. I just bypassed the addDataMember() in postDefineVar() which I think is more correct.

Can you take care of cleaning this up?

Constantin Asofiei wrote:

Greg Shah wrote:

I didn't see the inMethod since it was in the caller instead of the pre/post actions of the rule it affected. I also had to pass the flag downstream to handle things differently in the SymbolResolver and Variable. I think we can remove inMethod. Also, the use of inMethod bypasses annotations being set for a define variable inside a method, which I think is incorrect. I just bypassed the addDataMember() in postDefineVar() which I think is more correct.

Can you take care of cleaning this up?

Yes, it is already done in my version of the parser. :)

Finished testing 3751-270 (Added method overloading support).
I tested with variables, properties and method.
There is a problem I could spot: having both a property m1 and a setter named setM1(), with no parameters. If it has at least a parameter we are good. When method is defined without parameters, because of the name clash, the method is named in java to something like setM1_1(). I have no solution for this (indeed, rare) case.
Committed as r11408. Please review.

Code Review Task Branch 3750a Revision 11408

There are 2 things I don't understand:

1. In annotations_prep.xml, the two calls to convert_member_name pass mnode which is an XmlAst node. The code in the constructor for ConvertedClassName.Method will only parse out the signature on a ProgressAst that is a METHOD_DEF. I don't understand the purpose of passing mnode.

2. In convert_member_name there is javaname = cclass.getMethod(mname, mAst). cclass is a ConvertedClassName instance and that class has this version: public String getMethod(String legacy). Perhaps this is just a bug in TRPL that allows us to get away with this (extra parm that isn't used)?

Revision 11411:

Rework of converted interfaces and converted classes to ultimately inherit both from BaseObject which BaseObject implements. This enables interface instances to be defined as variables and assigned from actual class instances, since all now can find a common ancestor. Runtime APIs must reference BaseObject instead of BaseObject, all interfaces (built-in or converted) that don't already have a superinterface must extend BaseObject.

This solves #3751-274.

There is another issue here: the refid annotation (which gets computed from tempidx) must refer the correct method definition. I think the change is not only the method naming, but at the parser, too: the erasure of a method definition is not only the name, but the name, signature pair - and emit tempidx annotations accordingly. Otherwise, you will not be able to link to the proper method, to get (for example) the type of a certain parameter, to wrap it as necessary (if POLY or literal).

Ovidiu: Are you working on this issue (and the wrapping)?

This also relates to the issue I mentioned earlier in #3751-253: the need to calculate the chp_wrapper in some cases.

I can take these items if you aren't working it.

Greg, I think you forgot to commit _BaseObject_

Greg, I think you forgot to commit _BaseObject_

Sorry. Rev 11412 fixes that.

GES: SOLVED

Another OO issue: you have an integer property and do something like pi = pi + 1. This gets converted to setPi(plus(getPi(), 1));, and the compile fails because MathOps.plus is returning int64 (and setPi is integer).

I think the property getter/setters need to have as signature the wider sub-type.

GES: SOLVED

There are still issue with wrapping the parameters for method calls. Also, this wrapping must not widen the type (i.e. use longchar instead of character).

Constantin Asofiei wrote:

Another OO issue: you have an integer property and do something like pi = pi + 1. This gets converted to setPi(plus(getPi(), 1));, and the compile fails because MathOps.plus is returning int64 (and setPi is integer).

I think the property getter/setters need to have as signature the wider sub-type.

This is the chp_wrapper issue I have mentioned in #3751-253. I am working on this.

Greg Shah wrote:

Constantin Asofiei wrote:

Another OO issue: you have an integer property and do something like pi = pi + 1. This gets converted to setPi(plus(getPi(), 1));, and the compile fails because MathOps.plus is returning int64 (and setPi is integer).

I think the property getter/setters need to have as signature the wider sub-type.

This is the chp_wrapper issue I have mentioned in #3751-253. I am working on this.

If it helps, see oo_references.rules line 130 - there I'm trying to emit proper annotation, so the further rules will wrap the parameter.

Greg, static state in 4GL is not the same as static state in Java. This is because in 4GL there are no multiple clients being executed in the same context, where in Java there is.

Everything which is converted as static must be context-local.

More, promoting local vars in static methods should never happen (or at least this must keep the static modifier for the promoted var).

Yes, you're right.

However, we need to wait to implement this until after we have everything compiling in the current POC.

More, promoting local vars in static methods should never happen (or at least this must keep the static modifier for the promoted var).

This is a compile-time problem now, for us. I'm looking into this and a problem with leaking the signature of the define event (the emitted APIs get the wrong signature).

Greg, there are errors like this:

    [javac] ...1046: error: no suitable method found for add(character,BaseDataType)
    [javac]                               json.ref().add(hField.unwrap().name(), hField.unwrapBufferField().value());
    [javac]                                         ^
    [javac]     method JsonObject.add(character,character) is not applicable

Greg Shah wrote:

1. In annotations_prep.xml, the two calls to convert_member_name pass mnode which is an XmlAst node. The code in the constructor for ConvertedClassName.Method will only parse out the signature on a ProgressAst that is a METHOD_DEF. I don't understand the purpose of passing mnode.

This is correct. Fixed by passing null instead.

2. In convert_member_name there is javaname = cclass.getMethod(mname, mAst). cclass is a ConvertedClassName instance and that class has this version: public String getMethod(String legacy). Perhaps this is just a bug in TRPL that allows us to get away with this (extra parm that isn't used)?

TRPL is fine. The problem is that that particular line was never executed (as mtype could not be names.variable and names.method simultaneously). The second rule should have been executed only when the first fails (on="false" was missing).

Committed as r11419.

Greg, please review this function:

      <function name="isTopLevel">
         <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.type == prog.block

            <!-- external procedure, not OO -->
            <rule>target.parent == null

               <rule>!target.descendant(prog.class_def, 1) and   // this needs to be checked under the target's context, and not the current AST..
                     !target.descendant(prog.interface_def, 1)
                  <action>top = true</action>
               </rule>

               <!-- check for internal procedures, user-defined functions
                    or triggers -->
               <action on="false">ttype = target.parent.type</action>
               <rule on="false">
                  ttype == prog.procedure                        or
                  ttype == prog.function                         or
                  ttype == prog.trigger_block                    or
                  ttype == prog.method_def                       or
                  ttype == prog.constructor                      or
                  ttype == prog.destructor                       or
                  target.relativePath("DEFINE_PROPERTY/KW_GET/BLOCK")  or
                  target.relativePath("DEFINE_PROPERTY/KW_SET/BLOCK")            // this needs to end in BLOCK, right? (as target is BLOCK here)
                  <action>top = true</action>
               </rule>
            </rule>
         </rule>
      </function>

While I am writing the method matching/lookup logic I needed to check if the 4GL uses return type as part of a method signature. It does NOT. This is good since Java does not consider return type part of the signature either (for purposes of matching a call to the definition).

4GL example:

class oo.PolymorphicMethodReturnType:

   method public int test():
      return 29.
   end.

   /* compile error: */
   /* Duplicate signatures found for method 'test'. (13842) */
   /*
   method public char test():
      return "29".
   end.
   */   

   /* can't have an empty parm list but it can change the return type */
   method public char test(input num as int):
      return string(num).
   end.

end.

def var obj as oo.PolymorphicMethodReturnType.
def var num as int init -1.
def var txt as char init ?.

obj = new oo.PolymorphicMethodReturnType().

/* polymorphic method return types are supported */
num = obj:test().
txt = obj:test(64).

/* num + 2 = 31; txt = 64 */
message "num + 2 = " + string(num + 2) + "; txt = " + txt.

Java equivalent:

public class PolyReturnTypeInJava
{
   public int test()
   {
      return 29;
   }

   /*
   PolyReturnTypeInJava.java:10: error: method test() is already defined in class PolyReturnTypeInJava
   public String test()
   {
      return "29";
   }
   */

   /* can't have an empty parm list but it can change the return type */
   public String test(int num)
   {
      return String.format("%d", num);
   }

   public static void main(String[] args)
   {
      PolyReturnTypeInJava obj = new PolyReturnTypeInJava();

      int    num = -1;
      String txt = null;

      /* polymorphic method return types are supported */
      num = obj.test();
      txt = obj.test(64);

      /* num + 2 = 31; txt = 64 */
      System.out.printf("num + 2 = %d; txt = %s\n", num + 2, txt);      
   }
}

This makes things simpler. Return type is not used in either case and the programmer will have had to already separate all usage of that method name by differing parameter signatures. Our parse-time lookup only needs to consider method name and parms.

this needs to be checked under the target's context, and not the current AST..

You're right. However, I think the parameters are swapped in the Aast version.

this needs to end in BLOCK, right? (as target is BLOCK here)

Yes, though we could use upPath() instead of relativePath().

this needs to be checked under the target's context, and not the current AST..

You're right. However, I think the parameters are swapped in the Aast version.

The reason the original version worked is that we know that we are a block node with parent == null, which can only happen at the tree root and so the implicit check would always work. Your version is better.

I've been working on testcases for hours. I'm finding that there are fewer wrapping cases than we thought.

For example, you mentioned that we need decimal and integer and integer to decimal.

However, for a method method public int wrap-int(input num as int) and a decimal var named decnum, trying to call wrap-int(decnum). will generate Parameter 1 for METHOD wrap-int is not type compatible with its definition. (12905).

This also occurs for int64 to int and dec to int64. I have an example case of an integer property that can be assigned from int64. But method parameters do not seem to narrow, they only wrap for widening conversions.

I'm still working on wrapping the other data types.

Constantin: can you provide specific examples of these cases failing in method parameter wrapping?

Greg Shah wrote:

Constantin: can you provide specific examples of these cases failing in method parameter wrapping?

Try arithmetic expressions, not just passing vars. In FWD, our MathOps.plus(int64,in64) returns an int64 - passing this result to a method having an integer argument will not work.

Try passing a string literal to a longchar method parameter - we currently wrap this using character, and this is not working. 4GL also allows you to pass a character to a longchar - which FWD doesn't allow without wrapping.

Try arithmetic expressions, not just passing vars. In FWD, our MathOps.plus(int64,in64) returns an int64 - passing this result to a method having an integer argument will not work.

I just tried this. The 4GL does not allow this narrowing conversion for methods. Could that case have been for a property?

Try passing a string literal to a longchar method parameter - we currently wrap this using character, and this is not working. 4GL also allows you to pass a character to a longchar - which FWD doesn't allow without wrapping.

Yes, this is an implicit widening conversion that does work in the 4GL.

I have now tested all variants of longchar/char and date/datetime/datetime-tz. NO narrowing conversions work. All of the widening conversions DO work.

We will handle properties differently. But right now I'm trying to get the method signature resolution worked out. Unless you know of a narrowing case for method signatures, I think I've got the rules for wrapping.

I just need to add unknown value and POLY cases now.

Revision 11425: Rewrite of method resolution and related annotations to properly handle overloading and many of the parameter wrapping cases.

This is an intermediate version, but the parsing changes are nearly complete. The signature processing does not handle extents and it does not handle table parameters/buffers. Everything else is working well and a wide range of cases are fixed.

There are also many tweaks needed to the conversion process to handle various cases like unknown value and some POLY cases.

I should also mention that the ECW now returns integer as a classname for MINUS, PLUS and MULTIPLY when both operands are integer. This is how the 4GL does it. Unfortunately, javac will not find the right MathOps worker methods in this case so we need to wrap these when we know both operands are integer.

Anyway, I wanted to save my current work even though it is not done.

Constantin: it may be worth looking through the changes, though I expect to keep going with this in the morning. I have no pending changes, so feel free to tweak something or build on this to the degree that it makes sense.

The following is the list of issues that still need to be resolved:

• Add support for table and buffer parameters to the parse-time overloaded method resolution mechanism.
  • Make sure ECW handles these cases.
  • Add this knowledge to SymbolResolver.calculate*Signature() and SymbolResolver.translateType() methods.
  • Make sure that the ClassDefinition.[exact|fuzzy]MethodLookup() methods handle these cases properly.
• Add EXTENT parameter support.
• Rework ConvertedClassName to eliminate usage of fname2jname. The refid support is working now (except for tables/buffers and extent). Shift usage to a direct access of the method def reference using getAst(refid ) and read the javaname from that node.
• Make sure properties wrap when mismatched.
• Disambiguate Java method names when overloaded method signatures only differ by fixed/indeterminate array differences.
• Fix unknown value passed as an object reference. oo/Overloads.cls line 268/269 can be used to test this case.
• BUFFER-VALUE and :: work (though they "over-wrap"), but the wrapping of the DYNAMIC-FUNCTION() POLY case does not work. See oo/overloads-test.p lines 162 through 173.
• When both operands for MathOps.minus(), MathOps.plus() and MathOps.multiply() are integer, then we need to wrap those parameters so that the correct MathOps overloaded method is chosen. Otherwise the NumberType, NumberType signature will be picked by javac and the result will be int64 which will cause a problem in compiling converted method calls. The ECS now reports this case as integer so the conversion does not detect that wrapping is needed. And it really should not be needed as long as we match the 4GL behavior.
• Reduce the extra layer of wrapping for unknown value and for the widening conversions when passed for primitive BDT types. For example, an unknown decimal parm will look like new decimal(new decimal()) and a longchar var named txt would be new longchar(new longchar(txt)). We deliberately "over-annotate" the type information in ClassDefinition.annotateCallSignature() which may be interpreted as the need for multiple wrappers downstream. This overwrapping also occurs for the BUFFER-VALUE POLY case. Strangely, this doesn't affect the :: case. Use oo/Overloads.cls lines 185 through 198m 210 through 221, 227 through 241 to see these cases.
• Check EXTENT return value support. Fix if needed.
• Change the "access" annotation to "access-mode" for methods. Changes will be needed in ClassDefinition.addMethod(), ClassDefinition.annotateMethodCall() and any TRPL usage locations.

Constantin: Can you help me with the above items? I've split up the first of the issues based on who might handle them faster.

I want to clear all of these today (as early as possible) and get a full run of conversion with the customer application.

Greg Shah wrote:

Constantin: Can you help me with the above items? I've split up the first of the issues based on who might handle them faster.

Yes, I'm looking into them.

I've just committed some OO testcase changes in testcases/uast/oo/ as rev 1808.

Assuming there are definitions like:

   method public void mtable(input-output table tt1).
   end.

   method public void mtable(input-output table tt2).
   end.

o:mtable(input-output table-handle h).

the 4GL runtime produces a Ambiguous runtime method call. Could not resolve mtable reference. (13844) error - as it can't distinguish between the tt1 and tt2 temp-table signatures, when passing a table-handle argument.

For now, I'll produce a 4GL parser warning.

Greg Shah wrote:

• GES Rework ConvertedClassName to eliminate usage of fname2jname. The refid support is working now (except for tables/buffers and extent). Shift usage to a direct access of the method def reference using getAst(refid ) and read the javaname from that node.

fname2jname is needed for the builtin classes, as we don't have ASTs for these.

Good point. We'll have to have a dual approach.

Greg Shah wrote:

• Reduce the extra layer of wrapping for unknown value and for the widening conversions when passed for primitive BDT types. For example, an unknown decimal parm will look like new decimal(new decimal()) and a longchar var named txt would be new longchar(new longchar(txt)). We deliberately "over-annotate" the type information in ClassDefinition.annotateCallSignature() which may be interpreted as the need for multiple wrappers downstream. This overwrapping also occurs for the BUFFER-VALUE POLY case. Strangely, this doesn't affect the :: case. Use oo/Overloads.cls lines 185 through 198m 210 through 221, 227 through 241 to see these cases.

This is mostly because of this code in ClassDefinition.annotateCallSignature:

                  parm.putAnnotation("wrap", new Boolean(true));
                  parm.putAnnotation("wrap_parameter", new Boolean(true));
                  parm.putAnnotation("classname", classname);
                  parm.putAnnotation("chp_wrapper", classname);

I'm cleaning up the fixes for the three CA issues in #3751-315.

There is a case where we have a call like dateMethod(h:buffer-value), and dateMethod has 3 signatures, like:

dateMethod(input d as date).
dateMethod(input d as datetime).
dateMethod(input d as datetime-tz).

Another case where trim function is passed as an argument to a method call - I'm looking into this.

And another case where an unqualified static method call is performed from a static property getter (both private) - I'm looking into this.

3750a rev 11427 and testcases project rev 1811 has the changes for the BUFFER/TABLE method params and arguments, unknown value and dynamic-function from #3751-315

You've changed progress.g checks like:

(inStaticCtxt  && (sym.lookupObjectMethod(null, nextTxt, true) != -1))  ||
(!inStaticCtxt && (sym.lookupObjectMethod(null, nextTxt) != -1))

sym.isObjectMethod(null, nextTxt, inStaticCtxt)

This is not correct, as it prohibits calling unqualified static methods from instance methods - I'll change it to work as before (using isObjectMethod).

All #3750-322 problems were exposed by a (now fixed - see 3750a rev 11428) abend with the root cause in ClassDefinition.guessMethod (root cause for #3751-324), where a static method lookup must be done, if an instance method can't be found (and we are in an instance context).

But we still need to properly determine the method call target, when the POLY signature matches multiple targets. In this case, unless we change the conversion to use dynamic invocation and not Java calls, we need to patch the 4GL source code to disambiguate the signature.

But I wonder if we shouldn't emulate a DYNAMIC-INVOKE for method calls with POLY arguments, for which more than one target can be resolved.

But we still need to properly determine the method call target, when the POLY signature matches multiple targets. In this case, unless we change the conversion to use dynamic invocation and not Java calls, we need to patch the 4GL source code to disambiguate the signature.

For the current deadline, I'm OK with patches.

But I wonder if we shouldn't emulate a DYNAMIC-INVOKE for method calls with POLY arguments, for which more than one target can be resolved.

Yes, this is probably correct. It is MUCH better than always using dynamic invocation.

Greg Shah wrote:

But we still need to properly determine the method call target, when the POLY signature matches multiple targets. In this case, unless we change the conversion to use dynamic invocation and not Java calls, we need to patch the 4GL source code to disambiguate the signature.

For the current deadline, I'm OK with patches.

I'll add some logging to identify these cases, and depending on the number of cases, I'll decide if is faster to patch or to emit DYNAMIC-INVOKE.

I'm close to having the EXTENT changes ready. One problem I found is the fact that the 4GL treats the following as 2 different signatures:

   method public int extent-sig(output num as int, input txt as char extent):
      return 27.
   end.

   method public int extent-sig(output num as int, input txt as char extent 5):
      return 28.
   end.

Java does not have a fixed size array specification for variable/parm types. One solution is to implement a kind of dispatcher method that calls the right "worker" depending on the runtime state of the array parm. Something like this:

   public int extentSig(integer num, character[] txt)
   {
      // examine txt and call the right worker
   }

   public int extentSigWorker1(integer num, character[] txt)
   {
      // converted code for 1st method (indeterminate extent in this case)
   }

   public int extentSigWorker2(integer num, character[] txt)
   {
      // converted code for 2nd method (fixed extent in this case)
   }

I don't plan to fix this today. Hopefully there is no actual case of this in the current customer code.

I see that for methods that have extent return types, if the extent is fixed, we are altering the method name to add the array size to the end.

Where do we make this alteration? It must be after we resolve the method name at the call sites so the name is mismatched.

Greg Shah wrote:

I see that for methods that have extent return types, if the extent is fixed, we are altering the method name to add the array size to the end.

Where do we make this alteration? It must be after we resolve the method name at the call sites so the name is mismatched.

That's not added to the name. If the definition is like method public static integer extent 5 mx(), then the 5 literal gets emitted. We need to drop this.

That makes sense, I've got it.

Revision 11429 provides OO method EXTENT parameter support and minor fixes for OO method EXTENT return types.

It does not resolve the duplicated signature issue in #3751-328, but it otherwise resolves both EXTENT issues from #3751-315.

Fix unknown value passed as an object reference. oo/Overloads.cls line 268/269 can be used to test this case.

I think this is still an issue. Are you working on it?

Reduce the extra layer of wrapping for unknown value

Your changes cleared up the over-wrapping for other cases, but not for unknown. We can live with it for tonight but if you have a fix that is good.

I'm looking at the property wrapping next.

I think we can leave the fname2jname usage as is for tonight. But we do need to address the MINUS/PLUS/MULTIPLY with 2 integer operands wrapping.

To summarize, these are the things needed for tonight:

• CA Fix unknown value passed as an object reference. oo/Overloads.cls line 268/269 can be used to test this case.
• GES Property wrapping.
• MINUS/PLUS/MULTIPLY with 2 integer operands wrapping.

Is there anything else absolutely needed for tonight?

Greg Shah wrote:

Is there anything else absolutely needed for tonight?

Yes, see #3750-220

Greg Shah wrote:

• CA Fix unknown value passed as an object reference. oo/Overloads.cls line 268/269 can be used to test this case.

You mean lines 309/310 in the current version? These lines:

      method-num = oo-usage(flag, ?).                                          check-result(100, 23,  method-num).
      method-num = oo-usage(?, flag).                                          check-result(101, 24,  method-num).

         methodNum.assign(ooUsage(flag, new object(new object())));
         checkResult(new integer(100), new integer(23), methodNum);
         methodNum.assign(ooUsage(new object(new object()), flag));
         checkResult(new integer(101), new integer(24), methodNum);

Constantin Asofiei wrote:

But we still need to properly determine the method call target, when the POLY signature matches multiple targets.

3750a rev 11430 adds logging to determine the OO method calls with POLY signatures targeting more than one method.

• INPUT longchar can receive both char and longchar.
• INPUT char can receive only char
• OUTPUT longchar can receive only longchar
• OUTPUT char can receive both char and longchar
• input-output longchar can receive only longchar
• input-output char can receive only char.

More, the OO method signature in 4GL is using the parameter mode, too, not just the parameter type.

Constantin Asofiei wrote:

Greg Shah wrote:

• CA Fix unknown value passed as an object reference. oo/Overloads.cls line 268/269 can be used to test this case.

You mean lines 309/310 in the current version? These lines:
[...]
get converted to:
[...]
Beside the double-wrapping, what else is wrong? The code compiles.

I don't get that result. I get this:

         methodNum.assign(ooUsage(flag, new object<? extends oo.ComplexImplements>(new object<? extends oo.ComplexImplements>())));
         checkResult(new integer(100), new integer(23), methodNum);
         methodNum.assign(ooUsage(new object<? extends oo.SimplestInterface>(new object<? extends oo.SimplestInterface>()), flag));
         checkResult(new integer(101), new integer(24), methodNum);

Do you have any changes that aren't checked in?

Constantin Asofiei wrote:

So there is another level of fuzzy matching the arguments. It depends on the direction of the data (sent or received); for example, having at the method definition:

• INPUT longchar can receive both char and longchar.
• INPUT char can receive only char
• OUTPUT longchar can receive only longchar
• OUTPUT char can receive both char and longchar
• input-output longchar can receive only longchar
• input-output char can receive only char.

More, the OO method signature in 4GL is using the parameter mode, too, not just the parameter type.

I can look at this. It is going to take some time for writing testcases. I don't think I can get it done tonight.

Greg Shah wrote:

Do you have any changes that aren't checked in?

Hm... something regressed my fix, I'll check it.

#3751-338 fix is fixed now in 3750a 11431.

• INPUT can receive only the same or a narrower type
• OUTPUT can receive only the same or a wider type
• INPUT-OUTPUT can receive only the same type

Revision 11432:

Wrap property setter parameters when there would be a mis-match.

When multiply, plus and minus have both operands as integer sub-expressions, they will be wrapped in integer so that the correct MathOps worker will be found. This is needed to ensure that the operators return integer in this specific case, which matches the 4GL behavior.

GES LE: I believe this table is incorrect based on my testing. The version that is correct can be found in #3751-492.

The widening/narrowing rules are these:

• CharacterVarRef in case of a longchar argument for an output char parameter
• Int64VarRef in case of a decimal argument for an output int64 parameter
• IntegerVarRef in case of a decimal or int64 argument for an output integer parameter

Another case we need to fix is the field reference for OUTPUT and INPUT-OUTPUT - currently we have only HandleFieldRef. We need *FieldRef variants for all BDT sub-classes.

Constantin Asofiei wrote:

Another case we need to fix is the field reference for OUTPUT and INPUT-OUTPUT - currently we have only HandleFieldRef. We need *FieldRef variants for all BDT sub-classes.

Actually, this is already handled for function calls, via OutputParameter.wrap - for both field and var references, in OUTPUT or INPUT-OUTPUT modes. Will use this for method calls, too.

The OUTPUT and INPUT-OUTPUT cases are fixed in 3750a rev 11433 . Tests are in rev 1812 . Please review.

• class event regression.
• INPUT/OUTPUT/INPUT-OUPTUT for OO argument case.
• OUTPUT/INPUT-OUTPUT for EXTENT cases.

• first phase is matching without checking the arg modes - if a single match is found, use that. I'm not sure here if the match must be exact (without narrowing/widening checks) and how POLY interferes.
• second phase uses the arg modes, on the set of matches from previous step.

Code Review Task Branch 3750a Revision 11433

I'm good with the changes. ParameterKey does need class javadoc, but otherwise it is all good.

I'm not sure here if the match must be exact (without narrowing/widening checks) and how POLY interferes.

My previous tests showed that exact matches using type were honored all the way up the class hierarchy before POLY and widening checks were considered ("fuzzy lookup"). POLY was treated as a wildcard match.

second phase uses the arg modes, on the set of matches from previous step.

Is there a 3rd phase here? In other words, if there is a unique type match in the fuzzy lookup will that ignore modes and select that match?

Right now our fuzzy matching is only designed to find the first match. We don't check if there is a list of possible matches.

Should I look at the signature erasure issue? As you've noted in the testcases, we now have that issue with any signatures that are only differentiated by mode? I've noted this same issue exists for signatures that only differ by the fixed/indeterminate extent spec in #3751-328.

We may be able to get away without this support if the current application doesn't need it. As a solution, we would need to add a scanning step to detect these cases and change the method names for the ones that are conflicting.

For example:

   method public int extent-sig(output num as int, input txt as char extent):
      return 27.
   end.

   method public int extent-sig(output num as int, input txt as char extent 5):
      return 28.
   end.

   method public int extent-sig(output num as int, input txt as char extent 3):
      return 29.
   end.

Instead of using the dispatcher approach of #3751-328, we could just change the names:

   public int extentSig(integer num, character[] txt)
   {
      // indeterminate case
   }

   public int extentSig5(integer num, character[] txt)
   {
      // fixed extent 5
   }

   public int extentSig3(integer num, character[] txt)
   {
      // fixed extent 3
   }

We "lose" overloading, but I don't think there is any loss of functionality. The only loss is of "intention" that the methods have the same purpose.

Do you think this needs to be handled today?

Other than the overloading signature erasure issue, is there anything else that needs attention today which you are not already working on?

Greg Shah wrote:

We "lose" overloading, but I don't think there is any loss of functionality. The only loss is of "intention" that the methods have the same purpose.

Do you think this needs to be handled today?

I don't think so - there were no 'fuzzy failures' reported in the parsing phase related to this.

I want to stabilize the current fuzzy check (which includes the argument mode), and do a test parse - if no failures are reported, I'll start a full run. I'll have an answer in ~30 minutes or so.

Other than the overloading signature erasure issue, is there anything else that needs attention today which you are not already working on?

There are other issues related to class arguments, when the type is not matching exactly (i.e. a sub-class or super-class is used); I have tests for this, but I haven't tried to fix them - hopefully we will not need them now.

Greg Shah wrote:

second phase uses the arg modes, on the set of matches from previous step.

Is there a 3rd phase here? In other words, if there is a unique type match in the fuzzy lookup will that ignore modes and select that match?

• in ClassDefinition.exactMethodLookup - this will never match a parent method if the caller doesn't have all argument modes. We may need to look here without the arguments (the SignatureKey.hashCode includes the parameter modes, too).
• the same in fuzzyMethodLookup - when there are multiple or there is a fuzzy match, I'm not sure if a parent definition has precedence over a local one.

The POC parse phase has no warnings related to fuzzy lookup or OO method call annotation failure. I'm leaving the full run.

• annotation for a 64-bit numeric literal at the parsing phase (otherwise its type is computed incorrect).
• ETIME is a int64 function, not integer.

Greg, any idea what has changed, that now the builtin method names are all converted as lowercase?

def var o as Progress.Json.ObjectModel.JsonObject.
def var ch as char.
def var i as int.

i = o:GetInteger(ch).

Registering com.goldencode.p2j.oo.json.objectmodel.JsonObject builtin class from file ../../../skeleton/oo4gl/Progress/Json/ObjectModel/JsonObject.cls
- adding method [getdatetimetz] with [getdatetimetz] javaname
- adding method [addnumber] with [addnumber] javaname
- adding method [getrecid] with [getrecid] javaname
- adding method [getrowid] with [getrowid] javaname
- adding method [gettype] with [gettype] javaname
- adding method [isnull] with [isnull] javaname
- adding method [getdate] with [getdate] javaname
- adding method [getlongchar] with [getlongchar] javaname
- adding method [gethandle] with [gethandle] javaname
- adding method [getint64] with [getint64] javaname
- adding method [getcomhandle] with [getcomhandle] javaname
- adding method [remove] with [remove] javaname
- adding method [getlogical] with [getlogical] javaname
- adding method [getjsonarray] with [getjsonarray] javaname
- adding method [writestream] with [writestream] javaname
- adding method [getraw] with [getraw] javaname
- adding method [getjsonobject] with [getjsonobject] javaname
- adding method [has] with [has] javaname
- adding method [write] with [write] javaname
- adding method [add] with [add] javaname
- adding method [getinteger] with [getinteger] javaname
- adding method [read] with [read] javaname
- adding method [set] with [set] javaname
- adding method [getcharacter] with [getcharacter] javaname
- adding method [setnumber] with [setnumber] javaname
- adding method [getnames] with [getnames] javaname
- adding method [writefile] with [writefile] javaname
- adding method [setnull] with [setnull] javaname
- adding method [clone] with [clone] javaname
- adding method [getdatetime] with [getdatetime] javaname
- adding method [getdecimal] with [getdecimal] javaname
- adding method [getmemptr] with [getmemptr] javaname
- adding method [getjsontext] with [getjsontext] javaname
- adding method [addnull] with [addnull] javaname

which is incorrect... previously with 11424 they were converting like getString (i.e. uppercased words).

Constantin Asofiei wrote:

Greg, any idea what has changed, that now the builtin method names are all converted as lowercase?
[...]
produces:
[...]

which is incorrect... previously with 11424 they were converting like getString (i.e. uppercased words).

I think I found it, ClassDefinition.getDefinedMethods must return the names as defined, and not the map's keys.

I'm updating the gap analysis rules for the new OO features. I'm also trying to plan out the remaining OO work. Some questions:

1. Can I consider "data members including instantiation, scoping and life cycle" to be complete?

2. Can I consider "dereferencing objects including both method calls, data member access and chaining
data members including variables, temp-tables and prodatasets" to be complete (of course the prodataset support must wait until later)?

3. What runtime work (or even conversion work) is related to the following?

• VALID-OBJECT()
• CAST() and DYNAMIC-CAST()
• TYPE-OF()
• DYNAMIC-INVOKE()
• instantiation of objects via the NEW() function, NEW statement, DYNAMIC-NEW or Progress.Lang.Object:New()
• DELETE
• THIS-OBJECT (language statement and as a reference to the current class instance)
• SUPER (language statement)
• SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT (along with normal traversal using NEXT-SIBLING, PREV-SIBLING)
• DEFINE EVENT and CLASS event support
• ON event THROW
• UNDO THROW
• ROUTINE-LEVEL and BLOCK-LEVEL statements
• block option STOP-AFTER

4. I'm guessing that in order to duplicate the flow of control for CATCH, we might need to move the catch blocks to a lambda/delegated model in the runtime. Your thoughts?

I think the built-in 4GL error classes need to inherit from RuntimeException. This will potentially cause an issue with the inheritance model since BaseObject won't be in the hierarchy. I can't think of an alternative, since these classes must be throwable.

Greg Shah wrote:

4. I'm guessing that in order to duplicate the flow of control for CATCH, we might need to move the catch blocks to a lambda/delegated model in the runtime. Your thoughts?
...
I think the built-in 4GL error classes need to inherit from RuntimeException. This will potentially cause an issue with the inheritance model since BaseObject won't be in the hierarchy. I can't think of an alternative, since these classes must be throwable.

This is not an issue, as the current approach is not using the Java's catch block alternative. The converted code uses BM.catchError(Class type, Consumer catchBlock) (where type is the error class being caught and the block is a lambda withe the block's body), added into the Block.init, to register the caught exceptions (in order as they appear).

Our runtime will then use our existing ConditionException approach (via a sub-class) to throw this condition, and after that check into the registered exceptions which one can be used.

Why I used this: 4GL doesn't error on compile if multiple catch blocks reference the same error type, but only a warning. I assume they drop the blocks which are hidden by a previous one.

This approach doesn't have the RuntimeException issue and to me it seems the safest way, as we can hide in the runtime any other peculiarities we can find, with no or minimal impact into the converted code.

Greg Shah wrote:

1. Can I consider "data members including instantiation, scoping and life cycle" to be complete?

I think so, but this is linked with the more global resource management at the object instance level. There might be some peculiarities related to object instantiation failures, when not the entire hierarchy has a chance to initialize.

2. Can I consider "dereferencing objects including both method calls, data member access and chaining
data members including variables, temp-tables and prodatasets" to be complete (of course the prodataset support must wait until later)?

Yes, this looks pretty straightforward; but note that this isn't tested yet.

3. What runtime work (or even conversion work) is related to the following?

• VALID-OBJECT()
• CAST() and DYNAMIC-CAST()
• TYPE-OF()
• DYNAMIC-INVOKE()
• instantiation of objects via the NEW() function, NEW statement, DYNAMIC-NEW or Progress.Lang.Object:New()
• DELETE

These are pretty simple, require only runtime, and will be done with the first phase of the runtime, the object instantiation work (I think we'll have it early next week). I'm trying to include the object reference counting (and destructor call), too.

• THIS-OBJECT (language statement and as a reference to the current class instance)
• SUPER (language statement)
• SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT (along with normal traversal using NEXT-SIBLING, PREV-SIBLING)

Only runtime, I don't see any conversion work needed at this time. I'll include these in the second runtime iteration work. I think ~2 days of work.

• DEFINE EVENT and CLASS event support
• ON event THROW
• UNDO THROW
• ROUTINE-LEVEL and BLOCK-LEVEL statements
• block option STOP-AFTER

Same, only runtime. Another 2 days of work.

Open Items

This is in priority order.

• CA WIP Implement runtime for:
  • Instantiation of objects via the NEW() function, NEW statement, DYNAMIC-NEW as well as the runtime invocation of constructors/destructors.
  • VALID-OBJECT()
  • CAST() and DYNAMIC-CAST()
  • TYPE-OF()
  • DYNAMIC-INVOKE() conversion
  • name_map.xml will need to be changed to add the fully qualified OO name to the class-mapping node as an ooname attribute. This will allow SourceNameMapper to calculate proper targets for dynamic cast, dynamic instantiation and dynamic invocation.
  • DELETE
  • THIS-OBJECT (language statement and as a reference to the current class instance)
  • SUPER (language statement)
  • SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT (along with normal traversal using NEXT-SIBLING, PREV-SIBLING)
  • DYNAMIC-INVOKE() runtime
  • Instantiation of objects via Progress.Lang.Object:New()
  • static constructor invocation
  • dynamic method invocation
  • separate APIs for method invocation - when used in expression use ObjectOps.invoke (and check return type other than void), when used single, use ObjectOps.invokeStandalone and don't check return type
  • more indepth checks of non-dynamic method invocation, SUPER, THIS-OBJECT
  • DEFINE EVENT and CLASS event support (the only tricky part seems like matching the INPUT-OUTPUT and OUTPUT parameter behavior).
  • ON event THROW
  • UNDO THROW
  • ROUTINE-LEVEL and BLOCK-LEVEL statements
  • block option STOP-AFTER
• Write the backing implementation for the built-in classes which are needed for the current project (see #3750-238).
• Static data needs to be implemented as context local and access must properly de-reference.
• GES WIP Methods
  • Implement an implicit DYNAMIC-INVOKE for method calls with POLY arguments, for which more than one target can be resolved. See #3751-322.
  • Runtime dynamic extent validation when used as parameters.
  • Parameter mode enhancements still needed for overload matching. See #3751-346.
  • Disambiguate Java method names when overloaded method signatures only differ by fixed/indeterminate array differences, parameter mode differences.
  • TT parameter usage for two different tables is treated as a different signature in 4GL but not FWD. See #3751-318.
• Properties
  • The property getter/setter direct private member access is not right yet. Read should only occur in GET and write only in SET.
  • Properties defined in an interface or an abstract property (in an abstract class) don't convert properly.
  • Check that a getter with no return does return unknown in FWD.
  • Confirm that it is possible to override access modes for abstract props in the 4GL. Can you change the access mode, initial value and so forth? Determine a plan for this case.
  • Handle arbitrary chaining of property references in oo_references.rules.
  • Extent properties can be accessed as a whole array reference instead of always as an indexed getter/setter. Test this support and implement an additional getter (and maybe setter) as needed.
• Static Temp-Tables
  • FWD doesn't properly emit static temp tables (and maybe access modifiers).
  • Usage of this tt from multiple static methods doesn't have the proper scopes.
• Other things to check:
  • Do methods act like internal procedures when there is no return value and act like user-defined functions when there is a return value?
  • How do properties and data members work when used in frames and PUT statements? Is there any difference in external class versus internal class usage?
  • Error handling control flow.
  • Does throwing an error modify ERROR-STATUS?
  • The 4GL docs state that protected members can be accessed by anything "at the same level (or higher) in the class hierarchy". This is hopefully just poor phrasing, but we need to check if 2 sibling classes (different classes with the same parent class) can access protected members in the other sibling.
  • Does FWD properly handle triggers as members of a class?
• Emit unqualified class names when possible.
• Change the "access" annotation to "access-mode" to be consistent with other usage.
  • ClassDefinition.addMethod() and ClassDefinition.annotateMethodCall() are where we set the annotations.
  • TRPL usage locations need to be switched.
• CLASS statement support for USE-WIDGET-POOL and SERIALIZABLE
• Rework usage of ConvertedClassName and fname2jname to use a refid approach instead.
  • This is needed to support incremental conversion of code with OO references.
  • The approach requires that at parse time, the references are resolved and tempidx and tempidx-file annotations left behind.
  • During post-parse-fixups, the refids must be computed and stored.
  • Downstream code that looks up javanames from ConvertedClassName and fname2jname must be switched to a refid approach.
  • One dependency that is tricky to break is the usage for methods/vars/properties in built-in classes. Does it make sense to create ASTs for the built-ins so that the same approach can be used everywhere?
• Eliminate the double wrapping of unknown value.
• ENUMs and bitwise AND, OR, NOT
• prodatasets data members including chaining and proper scoping (this is dependent upon #3809)

Constantin Asofiei wrote:

Greg, there are errors like this:

    [javac] ...1046: error: no suitable method found for add(character,BaseDataType)
    [javac]                               json.ref().add(hField.unwrap().name(), hField.unwrapBufferField().value());
    [javac]                                         ^
    [javac]     method JsonObject.add(character,character) is not applicable

As we don't parse the JsonObject builtin class, I think we need to add the POLY method variants in the Java impl for this.

Is this an issue because we don't have ASTs to use for overloaded method matching?

If we knew the method picked, we could wrap things like we do for user-defined methods. Of course, we still could have the issue where the exact method is picked at runtime because multiple matches exist. In that case we would need the DYNAMIC-INVOKE approach.

Greg Shah wrote:

Is this an issue because we don't have ASTs to use for overloaded method matching?

This is a case where we need to emit a dynamic-invoke emulation instead of the direct call, as the caller's signature can't match uniquely against the defined methods, at conversion time. But this is no longer an issue for the POC, as it was solved via patches.

The following is a running list to keep track of the differences between Java and the OO 4GL support.

Missing in OO 4GL

• inner classes (named and anonymous)
• generics
• lambdas
• null
• can't de-reference a static member from an object instance
• data that is final
• classes, interfaces and enums are always public (there are no protected, private or access modifiers that can be specified on the definitions)
• no package private concept
• no literal form for a class reference (e.g. no String.class), you must use GET-CLASS(unquoted_type_name) function or call Progress.Lang.Class:GetClass("char_expr_fully_qualified_classname") or call GetClass() on an instance)

OO 4GL Features That Deviate From Java

• duck typing (see #3751-8)
• property syntax (instead of getter/setter method calls)
• properties and class events can be abstract data (implicit when included in an interface or explicit when in a class and the abstract keyword is used
• method overloading can be differentiated based on:
  • fixed extent sizes and/or indeterminate extent
  • parameter modes
  • implicit widening conversions
• runtime method signature resolution (#3751-325)
• default (and only) access for classes, interfaces and enums is public
• there is a difference between using the Object.equals() method and the equality operator (e.g. two instances of the same string will return true when equals method is used but when comparing them using eq those are actually false)

We will edit this over time as we have additional findings.

Constantin Asofiei wrote:

Greg Shah wrote:

Is this an issue because we don't have ASTs to use for overloaded method matching?

This is a case where we need to emit a dynamic-invoke emulation instead of the direct call, as the caller's signature can't match uniquely against the defined methods, at conversion time. But this is no longer an issue for the POC, as it was solved via patches.

Looking ahead, shouldn't we analyze the method defs in built-in classes too and match signatures? Otherwise we would need to use DYNAMIC-INVOKE more aggressively than might be needed. Likewise, if any of the built-in classes have instances of methods that conflict in Java (differentiated only by parameter modes or extent sizes) we might have the same issue as for the user-defined cases.

Greg, User is a builtin variable in 4GL (and reserved kw), UNLESS is used as a class member - can you recall me where we handle this?

Constantin Asofiei wrote:

Greg, User is a builtin variable in 4GL (and reserved kw), UNLESS is used as a class member - can you recall me where we handle this?

I assume you are thinking about the unqualified reference inside the class hierarchy.

If I understand your question correctly, we handle these unqualified references using semantic predicates in primary_expr, before the object_data_member rule. If this is matched, then the member is selected instead of the reserved variable.

The qualified case is handled from chained_object_members and it is unambiguous since the reserved vars can't be matched after the COLON.

Greg Shah wrote:

I assume you are thinking about the unqualified reference inside the class hierarchy.

No, I'm thinking about a var definition like def var user as char. - this works only if this is defined as a class member; same applies for a property definition (which actually exposed this issue, as we are finding the builtin var instead of the class property).

If I understand your question correctly, we handle these unqualified references using semantic predicates in primary_expr, before the object_data_member rule. If this is matched, then the member is selected instead of the reserved variable.

I think I'm remembering this case; the above var/property definition looks like something new.

def_var_stmt doesn't support this today. It matches using symbol which does not allow reserved keywords. If we should handle this differently in a class, we will have to put an alternative there.

def_prop_stmt does support this. It matches using any_symbol_at_all which DOES allow reserved keywords.

All 4GL global var names can be used as class variables, properties and method names.

Only these global names can be used as class event names:

define public event generate-pbe-salt signature void ().
define public event generate-random-key signature void ().
define public event generate-uuid signature void ().
define public event guid signature void ().
define public event mtime signature void ().
define public event page-size signature void ().
define public event process-architecture signature void ().
define public event rt-opsys signature void ().
define public event timezone signature void ().

This is the same list as the list of global names which can be used as standalone var names:

def var generate-pbe-salt as char.
def var generate-random-key as char.
def var generate-uuid as char.
def var guid as char.
def var mtime as char.
def var page-size as char.
def var process-architecture as char.
def var rt-opsys as char.
def var timezone as char.

Greg Shah wrote:

def_var_stmt doesn't support this today. It matches using symbol which does not allow reserved keywords. If we should handle this differently in a class, we will have to put an alternative there.

I don't think we have this case currently (as we would have failed parsing previously), so I'm not fixing this now.

def_prop_stmt does support this. It matches using any_symbol_at_all which DOES allow reserved keywords.

Yes, but even if the match is allowed, the lookup done by SymbolResolver.addLocalVariable will find the global/builtin var and will not add it in the proper scope. This needs to be fixed, I'm working on it now.

Greg, let me rephrase this: shouldn't pre_scan_class delete the class/iface scope after the rule has processed the class members?

shouldn't pre_scan_class delete the class/iface scope after the rule has processed the class members?

      { sym.addScope(); }
      (class_stmt | interface_stmt)
      { sym.deleteScope(); }

Are you asking if the deleteScope() in this code should be moved to the exit action? I think the answer is yes.

Greg Shah wrote:

Are you asking if the deleteScope() in this code should be moved to the exit action? I think the answer is yes.

Yes.

Please review 3750a rev 11444 - it includes a batch of regression fixes. Note that I've limited the 'integer' param wrapping only for object invocation or TO loops; the previous way was wrapping everything, including literals... and there was lots of noise in MAJIC and other apps.

Code Review 3750a Revision 11444

I'm good with the changes.

Constantin: What is the status of your current work?

Greg Shah wrote:

Constantin: What is the status of your current work?

I'm still working on the instantiation and reference counting. Parts of second phase are also WIP.

I'm starting to work on the open issues related to methods (see #3751-360). Considering the interconnected nature of our work, please try to check in whenever it is safe so that our work does not diverge or conflict too much.

Greg Shah wrote:

I'm starting to work on the open issues related to methods (see #3751-360).

I think we should fix the conversion problem for static members first...

I'm starting to work on the open issues related to methods (see #3751-360).

I think we should fix the conversion problem for static members first...

OK, I'll take that first.

FYI, the reference counting in the end depends on the variable scopes - a var going out of scope will decrement the counter, and if zero will destroy the object. I don't have rules for the temp-table fields yet.

It seems that our parsing of class event subscribe() and unsubscribe() does not work for the "simple" case of passing only a method name:

   def public static event evt-def void ().

   method public void test():
      evt-def:subscribe( handler ).  // this fails to find "handler", if qualified with "this-object:" then it will work
   end.

   method public void handler():
   end.

The qualified form using this-object: will parse but it does not convert properly.

Is this a known issue?

BTW, this seems to be related to the use of static for the define event. If not defined static then the unqualified form works.

Greg Shah wrote:

BTW, this seems to be related to the use of static for the define event. If not defined static then the unqualified form works.

This is something I missed - the method reference (by name) doesn't work if the event is static and the handler method is instance. FWD works only if both (event and handler method) are static or both are instance.

3750b rev 11302 has the fix for the static issue with events. It also fixes other latent problems with static processing in constructors and property getters/setters.

Revision 11301 fixes direct property access in property getters/setters. It is a pre-requisite for the move to context local.

Revision 11300 makes access-mode the annotation instead of access for method definitions.

I'm continuing to work on the context local changes.

I have noticed that when we create a default constructor, it is placed in a strange location in the AST:

    <ast col="0" id="897648164866" line="0" text="class definition" type="CLASS_DEF">
      <ast col="0" id="897648164978" line="0" text="constructor" type="CONSTRUCTOR">
        <annotation datatype="java.lang.Long" key="peerid" value="901943132202"/>
        <ast col="0" id="897648164979" line="0" text="constructor" type="KW_CONSTRUC">
          <annotation datatype="java.lang.String" key="javaname" value="__oo_StaticData_constructor__"/>
          <annotation datatype="java.lang.String" key="classname" value="void"/>
          <ast col="0" id="897648164980" line="0" text="public" type="KW_PUBLIC"/>
          <ast col="0" id="897648164981" line="0" text="(" type="LPARENS">
            <annotation datatype="java.lang.Long" key="peerid" value="901943132203"/>
          </ast>
        </ast>
        <ast col="0" id="897648164982" line="0" text="block" type="BLOCK">
          <annotation datatype="java.lang.Boolean" key="recordscoping" value="true"/>
          <annotation datatype="java.lang.Boolean" key="loop" value="false"/>
          <annotation datatype="java.lang.Boolean" key="loopnext" value="false"/>
          <annotation datatype="java.lang.Boolean" key="toplevel" value="true"/>
          <annotation datatype="java.lang.Long" key="translevel" value="135"/>
          <annotation datatype="java.lang.Long" key="reasons" value="0"/>
          <annotation datatype="java.lang.Long" key="peerid" value="901943132224"/>
          <annotation datatype="java.lang.Long" key="preid" value="901943132212"/>
          <annotation datatype="java.lang.Long" key="controlid" value="901943132216"/>
          <annotation datatype="java.lang.Long" key="enterid" value="901943132220"/>
          <annotation datatype="java.lang.Long" key="finiid" value="901943132228"/>
          <annotation datatype="java.lang.Long" key="instvarid" value="901943132205"/>
          <ast col="0" id="897648164983" line="0" text="statement" type="STATEMENT">
            <ast col="0" id="897648164984" line="0" text="super" type="KW_SUPER">
              <annotation datatype="java.lang.String" key="javaname" value="__lang_BaseObject_constructor__"/>
              <annotation datatype="java.lang.Long" key="peerid" value="901943132230"/>
              <ast col="0" id="897648164985" line="0" text="(" type="LPARENS"/>
            </ast>
          </ast>
        </ast>
      </ast>
      <ast col="1" id="897648164867" line="3" text="class" type="KW_CLASS">
        <annotation datatype="java.lang.Long" key="support_level" value="514"/>
        <ast col="7" id="897648164870" line="3" text="oo.StaticData" type="SYMBOL"/>
      </ast>
      <ast col="0" id="897648164873" line="0" text="block" type="BLOCK">
...

This seems wrong because it breaks the "contract" of having this code inside the CLASS_DEF/BLOCK node. Subsequent walks of the tree would expect the KW_CLASS to be the first child which is no longer the case.

Greg Shah wrote:

I have noticed that when we create a default constructor, it is placed in a strange location in the AST:

See this code in annotations/method_defs.rules:

   <ascent-rules>
      <rule>this.type == prog.class_def and not hasDefCtor
         <!-- create an implicit ctor, with a super call -->
         <action>
            tpl.graftAt("implicit_legacy_ctor_with_super", null, copy, 0, 
                        "javaname", cname,
                        "superjavaname", sqname)
         </action>
      </rule>
   </ascent-rules>

I think is safe to put it in the right place (i.e. this.type prog.block and parent.type prog.class_def instead of this.type == prog.class_def).

In 3750b revision 11305, static variables, properties and events have their backing variable converted as a ContextLocal instance and all usage is dereferenced using get().

I will test this with the POC code next to confirm it is complete and correct. My local testcases worked well (see uast/oo/StaticData.cls and uast/oo/PropertyHolder.cls).

Greg, how does a simple static member reference get converted?

I ask because something as simple as oo.Bar:e. will trigger the static c'tor execution for the entire class hierarchy.

1. update the reference count for the old value (decrement) and new value (increment)
2. decrement the reference count when the record gets deleted

Ovidiu, please focus on making this test work:

def temp-table tt1 field fobj as progress.lang.object.

create tt1.

The idea here is that the FWD server is not starting up, as we are missing the hibernate mappings, plus some issues in the HBM file.

Example:

class oo.StaticData:

   def public static var num as int.

   def public static event evt-def void ().

   method public void test():
      if num eq 0 then num = 29.

      evt-def:subscribe( /* this-object: */ handler ).
   end.

   method public void handler():
   end.

end.

this converts to:

package com.goldencode.testcases.oo;

import com.goldencode.p2j.util.*;
import com.goldencode.p2j.oo.lang.*;
import com.goldencode.p2j.security.*;

import static com.goldencode.p2j.util.BlockManager.*;
import static com.goldencode.p2j.util.CompareOps.*;

/**
 * Business logic (converted to Java from the 4GL source code
 * in oo/StaticData.cls).
 */
public class StaticData
extends BaseObject
{
   public static ContextLocal<integer> num = new ContextLocal<integer>()
   {
      protected integer initialValue()
      {
         return UndoableFactory.integer();
      }
   };

   private static ContextLocal<ClassEvent> evtDef = new ContextLocal<ClassEvent>()
   {
      protected ClassEvent initialValue()
      {
         return new ClassEvent();
      }
   };

   public void __oo_StaticData_execute__()
   {
      externalProcedure(StaticData.this, new Block((Body) () -> 
      {
         {
         }
      }));
   }

   public void __oo_StaticData_constructor__()
   {
      internalProcedure(new Block((Body) () -> 
      {
         __lang_BaseObject_constructor__();
      }));
   }

   private static void publish_evtDef()
   {
      evtDef.get().publish();
   }

   public static void subscribe_evtDef(handle h, character procName)
   {
      evtDef.get().subscribe(h, procName);
   }

   public static void subscribe_evtDef(object<? extends com.goldencode.p2j.oo.lang._BaseObject_> ref, character methName)
   {
      evtDef.get().subscribe(ref, methName);
   }

   public static void unsubscribe_evtDef(handle h, character procName)
   {
      evtDef.get().unsubscribe(h, procName);
   }

   public static void unsubscribe_evtDef(object<? extends com.goldencode.p2j.oo.lang._BaseObject_> ref, character methName)
   {
      evtDef.get().unsubscribe(ref, methName);
   }

   public void test()
   {
      internalProcedure(new Block((Body) () -> 
      {
         if (_isEqual(num.get(), 0))
         {
            num.get().assign(29);
         }

          /* this-object: */ subscribe_evtDef(ObjectOps.thisObject(), new character("handler"));
      }));
   }

   public void handler()
   {
      internalProcedure(new Block());
   }
}

Properties:

   define static property num as int
      get:
         return num.
      end.
      set (input new-num as int):
         num = new-num.
      end.

   define static property singleton as oo.BasicMethods
      get:
         if singleton eq ? then singleton = new oo.BasicMethods().

         return singleton.
      end.
      private set.      

...

   method public void test():
      ...      

      singleton:empty().
   end.

converts to:

   private static ContextLocal<integer> num = new ContextLocal<integer>()
   {
      protected integer initialValue()
      {
         return UndoableFactory.integer();
      }
   };

   private static ContextLocal<object<? extends com.goldencode.testcases.oo.BasicMethods>> singleton = new ContextLocal<object<? extends com.goldencode.testcases.oo.BasicMethods>>()
   {
      protected object<? extends com.goldencode.testcases.oo.BasicMethods> initialValue()
      {
         return UndoableFactory.object(com.goldencode.testcases.oo.BasicMethods.class);
      }
   };

...

   public static integer getNum()
   {
      return function("num", integer.class, new Block((Body) () -> 
      {
         returnNormal(num.get());
      }));
   }

   public static void setNum(final integer _newNum)
   {
      integer newNum = TypeFactory.initInput(_newNum);

      internalProcedure(new Block((Body) () -> 
      {
         num.get().assign(newNum);
      }));
   }

   public static object<? extends com.goldencode.testcases.oo.BasicMethods> getSingleton()
   {
      return function("singleton", object.class, new Block((Body) () -> 
      {
         if (_isUnknown(singleton.get()))
         {
            setSingleton(new object(ObjectOps.newInstance(com.goldencode.testcases.oo.BasicMethods.class)));
         }

         returnNormal(singleton.get());
      }));
   }

   private static void setSingleton(final object<? extends com.goldencode.testcases.oo.BasicMethods> _var)
   {
      object<? extends com.goldencode.testcases.oo.BasicMethods> var = TypeFactory.initInput(_var);

      internalProcedure(new Block((Body) () -> 
      {
         singleton.get().assign(var);
      }));
   }

...

   public void test()
   {
      internalProcedure(new Block((Body) () -> 
      {
         ...
         ((com.goldencode.testcases.oo.BasicMethods) getSingleton().ref()).empty();
      }));
   }

Greg, I mean a simple static reference, outside of a 4GL object class, like:

class oo.Bar:
def public static var xs as int.
end.

oo.Bar:xs = 10.

com.goldencode.testcases.oo.Bar.xs.assign(new integer(10));

1. the get() call is missing, to retrieve the context-local
2. I need a way to intercept in FWD that this is a class reference and force it to load the static constructor. Maybe we should emit something like oo.Bar.__load__().xs.get()?

This would mean a new synthetic static method added to every class, defined as:

public static oo.Bar __load__()
{
   ObjectOps.loadClass(Bar.class);
   return null; // this works, as we are accessing static methods, and not instance methods
}

This method will be emitted as a prefix for any static call (method, property, var, etc), executed outside of the defining class.

Constantin Asofiei wrote:

This method will be emitted as a prefix for any static call (method, property, var, etc), executed outside of the defining class.

If we go with this approach, then we can further limit this by emitting it only when a static c'tor exists in current class or any super-class.

the get() call is missing, to retrieve the context-local

Fixed in revision 11306. This is only an issue with vars. Events and properties can only be accessed via methods (which handle the dereferencing internally) so there are is no need for the get() in those cases.

I need a way to intercept in FWD that this is a class reference and force it to load the static constructor. Maybe we should emit something like oo.Bar.__load__().xs.get()?

Why not just emit a real Java static initializer? Then call loadClass() from inside.

Greg Shah wrote:

I need a way to intercept in FWD that this is a class reference and force it to load the static constructor. Maybe we should emit something like oo.Bar.__load__().xs.get()?

Why not just emit a real Java static initializer? Then call loadClass() from inside.

• we need to control exactly when this is called.
• the static constructor in 4GL (as the static vars) is not really static, but an initializer which needs to be called for every context, when the class if first loaded in that context. So we can't use the Java static initializer approach.

The static c'tor needs to emulate an external procedure and all static member initialization (like opening temp-table or buffer scopes) needs to be done in this static c'tor and not the program's execute method.

Why: the static state needs to be alive for the duration of the context, and destroyed when the context is terminated. This can be emulated with a persistent instance of this program (as we do with creating a 4GL object). I think this requires moving the instance member assignment to the execute method, and not at the definition. Otherwise creating a persistence instance (for the static state) will pick up all the instance members, too.

Constantin Asofiei wrote:

Ovidiu, please focus on making this test work:
[...]
The idea here is that the FWD server is not starting up, as we are missing the hibernate mappings, plus some issues in the HBM file.

Constantin,
I added the minimum necessary hibernate mappings so that the server that runs your testcase starts for me. Please see the revision 11308/3750b. The implementation is not complete, the persistence of the objects is not fully functional. To finish it, we need first a better implementation of the object and the reference counter. I suppose that the implementation of persistence will be based on a UUID (a long/int63 should also be enough I think) of the object that will be stored instead. When the object is restored from database, the UUID will be looked-up from a registry. I do not know yet how this behaves w.r.t. garbage collector/ reference counter of ABL language.

Ovidiu Maxiniuc wrote:

Constantin,
I added the minimum necessary hibernate mappings so that the server that runs your testcase starts for me.

There is something wrong - try running the program, I get this:

Caused by: org.hibernate.MappingException: No Dialect mapping for JDBC type: 2000
        at org.hibernate.dialect.TypeNames.get(TypeNames.java:76)
        at org.hibernate.dialect.TypeNames.get(TypeNames.java:99)
        at org.hibernate.dialect.Dialect.getTypeName(Dialect.java:297)
        at org.hibernate.mapping.Column.getSqlType(Column.java:227)
        at com.goldencode.p2j.persist.TempTableHelper.generateComputedColumnsSQLCreate(TempTableHelper.java:856)
        at com.goldencode.p2j.persist.TempTableHelper.generateSQLCreateTable(TempTableHelper.java:813)
        at com.goldencode.p2j.persist.TempTableHelper.generateSQLCreateTableList(TempTableHelper.java:765)
        at com.goldencode.p2j.persist.TempTableHelper.<init>(TempTableHelper.java:379)
        at com.goldencode.p2j.persist.LocalTempTableHelper.<init>(LocalTempTableHelper.java:107)
        at com.goldencode.p2j.persist.TempTableHelper.get(TempTableHelper.java:317)
        at com.goldencode.p2j.persist.TempTableHelper.sqlTempTableCreate(TempTableHelper.java:182)
        at com.goldencode.p2j.persist.TemporaryBuffer$Context.doCreateTable(TemporaryBuffer.java:5052)
        at com.goldencode.p2j.persist.TemporaryBuffer$Context.createTable(TemporaryBuffer.java:4861)
        at com.goldencode.p2j.persist.TemporaryBuffer.openScope(TemporaryBuffer.java:3261)
        at com.goldencode.p2j.persist.RecordBuffer.openScope(RecordBuffer.java:3378)
        at com.goldencode.testcases.Instantiate.lambda$1(Instantiate.java:31)
        at com.goldencode.p2j.util.Block.body(Block.java:604)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:7577)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:7368)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:403)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:374)
        at com.goldencode.testcases.Instantiate.execute(Instantiate.java:25)

I suppose that the implementation of persistence will be based on a UUID (a long/int63 should also be enough I think) of the object that will be stored instead. When the object is restored from database, the UUID will be looked-up from a registry. I do not know yet how this behaves w.r.t. garbage collector/ reference counter of ABL language.

I'll fix this.

Constantin Asofiei wrote:

I don't think we can use the JAVA_OBJECT type. If we are storing an UUID, we can use VARCHAR and/or another SQL type, right?

I switched to old plain VARCHAR as backing support for storing objects. Please see the revision 11309. It also contains a bit of debug traces that will be removed when things start to work. At any rate, my test client connected to server and successfully executed some simple db operations.

Ovidiu Maxiniuc wrote:

Constantin Asofiei wrote:

I don't think we can use the JAVA_OBJECT type. If we are storing an UUID, we can use VARCHAR and/or another SQL type, right?

I switched to old plain VARCHAR as backing support for storing objects. Please see the revision 11309. It also contains a bit of debug traces that will be removed when things start to work. At any rate, my test client connected to server and successfully executed some simple db operations.

Great, I'll let you know when you can help me with #3751-390.

I've confirmed that the customer code works properly with my recent context-local static data changes. I have been avoiding the runtime classes since many of them are related to the instantiation work. I'm picking up the methods work next.

Maybe we should emit something like oo.Bar.__load__().xs.get()?

Instead of emitting this everywhere something static is referenced, it would be better to put this inside the ContextLocal.initialValue() method. This is guaranteed to be called when get() is used, so any static data usage from outside the class will hook the __load__() method.

A similar approach could be added to any static method (including the ones that are emitted for properties and events).

Implementing this in the class is much less error prone to later changes. For example, when people manually edit or write code to interact with the converted code, it would be very likely that people will forget to add the __load__() usage.

Greg Shah wrote:

Maybe we should emit something like oo.Bar.__load__().xs.get()?

Instead of emitting this everywhere something static is referenced, it would be better to put this inside the ContextLocal.initialValue() method.
...
A similar approach could be added to any static method (including the ones that are emitted for properties and events).

You are right, we can hook these there. I'll work on it.

For dynamic method invocation, there is a ERROR condition raised if the method returns void and that method is used in an expression. Don't know how to handle this yet - maybe a special return value?

We can have 2 versions of DYNAMIC-INVOKE. One that is for a standalone expression and one that is known to be used as a su-expression or rvalue. In this second form we can throw an ErrorConditionException if the result is void.

• NEW, DYNAMIC-NEW, VALID-OBJECT, CAST, DYNAMIC-CAST, TYPE-OF, DELETE
• DYNAMIC-INVOKE parse and conversion (don't know how I missed this originally)
• DELETE
• THIS-OBJECT (the THIS-PROCEDURE counterpart)
• SESSION:FIRST-OBJECT and SESSION:LAST-OBJECT
• object shared variables
• object arrays (dynamic or static extent)
• reference counting, instance constructor and destructor invocation
• fixed the ObjectUserType to be able to use temp-table object fields at runtime
• object variables are instantiated as ObjectVar (by the TypeFactory or ArrayAssigner) - this is required to enable the reference counting ONLY for the defined vars. Also, they are registered at the defining top-level block via ObjectOps.register, so that when they get out of scope, their referenced object can update the counter.
• when instantiating a legacy object, we still emulate them as a persistent procedure. This is needed to track the created resources and other stuff. For this (and for the chaining reason), we have a ObjectResource synthetic resource, which registers the objects via HandleChain and tracks them naturally.

Still needs testing and not fully complete (there are still some TODOs left).

• Progress.Lang.Class:New() - this requires implementation of Progress.Lang.ParameterList and Progress.Lang.Class, otherwise it will not work.
• static constructors
  - emit class loading call at the beginning of static methods (before the BlockManager API call) and in the static member initialValue()
  - properly track class loading
  - not sure if they must be static or not. We need to keep static resource info in a persistent program, but I'm looking for a way to emulate this without creating a persistent instance for the object (as this complicates things, as instance-level resources will be created). So making the static c'tor really static might work combined with a 'fake' persistence object (just a java.lang.Object instance to track it as a persistent program).
• dynamic method invocation
• separate APIs for method invocation - when used in expression use ObjectOps.invoke (and check return type other than void), when used single, use ObjectOps.invokeStandalone and don't check return type.
• runtime for dynamic-invoke
• more indepth checks of non-dynamic method invocation, SUPER, THIS-OBJECT
• validation for access mode, in dynamic invoke and constructor call

Ovidiu, you can now look into the object reference tracking for the temp-table fields (what I mentioned in #3751-390). See ObjectOps.increment and ObjectOps.decrement. If you have doubts about something or other questions, please ask.

Code Review Task Branch 3750b Revision 11311

1. In ArrayAssigner, why is ObjectOps.reRegister(array, copy); being called unconditionally instead of only when isObject == true?

2. I think your explanation of ObjectVar from above ("Object variables are instantiated as ObjectVar (by the TypeFactory or ArrayAssigner) - this is required to enable the reference counting ONLY for the defined vars. Also, they are registered at the defining top-level block via ObjectOps.register, so that when they get out of scope, their referenced object can update the counter.") should be put into the class javadoc for ObjectVar. Just reading the source code does not explain this enough. In particular, the idea that transient object instances should not be reference counted is important.

3. SessionUtils.lastObject() actually returns the firstResource().

4. ObjectOps.register(), ObjectOps.newInstance(), ObjectOps.isValid(), ObjectOps.asResource(), ObjectOps.loadClass(), ObjectOps.delete(), ObjectOps.reRegister(), ObjectOps.getConstructorMethod(), ObjectOps.getExecuteMethod(), ObjectOps.getExecuteMethods() need javadoc.

5. AppError, BaseObject, LegacyClass, LegacyError, ParameterList, ProError, ObjectOps, object each need a history entry.

Constantin: The changes are very good. I'm excited to see this coming together.

Greg, have you checked properties defined in an interface or an abstract property (in an abstract class)? These don't convert properly at all.

3750b rev 11312 contains another batch of mostly conversion changes.

I've fixed review notes 1 to 3; 4 and 5 (comments and history entries) I usually add them when I'm close to complete that file.

Constantin,

In ObjectResource.delete(), a new ExternalProgramWrapper is used for some cleanup. I do not think this is correct. It will fail with:

java.util.NoSuchElementException
        at java.util.ArrayDeque.removeFirst(ArrayDeque.java:280)
        at java.util.ArrayDeque.pop(ArrayDeque.java:517)
        at com.goldencode.p2j.util.ProcedureManager.delete(ProcedureManager.java:1810)
        at com.goldencode.p2j.util.ExternalProgramWrapper.delete(ExternalProgramWrapper.java:785)

Ovidiu Maxiniuc wrote:

Constantin,

In ObjectResource.delete(), a new ExternalProgramWrapper is used for some cleanup. I do not think this is correct. It will fail with:
[...]

What test got you this exception?

DEFINE VARIABLE anOnj AS CLASS Progress.Lang.Object. 
anOnj = NEW Progress.Lang.Object().
anOnj = ?.

anOnj.assign(ObjectOps.newInstance(com.goldencode.p2j.oo.lang.BaseObject.class));
anOnj.setUnknown();

The stack trace is like this:

Caused by: java.util.NoSuchElementException
        at java.util.ArrayDeque.removeFirst(ArrayDeque.java:280)
        at java.util.ArrayDeque.pop(ArrayDeque.java:517)
        at com.goldencode.p2j.util.ProcedureManager.delete(ProcedureManager.java:1810)
        at com.goldencode.p2j.util.ExternalProgramWrapper.delete(ExternalProgramWrapper.java:785)
        at com.goldencode.p2j.util.ObjectResource.delete(ObjectResource.java:176)
        at com.goldencode.p2j.util.ObjectOps.delete(ObjectOps.java:787)
        at com.goldencode.p2j.util.ObjectResource.decrement(ObjectResource.java:161)
        at com.goldencode.p2j.util.ObjectOps.decrement(ObjectOps.java:818)
        at com.goldencode.p2j.util.ObjectVar.setUnknown(ObjectVar.java:157)
        at com.goldencode.testcases.p3751.P3751m.lambda$execute$0(P3758m.java:40)
        ...

Please don't use the builtin 4GL classes in FWD - these are not implemented yet.

• make a oo/Bar.cls file
• the content can be:
  

  class oo.Bar:
  end.
  

In ClassDefinition.fuzzyMethodLookup() code starting at line 1548:

• Shouldn't the code at line 1563 be checking for a match to the table's schema name? In other words: does the 4GL really treat two different temp-tables as a fuzzy match even though it cannot ever work?
• I also don't understand how the code on line 1563 would ever be true since the exact type match would already pass on line 1541.
• In the code at line 1572 we are doing an exact comparison. This is similar to the last question. I don't understand how this code can ever execute since we would have already made this exact match on line 1541.

Constantin Asofiei wrote:

Greg, have you checked properties defined in an interface or an abstract property (in an abstract class)? These don't convert properly at all.

I'll look at it.

Constantin Asofiei wrote:

Ovidiu, you can now look into the object reference tracking for the temp-table fields (what I mentioned in #3751-390). See ObjectOps.increment and ObjectOps.decrement. If you have doubts about something or other questions, please ask.

I tried to add this but I am not sure we can always keep a strict reference count of objects stored in temp-tables. Supposing that we increment each time object X is stored in a record and decrement each time the field is assigned to other object, how do we cope in the case of
Think only in the case of bulk delete when the entire table content is dropped with

tt1.deleteAll();

Ovidiu Maxiniuc wrote:

Think only in the case of bulk delete when the entire table content is dropped with

• when a record is deleted, it will update the reference counters for all references in the FIELD_CLASS objects.
• when a table is emptied, it will iterate through all records and update the reference counters

Did you find way to track if a field is changed (to unknown or some other ref)?

In ClassDefinition.fuzzyMethodLookup():

                     if (Objects.equals(candidateMode, sigMode) || 
                         (candidateMode == null && sigMode != null))
                     {
                        // either the same or the caller hasn't specified the argument mode

The caller's mode is sigMode so this comment seems incorrect. We are testing if the defined method's parameter mode is not specified and the caller's mode is specified. Do I need to change the comment or is the code wrong?

Greg Shah wrote:

In ClassDefinition.fuzzyMethodLookup() code starting at line 1548:

• Shouldn't the code at line 1563 be checking for a match to the table's schema name? In other words: does the 4GL really treat two different temp-tables as a fuzzy match even though it cannot ever work?

It's more than that; the tables are not matched by schema name, but by their field definition; consider this:

   def temp-table tt1 field f1 as int.
   def temp-table tt2 field f1 as int.
   method public void m1(input table tt1):
   end.
   method public void m1(input table tt2):
   end.

• I also don't understand how the code on line 1563 would ever be true since the exact type match would already pass on line 1541.

You are correct, the 1563 check is redundant in the current code.

• In the code at line 1572 we are doing an exact comparison. This is similar to the last question. I don't understand how this code can ever execute since we would have already made this exact match on line 1541.

Same as 1541, looks redundant - I don't recall, but I might have added them just to mark that these cases need more attention and forgot to comment it.

The caller's mode is sigMode so this comment seems incorrect. We are testing if the defined method's parameter mode is not specified and the caller's mode is specified. Do I need to change the comment or is the code wrong?

Hmm... the code looks wrong, they need to be switched.

From #3751-346:

INPUT/OUTPUT/INPUT-OUPTUT for OO argument case.

I assume this is the TODO in fuzzyMethodLookup() where the object narrowing must match the same or a subclass and object widening must match the same or super-class?

OUTPUT/INPUT-OUTPUT for EXTENT cases

Can you elaborate on this?

From #3751-347:

The two-phase fuzzy method matching is not complete yet; as I understand:

• first phase is matching without checking the arg modes - if a single match is found, use that. I'm not sure here if the match must be exact (without narrowing/widening checks) and how POLY interferes.
• second phase uses the arg modes, on the set of matches from previous step.

How much of this remains to be done? It seems that some of it is already there. If not, I may need more thoughts on how the current implementation is deficient.

Greg Shah wrote:

From #3751-346:

INPUT/OUTPUT/INPUT-OUPTUT for OO argument case.

I assume this is the TODO in fuzzyMethodLookup() where the object narrowing must match the same or a subclass and object widening must match the same or super-class?

Yes. But note that FWD can't compile two methods with the same name and arguments, only difference being a OO parameter's type:

   method public void m1(input v as oo.Bar):
   end.
   method public void m1(input v as oo.Foo): // Java compile error
   end.

OUTPUT/INPUT-OUTPUT for EXTENT cases

Can you elaborate on this?

These don't convert properly. See uast/oo/Overloads2.cls, overloads-test2.p and overloads2.i - these have a pretty comprehensive collection of method arguments, including fields, extents, etc.

How much of this remains to be done? It seems that some of it is already there. If not, I may need more thoughts on how the current implementation is deficient.

From #3751-349:

My previous tests showed that exact matches using type were honored all the way up the class hierarchy before POLY and widening checks were considered ("fuzzy lookup"). POLY was treated as a wildcard match.

This part was not investigated. Current matching works as a BFS (first class first, then super-class, and so on), when looking for a method. Beside this, the matching is pretty complete. 4GL might do a DFS search.

Are you planning to fix the method erasure where it can override only by parameter mode, not just type?

From #3751-353:

What I think is missing is the parent lookup:

• in ClassDefinition.exactMethodLookup - this will never match a parent method if the caller doesn't have all argument modes. We may need to look here without the arguments (the SignatureKey.hashCode includes the parameter modes, too).

Why do you say "We may need to look here without the arguments"? Do you have reason to believe that the exact matching should ignore modes?

Why does this relate to the parent lookup?

Are you planning to fix the method erasure where it can override only by parameter mode, not just type?

Yes.

Greg Shah wrote:

Why do you say "We may need to look here without the arguments"? Do you have reason to believe that the exact matching should ignore modes?

I think this was a typo and I meant 'modes'. But this is mostly a note that in 4GL, the parameter modes at the caller are not mandatory - so you can have a m1(i, j) call for a method void m1(output i as int, output j as int). Note how the signature definition has the modes. So exactMethodLookup may catch only few cases where the caller and method have the same signature exactly (including modes).

The modes are definitely used.

Why does this relate to the parent lookup?

Is not related only to parent lookup. I haven't fully investigated this, but it relates to how the method match is done, especially when fuzzy is in place: DFS or BFS. At compile-time, if the match can't be done uniquely, a compile error is shown. And the same rules will need to be applied for the dynamic-invoke case.

Constantin Asofiei wrote:

Did you find way to track if a field is changed (to unknown or some other ref)?

I started investigating the stored objects until it evolved to this:

CLASS C3758Bar:
   DESTRUCTOR PUBLIC C3758Bar():
      MESSAGE "Deleting" THIS-OBJECT. 
   END DESTRUCTOR.
END.

DEFINE VARIABLE anOnj AS CLASS Progress.Lang.Object. 
DEFINE VARIABLE secondOnj AS CLASS Progress.Lang.Object. 
anOnj = NEW C3758Bar().
secondOnj = NEW C3758Bar().

DEF TEMP-TABLE tt1 
   FIELD fobj AS Progress.Lang.Object.

CREATE tt1.
   fobj = anOnj.

RELEASE tt1.
anOnj = ?.
secondOnj = ?.

MESSAGE "Reference to initial object:" anOnj secondOnj.

FIND FIRST tt1.
MESSAGE "Found object:" fobj.

FIND FIRST tt1 WHERE fobj NE ? no-error.
IF AVAILABLE tt1 THEN DO:
    MESSAGE "Found non-null object:" fobj.
END.

FOR EACH tt1:
    DELETE tt1.
END. 

MESSAGE "EoP".

The output is the following:

Deleting C3758Bar_4309
Reference to initial object: ? ?
Found object: C3758Bar_4308
Found non-null object: C3758Bar_4308
EoP
Deleting C3758Bar_4308

• I added a message in the destructor just to know when it is called (and the object destroyed);
• the second object created (C3758Bar_4309) is dropped as soon as there are no direct references to it;
• at the first MESSAGE the references to C3758Bar_4308 are all kept in database;
• the deletion of all references to the object does not invokes the destructor of the object;
• de object is destructed after EoP is printed, most likely in the cleanup after the procedure (or maybe session) has finished.

As far as I can tell, if an object is stored in a TEMP-TABLE, it is not automatically discarded by gc when the last reference to it is lost (and the ref-counter decremented to 0). Instead, it is saved to some kind of data structure that is cleared after the procedure (or, more likely, the session) is over.

Ovidiu Maxiniuc wrote:

There are a few interesting notes:

• the second object created (C3758Bar_4309) is dropped as soon as there are no direct references to it;

Correct.

• at the first MESSAGE the references to C3758Bar_4308 are all kept in database;

OK.

• the deletion of all references to the object does not invokes the destructor of the object;

Not OK - I get the Deleting C3758Bar_4308 message before the EOP message, in both Linux and Windows.

• de object is destructed after EoP is printed, most likely in the cleanup after the procedure (or maybe session) has finished.

This is implicit table cleanup - which must trigger object reference decrement (and possible delete).

Sorry, my bad, the text capture was done with an object still referred by variable. I rerun the code and the destructor is called before of "EoP", as it should be.

Simple tests with 3750b rev 11319 are working for static temp-tables, static class initialization, static and instance method calls. I'm converting the app now, there might be conversion issues, as at least the static buffer references are not all solved.

Greg, for OO method calls performed directly from Java code we need to know the 'this' reference or the '.class' to which this method belongs - otherwise we can't know the context (i.e. THIS-OBJECT) under which this method is being executed. Also, currently only static temp-table scopes are moved to the static c'tor - but we need to do this for every other resource which has initialization code, it can't be in the 'execute' method, it needs to be in the static c'tor (which in turn acts like a 'external program' by itself).

Constantin Asofiei wrote:

Eric, is there an easy way to track a DMO property setter (when is being invoked, knowing also the old value) and when a record gets deleted? I need this for:

1. update the reference count for the old value (decrement) and new value (increment)
2. decrement the reference count when the record gets deleted

Sorry, I haven't been tracking this issue very closely and I missed this question.

1. When the setter is explicitly invoked by business logic, it goes through the RecordBuffer invocation handler. There already is a lot of code which is written to track changes to support undo processing. See RecordBuffer$Handler.invoke as a starting point, places where !isGetter is checked.
2. It is much trickier to know when a record is deleted. We optimize many types of bulk deletes (e.g., EMPTY-TEMP-TABLE, FOR EACH DELETE loops, temp-tables going out of scope) with SQL bulk DELETE or bulk UPDATE statements, and/or simply dropping the table, so there will not be a notification of an individual record being deleted in many cases. To de-optimize these would create an unacceptable hit to performance.

Ovidiu Maxiniuc wrote:

As far as I can tell, if an object is stored in a TEMP-TABLE, it is not automatically discarded by gc when the last reference to it is lost (and the ref-counter decremented to 0). Instead, it is saved to some kind of data structure that is cleared after the procedure (or, more likely, the session) is over.

How are you doing he reference counting for object references stored in temp-tables?

Eric Faulhaber wrote:

1. It is much trickier to know when a record is deleted. We optimize many types of bulk deletes (e.g., EMPTY-TEMP-TABLE, FOR EACH DELETE loops, temp-tables going out of scope) with SQL bulk DELETE or bulk UPDATE statements, and/or simply dropping the table, so there will not be a notification of an individual record being deleted in many cases. To de-optimize these would create an unacceptable hit to performance.

I was thinking about this, and the only way I see it (as I mentioned in note #3751-419), is to mark certain DMOs (which are temp and have at least a FIELD_CLASS) so that these deletes will have some preparation done before being executed (i.e. get all objects and 'dereference' them). I don't know how we can do it other way.

Eric Faulhaber wrote:

How are you doing he reference counting for object references stored in temp-tables?

It doesn't matter where the objects are stored, what matters is that they are referenced from a temp-table record - that's why we need to know when a FIELD_CLASS is assigned or a record is deleted. AFAIK we are not doing this yet, not sure the state of Ovidiu's work.

Are there side effects to an object being gc'd, such that the delete/gc has to happen at a very specific time/order, or is it just a matter of resource cleanup? In other words, can you run arbitrary business logic as part of the delete processing?

Eric Faulhaber wrote:

How are you doing he reference counting for object references stored in temp-tables?

We are not doing this. At least not yet. I had this short test procedure from note 427 which lead me to a wrong conclusion that the counter is delayed for 'persisted' objects. But I was tricked because of a piece of code I added because for the moment FWD cannot print object fields directly.

I don't think that this is possible (as you noted also, the bulk delete is the main cause here) without additional SQL code, hence a bit on extra performance penalty. For the bulk delete on temp-tables, we can query all the UUIDs that are about to be deleted and decrement the counter just before the delete SQL is called. I am working on that.

Eric Faulhaber wrote:

Are there side effects to an object being gc'd, such that the delete/gc has to happen at a very specific time/order,

The part where order might be required is when we convert to a bulk delete - here, we should iterate the records in the same order as the parent query.

or is it just a matter of resource cleanup?

Resource cleanup is performed (as the associated persistent program gets deleted).

In other words, can you run arbitrary business logic as part of the delete processing?

Good question. Yes, there is legacy DESTRUCTOR code which needs to be executed on delete.

Constantin Asofiei wrote:

Good question. Yes, there is legacy DESTRUCTOR code which needs to be executed on delete.

BTW, I see that the destructor is not yet called. Will it be called automatically when the counter reaches 0 (from ObjectResource.decrement())? I am counting on this in my new code.

Ovidiu Maxiniuc wrote:

BTW, I see that the destructor is not yet called. Will it be called automatically when the counter reaches 0 (from ObjectResource.decrement())? I am counting on this in my new code.

The DESTRUCTOR is called when the object is explicitly deleted (via DELETE OBJECT) or when the counter reaches 0. The call is performed in ObjectResource.resourceDelete.

Ovidiu Maxiniuc wrote:

I don't think that this is possible (as you noted also, the bulk delete is the main cause here) without additional SQL code, hence a bit on extra performance penalty. For the bulk delete on temp-tables, we can query all the UUIDs that are about to be deleted and decrement the counter just before the delete SQL is called. I am working on that.

In the end, I don't think we can allow bulk delete to be converted for temp-table with FIELD_CLASS - because the DESTRUCTOR code may raise i.e. a STOP and terminate the loop.

On implicit object delete, these conditions raised from DESTRUCTOR are always ignored - but not from business logic, they are raised back to the caller.