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 checkerframework and errorprone #2941

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
WIP Fix error from checkerframework and errorprone
  • Loading branch information
juherr committed Aug 7, 2023
commit d6fbea39881c4f6700ee917af3242b75fe797d58
47 changes: 33 additions & 14 deletions testng-asserts/src/main/java/org/testng/Assert.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ public static void fail(String message, Throwable realCause) {
*
* @param message the assertion error message
*/
public static void fail(@Nullable String message) {
// See https://github.com/typetools/checker-framework/issues/2076
public static /* @ThrowsException */ void fail(@Nullable String message) {
throw new AssertionError(message);
}

Expand Down Expand Up @@ -183,7 +184,8 @@ private static boolean areEqualImpl(@Nullable Object actual, @Nullable Object ex
}

/** returns not equal reason or null if equal */
private static @Nullable String getArrayNotEqualReason(Object actual, Object expected) {
private static @Nullable String getArrayNotEqualReason(
@Nullable Object actual, @Nullable Object expected) {
if (Objects.equals(actual, expected)) {
return null;
}
Expand Down Expand Up @@ -214,14 +216,16 @@ private static boolean areArraysEqual(Object actual, Object expected) {
return getArrayNotEqualReason(actual, expected) == null;
}

private static void assertArrayEquals(Object actual, Object expected, @Nullable String message) {
private static void assertArrayEquals(
@Nullable Object actual, @Nullable Object expected, @Nullable String message) {
String reason = getArrayNotEqualReason(actual, expected);
if (null != reason) {
failNotEquals(actual, expected, message == null ? "" : message + " (" + message + ")");
}
}

private static void assertArrayNotEquals(Object actual, Object expected, String message) {
private static void assertArrayNotEquals(
@Nullable Object actual, @Nullable Object expected, @Nullable String message) {
String reason = getArrayNotEqualReason(actual, expected);
if (null == reason) {
failEquals(actual, expected, message);
Expand Down Expand Up @@ -1584,7 +1588,8 @@ private static void failNotEquals(
fail(format(actual, expected, message, true));
}

private static void failEquals(Object actual, Object expected, String message) {
private static void failEquals(
@Nullable Object actual, @Nullable Object expected, @Nullable String message) {
fail(format(actual, expected, message, false));
}

Expand Down Expand Up @@ -1612,7 +1617,8 @@ static String format(
* @param actual the actual value
* @param expected the expected value
*/
public static void assertEquals(Collection<?> actual, Collection<?> expected) {
public static void assertEquals(
@Nullable Collection<@Nullable ?> actual, @Nullable Collection<@Nullable ?> expected) {
assertEquals(actual, expected, null);
}

Expand All @@ -1625,7 +1631,9 @@ public static void assertEquals(Collection<?> actual, Collection<?> expected) {
* @param message the assertion error message
*/
public static void assertEquals(
Collection<?> actual, Collection<?> expected, @Nullable String message) {
@Nullable Collection<@Nullable ?> actual,
@Nullable Collection<@Nullable ?> expected,
@Nullable String message) {
if (actual == expected) { // We don't use Objects.equals here because order is checked
return;
}
Expand All @@ -1636,6 +1644,7 @@ public static void assertEquals(
} else {
fail("Collections not equal: expected: " + expected + " and actual: " + actual);
}
return;
}

assertEquals(
Expand Down Expand Up @@ -1766,7 +1775,9 @@ public static void assertEquals(
* @param message the assertion error message
*/
public static void assertEquals(
@Nullable Object[] actual, @Nullable Object[] expected, @Nullable String message) {
@Nullable Object @Nullable [] actual,
@Nullable Object @Nullable [] expected,
@Nullable String message) {
if (Arrays.equals(actual, expected)) {
return;
}
Expand All @@ -1781,6 +1792,7 @@ public static void assertEquals(
+ " and actual: "
+ Arrays.toString(actual));
}
return;
}
if (actual.length != expected.length) {
failAssertNoEqual(
Expand Down Expand Up @@ -1818,7 +1830,9 @@ public static void assertEquals(
* @param message the assertion error message
*/
public static void assertEqualsNoOrder(
@Nullable Object[] actual, @Nullable Object[] expected, @Nullable String message) {
@Nullable Object @Nullable [] actual,
@Nullable Object @Nullable [] expected,
@Nullable String message) {
if (actual == expected) { // We don't use Arrays.equals here because order is not checked
return;
}
Expand All @@ -1836,6 +1850,7 @@ public static void assertEqualsNoOrder(
if (actual.length != expected.length) {
failAssertNoEqual(
"Arrays do not have the same size:" + actual.length + " != " + expected.length, message);
return;
}

List<Object> actualCollection = Lists.newArrayList(actual);
Expand All @@ -1861,7 +1876,9 @@ public static void assertEqualsNoOrder(
* @param message the assertion error message
*/
public static void assertEqualsNoOrder(
@Nullable Collection<?> actual, @Nullable Collection<?> expected, @Nullable String message) {
@Nullable Collection<@Nullable ?> actual,
@Nullable Collection<@Nullable ?> expected,
@Nullable String message) {
if (actual.size() != expected.size()) {
failAssertNoEqual(
"Collections do not have the same size: " + actual.size() + " != " + expected.size(),
Expand All @@ -1886,7 +1903,7 @@ public static void assertEqualsNoOrder(
* @param message the assertion error message
*/
public static void assertEqualsNoOrder(
@Nullable Iterator<?> actual, @Nullable Iterator<?> expected, @Nullable String message) {
Iterator<@Nullable ?> actual, Iterator<@Nullable ?> expected, @Nullable String message) {
List<?> actualCollection = Lists.newArrayList(actual);
List<?> expectedCollection = Lists.newArrayList(expected);

Expand Down Expand Up @@ -1921,7 +1938,7 @@ public static void assertEqualsNoOrder(
.collect(Collectors.joining(", "));
}

private static void failAssertNoEqual(String defaultMessage, String message) {
private static void failAssertNoEqual(String defaultMessage, @Nullable String message) {
if (message != null) {
fail(message);
} else {
Expand All @@ -1936,7 +1953,8 @@ private static void failAssertNoEqual(String defaultMessage, String message) {
* @param actual the actual value
* @param expected the expected value
*/
public static void assertEquals(Object[] actual, Object[] expected) {
public static void assertEquals(
@Nullable Object @Nullable [] actual, @Nullable Object @Nullable [] expected) {
assertEquals(actual, expected, null);
}

Expand All @@ -1949,7 +1967,8 @@ public static void assertEquals(Object[] actual, Object[] expected) {
* @param actual the actual value
* @param expected the expected value
*/
public static void assertEqualsNoOrder(@Nullable Object[] actual, @Nullable Object[] expected) {
public static void assertEqualsNoOrder(
@Nullable Object @Nullable [] actual, @Nullable Object @Nullable [] expected) {
assertEqualsNoOrder(actual, expected, null);
}

Expand Down
15 changes: 8 additions & 7 deletions testng-asserts/src/main/java/org/testng/FileAssert.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ private FileAssert() {
* @param tstvalue the file to evaluate
* @param message the assertion error message
*/
public static void assertDirectory(File tstvalue, String message) {
public static void assertDirectory(File tstvalue, @Nullable String message) {
boolean condition = false;
try {
condition = tstvalue.isDirectory();
Expand Down Expand Up @@ -108,7 +108,7 @@ public static void assertLength(File tstvalue, long expected) {
* @param expected the expected value
* @param message the assertion error message
*/
public static void assertMinLength(File tstvalue, long expected, String message) {
public static void assertMinLength(File tstvalue, long expected, @Nullable String message) {
long actual = -1L;
try {
actual = tstvalue.isDirectory() ? tstvalue.list().length : tstvalue.length();
Expand Down Expand Up @@ -139,7 +139,7 @@ public static void assertMinLength(File tstvalue, long expected) {
* @param expected The expected max length
* @param message the assertion error message
*/
public static void assertMaxLength(File tstvalue, long expected, String message) {
public static void assertMaxLength(File tstvalue, long expected, @Nullable String message) {
long actual = -1L;
try {
actual = tstvalue.isDirectory() ? tstvalue.list().length : tstvalue.length();
Expand Down Expand Up @@ -168,7 +168,7 @@ public static void assertMaxLength(File tstvalue, long expected) {
* @param tstvalue the file to evaluate
* @param message the assertion error message
*/
public static void assertReadable(File tstvalue, String message) {
public static void assertReadable(File tstvalue, @Nullable String message) {
boolean condition = false;
try {
condition = tstvalue.canRead();
Expand Down Expand Up @@ -222,7 +222,7 @@ public static void assertWriteable(File tstvalue) {
* @param tstvalue the file to evaluate
* @param message the assertion error message
*/
public static void assertReadWrite(File tstvalue, String message) {
public static void assertReadWrite(File tstvalue, @Nullable String message) {
boolean condition = false;
try {
condition = tstvalue.canRead() && tstvalue.canWrite();
Expand Down Expand Up @@ -260,7 +260,7 @@ public static void fail(String message, Throwable realCause) {
*
* @param message the assertion error message
*/
public static void fail(String message) {
public static void fail(@Nullable String message) {
throw new AssertionError(message);
}

Expand All @@ -270,7 +270,8 @@ public static void fail() {
}

/** Formats failure for file assertions. */
private static void failFile(File path, String actual, String expected, String message) {
private static void failFile(
File path, String actual, @Nullable String expected, @Nullable String message) {
String formatted = "";
if (message != null) {
formatted = message + " ";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ public SimpleAssert(String message) {
this(null, null, message);
}

public SimpleAssert(T actual, T expected) {
public SimpleAssert(@Nullable T actual, @Nullable T expected) {
this(actual, expected, null);
}

public SimpleAssert(T actual, T expected, String message) {
public SimpleAssert(@Nullable T actual, @Nullable T expected, String message) {
this.actual = actual;
this.expected = expected;
m_message = message;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
package org.testng.asserts;

import org.checkerframework.checker.nullness.qual.Nullable;

public interface IAssert<T> {
@Nullable
String getMessage();

void doAssert();

@Nullable
T getActual();

@Nullable
T getExpected();
}
4 changes: 1 addition & 3 deletions testng-asserts/src/main/java/org/testng/package-info.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.ALL)
package org.testng;

import org.checkerframework.checker.nullness.qual.NonNull;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.ALL)
package org.testng;

import org.checkerframework.checker.nullness.qual.NonNull;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static String escapeHtml(String text) {
return result;
}

public static String valueOf(Map<?, ?> m) {
public static <K, V> String valueOf(Map<K, V> m) {
return m.values().stream().map(Object::toString).collect(Collectors.joining(" "));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ public interface IObjectFactory2 extends ITestObjectFactory {
*/
@Deprecated
default Object newInstance(Class<?> cls) {
return newInstance(cls, new Object[0]);
return newInstance((Class<Object>) cls, new Object[0]);
}
}
2 changes: 1 addition & 1 deletion testng-core-api/src/main/java/org/testng/ISuite.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ public interface ISuite extends IAttributes {
/** Returns the object factory used to create all test instances. */
ITestObjectFactory getObjectFactory();

@Deprecated
/**
* Returns null.
*
* @deprecated This interface stands deprecated as of TestNG 7.5.0
*/
@Deprecated
default @Nullable IObjectFactory2 getObjectFactory2() {
return null;
}
Expand Down
3 changes: 2 additions & 1 deletion testng-core-api/src/main/java/org/testng/ITestResult.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.testng.internal.thread.ThreadTimeoutException;

/**
Expand Down Expand Up @@ -143,7 +144,7 @@ static List<String> finalStatuses() {
* @param result - The test result of a method
*/
static boolean wasFailureDueToTimeout(ITestResult result) {
Throwable cause = result.getThrowable();
@Nullable Throwable cause = result.getThrowable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking, @Nullable should be automatically inferred for local variables, so this could probably be omitted

Suggested change
@Nullable Throwable cause = result.getThrowable();
Throwable cause = result.getThrowable();

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose you're right.

It means I should not use TypeUseLocation.ALL in @DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.ALL)
Any other option I should exclude? https://checkerframework.org/api/org/checkerframework/framework/qual/TypeUseLocation.html

while (cause != null && !cause.getClass().equals(Throwable.class)) {
if (cause instanceof ThreadTimeoutException) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public static void addClassLoader(final ClassLoader loader) {

static List<ClassLoader> appendContextualClassLoaders(List<ClassLoader> currentLoaders) {
List<ClassLoader> allClassLoaders = Lists.newArrayList();
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
@Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();

?

if (contextClassLoader != null) {
allClassLoaders.add(contextClassLoader);
}
Expand Down Expand Up @@ -172,7 +172,7 @@ public static Set<Method> getAvailableMethods(Class<?> clazz) {
appendMethod(methods, declaredMethod);
}

Class<?> parent = clazz.getSuperclass();
@Nullable Class<?> parent = clazz.getSuperclass();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable Class<?> parent = clazz.getSuperclass();
Class<?> parent = clazz.getSuperclass();

while (parent != null && !Object.class.equals(parent)) {
Set<Map.Entry<String, Set<Method>>> extractedMethods =
extractMethods(clazz, parent, methods).entrySet();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
import java.lang.reflect.Executable;
import java.lang.reflect.Method;
import java.util.Objects;
import org.checkerframework.checker.nullness.qual.Nullable;

/** Encapsulation of either a method or a constructor. */
public class ConstructorOrMethod {

private Method m_method;
private Constructor<?> m_constructor;
private @Nullable Method m_method;
private @Nullable Constructor<?> m_constructor;
private boolean m_enabled = true;

public ConstructorOrMethod(Method m) {
Expand Down Expand Up @@ -62,7 +63,7 @@ private Executable getInternalConstructorOrMethod() {
}

@Override
public boolean equals(Object o) {
public boolean equals(@Nullable Object o) {
if (this == o) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* @author <a href="mailto:cedric@beust.com">Cedric Beust</a>
*/
public class PackageUtils {
private static String[] testClassPaths;
private static @Nullable String[] testClassPaths;

/** The additional class loaders to find classes in. */
private static final Collection<ClassLoader> classLoaders = new ConcurrentLinkedDeque<>();
Expand Down Expand Up @@ -85,7 +85,7 @@ public static String[] findClassesInPackage(
return testClassPaths;
}

String testClasspath = RuntimeBehavior.getTestClasspath();
@Nullable String testClasspath = RuntimeBehavior.getTestClasspath();
if (null == testClasspath) {
return null;
}
Expand Down Expand Up @@ -125,7 +125,7 @@ private static Function<ClassLoader, Stream<URL>> asURLs(String packageDir) {
}

private static boolean matchTestClasspath(URL url, String lastFragment, boolean recursive) {
String[] classpathFragments = getTestClasspath();
@Nullable String[] classpathFragments = getTestClasspath();
if (null == classpathFragments) {
return true;
}
Expand Down
Loading