8315248: AssertionError in Name.compareTo

Reviewed-by: vromero

src/jdk.compiler/share/classes/com/sun/tools/javac/util/Utf8NameTable.java

@@ -25,10 +25,6 @@
 
 package com.sun.tools.javac.util;
 
-import java.lang.ref.SoftReference;
-
-import com.sun.tools.javac.util.DefinedBy.Api;
-
 /**
  * Support superclass for {@link Name.Table} implementations that store
  * names as Modified UTF-8 data in {@code byte[]} arrays.
@@ -51,7 +47,7 @@ public abstract class Utf8NameTable extends Name.Table {
 
     /** Generate a hash value for a subarray.
      */
-    protected static int hashValue(byte buf[], int off, int len) {
+    protected static int hashValue(byte[] buf, int off, int len) {
         int hash = 0;
         while (len-- > 0)
             hash = (hash << 5) - hash + buf[off++];
@@ -100,6 +96,11 @@ public abstract class Utf8NameTable extends Name.Table {
          */
         protected abstract int getNameIndex();
 
+        @Override
+        protected boolean nameEquals(Name that) {
+            return ((NameImpl)that).getNameIndex() == getNameIndex();
+        }
+
     // CharSequence
 
         @Override
@@ -112,17 +113,12 @@ public abstract class Utf8NameTable extends Name.Table {
             try {
                 return Convert.utf2string(getByteData(), getByteOffset(), getByteLength(), Convert.Validation.NONE);
             } catch (InvalidUtfException e) {
-                throw new AssertionError();
+                throw new AssertionError("invalid UTF8 data", e);
             }
         }
 
     // javax.lang.model.element.Name
 
-        @Override
-        protected boolean nameEquals(Name that) {
-            return ((NameImpl)that).getNameIndex() == getNameIndex();
-        }
-
         @Override
         public int hashCode() {
             return getNameIndex();
@@ -132,8 +128,18 @@ public abstract class Utf8NameTable extends Name.Table {
 
         @Override
         public int compareTo(Name name0) {
-            NameImpl name = (NameImpl)name0;
+            // While most operations on Name that take a Name as an argument expect the argument
-            Assert.check(name.table == table);
+            // to come from the same table, in many cases, including here, that is not strictly
+            // required. Moreover, javac.util.Name implements javax.lang.model.element.Name,
+            // which extends CharSequence, which provides
+            //   static int compare(CharSequence cs1, CharSequence cs2)
+            // which ends up calling to this method via the Comparable<Object> interface
+            // and a bridge method when the two arguments have the same class.
+            // Therefore, for this method, we relax "same table", and delegate to the more
+            // general super method if necessary.
+            if (!(name0 instanceof NameImpl name)) {
+                return super.compareTo(name0);
+            }
             byte[] buf1 = getByteData();
             byte[] buf2 = name.getByteData();
             int off1 = getByteOffset();
@@ -180,7 +186,7 @@ public abstract class Utf8NameTable extends Name.Table {
             try {
                 return table.fromUtf(result, 0, result.length, Convert.Validation.NONE);
             } catch (InvalidUtfException e) {
-                throw new AssertionError();
+                throw new AssertionError("invalid UTF8 data", e);
             }
         }
 
@@ -202,7 +208,7 @@ public abstract class Utf8NameTable extends Name.Table {
             try {
                 return table.fromUtf(result, 0, result.length, Convert.Validation.NONE);
             } catch (InvalidUtfException e) {
-                throw new AssertionError();
+                throw new AssertionError("invalid UTF8 data", e);
             }
         }
 
@@ -252,7 +258,7 @@ public abstract class Utf8NameTable extends Name.Table {
         }
 
         @Override
-        public void getUtf8Bytes(byte buf[], int off) {
+        public void getUtf8Bytes(byte[] buf, int off) {
             System.arraycopy(getByteData(), getByteOffset(), buf, off, getByteLength());
         }
     }

test/langtools/tools/javac/nametable/TestNameTables.java

@@ -0,0 +1,127 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8315248
+ * @summary AssertionError in Name.compareTo
+ * @modules jdk.compiler/com.sun.tools.javac.util
+ *
+ * @run main TestNameTables
+ */
+
+import java.io.PrintStream;
+import java.util.List;
+import java.util.Objects;
+
+import com.sun.tools.javac.util.Context;
+import com.sun.tools.javac.util.Name;
+import com.sun.tools.javac.util.Names;
+import com.sun.tools.javac.util.Options;
+
+/**
+ * Tests that CharSequence.compareTo works on CharSequence objects that may
+ * come from different name tables.
+ *
+ * While the java.util.Name class generally specifies that operations involving
+ * multiple Name objects should use names from the same table, that restriction
+ * is harder to specify when the names are widened CharSequence objects.
+ *
+ * The test can be extended if necessary to cover other methods on Name,
+ * if the restrictions on Name are relaxed to permit more mix-and-match usages.
+ */
+public class TestNameTables {
+    public static void main(String... args) {
+        new TestNameTables().run();
+    }
+
+    public static final String USE_SHARED_TABLE = "useSharedTable";
+    public static final String USE_UNSHARED_TABLE = "useUnsharedTable";
+    public static final String USE_STRING_TABLE = "useStringTable";
+
+    public final List<String> ALL_TABLES = List.of(USE_SHARED_TABLE, USE_UNSHARED_TABLE, USE_STRING_TABLE);
+
+    private final PrintStream out = System.err;
+
+    void run() {
+        for (var s : ALL_TABLES) {
+            test(createNameTable(s));
+        }
+
+        for (var s1 : ALL_TABLES) {
+            for (var s2 : ALL_TABLES) {
+                test(createNameTable(s1), createNameTable(s2));
+            }
+        }
+    }
+
+    Name.Table createNameTable(String option) {
+        Context c = new Context();
+        Options o = Options.instance(c);
+        o.put(option, "true");
+        Names n = new Names(c);
+        return n.table;
+    }
+
+    /**
+     * Tests operations using a single name table.
+     *
+     * @param table the name table
+     */
+    void test(Name.Table table) {
+        test(table, table);
+    }
+
+    /**
+     * Tests operations using distinct name tables, of either the same
+     * or different impl types.
+     *
+     * @param table1 the first name table
+     * @param table2 the second name table
+     */
+    void test(Name.Table table1, Name.Table table2) {
+        if (table1 == table2) {
+            out.println("Testing " + table1);
+        } else {
+            out.println("Testing " + table1 + " : " + table2);
+        }
+
+        // tests are primarily that there are no issues manipulating names from
+        // distinct name tables
+        testCharSequenceCompare(table1, table2);
+    }
+
+    void testCharSequenceCompare(Name.Table table1, Name.Table table2) {
+        Name n1 = table1.fromString("abc");
+        Name n2 = table2.fromString("abc");
+        checkEquals(CharSequence.compare(n1, n2), 0, "CharSequence.compare");
+    }
+
+    void checkEquals(Object found, Object expect, String op) {
+        if (!Objects.equals(found, expect)) {
+            out.println("Failed: " + op);
+            out.println("     found: " + found);
+            out.println("    expect: " + expect);
+        }
+    }
+}