Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 262382 - EMF-generated model code can't be compiled if model contains class called "Container"
Summary: EMF-generated model code can't be compiled if model contains class called "Co...
Status: VERIFIED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: Core (show other bugs)
Version: 2.4.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Dave Steinberg CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 279074 288842 (view as bug list)
Depends on:
Blocks: 247980 410802
  Show dependency tree
 
Reported: 2009-01-26 10:38 EST by Alex Shatalin CLA
Modified: 2013-06-14 07:04 EDT (History)
5 users (show)

See Also:


Attachments
example of .ecore model used to generate java code producing errors (364 bytes, text/plain)
2009-01-26 10:38 EST, Alex Shatalin CLA
no flags Details
Patches to address the issue. (343.09 KB, patch)
2009-02-12 13:28 EST, Ed Merks CLA
no flags Details | Diff
Slight improvements to the patch (8.27 KB, patch)
2009-02-22 23:08 EST, Dave Steinberg CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Shatalin CLA 2009-01-26 10:38:51 EST
Created attachment 123753 [details]
example of .ecore model used to generate java code producing errors

- Create .ecore model
- Create new EClass with the name "Container"
- generate model code -> following compilation problems reported by java compiler:

1. The return type is incompatible with MainPackageFactory.createContainer()
2. Type mismatch: cannot convert from ContainerImpl to MinimalEObjectImpl.Container
Comment 1 Ed Merks CLA 2009-01-26 10:48:44 EST
Hmmm.  That's nasty. :-(

It's a reminder that all the things in the scope of the base class will be in scope for the derived classes.  So it seems I've picked quite a bad name. :-(

Probably I'll have to rename it to up an E in front and deprecate the one that's just created (which I'll then delete before the release).  Unless I get a more brilliant idea. E.g., one could specify a list of nested names that are in scope in the Root Extends Class.... Gross.
Comment 2 Dave Steinberg CLA 2009-02-09 17:40:45 EST
Gross, indeed.  I definitely don't think that's a good idea.  I think a slightly less gross idea would be to just hardcode the nested names for the classes we know about in GenBaseGeneratorAdapter.createImportManager().  Actually, that doesn't seem particularly worse than just using a less useful name. Both approaches seem fine. Neither does anything to help people using their own custom base class.
Comment 3 Ed Merks CLA 2009-02-12 13:28:54 EST
Created attachment 125549 [details]
Patches to address the issue.

Dave,

This approach isn't too gross...
Comment 4 Sven Efftinge CLA 2009-02-19 14:43:30 EST
We faced the same problem and the patch works fine.
Is someone going to commit it soon?
Comment 5 Sven Efftinge CLA 2009-02-19 14:52:07 EST
Sorry, I was too fast.
The patch is not working when applied against the HEAD, because the generated code references the AccessListener, which is not there. 
Comment 6 Ed Merks CLA 2009-02-19 15:20:23 EST
Yes, unfortunately I'm mixing up my patches and unfortunately time seems to disappear down a rat hole these days.  Hopefully Dave will have a chance to review it soon.
Comment 7 Dave Steinberg CLA 2009-02-20 14:58:23 EST
Ed,

I'm not sure I get it.  The patch seems to disallow an un-imported Container or Dynamic completely in the package and factory classes, regardless of the root extends class, while only selectively disallowing them in class interfaces and implementations.  So people who aren't even using MinimalEObjectImpl could very well be affected when they regen.  I thought we were going to try to minimize the disruption, so shouldn't we take the same care with packages and factories?  And, why would we need to worry about interfaces for classes at all?

Also, I don't get why we'd use the generated artifact name in the import, rather than the real, full class names (org.eclipse.emf.ecore.impl.MinimalEObjectImpl.Container and org.eclipse.emf.ecore.impl.MinimalEObjectImpl.Container.Dynamic). If ever a template were to refer to one of those MinimalEObjectImpl inner classes, the import manager won't behave properly unless the correct class name has been registered. I realize that's unlikely and we're mostly doing this to reserve the short name, but I don't see any reason to misuse the import manager.

Finally, wasn't part of the motivation behind doing this to add pseudo imports for the names of interfaces/classes imported/extended, so that the nested feature type thing would work? I don't see that being done.
Comment 8 Dave Steinberg CLA 2009-02-22 23:08:04 EST
Created attachment 126426 [details]
Slight improvements to the patch

Ed pointed out the obvious to me: that generated factories and packages *always* extend MinimalEObjectImpl.Container (via EFactoryImpl and EPackageImpl), not the root extends class for the model.

My points about these pseudo imports not being needed for interfaces and about using the real class names are valid, and I've fixed those in the attached patch.

I'm now convinced that we don't need to do anything for the names of interfaces/classes imported/extended. I think that other issue will work just fine as things are.
Comment 9 Dave Steinberg CLA 2009-02-22 23:09:19 EST
Oh, I meant to add that I've renamed in the new method to addClassPseudoImports(), since it's conceivable that we might one day need a similar method for some other class-level artifact.

Does this look good, Ed?
Comment 10 Ed Merks CLA 2009-02-23 13:28:39 EST
Dave,

It looks good.  You'll commit it?
Comment 11 Dave Steinberg CLA 2009-02-23 14:22:22 EST
Sure.
Comment 12 Dave Steinberg CLA 2009-02-23 14:30:37 EST
The fix is in CVS for EMF 2.5.
Comment 13 Dave Steinberg CLA 2009-03-17 15:40:54 EDT
Fix available in HEAD: 2.5.0.I200902241800.
Comment 14 Ed Merks CLA 2009-06-04 05:22:45 EDT
*** Bug 279074 has been marked as a duplicate of this bug. ***
Comment 15 Dave Steinberg CLA 2009-06-25 02:51:30 EDT
Fix available in HEAD: 2.5.0 (R200906151043).
Comment 16 Ed Merks CLA 2009-09-09 10:33:20 EDT
*** Bug 288842 has been marked as a duplicate of this bug. ***