4

Suppose we have the following 3 (top-level) classes:

@ImplementedBy(MyClass.class)
public interface MyInterface {}


public class MyClass implements MyInterface {

  @Inject
  MyClass(MySecondClass mySecondClass) {}
}


public class MySecondClass {}

Then the following code results in an exception (A just-in-time binding to MySecondClass was already configured on a parent injector.):

Guice.createInjector(
    binder -> binder.bind(MyInterface.class),
    binder -> binder.bind(MySecondClass.class)
);

However, if you swap around the order of the modules, there is no exception:

Guice.createInjector(
    binder -> binder.bind(MySecondClass.class),
    binder -> binder.bind(MyInterface.class)
);

I would expect the order of the modules not to matter. Also, the exception message is distinctly odd as there is no parent injector. Is this a bug?

I am using Guice version 5.1.0.

UPDATE

I've done a bit more digging and found this open issue, with some very similar code, so I'm now pretty confident that this is something in Guice that needs to be fixed (and has been an issue for at least a decade). The issue isn't limited to the order of modules passed to createInjector; in fact the order of bindings in a single configure method can make a difference.

cpp beginner
  • 512
  • 6
  • 23
  • Did this order of bindings worked in any previous version of Guice and you encounter this problem only because of an update? – Progman Oct 02 '22 at 09:07
  • Is it an option to use `Modules.override()`? See https://stackoverflow.com/questions/483087/overriding-binding-in-guice/531110#531110 – Progman Oct 02 '22 at 09:47
  • I don't know if this code works on any previous versions. I can try that out, but first I'm looking for confirmation that this is a bug. – cpp beginner Oct 02 '22 at 11:41
  • When you use the first code example, Guice will create an implicit binding for `MySecondClass`, as described in https://github.com/google/guice/wiki/JustInTimeBindings. Why do you think there is a bug? – Progman Oct 02 '22 at 11:45
  • The code ```Guice.createInjector(binder -> binder.bind(MyClass.class), binder -> binder.bind(MySecondClass.class));``` does not throw an exception, so this looks like it might be a bug involving `ImplementedBy`. I don't think there should be a JIT binding in any of these examples, because in all of the examples I've presented there has been an explicit binding for `MySecondClass`. – cpp beginner Oct 02 '22 at 12:29

2 Answers2

2

Explanation

The problem arises because the @ImplementedBy annotation in combination with the call to Binder.bind(...) are causing the resolution of the constructor injection with @Inject to be happen prematurely before all explicit bindings are known.

The yet unknown explicit binding is here (incorrectly) replaced by a just-in-time binding.

Is it a Bug?

I found no explicit documentation about this behaviour so this can be considered a bug.

There is a vague disclaimer about Guice's ability to validate bindings in the Binder API documentation

bind(ServiceImpl.class);

This statement does essentially nothing; it "binds the ServiceImpl class to itself" and does not change Guice's default behavior. You may still want to use this if you prefer your Module class to serve as an explicit manifest for the services it provides. Also, in rare cases, Guice may be unable to validate a binding at injector creation time unless it is given explicitly.

Despite the disclaimer the behaviour you are observing is contradicting the statements "does essentially nothing" and "does not change Guice's default behaviour".

Though one must admit that the description indirectly suggests - by using the name ServiceImpl.class, which is obviously the name of an implementation type - that it is not meant to be used with interface types unless used to create an explicit binding, for example with a following call to .to(...). This detail can be overlooked very easily and could be highlighted more explicitly.

Furthermore the Wiki entry for @ImplementedBy states:

The above annotation is equivalent to the following bind() statement:

bind(CreditCardProcessor.class).to(PayPalCreditCardProcessor.class);

Which is no longer true when it's used the way you are using it.

The severity of the bug is questionable, though, because the statements could be fully omitted without loss of functionality.

Conclusion

Yes, it is a bug with questionable severity.

One would better avoid calling Binder.bind(...) with interface types other than to create explicit bindings.

Final Note

Interestingly the following call sequence works, which could be a hint how to solve this bug in Guice:

        Injector injector = Guice.createInjector(
                binder -> binder.bind(MyClass.class),
                binder -> binder.bind(MyInterface.class),
                binder -> binder.bind(MySecondClass.class)
        );

Update 2022-10-08

I filed a PR: https://github.com/google/guice/pull/1650. Let's see, what happens.

nineninesevenfour
  • 607
  • 2
  • 16
0

I think this is a bug in Guice. The problem is that Guice is trying to be clever and optimize the bindings. It is trying to avoid creating a binding for MySecondClass if it doesn't need to. But it is doing this in a way that is not compatible with the way that @ImplementedBy works. The workaround is to add a binding for MySecondClass to the first module. This will force Guice to create a binding for MySecondClass and the problem will go away.

cactusboat
  • 778
  • 2
  • 7
  • 15