Throw IllegalArgumentException instead of InternalError in the builder

package

src/changes/changes.xml

@@ -123,6 +123,7 @@ The <action> type attribute can be add,update,fix,remove.
     <action                   type="fix" dev="ggregory" due-to="Dimitrios Efthymiou">Update Javadoc for the insert methods in ArrayUtils #1078.</action>
     <action                   type="fix" dev="ggregory" due-to="Gary Gregory">Deprecate ExceptionUtils.ExceptionUtils().</action>
     <action issue="LANG-1697" type="fix" dev="ggregory" due-to="Jan Arne Sparka, Gary Gregory">TypeUtils.getRawType() throws a NullPointerException on Wildcard GenericArrayType.</action>
+    <action                   type="fix" dev="ggregory" due-to="Gary Gregory">Throw IllegalArgumentException instead of InternalError in the builder package.</action>    
     <!-- ADD -->
     <action                   type="add" dev="ggregory" due-to="Gary Gregory">Add GitHub coverage.yml.</action>
     <action                   type="add" dev="ggregory" due-to="Gary Gregory">Add EnumUtils.getEnumSystemProperty(...).</action>

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -561,19 +561,13 @@ public class EqualsBuilder implements Builder<Boolean> {
             final Field[] fields = clazz.getDeclaredFields();
             AccessibleObject.setAccessible(fields, true);
             for (int i = 0; i < fields.length && isEquals; i++) {
-                final Field f = fields[i];
-                if (!ArrayUtils.contains(excludeFields, f.getName())
-                    && !f.getName().contains("$")
-                    && (testTransients || !Modifier.isTransient(f.getModifiers()))
-                    && !Modifier.isStatic(f.getModifiers())
-                    && !f.isAnnotationPresent(EqualsExclude.class)) {
-                    try {
-                        append(f.get(lhs), f.get(rhs));
-                    } catch (final IllegalAccessException e) {
-                        //this can't happen. Would get a Security exception instead
-                        //throw a runtime exception in case the impossible happens.
-                        throw new InternalError("Unexpected IllegalAccessException");
-                    }
+                final Field field = fields[i];
+                if (!ArrayUtils.contains(excludeFields, field.getName())
+                    && !field.getName().contains("$")
+                    && (testTransients || !Modifier.isTransient(field.getModifiers()))
+                    && !Modifier.isStatic(field.getModifiers())
+                    && !field.isAnnotationPresent(EqualsExclude.class)) {
+                    append(Reflection.getUnchecked(field, lhs), Reflection.getUnchecked(field, rhs));
                 }
             }
         } finally {

src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java

@@ -191,14 +191,7 @@ public class HashCodeBuilder implements Builder<Integer> {
                     && (useTransients || !Modifier.isTransient(field.getModifiers()))
                     && !Modifier.isStatic(field.getModifiers())
                     && !field.isAnnotationPresent(HashCodeExclude.class)) {
-                    try {
-                        final Object fieldValue = field.get(object);
-                        builder.append(fieldValue);
-                    } catch (final IllegalAccessException e) {
-                        // this can't happen. Would get a Security exception instead
-                        // throw a runtime exception in case the impossible happens.
-                        throw new InternalError("Unexpected IllegalAccessException");
-                    }
+                    builder.append(Reflection.getUnchecked(field, object));
                 }
             }
         } finally {

src/main/java/org/apache/commons/lang3/builder/Reflection.java

@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.builder;
+
+import java.lang.reflect.Field;
+import java.util.Objects;
+
+/**
+ * Package-private reflection code.
+ */
+class Reflection {
+
+    /**
+     * Delegates to {@link Field#get(Object)} and rethrows {@link IllegalAccessException} as {@link IllegalArgumentException}.
+     *
+     * @param field The receiver of the get call.
+     * @param obj   The argument of the get call.
+     * @return The result of the get call.
+     * @throws IllegalArgumentException Thrown after catching {@link IllegalAccessException}.
+     */
+    static Object getUnchecked(final Field field, final Object obj) {
+        try {
+            return Objects.requireNonNull(field, "field").get(obj);
+        } catch (IllegalAccessException e) {
+            throw new IllegalArgumentException(e);
+        }
+    }
+
+}

src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java

@@ -151,12 +151,11 @@ public class ReflectionDiffBuilder<T> implements Builder<DiffResult<T>> {
         for (final Field field : FieldUtils.getAllFields(clazz)) {
             if (accept(field)) {
                 try {
-                    diffBuilder.append(field.getName(), FieldUtils.readField(field, left, true),
-                            FieldUtils.readField(field, right, true));
-                } catch (final IllegalAccessException ex) {
+                    diffBuilder.append(field.getName(), FieldUtils.readField(field, left, true), FieldUtils.readField(field, right, true));
+                } catch (final IllegalAccessException e) {
                     // this can't happen. Would get a Security exception instead
                     // throw a runtime exception in case the impossible happens.
-                    throw new InternalError("Unexpected IllegalAccessException: " + ex.getMessage());
+                    throw new IllegalArgumentException("Unexpected IllegalAccessException: " + e.getMessage(), e);
                 }
             }
         }

src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java

@@ -657,16 +657,10 @@ public class ReflectionToStringBuilder extends ToStringBuilder {
         for (final Field field : fields) {
             final String fieldName = field.getName();
             if (this.accept(field)) {
-                try {
-                    // Warning: Field.get(Object) creates wrappers objects for primitive types.
-                    final Object fieldValue = this.getValue(field);
-                    if (!excludeNullValues || fieldValue != null) {
-                        this.append(fieldName, fieldValue, !field.isAnnotationPresent(ToStringSummary.class));
-                    }
-                } catch (final IllegalAccessException ex) {
-                    // this can't happen. Would get a Security exception instead
-                    // throw a runtime exception in case the impossible happens.
-                    throw new InternalError("Unexpected IllegalAccessException: " + ex.getMessage());
+                // Warning: Field.get(Object) creates wrappers objects for primitive types.
+                final Object fieldValue = Reflection.getUnchecked(field, getObject());
+                if (!excludeNullValues || fieldValue != null) {
+                    this.append(fieldName, fieldValue, !field.isAnnotationPresent(ToStringSummary.class));
                 }
             }
         }