Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some null-safety annotations and suppressions. #391

Merged
merged 1 commit into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading