[groovy] branch master updated: skip meta-object protocol for (G)String `<=>` (G)String/Character

[em...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

emilles pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git


The following commit(s) were added to refs/heads/master by this push:
     new 90e5751b53 skip meta-object protocol for (G)String `<=>` (G)String/Character
90e5751b53 is described below

commit 90e5751b534f4174df737c7f33d109da6b652e74
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sun Jul 9 12:39:22 2023 -0500

    skip meta-object protocol for (G)String `<=>` (G)String/Character
---
 .../typehandling/DefaultTypeTransformation.java    | 20 ++++++++------------
 .../groovy/runtime/DefaultGroovyMethodsTest.groovy | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java b/src/main/java/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
index 0393784dba..30238c2212 100644
--- a/src/main/java/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
+++ b/src/main/java/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
@@ -758,15 +758,14 @@ public class DefaultTypeTransformation {
     }
 
     private static int compareToWithEqualityCheck(Object left, Object right, boolean equalityCheckOnly) {
-        Exception cause = null;
         if (left == right) {
             return 0;
-        }
-        if (left == null) {
+        } else if (left == null) {
             return -1;
         } else if (right == null) {
             return 1;
         }
+        Exception cause = null;
         if (left instanceof Comparable || left instanceof Number) {
             if (left instanceof Number) {
                 if (right instanceof Character || right instanceof Number) {
@@ -792,18 +791,14 @@ public class DefaultTypeTransformation {
                 if (isValidCharacterString(left)) {
                     return DefaultGroovyMethods.compareTo(ShortTypeHandling.castToChar(left), (Number) right);
                 }
-            } else if (left instanceof String && right instanceof Character) {
-                return ((String) left).compareTo(right.toString());
-            } else if (left instanceof String && right instanceof GString) {
+            } else if (left instanceof  String && (right instanceof String || right instanceof GString || right instanceof Character)) {
                 return ((String) left).compareTo(right.toString());
-            } else if (left instanceof GString && right instanceof String) {
-                return ((GString) left).compareTo(right);
-            } else if (left instanceof GString && right instanceof Character) {
-                return ((GString) left).compareTo(right);
+            } else if (left instanceof GString && (right instanceof String || right instanceof GString || right instanceof Character)) {
+                return left.toString().compareTo(right.toString());
             }
             if (!equalityCheckOnly || left.getClass().isAssignableFrom(right.getClass())
-                    || (right.getClass() != Object.class && right.getClass().isAssignableFrom(left.getClass()) //GROOVY-4046
-                    || right instanceof Comparable) // GROOVY-7954
+                    || (right.getClass() != Object.class && right.getClass().isAssignableFrom(left.getClass()) // GROOVY-4046
+                        || right instanceof Comparable) // GROOVY-7954
             ) {
                 // GROOVY-7876: when comparing for equality we try to only call compareTo when an assignable
                 // relationship holds but with a container/holder class and because of erasure, we might still end
@@ -820,6 +815,7 @@ public class DefaultTypeTransformation {
         if (equalityCheckOnly) {
             return -1; // anything other than 0
         }
+
         String message = MessageFormat.format("Cannot compare {0} with value ''{1}'' and {2} with value ''{3}''",
                 left.getClass().getName(), left, right.getClass().getName(), right);
         if (cause != null) {
diff --git a/src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy b/src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy
index aea8ace1d9..58321ed067 100644
--- a/src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy
+++ b/src/test/org/codehaus/groovy/runtime/DefaultGroovyMethodsTest.groovy
@@ -221,7 +221,7 @@ class DefaultGroovyMethodsTest extends GroovyTestCase {
 
     /**
      * Tests that a List subclass without a constructor for Collections is still coerced
-     * into the correct list type. 
+     * into the correct list type.
      */
     void testCollectionTypeConstructors() {
         MyList list = DefaultGroovyMethods.asType([1, 2, 3], MyList.class)
@@ -231,6 +231,26 @@ class DefaultGroovyMethodsTest extends GroovyTestCase {
         assertEquals(3, list.get(2))
     }
 
+    void testCollectionMinusString() {
+        def values = ['foo','bar','baz']
+        def subset = null
+
+        String.metaClass = new DelegatingMetaClass(String) {
+            @Override invokeMethod(Object object, String methodName, Object[] arguments) {
+                if (methodName == 'compareTo') return -1 // what if?
+                super.invokeMethod(object, methodName, arguments)
+            }
+        }
+        try {
+            // (G)String <=> (G)String/Character should not be influenced!
+            subset = DefaultGroovyMethods.minus(values, new String('baz'))
+        } finally {
+            String.metaClass = null
+        }
+
+        assert subset.join('') == 'foobar'
+    }
+
     // GROOVY-7654
     void testIterableAsList() {
         def list = [1, 2, 3]