Skip to content

Commit

Permalink
Fix Guice's dependence on the system line separator by never using th…
Browse files Browse the repository at this point in the history
…e system line separator and always using \n explicitly. (We were previously inconsistent: many places used \n, and many other places used %n with a formatter, which would end up being the system line separator.) This allows tests to pass on Windows, where the line separator is different. Also update tests to do a run with the windows line separator, to catch regressions. Fixes #1213.

PiperOrigin-RevId: 525552063
  • Loading branch information
sameb authored and Guice Team committed Apr 19, 2023
1 parent 5c95376 commit fa8c21c
Show file tree
Hide file tree
Showing 21 changed files with 99 additions and 71 deletions.
9 changes: 9 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@
<argLine>-Dguice_bytecode_gen_option=DISABLED</argLine>
</configuration>
</execution>
<execution>
<id>with-windows-line-separators</id>
<phase>test</phase>
<goals><goal>test</goal></goals>
<configuration>
<!-- enable ShowHiddenFrames to help tests find AOP frames on Java17 -->
<argLine>-Dline.separator='\r\n' -XX:+UnlockDiagnosticVMOptions -XX:+ShowHiddenFrames</argLine>
</configuration>
</execution>
</executions>
</plugin>
<!--
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatt
.map(e -> ((BindingAlreadySetError) e).binding.getSource())
.map(ImmutableList::of)
.collect(Collectors.toList()));
formatter.format("%n%s%n", Messages.bold("Bound at:"));
formatter.format("\n%s\n", Messages.bold("Bound at:"));
for (int i = 0; i < sourcesList.size(); i++) {
ErrorFormatter.formatSources(i + 1, sourcesList.get(i), formatter);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ public boolean isMergeable(ErrorDetail<?> otherError) {

@Override
public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatter) {
formatter.format("%n%s%n", Messages.bold("Bound at:"));
formatter.format("\n%s\n", Messages.bold("Bound at:"));
int index = 1;
for (Object source : existingSources) {
formatter.format("%-2s: ", index++);
if (source.equals("")) {
formatter.format("as a just-in-time binding%n");
formatter.format("as a just-in-time binding\n");
} else {
new SourceFormatter(source, formatter, /* omitPreposition= */ true).format();
}
Expand All @@ -63,7 +63,7 @@ public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatt
.filter(list -> !list.isEmpty())
.collect(Collectors.toList());
if (!filteredSources.isEmpty()) {
formatter.format("%n%s%n", Messages.bold("Requested by:"));
formatter.format("\n%s\n", Messages.bold("Requested by:"));
for (int i = 0; i < sourcesList.size(); i++) {
ErrorFormatter.formatSources(i + 1, sourcesList.get(i), formatter);
}
Expand Down
10 changes: 5 additions & 5 deletions core/src/com/google/inject/internal/DuplicateElementError.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private DuplicateElementError(

@Override
protected void formatDetail(List<ErrorDetail<?>> others, Formatter formatter) {
formatter.format("%n%s%n", Messages.bold("Duplicates:"));
formatter.format("\n%s\n", Messages.bold("Duplicates:"));
int duplicateIndex = 1;
for (Map.Entry<T, Collection<Element<T>>> entry : elements.asMap().entrySet()) {
formatter.format("%-2s: ", duplicateIndex++);
Expand All @@ -49,8 +49,8 @@ protected void formatDetail(List<ErrorDetail<?>> others, Formatter formatter) {
.collect(Collectors.toSet());
if (valuesAsString.size() == 1) {
// String representation of the duplicates elements are the same, so only print out one.
formatter.format("Element: %s%n", Messages.redBold(valuesAsString.iterator().next()));
formatter.format(" Bound at:%n");
formatter.format("Element: %s\n", Messages.redBold(valuesAsString.iterator().next()));
formatter.format(" Bound at:\n");
int index = 1;
for (Element<T> element : entry.getValue()) {
formatter.format(" %-2s: ", index++);
Expand All @@ -69,14 +69,14 @@ protected void formatDetail(List<ErrorDetail<?>> others, Formatter formatter) {
} else {
indent = true;
}
formatter.format("Element: %s%n", Messages.redBold(element.value.toString()));
formatter.format("Element: %s\n", Messages.redBold(element.value.toString()));
formatter.format(" Bound at: ");
formatElement(element, formatter);
}
}
}
}
formatter.format("%n%s%n", Messages.bold("Multibinder declared at:"));
formatter.format("\n%s\n", Messages.bold("Multibinder declared at:"));
// Multibinder source includes the key of the set. Filter it out since it's not useful in the
// printed error stack.
List<Object> filteredSource =
Expand Down
10 changes: 5 additions & 5 deletions core/src/com/google/inject/internal/DuplicateMapKeyError.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ final class DuplicateMapKeyError<K, V> extends InternalErrorDetail<DuplicateMapK

@Override
protected final void formatDetail(List<ErrorDetail<?>> others, Formatter formatter) {
formatter.format("%n%s%n", Messages.bold("Duplicates:"));
formatter.format("\n%s\n", Messages.bold("Duplicates:"));

for (Map.Entry<K, Collection<Binding<V>>> entry : duplicates.asMap().entrySet()) {
formatter.format(" Key: \"%s\"%n", Messages.redBold(entry.getKey().toString()));
formatter.format(" Bound at:%n");
formatter.format(" Key: \"%s\"\n", Messages.redBold(entry.getKey().toString()));
formatter.format(" Bound at:\n");
int index = 1;
for (Binding<V> binding : entry.getValue()) {
formatter.format(" %-2s: ", index++);
Expand All @@ -42,10 +42,10 @@ protected final void formatDetail(List<ErrorDetail<?>> others, Formatter formatt
true)
.format();
}
formatter.format("%n");
formatter.format("\n");
}

formatter.format("%s%n", Messages.bold("MapBinder declared at:"));
formatter.format("%s\n", Messages.bold("MapBinder declared at:"));
ErrorFormatter.formatSources(getSources(), formatter);
}

Expand Down
24 changes: 12 additions & 12 deletions core/src/com/google/inject/internal/Errors.java
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public Errors withSource(Object source) {
public Errors aopDisabled(InterceptorBinding binding) {
return addMessage(
ErrorId.AOP_DISABLED,
"Binding interceptor is not supported when bytecode generation is disabled. %nInterceptor"
"Binding interceptor is not supported when bytecode generation is disabled. \nInterceptor"
+ " bound at: %s",
binding.getSource());
}
Expand Down Expand Up @@ -177,7 +177,7 @@ public Errors jitDisabled(Key<?> key) {
public Errors jitDisabledInParent(Key<?> key) {
return addMessage(
ErrorId.JIT_DISABLED_IN_PARENT,
"Explicit bindings are required and %s would be bound in a parent injector.%n"
"Explicit bindings are required and %s would be bound in a parent injector.\n"
+ "Please add an explicit binding for it, either in the child or the parent.",
key);
}
Expand All @@ -197,7 +197,7 @@ public Errors converterReturnedNull(
TypeConverterBinding typeConverterBinding) {
return addMessage(
ErrorId.CONVERTER_RETURNED_NULL,
"Received null converting '%s' (bound at %s) to %s%n using %s.",
"Received null converting '%s' (bound at %s) to %s\n using %s.",
stringValue,
convert(source),
type,
Expand All @@ -212,8 +212,8 @@ public Errors conversionTypeError(
Object converted) {
return addMessage(
ErrorId.CONVERSION_TYPE_ERROR,
"Type mismatch converting '%s' (bound at %s) to %s%n"
+ " using %s.%n"
"Type mismatch converting '%s' (bound at %s) to %s\n"
+ " using %s.\n"
+ " Converter returned %s.",
stringValue,
convert(source),
Expand All @@ -230,7 +230,7 @@ public Errors conversionError(
RuntimeException cause) {
return errorInUserCode(
cause,
"Error converting '%s' (bound at %s) to %s%n using %s.%n Reason: %s",
"Error converting '%s' (bound at %s) to %s\n using %s.\n Reason: %s",
stringValue,
convert(source),
type,
Expand All @@ -246,9 +246,9 @@ public Errors ambiguousTypeConversion(
TypeConverterBinding b) {
return addMessage(
ErrorId.AMBIGUOUS_TYPE_CONVERSION,
"Multiple converters can convert '%s' (bound at %s) to %s:%n"
+ " %s and%n"
+ " %s.%n"
"Multiple converters can convert '%s' (bound at %s) to %s:\n"
+ " %s and\n"
+ " %s.\n"
+ " Please adjust your type converter configuration to avoid overlapping matches.",
stringValue,
convert(source),
Expand Down Expand Up @@ -315,7 +315,7 @@ public Errors scopeAnnotationOnAbstractType(
return addMessage(
ErrorId.SCOPE_ANNOTATION_ON_ABSTRACT_TYPE,
"%s is annotated with %s, but scope annotations are not supported "
+ "for abstract types.%n Bound at %s.",
+ "for abstract types.\n Bound at %s.",
type,
scopeAnnotation,
convert(source));
Expand Down Expand Up @@ -368,7 +368,7 @@ public Errors duplicateScopes(
ScopeBinding existing, Class<? extends Annotation> annotationType, Scope scope) {
return addMessage(
ErrorId.DUPLICATE_SCOPES,
"Scope %s is already bound to %s at %s.%n Cannot bind %s.",
"Scope %s is already bound to %s at %s.\n Cannot bind %s.",
existing.getScope(),
annotationType,
existing.getSource(),
Expand Down Expand Up @@ -503,7 +503,7 @@ public Errors errorNotifyingTypeListener(
TypeListenerBinding listener, TypeLiteral<?> type, Throwable cause) {
return errorInUserCode(
cause,
"Error notifying TypeListener %s (bound at %s) of %s.%n Reason: %s",
"Error notifying TypeListener %s (bound at %s) of %s.\n Reason: %s",
listener.getListener(),
convert(listener.getSource()),
type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public static InternalProvisionException errorInUserInjector(
return errorInUserCode(
ErrorId.ERROR_IN_USER_INJECTOR,
cause,
"Error injecting %s using %s.%n Reason: %s",
"Error injecting %s using %s.\n Reason: %s",
type,
listener,
cause);
Expand All @@ -142,7 +142,7 @@ public static InternalProvisionException errorNotifyingInjectionListener(
return errorInUserCode(
ErrorId.OTHER,
cause,
"Error notifying InjectionListener %s of %s.%n Reason: %s",
"Error notifying InjectionListener %s of %s.\n Reason: %s",
listener,
type,
cause);
Expand Down Expand Up @@ -191,7 +191,7 @@ static void onNullInjectedIntoNonNullableDependency(Object source, Dependency<?>
: "the " + parameterName + " of " + memberStackTraceElement;
throw InternalProvisionException.create(
ErrorId.NULL_INJECTED_INTO_NON_NULLABLE,
"null returned by binding at %s%n but %s is not @Nullable",
"null returned by binding at %s\n but %s is not @Nullable",
source,
formattedDependency)
.addSource(source);
Expand Down
4 changes: 2 additions & 2 deletions core/src/com/google/inject/internal/Messages.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public static String format(String messageFormat, Object... arguments) {

/** Returns the formatted message for an exception with the specified messages. */
public static String formatMessages(String heading, Collection<Message> errorMessages) {
Formatter fmt = new Formatter().format(heading).format(":%n%n");
Formatter fmt = new Formatter().format(heading).format(":\n\n");
int index = 1;
boolean displayCauses = getOnlyCause(errorMessages) == null;

Expand Down Expand Up @@ -102,7 +102,7 @@ public static String formatMessages(String heading, Collection<Message> errorMes
cause.getClass().getName(), causeIdx);
}
}
fmt.format("%n");
fmt.format("\n");
index++;
}

Expand Down
12 changes: 6 additions & 6 deletions core/src/com/google/inject/internal/MissingConstructorError.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ public boolean isMergeable(ErrorDetail<?> other) {

@Override
protected void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatter) {
formatter.format("%n");
formatter.format("\n");
Class<?> rawType = type.getRawType();
if (atInjectRequired) {
formatter.format(
"Injector is configured to require @Inject constructors but %s does not have a @Inject"
+ " annotated constructor.%n",
+ " annotated constructor.\n",
rawType);
} else {
Constructor<?> noArgConstructor = null;
Expand All @@ -53,22 +53,22 @@ protected void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter form
}
if (noArgConstructor == null) {
formatter.format(
"%s does not have a @Inject annotated constructor or a no-arg constructor.%n", rawType);
"%s does not have a @Inject annotated constructor or a no-arg constructor.\n", rawType);
} else if (Modifier.isPrivate(noArgConstructor.getModifiers())
&& !Modifier.isPrivate(rawType.getModifiers())) {
formatter.format(
"%s has a private no-arg constructor but the class is not private. Guice can only use"
+ " a private no-arg constructor if it is defined in a private class.%n",
+ " a private no-arg constructor if it is defined in a private class.\n",
rawType);
}
}
formatter.format("%n");
formatter.format("\n");

List<List<Object>> sourcesList = new ArrayList<>();
sourcesList.add(getSources());
mergeableErrors.forEach(error -> sourcesList.add(error.getSources()));

formatter.format("%s%n", Messages.bold("Requested by:"));
formatter.format("%s\n", Messages.bold("Requested by:"));
int sourceListIndex = 1;
for (List<Object> sources : sourcesList) {
ErrorFormatter.formatSources(sourceListIndex++, Lists.reverse(sources), formatter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter formatt
.collect(Collectors.toList());

if (!filteredSourcesList.isEmpty()) {
formatter.format("%n%s%n", Messages.bold("Requested by:"));
formatter.format("\n%s\n", Messages.bold("Requested by:"));
int sourceListIndex = 1;
for (List<Object> sources : filteredSourcesList) {
ErrorFormatter.formatSources(sourceListIndex++, Lists.reverse(sources), formatter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,20 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
List<String> possibleMatches = new ArrayList<>();
List<Binding<T>> sameTypes = injector.findBindingsByType(type);
if (!sameTypes.isEmpty()) {
suggestions.add("%nDid you mean?");
suggestions.add("\nDid you mean?");
int howMany = min(sameTypes.size(), MAX_MATCHING_TYPES_REPORTED);
for (int i = 0; i < howMany; ++i) {
// 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", sameTypes.get(i).getKey()));
}
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");
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
Expand Down Expand Up @@ -91,9 +91,9 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
}

if (!possibleMatches.isEmpty() && (possibleMatches.size() <= MAX_RELATED_TYPES_REPORTED)) {
suggestions.add("%nDid you mean?");
suggestions.add("\nDid you mean?");
for (String possibleMatch : possibleMatches) {
suggestions.add(Messages.format("%n %s", possibleMatch));
suggestions.add(Messages.format("\n %s", possibleMatch));
}
}
}
Expand All @@ -105,7 +105,7 @@ static <T> ImmutableList<String> getSuggestions(Key<T> key, Injector injector) {
&& key.getAnnotationType() == null
&& COMMON_AMBIGUOUS_TYPES.contains(key.getTypeLiteral().getRawType())) {
// We don't recommend using such simple types without annotations.
suggestions.add("%nThe key seems very generic, did you forget an annotation?");
suggestions.add("\nThe key seems very generic, did you forget an annotation?");
}

return suggestions.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public T provision(InternalContext context, ProvisionCallback<T> callable)
throw InternalProvisionException.errorInUserCode(
ErrorId.OTHER,
caught,
"Error notifying ProvisionListener %s of %s.%n Reason: %s",
"Error notifying ProvisionListener %s of %s.\n Reason: %s",
listener,
binding.getKey(),
caught);
Expand Down
8 changes: 4 additions & 4 deletions core/src/com/google/inject/internal/RealMapBinder.java
Original file line number Diff line number Diff line change
Expand Up @@ -966,19 +966,19 @@ public List<Map.Entry<?, Binding<?>>> getEntries(Iterable<? extends Element> ele

if (!keysOnlyFromBindings.isEmpty()) {
sb.append(
Errors.format("%nFound these Bindings that were missing an associated entry:%n"));
Errors.format("\nFound these Bindings that were missing an associated entry:\n"));
for (Key<V> key : keysOnlyFromBindings) {
sb.append(
Errors.format(" %s bound at: %s%n", key, valueKeyToBinding.get(key).getSource()));
Errors.format(" %s bound at: %s\n", key, valueKeyToBinding.get(key).getSource()));
}
}

if (!keysOnlyFromProviderMapEntrys.isEmpty()) {
sb.append(Errors.format("%nFound these map keys without a corresponding value:%n"));
sb.append(Errors.format("\nFound these map keys without a corresponding value:\n"));
for (Key<V> key : keysOnlyFromProviderMapEntrys) {
sb.append(
Errors.format(
" '%s' bound at: %s%n",
" '%s' bound at: %s\n",
valueKeyToKey.get(key), valueKeyToEntryBinding.get(key).getSource()));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ protected void formatDetail(List<ErrorDetail<?>> mergeableErrors, Formatter form
sourcesSet.add(getSources());
mergeableErrors.stream().map(ErrorDetail::getSources).forEach(sourcesSet::add);

formatter.format("%n%s%n", "Used at:");
formatter.format("\n%s\n", "Used at:");
int sourceListIndex = 1;
for (List<Object> sources : sourcesSet) {
ErrorFormatter.formatSources(sourceListIndex++, Lists.reverse(sources), formatter);
Expand Down
Loading

0 comments on commit fa8c21c

Please sign in to comment.