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

Enable Nullaway #988

Merged
merged 5 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ apply plugin: 'com.palantir.consistent-versions'
version System.env.CIRCLE_TAG ?: gitVersion()

allprojects {
apply plugin: 'com.palantir.baseline-null-away'
apply plugin: 'com.palantir.java-format'
apply plugin: 'com.palantir.jakarta-package-alignment'
group = 'com.palantir.javaformat'
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-988.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Enable Nullaway
links:
- https://github.com/palantir/palantir-java-format/pull/988
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.palantir.javaformat.java.SnippetFormatter.SnippetKind;
import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nullable;
import org.eclipse.jdt.core.dom.ASTParser;
import org.eclipse.jdt.core.formatter.CodeFormatter;
import org.eclipse.jface.text.IRegion;
Expand All @@ -32,13 +33,15 @@ public class PalantirJavaFormatter extends CodeFormatter {

private static final int INDENTATION_SIZE = 4;

@Nullable
@Override
public TextEdit format(
int kind, String source, int offset, int length, int indentationLevel, String _lineSeparator) {
IRegion[] regions = new IRegion[] {new Region(offset, length)};
return formatInternal(kind, source, regions, indentationLevel);
}

@Nullable
@Override
public TextEdit format(int kind, String source, IRegion[] regions, int indentationLevel, String _lineSeparator) {
return formatInternal(kind, source, regions, indentationLevel);
Expand All @@ -57,6 +60,7 @@ public String createIndentationString(int indentationLevel) {
}

/** Runs the Google Java formatter on the given source, with only the given ranges specified. */
@Nullable
private TextEdit formatInternal(int kind, String source, IRegion[] regions, int initialIndent) {
try {
boolean includeComments = (kind & CodeFormatter.F_INCLUDE_COMMENTS) == CodeFormatter.F_INCLUDE_COMMENTS;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import javax.annotation.Nullable;

/** A parser for {@link CommandLineOptions}. */
final class CommandLineOptionsParser {
Expand All @@ -50,7 +51,7 @@ static CommandLineOptions parse(Iterable<String> options) {
break;
}
String flag;
String value;
@Nullable String value;
int idx = option.indexOf('=');
if (idx >= 0) {
flag = option.substring(0, idx);
Expand Down Expand Up @@ -151,7 +152,7 @@ private static Integer parseInteger(Iterator<String> it, String flag, String val
}
}

private static String getValue(String flag, Iterator<String> it, String value) {
private static String getValue(String flag, Iterator<String> it, @Nullable String value) {
if (value != null) {
return value;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.annotation.Nullable;

/** {@code JavaCommentsHelper} extends {@link CommentsHelper} to rewrite Java comments. */
public final class JavaCommentsHelper implements CommentsHelper {

private final String lineSeparator;
private final JavaFormatterOptions options;

@Nullable
private final JavadocFormatter javadocFormatter;

public JavaCommentsHelper(String lineSeparator, JavaFormatterOptions options) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -130,7 +132,7 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti
}
}

for (Map.Entry<Path, Future<String>> result : results.entrySet()) {
for (Entry<Path, Future<String>> result : results.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional? I'm surprised this didn't trigger error-prone BadImport

Suggested change
for (Entry<Path, Future<String>> result : results.entrySet()) {
for (Map.Entry<Path, Future<String>> result : results.entrySet()) {

Copy link
Contributor Author

@ash211 ash211 Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, looks like Entry was removed from the BadImport list 3 years ago, but my brain has already been conditioned to qualify Entry.

google/error-prone@38a0e4b

Path path = result.getKey();
String formatted;
try {
Expand All @@ -145,8 +147,11 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti
errWriter.println(path + ":" + diagnostic.toString());
}
} else {
errWriter.println(path + ": error: " + e.getCause().getMessage());
e.getCause().printStackTrace(errWriter);
errWriter.println(path + ": error: "
+ Optional.ofNullable(e.getCause())
.map(Throwable::getMessage)
.orElse("null"));
Optional.ofNullable(e.getCause()).ifPresent(cause -> cause.printStackTrace(errWriter));
}
allOk = false;
continue;
Expand Down Expand Up @@ -215,10 +220,10 @@ public static CommandLineOptions processArgs(String... args) throws UsageExcepti
try {
parameters = CommandLineOptionsParser.parse(Arrays.asList(args));
} catch (IllegalArgumentException e) {
throw new UsageException(e.getMessage());
throw new UsageException(Optional.ofNullable(e.getMessage()).orElse("null"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to change this, but in other types of hot paths java.until.Objects.requireNonNullElse can avoid the Optional allocation

Suggested change
throw new UsageException(Optional.ofNullable(e.getMessage()).orElse("null"));
throw new UsageException(Objects. requireNonNullElse(e.getMessage(), "null"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing -- I've needed this before and its good to know a way to do this without the allocation

} catch (Throwable t) {
t.printStackTrace();
throw new UsageException(t.getMessage());
throw new UsageException(Optional.ofNullable(t.getMessage()).orElse("null"));
}
int filesToFormat = parameters.files().size();
if (parameters.stdin()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.base.Joiner;
import javax.annotation.Nullable;

/** Checked exception class for formatter command-line usage errors. */
final class UsageException extends Exception {
Expand Down Expand Up @@ -88,7 +89,7 @@ final class UsageException extends Exception {
super(buildMessage(checkNotNull(message)));
}

private static String buildMessage(String message) {
private static String buildMessage(@Nullable String message) {
StringBuilder builder = new StringBuilder();
if (message != null) {
builder.append(message).append('\n');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import javax.annotation.Nullable;

/**
* Stateful object that accepts "requests" and "writes," producing formatted Javadoc.
Expand All @@ -53,7 +54,10 @@ final class JavadocWriter {
private int remainingOnLine;
private boolean atStartOfLine;
private RequestedWhitespace requestedWhitespace = NONE;

@Nullable
private Token requestedMoeBeginStripComment;

private int indentForMoeEndStripComment;
private boolean wroteAnythingSignificant;

Expand Down