search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[CodeGen] Unable to generate java code from recursive operations #1448

Closed eclipse-ocl-bot closed 2 weeks ago

eclipse-ocl-bot commented 2 weeks ago

| --- | --- | | Bugzilla Link | 459225 | | Status | RESOLVED FIXED | | Importance | P3 normal | | Reported | Feb 05, 2015 09:27 EDT | | Modified | Feb 17, 2015 10:21 EDT | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

I've found an scenario which prevents the Java source code generator to properly work when defining operations which can't be inlined. For example, a recursive operation.

I'll attach a test case which demonstrates the issue

[NB, this seems an OCL2Java CodeGen issue but since the test case is based on QVTi transformation scenario, I'm reporting in the QVTd project]

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 05, 2015 09:41

I've recently disabled inlining for library operations since Boolean::anbd could go exponential. - Bug 458774

You might try just disabling inlining for non-library operations too, but you might hit a gotcha that there is no class file in which to synthesize the non-inline operation.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 05, 2015 10:12

Hi

[Branch asanchez/459225, with demonstrating test case, pushed]

CodeGenAnalysis is not (sensibly) in-lining in this scenario because the operation is recursive. Inlined ContrainedOperations work, non-inlined ConstrainedOperations don't work.

I presume, that the generator is erroneously considering that when in-lining is enabled, an in-lined expression will always arrive, which is not true when recursion might happen. While debugging the test cases, I noticed how a CGExecutorOperation is being used as the initExpression of the CGLetExp, which doesn't make sense.

I don't know how the generator largely works, but I assume that the operation must be generated somewhere, probably in the own transformation class, and receiving the context, on which the operation has been defined, as a parameter.

I'll be spending the day on this.

Regards,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 05, 2015 10:56

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #2)

CodeGenAnalysis is not (sensibly) in-lining in this scenario because the operation is recursive. Inlined ContrainedOperations work, non-inlined ConstrainedOperations don't work.

I think that's the gotcha. If we have an OCL body we don't need to generate the operation, which is very convenient when the Java Class may have been genmodeled somewhere else.

The constrained operation must be generated as a static method of some spurious host class with an extra 'self' first argument.

Your case may be simpler since you may be generating the self class and so can host it normally.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 05, 2015 13:46

Design-wise is not ideal, but the asanchez/459225 (ocl and QVTd) contains a fix/workaround. If you have any other idea/suggestion you are welcome.

The test case doesn't pass yet, because an unwanted @Override is being generated. I believe CGOperation should contain a flag to dictate if the label is generated or not, rather than encoded in the CG2Java. If you agree on that I could complete the patch tomorrow.

I also want to use the operationId as the name of the operation which is being generated in the class (to reduce the chance of clash naming).

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 06, 2015 03:24

In principle the approach is probably right, but...

I don't see why you maintain a stack of both classes and operations. Surely only the operation call history is important?

I don't see why you have the stack in the CodeGenAnalyzer, where the dead stack remains throughout subsequent CG2Java activities that have no need of it; it should be empty.

If you look in the AS2CGVisitor, you will find a Stack : "a stack that is pushed when inline operations are exapanded". This sounds like exactly your problem. I see no push/pop for the variablesStack, so I think it is this missing mechanism you need. A new set of variables for each inline operation call.

So I suggest a push/popOperation method that pushes a new Variables, with an Operation field. (I prefer "push"/"pop" to "add"/"remove" for necessarily matched methods.)

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #4)

I believe CGOperation should contain a flag to dictate if the label is generated or not, rather than encoded in the CG2Java.

What do you mean by "label"? Where is the CG2Java hard coding?

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 06, 2015 05:25

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #4)

I believe CGOperation should contain a flag to dictate if the label is generated or not, rather than encoded in the CG2Java.

What do you mean by "label"? Where is the CG2Java hard coding?

Sorry, I meant (java) annotation. CG2JavaVisitor line 177

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 06, 2015 05:49

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #6)

What do you mean by "label"? Where is the CG2Java hard coding? Sorry, I meant (java) annotation. CG2JavaVisitor line 177

Line 177 is appendAtOverride that has some logic attempting to be intelligent.

A flag might fudge the problem in the short term.

In the long term the problem might go away as a built-in algorithm of a Java model to Java text serializer.

More likely we need something like the FinalAnalysis to accurately compute @Overrides.

Your additional code in visitOperationCallExp() uses getName(). This is very undesirable since names are subject to all sorts of re-use problems. It should be possible to use the Operation or perhaps CGOperation..

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 06, 2015 06:08

(In reply to comment #5)

In principle the approach is probably right, but...

I don't see why you maintain a stack of both classes and operations. Surely only the operation call history is important?

No. It needs to be a stack of operations. In principle, when calling an operation which can't be inlined I have to create the operation.

Prior to create a CGOperation I do a check by name so that I don't create the same operation twice. However, when the operation is being created, and thefore its body is visited, it might call to the same operation again (when the CGOperation has not been created at all, or at least added to the corresponding CGClass). The history of visited Operations helps to prevent an infinite loop.

For CGClass, in principle only one "active" (visited) CGClass is necessary for this particular problem (to obtain the CGClass in which create the created CGOperations). But then, I thought that a similar structure might be needed in the future (a history of classes which are being visited), and still valid to provide the "active" CGClass (the top of the stack). It also prevents me to hold the old "active"(visited) CGClass when the visitClass occurs, so that the previous value is restored when finishing visitClass.

I don't see why you have the stack in the CodeGenAnalyzer, where the dead stack remains throughout subsequent CG2Java activities that have no need of it; it should be empty.

CodeGenAnalyzer is the visitor context, in which I tend to include contextual info. This is contextual info for the visitor. Since I'm not sure about scenarios in which this history of visited Classes/Operations might be useful to pass through different visitor instances, I'll move it to the own visitor as private fields.

If you look in the AS2CGVisitor, you will find a Stack : "a stack that is pushed when inline operations are exapanded". This sounds like exactly your problem. I see no push/pop for the variablesStack, so I think it is this missing mechanism you need. A new set of variables for each inline operation call.

So I suggest a push/popOperation method that pushes a new Variables, with an Operation field. (I prefer "push"/"pop" to "add"/"remove" for necessarily matched methods.)

I think that you have misunderstood the problem/fix, because I don't understand how inlines / variables need to be involved here. I could create a similar class for Operation/Class but I don't see any benefit for it.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 06, 2015 06:17

(In reply to comment #7)

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #6)

What do you mean by "label"? Where is the CG2Java hard coding? Sorry, I meant (java) annotation. CG2JavaVisitor line 177

Line 177 is appendAtOverride that has some logic attempting to be intelligent.

A flag might fudge the problem in the short term.

In the long term the problem might go away as a built-in algorithm of a Java model to Java text serializer.

More likely we need something like the FinalAnalysis to accurately compute @Overrides.

That FinalyAnalysis would probably need that flag anyway. So, for the time being, I'd try to move that logic to CGOperation::override flag, so we act somehow without the need of customizing the CG2Java to handle the overrides logic.

Your additional code in visitOperationCallExp() uses getName(). This is very undesirable since names are subject to all sorts of re-use problems. It should be possible to use the Operation or perhaps CGOperation..

I had in mind setting operationId as the name of the CGOperation to reduce the chances of name clashing, but for this specific case checking the Operation as you suggest sounds more convenient. The check is to see if we need to create the CGOperation or not, so CGOperation that can't be used at that point.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 06, 2015 06:43

Some suggestions pushed to asanchez/459225

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 06, 2015 10:19

(In reply to comment #10)

Some suggestions pushed to asanchez/459225

I meant some of your suggestions. The problem in the test case with respect the @Override can be workaround with a better analysis in the CGOperation creation (AS2CG::visitOperation): ConstrainedOperations are not considered, and by default a CGLibraryOperation is created, even though for user defined operations. That's not right.

The discussion of introducing an "overrides" boolean attribute for CGOperation can be moved to another bugzilla.

Let's see if I can produce some working code for the test case. Now I'm studying another problem: the NativeOperationCallExp is being replaced by a "Throw invalid" in some of the intermediate analysis/rewrites (after CGCreation and prior to Java code generation).

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 08, 2015 05:57

Let's see if I can produce some working code for the test case. Now I'm studying another problem: the NativeOperationCallExp is being replaced by a "Throw invalid" in some of the intermediate analysis/rewrites (after CGCreation and prior to Java code generation).

The problem was that as soon as the source is part of the arguments, the OpCallExp didn't have any source and at some point the whole OpCallExp was generated as a simply throw invalid. I was revolving on that, even daring to deal with CGModel, by creating a CGThisExp, tuning CGValuedModelSpec, etc. I finally discarded everything and used a "CGTextParameter" (with a "this" text value), which seems to generates what I want: "this."

[If you have any other suggestion, let me know]

The problem now, is that CG expects to have the same number of CGValuedElement arguments as the number of the Operation parameters. Adding an extra contextual parameter will never work giving current design. Creating a new CGContextualOperation, to give support to that is not desirable giving current CG2Java design.

I'm inclined to include an optional contextualArgument containment reference at CGNativeOperation. No idea if it made sense in other CGOperation specializations, so let me know if you have any other suggestion.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 08, 2015 06:06

(In reply to comment #12)\ Sorry. In my last comment, when I said CGNativeOperation/CGOperation, I meant CGNativeOperationCallExp/CGOperationCallExp

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 08, 2015 07:02

Hi Ed,

current asanchez/459225 (OCL and QVTd) makes the test case pass with the aforementioned solutions. Additional comments:

Cheers,\ Adolfo.

[1] commit 1069f3ab35dc459d84dff220bd8f6232397408e3

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 17, 2015 08:20

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #14)

  • Regenerating CG models gives me a lot of changes

Ed M changed the comment templates and messed up the

's, so we have had one all-change to double

's and now another all change back to what we used to have.

The major issue is the need for an alternative CGNativeOperation API to accommodate the independence of Java's this from OCL's self. This does not require an additional contextualArgument; just a thisIsSelf boolean to indicate with self, the cgSource, is Java's this or an additional first argument.

Ensuring that the CGNativeOperation has a host class requires at least a class 'stack', with at most one entry. Ensuring that recursion is handled requires an operation call stack.

OCL change pushed to master for M6.

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 17, 2015 09:07

(In reply to Ed Willink from comment #15)

OCL change pushed to master for M6.

1 wierd test falure to resolve. Not reproducible locally. May be UML2 has 'improved'.

QVTd change pushed to master for M6.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 17, 2015 09:35

(In reply to Ed Willink from comment #16)

(In reply to Ed Willink from comment #15)

OCL change pushed to master for M6.

1 wierd test falure to resolve. Not reproducible locally. May be UML2 has 'improved'.

QVTd change pushed to master for M6.

Hi,

Thanks for the pushes.

Hopefully, there won't appear a CG scenario in which classes are transitively analysed (e.g. through superClass reference or whatever). If it happens, impl

One last comment about the pushed code: 'operation call stack' is misleading/confusion. Actually you are stacking operations rather than operation calls. We want to remember operations which have been visited or if you prefer 'called operation stack'.

Regards,\ Adolfo.

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 17, 2015 09:54

(In reply to Ed Willink from comment #16)

1 wierd test falure to resolve. Not reproducible locally. May be UML2 has 'improved'.

No change in UML2, trivial change in EMF, more in Xtext. Bug 460122 raised to track clues/history.

eclipse-ocl-bot commented 2 weeks ago

By Ed Willink on Feb 17, 2015 09:56

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #17)

One last comment about the pushed code: 'operation call stack' is misleading/confusion. Actually you are stacking operations rather than operation calls. We want to remember operations which have been visited or if you prefer 'called operation stack'.

I don't understand the difference.

The stack tracks AS2CG visitOperationCallExp invocations.

eclipse-ocl-bot commented 2 weeks ago

By Adolfo Sanchez-Barbudo Herrera on Feb 17, 2015 10:21

(In reply to Ed Willink from comment #19)

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #17)

One last comment about the pushed code: 'operation call stack' is misleading/confusion. Actually you are stacking operations rather than operation calls. We want to remember operations which have been visited or if you prefer 'called operation stack'.

I don't understand the difference.

The stack tracks AS2CG visitOperationCallExp invocations.

I'll try to explain comment 8 in a different way.

Many different OperationCallExp can call the same Operation. You are tracking the called operation:

protected void pushOperationCall(@NonNull Operation asOperation) {\ operationCallStack.push(asOperation);\ }

You want to track the called operation to avoid generating the same CGOperation more than once. And regardless the many different OperationCallExp which call the same Operation, you will generate the "same many" CGNativeOperationCalls.

Therefore, you/we are tracking "called operations" rather than "operation calls".

Even better since you are tracking the Operation rather than the CGOperation, I'm also thinking that if you want to indeed track "called operation", you might also want to consider moving the push/pop calls when visiting the operation call (rather than in the visitOperation), i.e. AS2CGVisitor line 548 :

if (!containsOp) {\ pushCalledOperation(finalOperation)\ CGOperation cgOp = visitOperation(finalOperation);\ popCalledOperation(finalOperation)\ currentClass.getOperations().add(cgOp);\ }

This would also avoid having to change the visitFunction in QVTd. The same algorithm should work regardless the OperationCallExp refers to an OCL::Operation or QVT::Function

Regards,\ Adolfo.