[LANG-1692] Cast FieldUtils.readField result to the recipient type

[tisonkun]

This patch should be easy to understand.

Generally, it won't affect user cases as long as the type can be inferred by the recipient or JDK rules. There's a case as shown in ReflectionDiffBuilder.java that if the type inference results in ambiguity, it may cause compile error.

Thus, I'm unsure whether this is proper to be included with a minor version bump. I don't think adding another series of methods helps - it causes further confusion.

This patch doesn't cause extra class cast exception because if the recipient is Object the cast should always succeed. And if the recipient already cast it into another type, there will be an exception if the cast cannot be performed.

[codecov-commenter]

Codecov Report

Merging #951 (12897d0) into master (e81855a) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #951   +/-   ##
=========================================
  Coverage     91.98%   91.98%           
  Complexity     7423     7423           
=========================================
  Files           189      189           
  Lines         15666    15667    +1     
  Branches       2913     2913           
=========================================
+ Hits          14411    14412    +1     
  Misses          677      677           
  Partials        578      578           

Impacted Files	Coverage Δ
...e/commons/lang3/builder/ReflectionDiffBuilder.java	91.30% <100.00%> (+0.39%)	:arrow_up:
...a/org/apache/commons/lang3/reflect/FieldUtils.java	93.33% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[garydgregory]

This PR has no matching tests.

[tisonkun]

This PR has no matching tests.

Please read the description:

• it won't affect user cases as long as the type can be inferred by the recipient or JDK rules
• This patch doesn't cause extra class cast exception ...

No existing tests break is fine. I don't know how to add a new effective test.

[tisonkun]

I may write something like:

diff --git a/src/test/java/org/apache/commons/lang3/reflect/FieldUtilsTest.java b/src/test/java/org/apache/commons/lang3/reflect/FieldUtilsTest.java
index b68a994f3..dcecbca06 100644
--- a/src/test/java/org/apache/commons/lang3/reflect/FieldUtilsTest.java
+++ b/src/test/java/org/apache/commons/lang3/reflect/FieldUtilsTest.java
@@ -480,7 +480,8 @@ public void testReadDeclaredNamedStaticFieldForceAccess() throws Exception {
     @Test
     public void testReadField() throws Exception {
         final Field parentS = FieldUtils.getDeclaredField(parentClass, "s");
-        assertEquals("s", FieldUtils.readField(parentS, publicChild));
+        String s = FieldUtils.readField(parentS, publicChild);
+        assertEquals("s", s);
         assertEquals("s", FieldUtils.readField(parentS, publiclyShadowedChild));
         assertEquals("s", FieldUtils.readField(parentS, privatelyShadowedChild));
         final Field parentB = FieldUtils.getDeclaredField(parentClass, "b", true);

to prove this change works. But it seems meaningless.

[garydgregory]

You need to think about how to avoid regressions in case the change is reverted. IOW, in this case, you should get a compilation error. You'll probably want a comment or more code in the test that avoids a future maintainer from inlining the variable.

[tisonkun]

@garydgregory Thanks for your comments. Will push a followup later this week.

[tisonkun]

Tests added.

[tisonkun]

@garydgregory is there any remaining blocker for this patch?

[garydgregory]

I'll take a look this weekend...

[tisonkun]

Closed as no response. I'll add wrappers for this changes downstream when necessary.