Prefer NullPointerException for Null pointers #22723

elharo · 2024-05-12T15:00:53Z

Description

checkArgument should not be used for null checks. Use checkNotNull/requireNonNull instead.

Motivation and Context

As recommended by Effective Java

Impact

Many IllegalArgumentExceptions are now NullPointerExceptions

Test Plan

CI

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== NO RELEASE NOTE ==

sdruzkin · 2024-05-13T16:40:27Z

...code/src/main/java/com/facebook/presto/bytecode/expression/ArithmeticBytecodeExpression.java

@@ -157,7 +157,7 @@ private static Class<?> getPrimitiveType(BytecodeExpression expression, String n
    {
        requireNonNull(expression, name + " is null");
        Class<?> leftType = expression.getType().getPrimitiveType();
-        checkArgument(leftType != null, name + " is not a primitive");
+        requireNonNull(leftType, name + " is not a primitive");


Update the message to use %s instead of string concat

sdruzkin · 2024-05-13T16:40:36Z

...code/src/main/java/com/facebook/presto/bytecode/expression/ComparisonBytecodeExpression.java

@@ -253,7 +253,7 @@ private static Class<?> getPrimitiveType(BytecodeExpression expression, String n
    {
        requireNonNull(expression, name + " is null");
        Class<?> leftType = expression.getType().getPrimitiveType();
-        checkArgument(leftType != null, name + " is not a primitive");
+        requireNonNull(leftType, name + " is not a primitive");


Update the message to use %s instead of string concat

sdruzkin · 2024-05-13T16:43:09Z

presto-cassandra/src/main/java/com/facebook/presto/cassandra/CassandraPageSink.java

@@ -98,7 +98,7 @@ public CassandraPageSink(
        }
        for (int i = 0; i < columnNames.size(); i++) {
            String columnName = columnNames.get(i);
-            checkArgument(columnName != null, "columnName is null at position: %d", i);
+            checkNotNull(columnName, "columnName is null at position: %d", i);


Preconditions recognizes only %s. Replace %d with %s.

sdruzkin · 2024-05-13T16:47:40Z

...on-namespace/src/test/java/com/facebook/presto/hive/functions/AbstractTestHiveFunctions.java

@@ -92,7 +92,7 @@ public void assertInvalidFunction(String expr, String exceptionPattern)

    public void assertQuery(@Language("SQL") String sql, Column... cols)
    {
-        checkArgument(cols != null && cols.length > 0);
+        checkArgument(cols.length > 0);


This is incorrect replacement, it will fail when cols is null

sdruzkin · 2024-05-13T16:48:05Z

...namespace/src/test/java/com/facebook/presto/hive/functions/TestHiveAggregationFunctions.java

@@ -112,7 +112,7 @@ public void aggregationFunctions()
    @SuppressWarnings("UnknownLanguage")
    public void check(@Language("SQL") String query, Column... expectedColumns)
    {
-        checkArgument(expectedColumns != null && expectedColumns.length > 0);
+        checkArgument(expectedColumns.length > 0);


This is incorrect replacement, it will fail when expectedColumns is null

Yes, it fails with a null pointer exception, exactly as desired

sdruzkin · 2024-05-13T16:52:14Z

...ain/java/com/facebook/presto/operator/scalar/annotations/ParametricScalarImplementation.java

@@ -483,7 +483,7 @@ private Parser(ScalarImplementationHeader header, Method method, Optional<Constr
                    .collect(toImmutableSet());

            SqlType returnType = method.getAnnotation(SqlType.class);
-            checkArgument(returnType != null, format("Method [%s] is missing @SqlType annotation", method));
+            requireNonNull(returnType, format("Method [%s] is missing @SqlType annotation", method));


Remove format

sdruzkin · 2024-05-13T16:53:49Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RowExpressionInterpreter.java

@@ -748,8 +748,8 @@ private Object invokeOperator(OperatorType operatorType, List<? extends Type> ar

        private List<RowExpression> toRowExpressions(List<Object> values, List<RowExpression> unchangedValues)
        {
-            checkArgument(values != null, "value is null");
-            checkArgument(unchangedValues != null, "value is null");
+            requireNonNull(values, "value is null");


Please update the message to use the right "value" name

sdruzkin · 2024-05-13T16:55:33Z

...c/src/test/java/com/facebook/presto/orc/metadata/statistics/TestStringStatisticsBuilder.java

@@ -315,7 +315,7 @@ private void assertMergedStringStatistics(List<ColumnStatistics> statisticsList,

    private static void assertMinMaxValuesWithLimit(Slice expectedMin, Slice expectedMax, List<Slice> values, int limit)
    {
-        checkArgument(values != null && values.size() > 0);
+        checkArgument(values.size() > 0);


This is incorrect replacement

This is deliberate. values== null causes a NullPointerException. values <= 0 causes an IllegalArgumentException

sdruzkin · 2024-05-13T16:55:36Z

...c/src/test/java/com/facebook/presto/orc/metadata/statistics/TestStringStatisticsBuilder.java

@@ -325,7 +325,7 @@ private static void assertMinMaxValuesWithLimit(Slice expectedMin, Slice expecte

    private static void assertMinMaxValuesWithLimit(Slice expectedMin, Slice expectedMax, List<Slice> values, int limit, long expectedSum)
    {
-        checkArgument(values != null && values.size() > 0);
+        checkArgument(values.size() > 0);


This is incorrect replacement

This is deliberate. values== null causes a NullPointerException. values <= 0 causes an IllegalArgumentException

sdruzkin · 2024-05-13T16:56:11Z

presto-parquet/src/main/java/com/facebook/presto/parquet/rule/ParquetDereferencePushDown.java

@@ -169,7 +168,7 @@ private Subfield createNestedColumn(
                Collections.reverse(elements);
                String name = ((VariableReferenceExpression) currentRowExpression).getName();
                ColumnHandle handle = baseColumnHandles.get(name);
-                checkArgument(handle != null, "Missing Column handle: " + name);
+                requireNonNull(handle, "Missing Column handle: " + name);


Use %s for name?

tdcmeehan · 2024-05-13T17:05:50Z

I don't think we want to use Guava's checkNotNull, instead we use JDK's requireNonNull.

Most of the refactoring in this PR seems to be checking that the input argument corresponds in some way to an object's internal state. e.g. that a column is a member of a table. Most don't seem to be directly validating that the input arguments are null themselves. I'm slightly more in favor of keeping the thrown exception as IllegalArgumentException, because to me it's just more clear that the problem was with the state of my input argument, not that I somehow passed in something that's null.

There's a lot of prior art to using requireNonNull to directly validate that arguments to methods and constructors are not null. I think that's a little different: here, we are moving forward a potential null pointer exception, in the hopes that we fix it sooner (ideally before merge).

elharo · 2024-05-13T23:12:01Z

The modernizer plugin catches the cases where Guava's checkNotNull can be replaced by Objects.requireNonNull, but it's not always a replacement. Objects.requireNonNull doesn't handle format strings and multiple argiuments whereas Guava's checkNotNull does.

elharo · 2024-05-13T23:15:43Z

It varies from call site to call site. In quite a few cases it's not checking an argument at all:

       ConnectorMetadata metadata = transactions.get(transactionHandle);
        checkArgument(metadata != null, "no such transaction: %s", transactionHandle);

elharo force-pushed the nonnull branch 14 times, most recently from 187467d to 10b8e8c Compare May 12, 2024 21:36

Prefer NullPointerException for Null pointers

0591ede

elharo force-pushed the nonnull branch from 10b8e8c to 0591ede Compare May 12, 2024 21:38

elharo marked this pull request as ready for review May 13, 2024 15:00

elharo requested review from sdruzkin, a team, shangxinli, hantangwangd, vinothchandar, 7c00, jaystarshot and feilong-liu as code owners May 13, 2024 15:00

elharo requested a review from presto-oss May 13, 2024 15:00

sdruzkin reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer NullPointerException for Null pointers #22723

Prefer NullPointerException for Null pointers #22723

elharo commented May 12, 2024 •

edited

sdruzkin May 13, 2024

sdruzkin May 13, 2024

sdruzkin May 13, 2024

sdruzkin May 13, 2024

sdruzkin May 13, 2024

elharo May 13, 2024

sdruzkin May 13, 2024

sdruzkin May 13, 2024

sdruzkin May 13, 2024

elharo May 13, 2024

sdruzkin May 13, 2024

elharo May 13, 2024

sdruzkin May 13, 2024

tdcmeehan commented May 13, 2024

elharo commented May 13, 2024 •

edited

elharo commented May 13, 2024

Prefer NullPointerException for Null pointers #22723

Are you sure you want to change the base?

Prefer NullPointerException for Null pointers #22723

Conversation

elharo commented May 12, 2024 • edited

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan commented May 13, 2024

elharo commented May 13, 2024 • edited

elharo commented May 13, 2024

elharo commented May 12, 2024 •

edited

elharo commented May 13, 2024 •

edited