Skip to content

Commit

Permalink
Add some null-safety annotations and suppressions.
Browse files Browse the repository at this point in the history
RELNOTES=n/a
PiperOrigin-RevId: 691819089
  • Loading branch information
netdpb authored and Flogger Team committed Nov 21, 2024
1 parent febddca commit 5b3baeb
Show file tree
Hide file tree
Showing 12 changed files with 57 additions and 26 deletions.
2 changes: 1 addition & 1 deletion MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ bazel_dep(name = "rules_java", version = "7.12.1")
bazel_dep(name = "google_bazel_common")
git_override(
module_name = "google_bazel_common",
commit = "e2204d625d97dda88112af0b0a9e56ed7712d15c",
commit = "a7761f9d45b65283274d8d86abb3b87e9fc87258",
remote = "https://github.com/google/bazel-common",
)
5 changes: 4 additions & 1 deletion api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ genrule(
srcs = STACK_GETTER_STACK_WALKER_IMPL_SRCS + [
":stack_getter_common",
":checks",
"@google_bazel_common//third_party/java/jspecify_annotations",
],
outs = ["stack_getter_stack_walker_impl.jar"],
cmd = """
Expand All @@ -97,9 +98,11 @@ TMPDIR=$$(mktemp -d)
OUTPUT=$$PWD/$@
STACK_GETTER_COMMON=$(location :stack_getter_common)
CHECKS=$(location :checks)
JSPECIFY_ANNOTATIONS=$(location @google_bazel_common//third_party/java/jspecify_annotations)
JAVABASE=$$PWD/$(JAVABASE)
JAR=$$PWD/$(JAVABASE)/bin/jar
$$JAVABASE/bin/javac -d $$TMPDIR -source 8 -target 8 -cp $${STACK_GETTER_COMMON}:$${CHECKS} -implicit:none \
$$JAVABASE/bin/javac -d $$TMPDIR -source 8 -target 8 \
-cp $${STACK_GETTER_COMMON}:$${CHECKS}:$${JSPECIFY_ANNOTATIONS} -implicit:none \
$(location src/main/java/com/google/common/flogger/util/StackWalkerStackGetter.java)
cd $$TMPDIR && $$JAR cf $$OUTPUT com/google/common/flogger/util/*StackGetter*.class
""",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.common.flogger.backend;

import static java.util.Objects.requireNonNull;
import static org.objectweb.asm.Opcodes.ACC_FINAL;
import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
Expand Down Expand Up @@ -108,7 +109,7 @@ public static void main(String[] args) throws IOException {

// Write the class to the output file.
Path path = Paths.get(args[0]);
Files.createDirectories(path.getParent());
Files.createDirectories(requireNonNull(path.getParent(), "path must have a parent: " + path));
try (JarOutputStream jar =
new JarOutputStream(Files.newOutputStream(path, StandardOpenOption.CREATE_NEW))) {
ZipEntry entry = new ZipEntry("com/google/common/flogger/backend/PlatformProvider.class");
Expand Down
13 changes: 8 additions & 5 deletions api/src/main/java/com/google/common/flogger/LogContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.common.flogger.util.CallerFinder.getStackForCallerOf;
import static com.google.common.flogger.util.Checks.checkNotNull;
import static com.google.common.flogger.util.Checks.checkState;
import static java.util.concurrent.TimeUnit.NANOSECONDS;

import com.google.common.flogger.DurationRateLimiter.RateLimitPeriod;
Expand Down Expand Up @@ -435,16 +436,18 @@ public final LogSite getLogSite() {

@Override
public final Object[] getArguments() {
if (templateContext == null) {
throw new IllegalStateException("cannot get arguments unless a template context exists");
checkState(templateContext != null, "cannot get arguments unless a template context exists");
if (args == null) {
throw new IllegalStateException("cannot get arguments before calling log()");
}
return args;
}

@Override
public final Object getLiteralArgument() {
if (templateContext != null) {
throw new IllegalStateException("cannot get literal argument if a template context exists");
checkState(templateContext == null, "cannot get literal argument if a template context exists");
if (args == null) {
throw new IllegalStateException("cannot get literal argument before calling log()");
}
return args[0];
}
Expand Down Expand Up @@ -683,7 +686,7 @@ private boolean shouldLog() {
if (rateLimitStatus != null) {
// We check rate limit status even if it is "DISALLOW" to update the skipped logs count.
int skippedLogs = RateLimitStatus.checkStatus(rateLimitStatus, logSiteKey, metadata);
if (shouldLog && skippedLogs > 0) {
if (shouldLog && skippedLogs > 0 && metadata != null) {
metadata.addValue(Key.SKIPPED_LOG_COUNT, skippedLogs);
}
// checkStatus() returns -1 in two cases:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ protected Random initialValue() {
}
};

// Android annotates ThreadLocal.get() with @RecentlyNullable, but random.get() is never null.
@SuppressWarnings("nullness")
private static Random getRandom() {
return random.get();
}

// Visible for testing.
final AtomicInteger pendingCount = new AtomicInteger();

Expand All @@ -72,7 +78,7 @@ RateLimitStatus sampleOneIn(int rateLimitCount) {
// zero and the number of logs will end up statistically close to 1-in-N (even if at
// times they can be "bursty" due to the action of other rate limiting mechanisms).
int pending;
if (random.get().nextInt(rateLimitCount) == 0) {
if (getRandom().nextInt(rateLimitCount) == 0) {
pending = pendingCount.incrementAndGet();
} else {
pending = pendingCount.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,12 @@ private static Platform loadFirstAvailablePlatform(String[] platformClass) {
return (Platform) Class.forName(clazz).getConstructor().newInstance();
} catch (Throwable e) {
// Catch errors so if we can't find _any_ implementations, we can report something useful.
// Unwrap any generic wrapper exceptions for readability here (extend this as needed).
if (e instanceof InvocationTargetException) {
e = e.getCause();
}
errorMessage.append('\n').append(clazz).append(": ").append(e);
errorMessage
.append('\n')
.append(clazz)
.append(": ")
// Unwrap any wrapper exceptions for readability here (extend as needed).
.append(e instanceof InvocationTargetException ? e.getCause() : e);
}
}
throw new IllegalStateException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
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 @@ -146,7 +145,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(@Nullable Logger logger, LogRecord record) {
private static void publish(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 All @@ -155,9 +154,9 @@ private static void publish(@Nullable Logger logger, LogRecord record) {
handler.publish(record);
}
if (logger.getUseParentHandlers()) {
logger = logger.getParent();
if (logger != null) {
publish(logger, record);
Logger parent = logger.getParent();
if (parent != null) {
publish(parent, record);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ public final void setParameters(Object @Nullable [] parameters) {
super.setParameters(parameters);
}

@Override
@SuppressWarnings("nullness") // setParameters() above guarantees this is never null.
public final Object[] getParameters() {
return super.getParameters();
}

@Override
public final void setMessage(@Nullable String message) {
if (message == null) {
Expand Down
9 changes: 7 additions & 2 deletions api/src/main/java/com/google/common/flogger/context/Tags.java
Original file line number Diff line number Diff line change
Expand Up @@ -513,8 +513,12 @@ private int mergeTagMaps(
int newEntryIndex = 0;
while (lhsEntry != null || rhsEntry != null) {
// Nulls count as being *bigger* than anything (since they indicate the end of the array).
int signum = (lhsEntry == null) ? 1 : (rhsEntry == null) ? -1 : 0;
if (signum == 0) {
int signum;
if (lhsEntry == null) {
signum = 1;
} else if (rhsEntry == null) {
signum = -1;
} else {
// Both entries exist and must be compared.
signum = lhsEntry.getKey().compareTo(rhsEntry.getKey());
if (signum == 0) {
Expand All @@ -528,6 +532,7 @@ private int mergeTagMaps(
continue;
}
}

// Signum is non-zero and indicates which entry to process next (without merging).
if (signum < 0) {
valueStart = copyEntryAndValues(lhsEntry, newEntryIndex++, valueStart, array, offsets);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static SimpleParameter of(int index, FormatChar formatChar, FormatOptions
// We can safely test FormatSpec with '==' because the factory methods always return the default
// instance if applicable (and the class has no visible constructors).
if (index < MAX_CACHED_PARAMETERS && options.isDefault()) {
return DEFAULT_PARAMETERS.get(formatChar)[index];
return checkNotNull(DEFAULT_PARAMETERS.get(formatChar), "default parameter")[index];
}
return new SimpleParameter(index, formatChar, options);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,15 @@ protected RecursionDepth initialValue() {
}
};

// Android annotates ThreadLocal.get() with @RecentlyNullable, but holder.get() is never null.
@SuppressWarnings("nullness")
private static RecursionDepth getRecursionDepth() {
return holder.get();
}

/** Do not call this method directly, use {@code Platform.getCurrentRecursionDepth()}. */
public static int getCurrentDepth() {
return holder.get().value;
return getRecursionDepth().value;
}

/** Do not call this method directly, use {@code Platform.getCurrentRecursionDepth()}. */
Expand All @@ -48,7 +54,7 @@ public int getValue() {

/** Internal API for use by core Flogger library. */
public static RecursionDepth enterLogStatement() {
RecursionDepth depth = holder.get();
RecursionDepth depth = getRecursionDepth();
// Can only reach 0 if it wrapped around completely or someone is manipulating the value badly.
// We really don't expect 2^32 levels of recursion however, so assume it's a bug.
if (++depth.value == 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.lang.StackWalker.StackFrame;
import java.util.function.Predicate;
import java.util.stream.Stream;
import org.jspecify.annotations.Nullable;

/**
* StackWalker based implementation of the {@link StackGetter} interface.
Expand All @@ -39,9 +40,9 @@ public StackWalkerStackGetter() {
}

@Override
public StackTraceElement callerOf(Class<?> target, int skipFrames) {
public @Nullable StackTraceElement callerOf(Class<?> target, int skipFrames) {
checkArgument(skipFrames >= 0, "skipFrames must be >= 0");
return STACK_WALKER.walk(
return STACK_WALKER.<@Nullable StackTraceElement>walk(
s ->
filterStackTraceAfterTarget(isTargetClass(target), skipFrames, s)
.findFirst()
Expand Down

0 comments on commit 5b3baeb

Please sign in to comment.