[releng] Fix 3.6M4 compilation warnings

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 298128 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Dec 17, 2009 17:30 EDT | | Modified | May 22, 2013 14:16 EDT | | Reporter | Ed Willink |

Description

3.6M4 gives us stronger generic argument validation and about 300 warnings.

Most just require a load of <?>'s.

However List UMLReflection.getConstrainedElements(CT constraint) is detected as unsound. The true return is List where N is ENamedElement or NamedElement; there is no N parameter, hence the EObject. Correcting the implementation to ? extends EObject breaks the API which allows callers to modifiy the list. The API was always broken but not detected. A new addConstrainedElement must be added to the API to make up for the loss of a modifiable return.

[eclipse-ocl-bot]

By Ed Willink on Dec 18, 2009 01:48

The trivial fixes are committed to CVS HEAD.

Adolfo: This unfortunately hits the parser grammars hard, and since many of the gratuitous loose CSTNode cst = anXxxCS threw away now required type-information, I've endeavoured to preserve the type by using XxxCS cst = anXxxCS. This should save work when LPG 2's support for typed CST nodes is exploited.

The nasty API change remains to be explored. Appears as just 2 warnings.

[eclipse-ocl-bot]

By Ed Willink on Dec 18, 2009 03:13

Sergei: for QVTd it was sufficient to just rerun LPG to pick up this change and see all the warnings go. The result is still 3.5 compatible.

However EMF 2.6M4 has added EFactory methods that break three org.eclipse.qvt.declarative.ecore.adapters classes, so you may want to defer building on M4; I'm working on it. QVTd pre and post M4 will not be compatible!

This will be true of MDT/OCL too as soon as the eInvoke that EMF is adding, in some respects on behalf of MDT/OCL, ripples through.

Henceforth, all MDT/OCL builds must use an M4 or greater EMF.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Dec 18, 2009 06:26

(In reply to comment #1)

The trivial fixes are committed to CVS HEAD.

Adolfo: This unfortunately hits the parser grammars hard, and since many of the gratuitous loose CSTNode cst = anXxxCS threw away now required type-information, I've endeavoured to preserve the type by using XxxCS cst = anXxxCS. This should save work when LPG 2's support for typed CST nodes is exploited.

The nasty API change remains to be explored. Appears as just 2 warnings.

I could understand, any fixing related to java 1.5 generics, but I don't see why CSTNode must be the specific XxxCS type. Is JDT prohibiting the OOP features ?. I don't simply understand >.<. Assuming that now I have to change all .gi grammars again to accomdate this, I don't feel happy with this -still-non-understandable- change.

I thought we had agreed not changing any-parser-related issue until the end of the next week, and assuming that narrowing the type of the created cst nodes were required by JDT, I think you could have lived with those warnings until then...

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Dec 18, 2009 08:30

Moreover, QVTo uses some custom launching infrastructure based on slightly modified clones of OCL Grammars, I would also need to change those cloned grammars. Is there any chance to revert this change ?.

If all is going fine with IP approval, we could have LPGv2 migration for OCL, QVTo and probably QVTc by the end of the next week. Then you could change grammars to remove these warnings, provoked by JDT in Eclipse 3.6M4.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Dec 18, 2009 08:48

Adolfo; I'm sorry if this inconveniences you. I wasn't sure if you'd actually\ started, since LPG 2 in Orbit was blocking you. Changing the .g files proved\ pretty easy once you done one, but we can redo the (*.g) edits if it's a problem.

The main CSTNode related problem occurred in:

CSTNode x = createX(...);\
listOfX.add(x);

so CSTNode had to be changed now that listOfX has to be List to satisfy the\ createY(listOfX) usage;

The $EmptyListAction is now deprecated, since new BasicEList() cannot be\ repaired sensibly.

The optional CSTNode related problem is in:

CSTNode x = createX(..);\
setResult(x);

which will become an error once LPG 2 propagates types.

I was steering clear of the parser, but I hadn't realised that JDT M4 as well\ as EMF M4 would break us. I hoped to minimise the exposure of incompetent CVS.\ Apologies again for the inconvenience.

Patch to fix the remaining two warnings still pending.

[eclipse-ocl-bot]

By Ed Willink on Dec 18, 2009 10:29

Created attachment 154780 Fix for API breaking 3.6M4 warnings

The incorrect UMLReflection.getConstrainedElements() return type of List could be changed to List by adding an additional CN generic to UMLReflection, this hits a lot of code hard and then requires an additional parameter to Environment which hits even more code even harder. Not kind to our clients.

A much more manageable change is to make the declaration what it actually is: a return type of List<? extends EObject>. The documented API that the returned list is changeable is still true (sort of); the difference is callers have to do the unsafe cast themselves to ensure that the type T of List.add(T) is true for ? extends EObject. Currently this unsafe cast is performed by UMLReflectionImpl and it is this lack of safety that M4 diagnoses.

In practice the new return type breaks all code that uses UMLReflection.getConstrainedElements(). Non-changing list access just needs a type change. Changing list access needs a new UMLReflection.addConstrainedElement() method whose implementation has a checked cast.

Changing the return type has very little impact. The tests are unaffected. QVTd is unaffected, QVTo has a trivial change of its derived QVTReflection. This is included in the patch.

:notepad_spiral: Bug298128.patch

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Dec 18, 2009 12:03

(In reply to comment #5)

Adolfo; I'm sorry if this inconveniences you. I wasn't sure if you'd actually started, since LPG 2 in Orbit was blocking you. Changing the .g files proved pretty easy once you done one, but we can redo the (*.g) edits if it's a problem.

Orbit blocks any chance for commitment, however I had already done the migration against the CVS current HEAD. I gave up the branch work, since the grammars were completely refactored/changed some weeks ago.

I'll align all the .gi grammars, for the OCL side. My current QVTo-side work, will keep the current cloned OCL grammars. Fortunately, Sergey could adopt the changes of this bug after migration is done.

I will need to run 3.6M4 IDE to figure out the problem. I'm still using Galileo as IDE, and the 3.6M4 installation as a configured target platform.

Regards,\ Adolfo.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Jan 25, 2010 08:40

Ed,

I think this bugzilla is already fixed, isn't it?

What about assigning yourself and resolving as fixed ?

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Jan 25, 2010 10:38

I think this bugzilla is already fixed, isn't it?

I think that your last patch has not been reviewed yet. I'll have a look it tomorrow.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Jan 26, 2010 05:26

Ed,

I'm still using Galileo as development IDE (Helios M4 as target platform), so I'm not really catching the warnings Helios M4 throws. I tend to switch to Helios M5 as development IDE for further development so, I'll postpone the reviewing of this bug to the next week when MDT-OCL M5 arrives.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

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

Ed,

After installling a new M5 installation and setting up a new workspace.

+1 to the patch. Just remember to merge some conflicts due to the delay of the review.

BTW. A metamodel question. Why in the Ecore version, the API of the Contraint.getConstrainedElement returns a list ENamedElement ?. Type casting from EObject to ENamedElement seems suspicious. We won't probably have any problem considering that as nobody has noticed it before, although I think that, at least EModelElement should have been type returned by the Contraint.getContrainedElement.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Feb 09, 2010 01:15

Fixed in CVS, verified in build #118.

Sergei: You need to apply the 6 line QVTo part of the patch.

Adolfo: You're right the Ecore constraints should be List to match UML's List. Since the list-of API is changing anyway, might as well change this too.

[eclipse-ocl-bot]

By Sergey Boyko on Feb 09, 2010 12:07

(In reply to comment #12)\ Hi Ed,

Thank you for the patch. Applied.

Cheers,\ Sergey

Fixed in CVS, verified in build #118.

Sergei: You need to apply the 6 line QVTo part of the patch.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2011 02:54

Closing