Skip to content

Commit 611baf6

Browse files
committed
Improve MissingImplementationError to lazily calculate suggestions and standardize its output.
The error will now generate suggestions only when the message is used (as opposed to on instantiation). This should significantly improve the performance of any code that happens to call `injector.get{Instance,Provider,Binding}` and catches/ignores a `ProvisionException` (with the intention of falling back to code that deals with the type not being bound). While this isn't a great pattern for code to use, and there are far better ways of doing it... there's enough code that does this that it's worthwhile improving. This pattern in particular was very problematic with the introduction of the jakarta.inject.Provider variants for Multibinders/MapBinders & OptionalBinders, because we have *lots* of those. By adding N more bindings to each multi/map/optionalbinder, we added a very large number of bindings to the overall total, which resulted in code using this pattern getting much slower. This code also fixes suggestions to skip "UntargettedBindings", which are bindings like `bind(SomeInterface.class)` and aren't a valid suggestion (because they're missing a .to(..)). By standardizing, this otherwise broke a test in BinderTest that was asserting we failed those with proper error messages (showing their source). We would have otherwise started showing their source twice, once in a "Did you mean?" section and again in the normal "bound here" section. But really we shouldn't show these in "Did you mean?" at all, because it's just a repetition (or incorrect suggestion) of the wrong binding. (cherry-picked into the 6.0 release)
1 parent e897e89 commit 611baf6

6 files changed

+82
-20
lines changed

core/src/com/google/inject/internal/MissingImplementationError.java

+13-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.google.inject.internal;
22

3+
import com.google.common.base.Suppliers;
34
import com.google.common.collect.ImmutableList;
45
import com.google.common.collect.Lists;
56
import com.google.inject.Injector;
@@ -8,28 +9,34 @@
89
import java.util.ArrayList;
910
import java.util.Formatter;
1011
import java.util.List;
12+
import java.util.function.Supplier;
1113
import java.util.stream.Collectors;
1214

1315
/** Error reported by Guice when a key is not bound in the injector. */
1416
final class MissingImplementationError<T>
1517
extends InternalErrorDetail<MissingImplementationError<T>> {
1618

1719
private final Key<T> key;
18-
private final ImmutableList<String> suggestions;
20+
private final Supplier<ImmutableList<String>> suggestionsSupplier;
1921

2022
public MissingImplementationError(Key<T> key, Injector injector, List<Object> sources) {
21-
this(key, MissingImplementationErrorHints.getSuggestions(key, injector), sources);
23+
this(
24+
key,
25+
// Defer building suggestions until messages are requested, to avoid the work associated
26+
// with iterating bindings in scenarios where the exceptions are discarded.
27+
Suppliers.memoize(() -> MissingImplementationErrorHints.getSuggestions(key, injector)),
28+
sources);
2229
}
2330

2431
private MissingImplementationError(
25-
Key<T> key, ImmutableList<String> suggestions, List<Object> sources) {
32+
Key<T> key, Supplier<ImmutableList<String>> suggestionsSupplier, List<Object> sources) {
2633
super(
2734
ErrorId.MISSING_IMPLEMENTATION,
2835
String.format("No implementation for %s was bound.", Messages.convert(key)),
2936
sources,
3037
null);
3138
this.key = key;
32-
this.suggestions = suggestions;
39+
this.suggestionsSupplier = suggestionsSupplier;
3340
}
3441

3542
@Override
@@ -40,6 +47,7 @@ public boolean isMergeable(ErrorDetail<?> otherError) {
4047

4148
@Override
4249
public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatter) {
50+
ImmutableList<String> suggestions = suggestionsSupplier.get();
4351
if (!suggestions.isEmpty()) {
4452
suggestions.forEach(formatter::format);
4553
}
@@ -65,7 +73,7 @@ public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatt
6573

6674
@Override
6775
public MissingImplementationError<T> withSources(List<Object> newSources) {
68-
return new MissingImplementationError<T>(key, suggestions, newSources);
76+
return new MissingImplementationError<T>(key, suggestionsSupplier, newSources);
6977
}
7078

7179
/** Omit the key itself in the source list since the information is redundant. */

core/src/com/google/inject/internal/MissingImplementationErrorHints.java

+27-11
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.google.inject.internal;
22

3+
import static com.google.common.collect.ImmutableList.toImmutableList;
34
import static java.lang.Math.min;
45

56
import com.google.common.collect.ImmutableList;
@@ -10,6 +11,7 @@
1011
import com.google.inject.Key;
1112
import com.google.inject.TypeLiteral;
1213
import com.google.inject.spi.BindingSourceRestriction;
14+
import com.google.inject.spi.UntargettedBinding;
1315
import java.util.ArrayList;
1416
import java.util.Formatter;
1517
import java.util.List;
@@ -47,22 +49,29 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
4749

4850
// Keys which have similar strings as the desired key
4951
List<String> possibleMatches = new ArrayList<>();
50-
List<Binding<T>> sameTypes = injector.findBindingsByType(type);
52+
ImmutableList<Binding<T>> sameTypes =
53+
injector.findBindingsByType(type).stream()
54+
.filter(b -> !(b instanceof UntargettedBinding)) // These aren't valid matches
55+
.collect(toImmutableList());
5156
if (!sameTypes.isEmpty()) {
5257
suggestions.add("\nDid you mean?");
5358
int howMany = min(sameTypes.size(), MAX_MATCHING_TYPES_REPORTED);
5459
for (int i = 0; i < howMany; ++i) {
60+
Key<?> bindingKey = sameTypes.get(i).getKey();
5561
// TODO: Look into a better way to prioritize suggestions. For example, possbily
5662
// use levenshtein distance of the given annotation vs actual annotation.
57-
suggestions.add(Messages.format("\n * %s", sameTypes.get(i).getKey()));
63+
suggestions.add(
64+
Messages.format(
65+
"\n * %s",
66+
formatSuggestion(bindingKey, injector.getExistingBinding(bindingKey))));
5867
}
5968
int remaining = sameTypes.size() - MAX_MATCHING_TYPES_REPORTED;
6069
if (remaining > 0) {
6170
String plural = (remaining == 1) ? "" : "s";
6271
suggestions.add(
63-
Messages.format("\n %d more binding%s with other annotations.", remaining, plural));
72+
Messages.format(
73+
"\n * %d more binding%s with other annotations.", remaining, plural));
6474
}
65-
suggestions.add("\n");
6675
} else {
6776
// For now, do a simple substring search for possibilities. This can help spot
6877
// issues when there are generics being used (such as a wrapper class) and the
@@ -73,14 +82,14 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
7382
String want = type.toString();
7483
Map<Key<?>, Binding<?>> bindingMap = injector.getAllBindings();
7584
for (Key<?> bindingKey : bindingMap.keySet()) {
85+
Binding<?> binding = bindingMap.get(bindingKey);
86+
// Ignore untargeted bindings, those aren't valid matches.
87+
if (binding instanceof UntargettedBinding) {
88+
continue;
89+
}
7690
String have = bindingKey.getTypeLiteral().toString();
7791
if (have.contains(want) || want.contains(have)) {
78-
Formatter fmt = new Formatter();
79-
fmt.format("%s bound ", Messages.convert(bindingKey));
80-
new SourceFormatter(
81-
bindingMap.get(bindingKey).getSource(), fmt, /* omitPreposition= */ false)
82-
.format();
83-
possibleMatches.add(fmt.toString());
92+
possibleMatches.add(formatSuggestion(bindingKey, bindingMap.get(bindingKey)));
8493
// TODO: Consider a check that if there are more than some number of results,
8594
// don't suggest any.
8695
if (possibleMatches.size() > MAX_RELATED_TYPES_REPORTED) {
@@ -93,7 +102,7 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
93102
if (!possibleMatches.isEmpty() && (possibleMatches.size() <= MAX_RELATED_TYPES_REPORTED)) {
94103
suggestions.add("\nDid you mean?");
95104
for (String possibleMatch : possibleMatches) {
96-
suggestions.add(Messages.format("\n %s", possibleMatch));
105+
suggestions.add(Messages.format("\n * %s", possibleMatch));
97106
}
98107
}
99108
}
@@ -110,4 +119,11 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
110119

111120
return suggestions.build();
112121
}
122+
123+
private static String formatSuggestion(Key<?> key, Binding<?> binding) {
124+
Formatter fmt = new Formatter();
125+
fmt.format("%s bound ", Messages.convert(key));
126+
new SourceFormatter(binding.getSource(), fmt, /* omitPreposition= */ false).format();
127+
return fmt.toString();
128+
}
113129
}

core/test/com/google/inject/errors/MissingImplementationErrorTest.java

+37
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
package com.google.inject.errors;
22

3+
import static com.google.common.truth.Truth.assertThat;
34
import static com.google.inject.errors.ErrorMessageTestUtils.assertGuiceErrorEqualsIgnoreLineNumber;
45
import static java.lang.annotation.RetentionPolicy.RUNTIME;
56
import static org.junit.Assert.assertThrows;
67
import static org.junit.Assume.assumeTrue;
78

89
import com.google.inject.AbstractModule;
10+
import com.google.inject.ConfigurationException;
911
import com.google.inject.CreationException;
1012
import com.google.inject.Guice;
1113
import com.google.inject.Inject;
14+
import com.google.inject.Injector;
1215
import com.google.inject.Provides;
1316
import com.google.inject.internal.InternalFlags;
1417
import com.google.inject.internal.InternalFlags.IncludeStackTraceOption;
@@ -170,4 +173,38 @@ public void missingImplementationWithHints() throws Exception {
170173
assertGuiceErrorEqualsIgnoreLineNumber(
171174
exception.getMessage(), "missing_implementation_with_hints" + GOLDEN_SUFFIX);
172175
}
176+
177+
private static interface CustomType {
178+
static class InnerType {}
179+
}
180+
181+
@Test
182+
public void missingImplementationWithHints_memoizesSuggestion() throws Exception {
183+
Injector injector = Guice.createInjector();
184+
ConfigurationException ex =
185+
assertThrows(ConfigurationException.class, () -> injector.getInstance(CustomType.class));
186+
// Ensure that the message doesn't contain a "Did you mean?" by default,
187+
// because there's no other type that fits.
188+
assertThat(ex).hasMessageThat().doesNotContain("Did you mean?");
189+
// And even after we insert another type that fits, we don't redo the suggestions.
190+
injector.getInstance(CustomType.InnerType.class);
191+
assertThat(ex).hasMessageThat().doesNotContain("Did you mean?");
192+
}
193+
194+
@Test
195+
public void missingImplementationWithHints_lazyInjectorUsage() throws Exception {
196+
// Note: this test is extremely contrived. This scenario is unlikely to happen for real, but
197+
// it's a very convenient way to assert that usage of the injector is lazy.
198+
// By adding a type into the injector after the exception is thrown but before we
199+
// call getMessage, we're validating that the suggestions are populated only on getMessage
200+
// usage.
201+
// This test works in tandem with the above one which asserts that by default,
202+
// the message *will not* have suggestions.
203+
Injector injector = Guice.createInjector();
204+
ConfigurationException ex =
205+
assertThrows(ConfigurationException.class, () -> injector.getInstance(CustomType.class));
206+
injector.getInstance(CustomType.InnerType.class);
207+
assertThat(ex).hasMessageThat().containsMatch("Did you mean?");
208+
assertThat(ex).hasMessageThat().containsMatch("InnerType");
209+
}
173210
}

core/test/com/google/inject/errors/testdata/missing_implementation_with_hints.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ Unable to create injector, see the following errors:
33
1) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$B() was bound.
44

55
Did you mean?
6-
MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117)
6+
* MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117)
77

88
Requested by:
99
1 : MissingImplementationErrorTest$HintsModule.provideString(MissingImplementationErrorTest.java:123)
@@ -16,7 +16,7 @@ Learn more:
1616
2) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass2 annotated with @MissingImplementationErrorTest$A() was bound.
1717

1818
Did you mean?
19-
MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117)
19+
* MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117)
2020

2121
Requested by:
2222
1 : MissingImplementationErrorTest$HintsModule.provideAString(MissingImplementationErrorTest.java:130)

core/test/com/google/inject/errors/testdata/missing_implementation_with_hints_with_dot_annots.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ Unable to create injector, see the following errors:
33
1) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.B() was bound.
44

55
Did you mean?
6-
MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149)
6+
* MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149)
77

88
Requested by:
99
1 : MissingImplementationErrorTest$HintsModule.provideString(MissingImplementationErrorTest.java:155)
@@ -16,7 +16,7 @@ Learn more:
1616
2) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass2 annotated with @MissingImplementationErrorTest.A() was bound.
1717

1818
Did you mean?
19-
MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149)
19+
* MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149)
2020

2121
Requested by:
2222
1 : MissingImplementationErrorTest$HintsModule.provideAString(MissingImplementationErrorTest.java:162)

core/test/com/google/inject/internal/OptionalBinderTest.java

+1
Original file line numberDiff line numberDiff line change
@@ -809,6 +809,7 @@ protected void configure() {
809809
assertContains(
810810
expected.getMessage(),
811811
"No implementation for Integer",
812+
"Requested by:",
812813
getShortName(module) + ".configure");
813814
}
814815
}

0 commit comments

Comments
 (0)