From bee813b7cc15e46695ca1baf5041a00e0a612f91 Mon Sep 17 00:00:00 2001 From: Sam Berlin Date: Wed, 10 May 2023 18:46:55 -0700 Subject: [PATCH] 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. PiperOrigin-RevId: 531062151 --- .../internal/MissingImplementationError.java | 18 ++++++--- .../MissingImplementationErrorHints.java | 38 +++++++++++++------ .../MissingImplementationErrorTest.java | 37 ++++++++++++++++++ .../missing_implementation_with_hints.txt | 4 +- ...lementation_with_hints_with_dot_annots.txt | 4 +- .../inject/internal/OptionalBinderTest.java | 1 + 6 files changed, 82 insertions(+), 20 deletions(-) diff --git a/core/src/com/google/inject/internal/MissingImplementationError.java b/core/src/com/google/inject/internal/MissingImplementationError.java index b504b88c27..2e4ab9f379 100644 --- a/core/src/com/google/inject/internal/MissingImplementationError.java +++ b/core/src/com/google/inject/internal/MissingImplementationError.java @@ -1,5 +1,6 @@ package com.google.inject.internal; +import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.inject.Injector; @@ -8,6 +9,7 @@ import java.util.ArrayList; import java.util.Formatter; import java.util.List; +import java.util.function.Supplier; import java.util.stream.Collectors; /** Error reported by Guice when a key is not bound in the injector. */ @@ -15,21 +17,26 @@ final class MissingImplementationError extends InternalErrorDetail> { private final Key key; - private final ImmutableList suggestions; + private final Supplier> suggestionsSupplier; public MissingImplementationError(Key key, Injector injector, List sources) { - this(key, MissingImplementationErrorHints.getSuggestions(key, injector), sources); + this( + key, + // Defer building suggestions until messages are requested, to avoid the work associated + // with iterating bindings in scenarios where the exceptions are discarded. + Suppliers.memoize(() -> MissingImplementationErrorHints.getSuggestions(key, injector)), + sources); } private MissingImplementationError( - Key key, ImmutableList suggestions, List sources) { + Key key, Supplier> suggestionsSupplier, List sources) { super( ErrorId.MISSING_IMPLEMENTATION, String.format("No implementation for %s was bound.", Messages.convert(key)), sources, null); this.key = key; - this.suggestions = suggestions; + this.suggestionsSupplier = suggestionsSupplier; } @Override @@ -40,6 +47,7 @@ public boolean isMergeable(ErrorDetail otherError) { @Override public void formatDetail(List> mergeableErrors, Formatter formatter) { + ImmutableList suggestions = suggestionsSupplier.get(); if (!suggestions.isEmpty()) { suggestions.forEach(formatter::format); } @@ -65,7 +73,7 @@ public void formatDetail(List> mergeableErrors, Formatter formatt @Override public MissingImplementationError withSources(List newSources) { - return new MissingImplementationError(key, suggestions, newSources); + return new MissingImplementationError(key, suggestionsSupplier, newSources); } /** Omit the key itself in the source list since the information is redundant. */ diff --git a/core/src/com/google/inject/internal/MissingImplementationErrorHints.java b/core/src/com/google/inject/internal/MissingImplementationErrorHints.java index a48ed5561d..e0ca06d8ae 100644 --- a/core/src/com/google/inject/internal/MissingImplementationErrorHints.java +++ b/core/src/com/google/inject/internal/MissingImplementationErrorHints.java @@ -1,5 +1,6 @@ package com.google.inject.internal; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.lang.Math.min; import com.google.common.collect.ImmutableList; @@ -10,6 +11,7 @@ import com.google.inject.Key; import com.google.inject.TypeLiteral; import com.google.inject.spi.BindingSourceRestriction; +import com.google.inject.spi.UntargettedBinding; import java.util.ArrayList; import java.util.Formatter; import java.util.List; @@ -47,22 +49,29 @@ static ImmutableList getSuggestions(Key key, Injector injector) { // Keys which have similar strings as the desired key List possibleMatches = new ArrayList<>(); - List> sameTypes = injector.findBindingsByType(type); + ImmutableList> sameTypes = + injector.findBindingsByType(type).stream() + .filter(b -> !(b instanceof UntargettedBinding)) // These aren't valid matches + .collect(toImmutableList()); if (!sameTypes.isEmpty()) { suggestions.add("\nDid you mean?"); int howMany = min(sameTypes.size(), MAX_MATCHING_TYPES_REPORTED); for (int i = 0; i < howMany; ++i) { + Key bindingKey = sameTypes.get(i).getKey(); // TODO: Look into a better way to prioritize suggestions. For example, possbily // use levenshtein distance of the given annotation vs actual annotation. - suggestions.add(Messages.format("\n * %s", sameTypes.get(i).getKey())); + suggestions.add( + Messages.format( + "\n * %s", + formatSuggestion(bindingKey, injector.getExistingBinding(bindingKey)))); } int remaining = sameTypes.size() - MAX_MATCHING_TYPES_REPORTED; if (remaining > 0) { String plural = (remaining == 1) ? "" : "s"; suggestions.add( - Messages.format("\n %d more binding%s with other annotations.", remaining, plural)); + Messages.format( + "\n * %d more binding%s with other annotations.", remaining, plural)); } - suggestions.add("\n"); } else { // For now, do a simple substring search for possibilities. This can help spot // issues when there are generics being used (such as a wrapper class) and the @@ -73,14 +82,14 @@ static ImmutableList getSuggestions(Key key, Injector injector) { String want = type.toString(); Map, Binding> bindingMap = injector.getAllBindings(); for (Key bindingKey : bindingMap.keySet()) { + Binding binding = bindingMap.get(bindingKey); + // Ignore untargeted bindings, those aren't valid matches. + if (binding instanceof UntargettedBinding) { + continue; + } String have = bindingKey.getTypeLiteral().toString(); if (have.contains(want) || want.contains(have)) { - Formatter fmt = new Formatter(); - fmt.format("%s bound ", Messages.convert(bindingKey)); - new SourceFormatter( - bindingMap.get(bindingKey).getSource(), fmt, /* omitPreposition= */ false) - .format(); - possibleMatches.add(fmt.toString()); + possibleMatches.add(formatSuggestion(bindingKey, bindingMap.get(bindingKey))); // TODO: Consider a check that if there are more than some number of results, // don't suggest any. if (possibleMatches.size() > MAX_RELATED_TYPES_REPORTED) { @@ -93,7 +102,7 @@ static ImmutableList getSuggestions(Key key, Injector injector) { if (!possibleMatches.isEmpty() && (possibleMatches.size() <= MAX_RELATED_TYPES_REPORTED)) { suggestions.add("\nDid you mean?"); for (String possibleMatch : possibleMatches) { - suggestions.add(Messages.format("\n %s", possibleMatch)); + suggestions.add(Messages.format("\n * %s", possibleMatch)); } } } @@ -110,4 +119,11 @@ static ImmutableList getSuggestions(Key key, Injector injector) { return suggestions.build(); } + + private static String formatSuggestion(Key key, Binding binding) { + Formatter fmt = new Formatter(); + fmt.format("%s bound ", Messages.convert(key)); + new SourceFormatter(binding.getSource(), fmt, /* omitPreposition= */ false).format(); + return fmt.toString(); + } } diff --git a/core/test/com/google/inject/errors/MissingImplementationErrorTest.java b/core/test/com/google/inject/errors/MissingImplementationErrorTest.java index 519ed602bf..e3ef0e809f 100644 --- a/core/test/com/google/inject/errors/MissingImplementationErrorTest.java +++ b/core/test/com/google/inject/errors/MissingImplementationErrorTest.java @@ -1,14 +1,17 @@ package com.google.inject.errors; +import static com.google.common.truth.Truth.assertThat; import static com.google.inject.errors.ErrorMessageTestUtils.assertGuiceErrorEqualsIgnoreLineNumber; import static java.lang.annotation.RetentionPolicy.RUNTIME; import static org.junit.Assert.assertThrows; import static org.junit.Assume.assumeTrue; import com.google.inject.AbstractModule; +import com.google.inject.ConfigurationException; import com.google.inject.CreationException; import com.google.inject.Guice; import com.google.inject.Inject; +import com.google.inject.Injector; import com.google.inject.Provides; import com.google.inject.internal.InternalFlags; import com.google.inject.internal.InternalFlags.IncludeStackTraceOption; @@ -170,4 +173,38 @@ public void missingImplementationWithHints() throws Exception { assertGuiceErrorEqualsIgnoreLineNumber( exception.getMessage(), "missing_implementation_with_hints" + GOLDEN_SUFFIX); } + + private static interface CustomType { + static class InnerType {} + } + + @Test + public void missingImplementationWithHints_memoizesSuggestion() throws Exception { + Injector injector = Guice.createInjector(); + ConfigurationException ex = + assertThrows(ConfigurationException.class, () -> injector.getInstance(CustomType.class)); + // Ensure that the message doesn't contain a "Did you mean?" by default, + // because there's no other type that fits. + assertThat(ex).hasMessageThat().doesNotContain("Did you mean?"); + // And even after we insert another type that fits, we don't redo the suggestions. + injector.getInstance(CustomType.InnerType.class); + assertThat(ex).hasMessageThat().doesNotContain("Did you mean?"); + } + + @Test + public void missingImplementationWithHints_lazyInjectorUsage() throws Exception { + // Note: this test is extremely contrived. This scenario is unlikely to happen for real, but + // it's a very convenient way to assert that usage of the injector is lazy. + // By adding a type into the injector after the exception is thrown but before we + // call getMessage, we're validating that the suggestions are populated only on getMessage + // usage. + // This test works in tandem with the above one which asserts that by default, + // the message *will not* have suggestions. + Injector injector = Guice.createInjector(); + ConfigurationException ex = + assertThrows(ConfigurationException.class, () -> injector.getInstance(CustomType.class)); + injector.getInstance(CustomType.InnerType.class); + assertThat(ex).hasMessageThat().containsMatch("Did you mean?"); + assertThat(ex).hasMessageThat().containsMatch("InnerType"); + } } diff --git a/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints.txt b/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints.txt index 3e8d90c814..9d133a8e61 100644 --- a/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints.txt +++ b/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints.txt @@ -3,7 +3,7 @@ Unable to create injector, see the following errors: 1) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$B() was bound. Did you mean? - MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117) + * MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117) Requested by: 1 : MissingImplementationErrorTest$HintsModule.provideString(MissingImplementationErrorTest.java:123) @@ -16,7 +16,7 @@ Learn more: 2) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass2 annotated with @MissingImplementationErrorTest$A() was bound. Did you mean? - MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117) + * MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest$A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:117) Requested by: 1 : MissingImplementationErrorTest$HintsModule.provideAString(MissingImplementationErrorTest.java:130) diff --git a/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints_with_dot_annots.txt b/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints_with_dot_annots.txt index 4ef77d087a..7f8d4fa594 100644 --- a/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints_with_dot_annots.txt +++ b/core/test/com/google/inject/errors/testdata/missing_implementation_with_hints_with_dot_annots.txt @@ -3,7 +3,7 @@ Unable to create injector, see the following errors: 1) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.B() was bound. Did you mean? - MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149) + * MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149) Requested by: 1 : MissingImplementationErrorTest$HintsModule.provideString(MissingImplementationErrorTest.java:155) @@ -16,7 +16,7 @@ Learn more: 2) [Guice/MissingImplementation]: No implementation for MissingImplementationErrorTest$Klass2 annotated with @MissingImplementationErrorTest.A() was bound. Did you mean? - MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149) + * MissingImplementationErrorTest$Klass annotated with @MissingImplementationErrorTest.A() bound at MissingImplementationErrorTest$HintsModule.provideKlass(MissingImplementationErrorTest.java:149) Requested by: 1 : MissingImplementationErrorTest$HintsModule.provideAString(MissingImplementationErrorTest.java:162) diff --git a/core/test/com/google/inject/internal/OptionalBinderTest.java b/core/test/com/google/inject/internal/OptionalBinderTest.java index abf72488b3..e016681cc9 100644 --- a/core/test/com/google/inject/internal/OptionalBinderTest.java +++ b/core/test/com/google/inject/internal/OptionalBinderTest.java @@ -775,6 +775,7 @@ protected void configure() { assertContains( expected.getMessage(), "No implementation for Integer", + "Requested by:", getShortName(module) + ".configure"); } }