Skip to content

Commit

Permalink
Automatically add some nullness annotations to Flogger.
Browse files Browse the repository at this point in the history
I suspect that a test or two and _maybe_ even some prod code (maybe all just in Google-internal code?) uses `return null` to implement a method that should never actually return `null`. This CL adds `@Nullable` anyway, but we may want to ultimately change the code to return a proper object (even if a non-functional one) or to just throw an exception. I figure that that will happen naturally in the course of adding `@NullMarked` and enabling static analysis, but if anything jumps out that you want me to revert now, let me know.

PiperOrigin-RevId: 662936485
  • Loading branch information
cpovirk authored and Flogger Team committed Aug 15, 2024
1 parent 474b113 commit 61a0b09
Show file tree
Hide file tree
Showing 32 changed files with 117 additions and 83 deletions.
5 changes: 4 additions & 1 deletion api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,10 @@ java_library(
name = "stack_getter_common",
srcs = STACK_GETTER_COMMON_SRCS,
javacopts = ["-source 8 -target 8"],
deps = [":checks"],
deps = [
":checks",
"@google_bazel_common//third_party/java/jspecify_annotations",
],
)

java_library(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public int hashCode() {
}

@Override
public boolean equals(Object obj) {
public boolean equals(@Nullable Object obj) {
if (obj instanceof RateLimitPeriod) {
RateLimitPeriod that = (RateLimitPeriod) obj;
return this.n == that.n && this.unit == that.unit;
Expand Down
16 changes: 10 additions & 6 deletions api/src/main/java/com/google/common/flogger/LogContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -329,15 +329,19 @@ public String toString() {
private final long timestampNanos;

/** Additional metadata for this log statement (added via fluent API methods). */
private MutableMetadata metadata = null;
private @Nullable MutableMetadata metadata = null;

/** The log site information for this log statement (set immediately prior to post-processing). */
private LogSite logSite = null;
private @Nullable LogSite logSite = null;

/** Rate limit status (only set if rate limiting occurs). */
private RateLimitStatus rateLimitStatus = null;
private @Nullable RateLimitStatus rateLimitStatus = null;

/** The template context if formatting is required (set only after post-processing). */
private TemplateContext templateContext = null;
private @Nullable TemplateContext templateContext = null;

/** The log arguments (set only after post-processing). */
private Object[] args = null;
private Object @Nullable [] args = null;

/**
* Creates a logging context with the specified level, and with a timestamp obtained from the
Expand Down Expand Up @@ -758,7 +762,7 @@ private void logImpl(String message, Object... args) {
// ---- Log site injection (used by pre-processors and special cases) ----

@Override
public final API withInjectedLogSite(LogSite logSite) {
public final API withInjectedLogSite(@Nullable LogSite logSite) {
// First call wins (since auto-injection will typically target the log() method at the end of
// the chain and might not check for previous explicit injection). In particular it MUST be
// allowed for a caller to specify the "INVALID" log site, and have that set the field here to
Expand Down
14 changes: 7 additions & 7 deletions api/src/main/java/com/google/common/flogger/LogSite.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ public abstract class LogSite implements LogSiteKey {
/**
* An singleton LogSite instance used to indicate that valid log site information cannot be
* determined. This can be used to indicate that log site information is not available by
* injecting it via {@link LoggingApi#withInjectedLogSite} which will suppress any further
* log site analysis for that log statement. This is also returned if stack trace analysis
* fails for any reason.
* <p>
* If a log statement does end up with invalid log site information, then any fluent logging
* injecting it via {@link LoggingApi#withInjectedLogSite} which will suppress any further log
* site analysis for that log statement. This is also returned if stack trace analysis fails for
* any reason.
*
* <p>If a log statement does end up with invalid log site information, then any fluent logging
* methods which rely on being able to look up site specific metadata will be disabled and
* essentially become "no ops".
*/
Expand All @@ -70,7 +70,7 @@ public int getLineNumber() {
}

@Override
public String getFileName() {
public @Nullable String getFileName() {
return null;
}
// No need to implement equals() or hashCode() for a singleton instance.
Expand Down Expand Up @@ -194,7 +194,7 @@ public int getLineNumber() {
}

@Override
public boolean equals(Object obj) {
public boolean equals(@Nullable Object obj) {
if (obj instanceof InjectedLogSite) {
InjectedLogSite other = (InjectedLogSite) obj;
// Probably not worth optimizing for "this == obj" because all strings should be interned.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ public final int hashCode() {
}

@Override
public final boolean equals(Object obj) {
public final boolean equals(@Nullable Object obj) {
return super.equals(obj);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.common.flogger.backend;

import org.jspecify.annotations.Nullable;

/**
* An enum representing the printf-like formatting characters that must be supported by all logging
* backends. It is important to note that while backends must accept any of these format specifiers,
Expand Down Expand Up @@ -129,10 +131,10 @@ private static boolean isLowerCase(char letter) {
}

/**
* Returns the FormatChar instance associated with the given printf format specifier. If the
* given character is not an ASCII letter, a runtime exception is thrown.
* Returns the FormatChar instance associated with the given printf format specifier. If the given
* character is not an ASCII letter, a runtime exception is thrown.
*/
public static FormatChar of(char c) {
public static @Nullable FormatChar of(char c) {
// Get from the map by converting the char to lower-case (which is the most common case by far).
// If the given value wasn't an ASCII letter then the index will be out-of-range, but when
// called by the parser, it's always guaranteed to be an ASCII letter (but perhaps not a valid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,13 @@ public final class KeyValueFormatter implements KeyValueHandler {
Double.class));

/**
* Helper method to emit metadata key/value pairs in a format consistent with JSON. String
* values which need to be quoted are JSON escaped, while other values are appended without
* quoting or escaping. Labels are expected to be JSON "safe", and are never quoted. This format
* is compatible with various "lightweight" JSON representations.
* Helper method to emit metadata key/value pairs in a format consistent with JSON. String values
* which need to be quoted are JSON escaped, while other values are appended without quoting or
* escaping. Labels are expected to be JSON "safe", and are never quoted. This format is
* compatible with various "lightweight" JSON representations.
*/
public static void appendJsonFormattedKeyAndValue(String label, Object value, StringBuilder out) {
public static void appendJsonFormattedKeyAndValue(
String label, @Nullable Object value, StringBuilder out) {
out.append(label).append('=');
// We could also consider enums as safe if we used name() rather than toString().
if (value == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.FormattableFlags;
import java.util.Formatter;
import java.util.Locale;
import org.jspecify.annotations.Nullable;

/**
* Static utilities for classes wishing to implement their own log message formatting. None of the
Expand Down Expand Up @@ -82,7 +83,7 @@ public static String safeToString(Object value) {
* @param value the value to be formatted (possibly null).
* @return a non-null string representation of the given value (possibly "null").
*/
private static String toNonNullString(Object value) {
private static String toNonNullString(@Nullable Object value) {
if (value == null) {
return "null";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import org.jspecify.annotations.Nullable;

/**
* Callback API for logger backend implementations to handle metadata keys/values. The API methods
Expand Down Expand Up @@ -150,7 +151,7 @@ public void handle(MetadataKey<Object> key, Iterator<Object> value, Object conte
private final Map<MetadataKey<?>, RepeatedValueHandler<?, ? super C>> repeatedValueHandlers =
new HashMap<MetadataKey<?>, RepeatedValueHandler<?, ? super C>>();
private final ValueHandler<Object, ? super C> defaultHandler;
private RepeatedValueHandler<Object, ? super C> defaultRepeatedHandler = null;
private @Nullable RepeatedValueHandler<Object, ? super C> defaultRepeatedHandler = null;

private Builder(ValueHandler<Object, ? super C> defaultHandler) {
this.defaultHandler = checkNotNull(defaultHandler, "default handler");
Expand Down Expand Up @@ -289,7 +290,7 @@ private static final class MapBasedhandler<C> extends MetadataHandler<C> {
private final Map<MetadataKey<?>, RepeatedValueHandler<?, ? super C>> repeatedValueHandlers =
new HashMap<MetadataKey<?>, RepeatedValueHandler<?, ? super C>>();
private final ValueHandler<Object, ? super C> defaultHandler;
private final RepeatedValueHandler<Object, ? super C> defaultRepeatedHandler;
private final @Nullable RepeatedValueHandler<Object, ? super C> defaultRepeatedHandler;

private MapBasedhandler(Builder<C> builder) {
this.singleValueHandlers.putAll(builder.singleValueHandlers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.jspecify.annotations.Nullable;

/**
* Processor combining scope and log-site metadata into a single view. This is necessary when
Expand All @@ -53,28 +54,29 @@
*/
public abstract class MetadataProcessor {
// Immutable empty processor which never handles any metadata.
private static final MetadataProcessor EMPTY_PROCESSOR = new MetadataProcessor() {
@Override
public <C> void process(MetadataHandler<C> handler, C context) {}
private static final MetadataProcessor EMPTY_PROCESSOR =
new MetadataProcessor() {
@Override
public <C> void process(MetadataHandler<C> handler, C context) {}

@Override
public <C> void handle(MetadataKey<?> key, MetadataHandler<C> handler, C context) {}
@Override
public <C> void handle(MetadataKey<?> key, MetadataHandler<C> handler, C context) {}

@Override
public <T> T getSingleValue(MetadataKey<T> key) {
return null;
}
@Override
public <T> @Nullable T getSingleValue(MetadataKey<T> key) {
return null;
}

@Override
public int keyCount() {
return 0;
}
@Override
public int keyCount() {
return 0;
}

@Override
public Set<MetadataKey<?>> keySet() {
return Collections.emptySet();
}
};
@Override
public Set<MetadataKey<?>> keySet() {
return Collections.emptySet();
}
};

/**
* Returns a new processor for the combined scope and log-site metadata. Note that this returned
Expand Down Expand Up @@ -220,7 +222,7 @@ public <C> void handle(MetadataKey<?> key, MetadataHandler<C> handler, C context
}

@Override
public <T> T getSingleValue(MetadataKey<T> key) {
public <T> @Nullable T getSingleValue(MetadataKey<T> key) {
checkArgument(!key.canRepeat(), "key must be single valued");
int index = indexOf(key, keyMap, keyCount);
// For single keys, the keyMap values are just the value index.
Expand Down Expand Up @@ -446,7 +448,7 @@ public <C> void handle(MetadataKey<?> key, MetadataHandler<C> handler, C context
// It's safe to ignore warnings since single keys are only ever 'T' when added to the map.
@Override
@SuppressWarnings("unchecked")
public <T> T getSingleValue(MetadataKey<T> key) {
public <T> @Nullable T getSingleValue(MetadataKey<T> key) {
checkArgument(!key.canRepeat(), "key must be single valued");
Object value = map.get(key);
return (value != null) ? (T) value : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.flogger.util.Checks.checkNotNull;

import com.google.common.flogger.parser.MessageParser;
import org.jspecify.annotations.Nullable;

/**
* A context object for templates that allows caches to validate existing templates or create new
Expand Down Expand Up @@ -49,7 +50,7 @@ public String getMessage() {
}

@Override
public boolean equals(Object obj) {
public boolean equals(@Nullable Object obj) {
if (obj instanceof TemplateContext) {
TemplateContext other = (TemplateContext) obj;
return parser.equals(other.parser) && message.equals(other.message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import org.jspecify.annotations.Nullable;

/**
* Common backend to handle everything except formatting of log message and metadata. This is an
Expand Down Expand Up @@ -145,7 +146,7 @@ public final void log(LogRecord record, boolean wasForced) {
// would get a lot of extra stack frames to search through). However for Flogger, the LogSite
// was already determined in the "shouldLog()" method because it's needed for things like
// rate limiting. Thus we don't have to care about using iterative methods vs recursion here.
private static void publish(Logger logger, LogRecord record) {
private static void publish(@Nullable Logger logger, LogRecord record) {
// Annoyingly this method appears to copy the array every time it is called, but there's
// nothing much we can do about this (and there could be synchronization issues even if we
// could access things directly because handlers can be changed at any time). Most of the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.ResourceBundle;
import java.util.logging.Formatter;
import java.util.logging.LogRecord;
import org.jspecify.annotations.Nullable;

/**
* Abstract base class for {@code java.util.logging} compatible log records produced by Flogger
Expand Down Expand Up @@ -157,7 +158,7 @@ protected LogMessageFormatter getLogMessageFormatter() {
}

@Override
public final void setParameters(Object[] parameters) {
public final void setParameters(Object @Nullable [] parameters) {
// IMPORTANT: We call getMessage() to cache the internal formatted message if someone indicates
// they want to change the parameters. This is to avoid a situation in which parameters are set,
// but the underlying message is still null. Do this first to switch internal states.
Expand All @@ -170,7 +171,7 @@ public final void setParameters(Object[] parameters) {
}

@Override
public final void setMessage(String message) {
public final void setMessage(@Nullable String message) {
if (message == null) {
message = "";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ public ScopeList(ScopeType key, LoggingScope scope, @Nullable ScopeList next) {
* reference to a builder instance for any length of time is not recommended.
*/
public abstract static class Builder {
private Tags tags = null;
private ContextMetadata.Builder metadata = null;
private LogLevelMap logLevelMap = null;
private @Nullable Tags tags = null;
private ContextMetadata.@Nullable Builder metadata = null;
private @Nullable LogLevelMap logLevelMap = null;
private boolean hasLogLevelMap = false;

protected Builder() {}
Expand Down
6 changes: 3 additions & 3 deletions api/src/main/java/com/google/common/flogger/context/Tags.java
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,8 @@ public int compare(Object s1, Object s2) {
new SortedArraySet<Entry<String, Set<Object>>>(-1);

// Cache these if anyone needs them (not likely in normal usage).
private Integer hashCode = null;
private String toString = null;
private @Nullable Integer hashCode = null;
private @Nullable String toString = null;

// ---- Singleton constructor ----

Expand Down Expand Up @@ -641,7 +641,7 @@ private Map.Entry<String, SortedArraySet<Object>> newEntry(String key, int index
}

@SuppressWarnings("unchecked") // Safe when the index is in range.
private Map.Entry<String, SortedArraySet<Object>> getEntryOrNull(int index) {
private Map.@Nullable Entry<String, SortedArraySet<Object>> getEntryOrNull(int index) {
return index < offsets[0] ? (Map.Entry<String, SortedArraySet<Object>>) array[index] : null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private static StackGetter getBestStackGetter() {
return new ThrowableStackGetter();
}

private static StackGetter maybeCreateStackGetter(String className) {
private static @Nullable StackGetter maybeCreateStackGetter(String className) {
try {
return Class.forName(className)
.asSubclass(StackGetter.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static <T> T getInstanceFromSystemProperty(String propertyName, Class<T>
return null;
}

private static String readProperty(String propertyName, @Nullable String defaultValue) {
private static @Nullable String readProperty(String propertyName, @Nullable String defaultValue) {
Checks.checkNotNull(propertyName, "property name");
try {
return System.getProperty(propertyName, defaultValue);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@

import static com.google.common.flogger.util.Checks.checkArgument;

import org.jspecify.annotations.Nullable;

/** Default implementation of {@link StackGetter} using {@link Throwable#getStackTrace}. */
final class ThrowableStackGetter implements StackGetter {

@Override
public StackTraceElement callerOf(Class<?> target, int skipFrames) {
public @Nullable StackTraceElement callerOf(Class<?> target, int skipFrames) {
checkArgument(skipFrames >= 0, "skipFrames must be >= 0");
StackTraceElement[] stack = new Throwable().getStackTrace();
int callerIndex = findCallerIndex(stack, target, skipFrames + 1);
Expand Down
Loading

0 comments on commit 61a0b09

Please sign in to comment.