Bug 371115

Comment 1 Remy Suen 2012-02-09 13:11:09 EST

This was implemented for bug 302533.

Oleg?

Comment 2 Lars Vogel 2012-02-09 13:28:17 EST

I think the currently behavior is really bad, for example according to the Java Beans spec you have to have a non-arg Constructor.

Comment 3 Brian de Alwis 2012-02-09 14:32:24 EST

(In reply to comment #2)
> I think the currently behavior is really bad, for example according to the Java
> Beans spec you have to have a non-arg Constructor.

And the problem for Java Beans is… ?

Comment 4 Lars Vogel 2012-02-09 14:37:39 EST

They are used frequently as some frameworks require them, hence the auto-gen stuff will be frequently create objects of this kind in case they are missing in the context.. And I assume this will lead to unexpected and hard to find errors.

Comment 5 Brian de Alwis 2012-02-09 15:19:23 EST

I meant — is there a problem with Java Beans being auto-generated?  Or is it a question of surprise?

I too have been surprised by this behaviour — I spent hours trying to track down an issue that occurred because I had an injectable method with a Shell argument.  But I think this issue is better addressed through education and documentation.

(Novices are surprised by all sorts of things, e.g., shadowing fields in subclasses, method overriding, classpath differences between Java projects and plugin projects.)

I think the better approach is to add some output code to the injector to explain what's happening as part of the injection.  We could have a debug annotation on the class, like @org.eclipse.e4.di.debug.Explain?  It could output when objects are created, or where objects have been obtained from (e.g., context for MPart, obtained through OSGi service, etc.).

Comment 6 Lars Vogel 2012-02-09 17:45:07 EST

Input from a Spring person: Spring only create automatic instances of objects if they are explicitly marked as components, e.g. a non-arg Bean must be declared as component otherwise the Spring DI  would not create an instance of it.

Comment 7 Lars Vogel 2012-02-09 17:47:49 EST

@Brian: the problem is IMHO an inconsistent behavior. Java objects would be auto-generated depending on their constructor and other standards my define that they have to have non-arg constructors. It also seems to be inconsistent with the Spring DI framework.

I think the suggestion from Paul is great to make this opt-in in case this is required.

Comment 8 Thomas Schindl 2012-02-10 04:49:49 EST

I have to agree with Lars and that this auto creation of instances is confusing take this example of a completely bogus injection:

--------8<--------
public class MyTest {
  private Composite comp;

  public MyTest(Composite comp) {
    this.comp = comp;    
  }

  public void run() {
    System.err.println(comp);
  }
}


IEclipseContext ctx = EclipseContextFactory.getServiceContext(Activator.getDefault().getContext());
MyTest t = ContextInjectionFactory.make(WorkbenchLogger.class, ctx);
t.run(); // will fail with an SWT-Exception
--------8<--------

The composite instance created here is only created because the Composite has a package scoped no-arg constructor which clearly isn't meant to be called:

--------8<--------
/**
 * Prevents uninitialized instances from being created outside the package.
 */
Composite () {
}
--------8<--------

I can understand (though still question it) if one creates Bean-Instances who have a *public* constructor but not in any other case. 

In this case the user code will always fail but in other cases (because we are calling none API stuff) code that worked in version 1.0 might not work in 1.1 because the library vendor removed a *none* API thing rendering API guidelines useless.

Comment 9 Brian de Alwis 2012-02-10 12:15:25 EST

It's a feature that's been found useful in other injectors [1], I can imagine some scenarios where it could be useful, I'd be loathe to remove it.  

Having a single switch to enable or disable autogen behaviour is not good as it means that code that has been tested in one mode only will cease to function.

I'd prefer to have some a configuration option to disallow auto-gen on a per-class basis.  So we could disable the autogen for Composite and Shell, for example.  (This wouldn't avoid Lars' issue in 371114 though.)  Or add a service that throws an error.


[1] For exampleL "This will seem disconcerting at first, but after a while you'll get used to it."  http://www.ibm.com/developerworks/java/library/j-guice/index.html#N101B6

Comment 10 Brian de Alwis 2012-02-10 12:16:55 EST

(In reply to comment #8)
> The composite instance created here is only created because the Composite has a
> package scoped no-arg constructor which clearly isn't meant to be called:

This may be a bug — Guice only calls public no-arg constructors.

Comment 11 Lars Vogel 2012-02-10 13:16:10 EST

I just learned from Brian that auto-gen also works for class with arg-constructors if they are annotated with @Inject. 

And that this also work recursively, e.g. a class would be created with injects another class which defines its dependency via @Inject.

Now that is cool! 

Under this information I suggest that we turn of auto-gen in default, but create a new annotation, e.g. @Component (Spring uses this one) and if a class is marked with @Component we auto-gen it.

This would be very similar to Spring and extremely useful in implementations.

Comment 12 Lars Vogel 2012-02-10 13:19:40 EST

Changed title of this bug, as auto-gen works also for class with args-constructors.

Comment 13 Oleg Besedin 2012-02-10 16:16:17 EST

Personally, I always felt uneasy about auto-creation, see bug 302533 comment 2.

Since that time I saw two related bugs in the SDK code: a [bogus] value was auto-created in one case masking the real problem and in another case causing unexpected behavior.

- On the visibility of constructors: generally we use only public and package visibility constructors. I'll check if this is correct for auto-created objects. If not, that is likely a bug.

- On using annotation to describe auto-create classes.
This looks interesting and might be a good compromise between two view points on the auto-creation. There are couple details that need to be ironed here:
a) should the annotation apply to the class or to a constructor?
b) annotation name - for people not familiar with the Spring framework, "@Component" name won't make much sense. How about "@Instanciate" or "@CreateAsNeeded" ?

Comment 14 Lars Vogel 2012-02-12 12:03:57 EST

@Oleg, the JSR 330 uses @Named to define a component.

@javax.inject.Named
public class ServiceImpl implements Service
{
...
}

I think we should follow this standard, this also makes it easier for us, as the @Named annotation is already used. We would also be compliant with other standard DI container and also Spring supports this.

Some links to the topic:
http://matthiaswessendorf.wordpress.com/2010/04/20/spring-3-0-and-jsr-330-part-2/
http://jcp.org/en/jsr/detail?id=330

Btw. the @Qualifier annotation also looks nice, we could use that to inject OSGi services based on properties, e.g. service.ranking or other properties.

Comment 15 Paul Webster 2012-02-13 08:23:09 EST

So you have to pre-determine that the class can be instantiated by DI?  You don't get to choose in your consumer (I know that if I get one from DI it'll be fine)?

PW

Comment 16 Lars Vogel 2012-02-13 08:40:24 EST

@Paul, I not sure if I understand your question correctly. 

I think components marked with @Named should be available for DI and the consumer of the components should not be aware how it is constructed. I think that is similar to the current approach; the consumer does not know how the object was added to the context or if it was an OSGi service.

The way I see this, if you mark a component as component it is similar to defining an OSGi service. You define your intent that it should get injected. 

IMHO the platform should not use this for  Composite and the like but rather follow the current approach of explicitly adding these object to the context. 

Did this answer your question?

Comment 17 Paul Webster 2012-02-13 09:05:13 EST

(In reply to comment #16)
> Did this answer your question?

Sorta.  We're saying we'll only auto-instantiate components if the component declares it has a useful no-arg constructor.

PW

Comment 18 Lars Vogel 2012-02-13 09:13:58 EST

@Paul: The non-arg constructor is not a limitation. We can also create constructors with arguments if the constructor is annotated with @Inject. This is also how it works right now (but for all classes).

Example: Lets define the following components:

@Named public class Test {


@Inject 
 public Test(IApplicationContext context, MyOSGIService service, Todo todo){

}
}

@Named public Todo {
// Default non-arg constructor
}


Now in my model object I would write @Inject Test and the DI would construct an instance of the Test class, fulfilling its requirements.

That is how other DI container also work and I think that is a killer feature for DI in Eclipse 4.

Brian explained to me that this is already how Eclipse DI works today, expect that here is no explicit declaration of the components which I think is important to avoid undesired side effects.

Comment 19 Oleg Besedin 2012-02-13 09:30:26 EST

(In reply to comment #15)
> So you have to pre-determine that the class can be instantiated by DI?  You
> don't get to choose in your consumer (I know that if I get one from DI it'll be
> fine)?
> 
> PW

No, no, this bug is not about "normal" code paths where we explicitly create or inject an object (#make or #inject).

This is about creating an object "behind the scene" if it is not in the context. 

For instance, if we create a Part (#make(MyPart.class)) and its only constructor says it wants an ABC object injected (@Inject MyPart(ABC myABC)), and ABC is not in the context, then injector will try to create ABC behind the scene (and put it in the context).

Comment 20 Oleg Besedin 2012-02-13 09:48:21 EST

(In reply to comment #14)
> @Oleg, the JSR 330 uses @Named to define a component.
> 
> @javax.inject.Named
> public class ServiceImpl implements Service
> {
> ...
> }
> 
> I think we should follow this standard, this also makes it easier for us, as
> the @Named annotation is already used. We would also be compliant with other
> standard DI container and also Spring supports this.
> 
> Some links to the topic:
> http://matthiaswessendorf.wordpress.com/2010/04/20/spring-3-0-and-jsr-330-part-2/
> http://jcp.org/en/jsr/detail?id=330
> 
> Btw. the @Qualifier annotation also looks nice, we could use that to inject
> OSGi services based on properties, e.g. service.ranking or other properties.

This is not correct. Spring uses "@Named" annotation in a special way, not defined by the JSR. For instance, see:

https://jira.springsource.org/browse/SPR-6465

"This is of course not covered in the JSR-330 spec at all but rather our best effort try to find a JSR-330 equivalent of @Component."

=====

We have several annotations in the "org.eclipse.e4.core.di.annotations" package such as "@Optional" and "@Execute" (they derive from "@Qualifier").

I think I'll add something like "@Create" to the same place and change injector implementation to only auto-create objects that have this annotation.

Later we could add an optional string argument to "@Create" if we want to get creative with different creation modes.

Unless anybody objects?

Comment 21 Lars Vogel 2012-02-13 09:55:40 EST

@Oleg,

The @Create annotation would be fine too, but I don't see harm is doing it with @Named. Feels like it would be closer to the standard, but I might be wrong here. 

Having with autowire capabilities in Eclipse 4 would be great in general, no matter which annotation we start using to indicate components.

Comment 22 Brian de Alwis 2012-02-13 10:24:43 EST

(In reply to comment #19)
> For instance, if we create a Part (#make(MyPart.class)) and its only
> constructor says it wants an ABC object injected (@Inject MyPart(ABC myABC)),
> and ABC is not in the context, then injector will try to create ABC behind the
> scene (and put it in the context).

I hadn't realized  that the object was placed in the context.  So it's effectively a singleton.  Interesting.

(In reply to comment #20)
> I think I'll add something like "@Create" to the same place and change injector
> implementation to only auto-create objects that have this annotation.

I'd kind of prefer an adjective like @Creatable, since it's a description of the 

It would be great if we then could support configuring an annotation for after-the-fact uses, like:

   InjectorFactory.getDefault().bind(MyCreatelessClass.class).annotateWith(Create.class);

Though this could wait until someone requested it.

Comment 23 Lars Vogel 2012-02-13 10:30:17 EST

@Oleg: Would it be possible to avoid putting the newly generated object into the Context. I think this auto-gen is more useful if it does not generate Singletons.

@Brian: Would you prefer Singletons? I think this limits the usage of auto-gen.

Comment 24 Brian de Alwis 2012-02-13 10:37:37 EST

(In reply to comment #23)
> @Oleg: Would it be possible to avoid putting the newly generated object into
> the Context. I think this auto-gen is more useful if it does not generate
> Singletons.
> 
> @Brian: Would you prefer Singletons? I think this limits the usage of auto-gen.

No, I wouldn't prefer Singletons.  But they're only singletons within that particular context (e.g., for that part).  Another part would have a different injected instance.

Comment 25 Lars Vogel 2012-02-13 10:55:26 EST

Thanks Brian, I also would prefer not to have Singletons, if possible.

I think it is a good model to think that everything in the context is explicitly added to it. On the top side we have dynamic OSGi services and with this auto-gen we also would have dynamic components also on top of the context hierarchy.

I think that would be also result in a consistent search for components.

Context
 - Context 
   - Context
    - OSGi 
      - Auto-gen

If the new component is not placed in the local context then the search would be deterministic, e.g. every search would follow the same hierarchy and ends with the same result. In this case it would be idempotent.

If the first search ends up putting the auto-gen into the local context, then the second for a component would not be idempotent.

Comment 26 Brian de Alwis 2012-02-13 13:44:04 EST

(In reply to comment #21 from Lars)
> The @Create annotation would be fine too, but I don't see harm is doing it with
> @Named. Feels like it would be closer to the standard, but I might be wrong
> here. 

If it's not described in JSR-330, then I wouldn't do it — it opens us up to problems if it's extended in a different way in the future.

(In reply to comment #25 from Lars)
> I think it is a good model to think that everything in the context is
> explicitly added to it.

I don't think of it in that way: the context reflects whatever is available.  OSGi services & context functions aren't explicitly added to the context, but they are available from the OSGi context.

And the order is, I believe:

OSGi 
E4Application
MApplication
MWindow
MPerspective
MPart (w/Auto-gen)

> If the new component is not placed in the local context then the search would
> be deterministic, e.g. every search would follow the same hierarchy and ends
> with the same result. In this case it would be idempotent.
> 
> If the first search ends up putting the auto-gen into the local context, then
> the second for a component would not be idempotent.

They'd be idempotent providing the searches are from the same leaf context.

Comment 27 Lars Vogel 2012-02-14 04:54:26 EST

@Brian: I initial thought that a second call might return an incorrect result, e.g. by overriding another object in the context. But I was unsuccessful in finding a valid example, as in example from above if an object would already existing, di would find it and would not autogen it.

So, I guess adding the object to the context wouldn't do any harm.

Comment 28 Oleg Besedin 2012-02-16 11:42:07 EST

I added new annotation org.eclipse.e4.core.di.annotations.Creatable.

Only classes that specify this annotation will be auto-constructed by the injector.

At this time we do not place auto-constructed classes in the injector, but that might change in future if there is a good argument one way or another.

Git reference:

http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=aaf521c0b09d327997a36c2a7cb36ad41d879301

Comment 29 Lars Vogel 2012-02-16 11:55:45 EST

Thanks Oleg, these are good news.

One clarification question: What does this mean: we do not place auto-constructed classes in the injector

Do you mean:  we do not place auto-constructed classes in the Context?

Comment 30 Remy Suen 2012-02-23 14:57:20 EST

(In reply to comment #28)
> http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=aaf521c0b09d327997a36c2a7cb36ad41d879301

"Specifies that the target class that can be created by an injector as needed."

Please delete the second "that" in Creatable's javadoc. It is unnecessary.

Comment 31 Oleg Besedin 2012-02-23 15:27:11 EST

(In reply to comment #30)
> (In reply to comment #28)
> > http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=aaf521c0b09d327997a36c2a7cb36ad41d879301
> 
> "Specifies that the target class that can be created by an injector as needed."
> 
> Please delete the second "that" in Creatable's javadoc. It is unnecessary.

Done, thanks!

http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=415730bef94c5160d7bce6c682ba1267055f0c7d

Comment 32 Sven Efftinge 2012-02-29 09:29:15 EST

This change will prevent us from using third party classes written against plain JSR-330 since they won't be annotated with @Creatable. This kind of compatibility was the idea behind JSR-330.

We really should do it like in JSR 330. The JavaDoc for @Inject states :

Injectable constructors are annotated with @Inject and accept zero or more dependencies as arguments. @Inject can apply to at most one constructor per class.

@Inject is optional for public, no-argument constructors when no other constructors are present. This enables injectors to invoke default constructors.

So Tom's example is of course invalid.

With regards to whether the context should create a fresh instance for each dependency we should consider @Singleton.
It's JavaDoc states:

Identifies a type that the injector only instantiates once. Not inherited.

Comment 33 Paul Webster 2012-02-29 09:33:08 EST

for investigation.

PW

Comment 34 Oleg Besedin 2012-02-29 09:33:51 EST

(In reply to comment #32)
> This change will prevent us from using third party classes written against
> plain JSR-330 since they won't be annotated with @Creatable. This kind of
> compatibility was the idea behind JSR-330.

This is not correct. The "@Creatable" only applies to objects not known to the
injector. The explicit injection requests work as before and comply to the JSR,
see comment 19.

Comment 35 Lars Vogel 2012-02-29 09:35:39 EST

@Sven: Spring uses the @Named annotation to identify injectable components. See Comment 20 from Oleg, where he describes that the JSR330 does not define a standard for defining components.

Comment 36 Sven Efftinge 2012-02-29 09:51:33 EST

(In reply to comment #34)
> (In reply to comment #32)
> > This change will prevent us from using third party classes written against
> > plain JSR-330 since they won't be annotated with @Creatable. This kind of
> > compatibility was the idea behind JSR-330.
> 
> This is not correct. The "@Creatable" only applies to objects not known to the
> injector. The explicit injection requests work as before and comply to the JSR,
> see comment 19.

It is true for just-in-time bindings and their transitive dependencies.
Just-In-Time bindings are very helpful and are something you'll find in every type-based DI container
(e.g. Guice, CDI). Also JSR-330 suggest them (see the @Inject doc).

Spring doesn't do it because it is name-based (that's why they use @Named).

E4 is not primarily name-based but type-based.

see also : http://jawsy.fi/blog/2011/11/29/spring-vs-guice-the-one-critical-difference-that-matters/

Comment 37 Oleg Besedin 2012-03-13 14:22:29 EDT

Verified in I20120313-0610.