eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[CS2AS] imports are not translated to the AS #1399

Closed eclipse-ocl-bot closed 2 hours ago

eclipse-ocl-bot commented 2 hours ago

| --- | --- | | Bugzilla Link | 450196 | | Status | RESOLVED FIXED | | Importance | P3 normal | | Reported | Nov 05, 2014 15:00 EDT | | Modified | Nov 08, 2014 12:43 EDT | | Version | 5.0.0 | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

If you have a Complete OCL document, the imports specified in the document are not reflected in the AS.

eclipse-ocl-bot commented 2 hours ago

By Adolfo Sanchez-Barbudo Herrera on Nov 05, 2014 15:26

branch asanchez/450196 has potential fix for the OCL25 branch. I can see now proper imports (referring corresponding namespaces) from a serialised .oclas document.

I'll do some more testing (including test cases) tomorrow. I'll also create a test case to add to the suite.

As additional comment, I'm wondering how the "concrete URI/pathname" can be traced back to the text given current AS of the Import (and providing that there might be different ways/uris to refer the same namespace).

P.S: I need this in master.

eclipse-ocl-bot commented 2 hours ago

By Ed Willink on Nov 06, 2014 03:52

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

If you have a Complete OCL document, the imports specified in the document are not reflected in the AS.

Import has evolved leading to asymmetries. There is now a Pivot Import class so asymmetries are now questionable.

BaseCSContainmentVisitor looks like a clear improvement.

CompleteOCLCSContainmentVisitor.visitImportCS

if ((newNamespace != oldNamespace) && ((newNamespace == null) || !newNamespace.equals(oldNamespace))) {

seems wrong

if ((newNamespace != oldNamespace) || ((newNamespace != null) && !newNamespace.equals(oldNamespace))) {

surely?

The historic BaseCSContainmentVisitor.visitImportCS override is an asymmetry.

It seems that all the CompleteOCLCSContainmentVisitor.visitImportCS code can be promoted to BaseCSContainmentVisitor.visitImportCS.


Currently setImportedNamespace occurs in BaseCSContainmentVisitor.visitRootPackageCS and so your real problem was that CompleteOCLCSContainmentVisitor.visitCompleteOCLDocumentCS did not provide similar functionality. Another historic anomally. Difficult to know whether introduction of Pivot makes things regular. My changes on ewillink/450196 suggest that further clean up is possible. You might want to think about whether the earlier import is a good idea since the recursive Xtext parsing occurs earlier in a more nested scope. Does it create a problem with cyclic includes/forward references?

Overall: Import used to be complicated, I hope it is now simple as your improvement indicates.

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

I'll do some more testing (including test cases) tomorrow. I'll also create a test case to add to the suite.

ImportTests demonstrates similar load capabilites with small inline test files.

As additional comment, I'm wondering how the "concrete URI/pathname" can be traced back to the text given current AS of the Import (and providing that there might be different ways/uris to refer the same namespace).

ElementUtil.getCsElement should find the URIPathElementCS from which NodeModelUtils will give you the source text.\

P.S: I need this in master.

Looks like a very simple change that should backport easily,

eclipse-ocl-bot commented 2 hours ago

By Adolfo Sanchez-Barbudo Herrera on Nov 06, 2014 06:58

I clearly messed up with either Git Staging view or a Commit Squash I did, because I had tided up that code. I had originally taken the code from the refreshName method, which takes into account that the new value might be null (when the previous one wasn't). However, that was for String valued property, and it might be further simplified since the "new namespace" is checked earlier. The simplification is properly pushed in my branch now.

I agree with your enhancement about the asymmetry. QVTd should also benefit from the fix, by promoting that code to Base.

I've also created a new branch asanchez/450196b for master, which includes a new test case for the bug. No problems with xtext (Plugin and Standalone) test cases. I've just also kicked off the branch tests to verify (long time I didn't do that :P)

P.S: I don't know what you mean with "earlier import".

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 hours ago

By Ed Willink on Nov 06, 2014 07:19

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

P.S: I don't know what you mean with "earlier import".

The old 'schedule' was

Start containment pass

The new 'schedule' is

Start containment pass

eclipse-ocl-bot commented 2 hours ago

By Ed Willink on Nov 06, 2014 07:22

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

I clearly messed up with either Git Staging view or a Commit Squash I did, because I had tided up that code.

Easy done. I'm told that even though the GIT commit history may indicate that something is fatally lost, the Git Reflog view retains the detailed history and can be used to recover 'lost' content.

eclipse-ocl-bot commented 2 hours ago

By Ed Willink on Nov 08, 2014 08:06

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

I've also created a new branch asanchez/450196b for master

Unfortunately this breaks QVTc/QVTr for which there is no official 'import' statement and so no Import class in the AS. This is a pain since the Eclipse QVT import statements have to be synthesized during AS2CS and they do not exactly match the originals in a round-trip test without a little fudging. It is these fudges that fall apart.

Since this change is cleaner, probably need to fix up QVTd instead.

eclipse-ocl-bot commented 2 hours ago

By Ed Willink on Nov 08, 2014 12:43

(In reply to Ed Willink from comment #6)

Since this change is cleaner, probably need to fix up QVTd instead.

Too hard. QVT has a Unit class that conflicts with Import.

Rework so that the enhanced imports copy is only used by Complete OCL. No QVT chnages needed.

commit 0d5d763921811e241cb31f2ea10e33d29b7aa5df

pushed to master for M3.