8064362: WriteableScope.dupUnshared misbehaves on shared Scopes
authorjlahoda
Wed, 12 Nov 2014 19:05:17 +0100
changeset 27553 75321debd020
parent 27552 8a4b2d3639c1
child 27554 9b1670378f8a
8064362: WriteableScope.dupUnshared misbehaves on shared Scopes Summary: When calling dupUnshared on a shared scope, make sure the result does not contain Symbols that don't belong to the scope that is being dupUnshared. Reviewed-by: mcimadamore
langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Scope.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
langtools/test/tools/javac/scope/DupUnsharedTest.java
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Scope.java	Wed Nov 12 15:16:35 2014 +0200
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Scope.java	Wed Nov 12 19:05:17 2014 +0100
@@ -305,7 +305,32 @@
          *  the table of its outer scope.
          */
         public WriteableScope dupUnshared(Symbol newOwner) {
-            return new ScopeImpl(this, newOwner, this.table.clone(), this.nelems);
+            if (shared > 0) {
+                //The nested Scopes might have already added something to the table, so all items
+                //that don't originate in this Scope or any of its outer Scopes need to be cleared:
+                Set<Scope> acceptScopes = Collections.newSetFromMap(new IdentityHashMap<>());
+                ScopeImpl c = this;
+                while (c != null) {
+                    acceptScopes.add(c);
+                    c = c.next;
+                }
+                int n = 0;
+                Entry[] oldTable = this.table;
+                Entry[] newTable = new Entry[this.table.length];
+                for (int i = 0; i < oldTable.length; i++) {
+                    Entry e = oldTable[i];
+                    while (e != null && e != sentinel && !acceptScopes.contains(e.scope)) {
+                        e = e.shadowed;
+                    }
+                    if (e != null) {
+                        n++;
+                        newTable[i] = e;
+                    }
+                }
+                return new ScopeImpl(this, newOwner, newTable, n);
+            } else {
+                return new ScopeImpl(this, newOwner, this.table.clone(), this.nelems);
+            }
         }
 
         /** Remove all entries of this scope from its table, if shared
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java	Wed Nov 12 15:16:35 2014 +0200
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java	Wed Nov 12 19:05:17 2014 +0100
@@ -1196,35 +1196,35 @@
             boolean hasDefault = false;      // Is there a default label?
             for (List<JCCase> l = tree.cases; l.nonEmpty(); l = l.tail) {
                 JCCase c = l.head;
+                if (c.pat != null) {
+                    if (enumSwitch) {
+                        Symbol sym = enumConstant(c.pat, seltype);
+                        if (sym == null) {
+                            log.error(c.pat.pos(), "enum.label.must.be.unqualified.enum");
+                        } else if (!labels.add(sym)) {
+                            log.error(c.pos(), "duplicate.case.label");
+                        }
+                    } else {
+                        Type pattype = attribExpr(c.pat, switchEnv, seltype);
+                        if (!pattype.hasTag(ERROR)) {
+                            if (pattype.constValue() == null) {
+                                log.error(c.pat.pos(),
+                                          (stringSwitch ? "string.const.req" : "const.expr.req"));
+                            } else if (labels.contains(pattype.constValue())) {
+                                log.error(c.pos(), "duplicate.case.label");
+                            } else {
+                                labels.add(pattype.constValue());
+                            }
+                        }
+                    }
+                } else if (hasDefault) {
+                    log.error(c.pos(), "duplicate.default.label");
+                } else {
+                    hasDefault = true;
+                }
                 Env<AttrContext> caseEnv =
                     switchEnv.dup(c, env.info.dup(switchEnv.info.scope.dup()));
                 try {
-                    if (c.pat != null) {
-                        if (enumSwitch) {
-                            Symbol sym = enumConstant(c.pat, seltype);
-                            if (sym == null) {
-                                log.error(c.pat.pos(), "enum.label.must.be.unqualified.enum");
-                            } else if (!labels.add(sym)) {
-                                log.error(c.pos(), "duplicate.case.label");
-                            }
-                        } else {
-                            Type pattype = attribExpr(c.pat, switchEnv, seltype);
-                            if (!pattype.hasTag(ERROR)) {
-                                if (pattype.constValue() == null) {
-                                    log.error(c.pat.pos(),
-                                              (stringSwitch ? "string.const.req" : "const.expr.req"));
-                                } else if (labels.contains(pattype.constValue())) {
-                                    log.error(c.pos(), "duplicate.case.label");
-                                } else {
-                                    labels.add(pattype.constValue());
-                                }
-                            }
-                        }
-                    } else if (hasDefault) {
-                        log.error(c.pos(), "duplicate.default.label");
-                    } else {
-                        hasDefault = true;
-                    }
                     attribStats(c.stats, caseEnv);
                 } finally {
                     caseEnv.info.scope.leave();
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/langtools/test/tools/javac/scope/DupUnsharedTest.java	Wed Nov 12 19:05:17 2014 +0100
@@ -0,0 +1,217 @@
+/*
+ * Copyright (c) 2014, 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
+ * @summary WriteableScope.dupUnshared not working properly for shared Scopes.
+ */
+
+import com.sun.tools.javac.util.*;
+import com.sun.tools.javac.code.*;
+import com.sun.tools.javac.code.Scope.*;
+import com.sun.tools.javac.code.Symbol.*;
+import com.sun.tools.javac.file.JavacFileManager;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Objects;
+import java.util.Set;
+
+public class DupUnsharedTest {
+    public static void main(String... args) throws Exception {
+        new DupUnsharedTest().run();
+    }
+
+    Context context;
+    Names names;
+    Symtab symtab;
+    Name a;
+    Name b;
+    int errors;
+
+    public DupUnsharedTest() {
+        context = new Context();
+        JavacFileManager.preRegister(context); // required by ClassReader which is required by Symtab
+        names = Names.instance(context);
+        symtab = Symtab.instance(context);
+        a = names.fromString("a");
+        b = names.fromString("b");
+    }
+
+    void run() throws Exception {
+        runScopeContentTest();
+        runClashTest();
+
+        if (errors > 0)
+            throw new AssertionError("Errors detected (" + errors + ").");
+    }
+
+    void runScopeContentTest() throws Exception {
+        Set<Symbol> expected = Collections.newSetFromMap(new IdentityHashMap<>());
+        Set<Symbol> notExpected = Collections.newSetFromMap(new IdentityHashMap<>());
+        WriteableScope s1 = WriteableScope.create(symtab.rootPackage);
+        ClassSymbol acceptSym = symtab.arrayClass;
+        s1.enter(acceptSym);
+        expected.add(acceptSym);
+        WriteableScope s2 = s1.dup();
+        fillScope(s2, notExpected, a);
+        WriteableScope s3 = s2.dup();
+        fillScope(s3, notExpected, b);
+        WriteableScope s4 = s1.dupUnshared();
+        assertEquals(toSet(s4.getSymbols()), expected);
+        assertEquals(toSet(s4.getSymbolsByName(a)), Collections.emptySet());
+        assertEquals(toSet(s4.getSymbolsByName(b)), Collections.emptySet());
+        assertEquals(toSet(s4.getSymbolsByName(acceptSym.name)), expected);
+        for (Symbol sym : notExpected) {
+            try {
+                s4.remove(sym);
+            } catch (Exception ex) {
+                System.err.println("s4.remove(" + sym + "); crashes with exception:");
+                ex.printStackTrace();
+                errors++;
+            }
+        }
+    }
+
+    void fillScope(WriteableScope scope, Set<Symbol> notExpected, Name name) {
+        VarSymbol var1 = new VarSymbol(0, name, Type.noType, symtab.arrayClass);
+        VarSymbol var2 = new VarSymbol(0, name, Type.noType, symtab.autoCloseableClose.owner);
+        scope.enter(var1);
+        scope.enter(var2);
+        scope.remove(var1);
+        notExpected.add(var1);
+        notExpected.add(var2);
+    }
+
+    Set<Symbol> toSet(Iterable<Symbol> it) {
+        Set<Symbol> result = Collections.newSetFromMap(new IdentityHashMap<>());
+
+        for (Symbol sym : it) {
+            result.add(sym);
+        }
+
+        return result;
+    }
+
+    void assertEquals(Set<Symbol> set1, Set<Symbol> set2) {
+        if (!Objects.equals(set1, set2)) {
+            System.err.println("Sets are not equals: s1=" + set1 + "; s2=" + set2);
+            errors++;
+        }
+    }
+
+    /**
+     * This tests tests the following situation.
+     * - consider empty Scope S1
+     * - a Symbol with name 'A' is added into S1
+     * - S1 is dupped into S2
+     * - a Symbol with name 'B', clashing with 'A', is added into S2
+     * - so the table now looks like: [..., A, ..., B, ...]
+     * - S2 is doubled. As a consequence, the table is re-hashed, and looks like:
+     *   [..., B, ..., A, ...] (note that re-hashing goes from the end, hence the original order).
+     * - B has been chosen so that it clashes in the doubled scope as well. So when looking up 'A',
+     *   B is found (and rejected) first, and only then the A's bucket is tested.
+     * - S2 is dupUshared - the resulting table needs to look like: [..., /sentinel/, ..., A, ...], not
+     *   [..., null, ..., A, ...], as in the latter case lookups would see 'null' while looking for
+     *   'A' and would stop the search prematurely.
+     */
+    void runClashTest() throws Exception {
+        WriteableScope emptyScope = WriteableScope.create(symtab.unnamedPackage);
+        Field tableField = emptyScope.getClass().getDeclaredField("table");
+        tableField.setAccessible(true);
+        Method dble = emptyScope.getClass().getDeclaredMethod("dble");
+        dble.setAccessible(true);
+        Method getIndex = emptyScope.getClass().getDeclaredMethod("getIndex", Name.class);
+        getIndex.setAccessible(true);
+
+        int tries = 0;
+
+        //find a name that will be in the first bucket in table (so that a conflicting name
+        //will be in placed in a bucket after this one).
+        Name first = names.fromString("a");
+        while ((Integer) getIndex.invoke(emptyScope, first) != 0) {
+            if (tries++ > MAX_TRIES) {
+                System.err.println("could not find a name that would be placed in the first bucket");
+                errors++;
+                return ;
+            }
+            first = names.fromString("a" + first.toString());
+        }
+
+        System.out.println("first name: " + first);
+
+        //now, find another name, that will clash with the first one both in the empty and a doubled scope:
+        Scope doubledEmptyScope = WriteableScope.create(symtab.unnamedPackage);
+        dble.invoke(doubledEmptyScope);
+        Integer firstNameTestScopeIndex = (Integer) getIndex.invoke(emptyScope, first);
+        Integer firstNameDoubleScopeIndex = (Integer) getIndex.invoke(doubledEmptyScope, first);
+        Name other = names.fromString("b");
+        while (!Objects.equals(firstNameTestScopeIndex, getIndex.invoke(emptyScope, other)) ||
+               !Objects.equals(firstNameDoubleScopeIndex, getIndex.invoke(doubledEmptyScope, other))) {
+            if (tries++ > MAX_TRIES) {
+                System.err.println("could not find a name that would properly clash with the first chosen name");
+                errors++;
+                return ;
+            }
+            other = names.fromString("b" + other);
+        }
+
+        System.out.println("other name: " + other);
+
+        Symbol firstSymbol = new VarSymbol(0, first, Type.noType, null);
+        Symbol otherSymbol = new VarSymbol(0, other, Type.noType, null);
+
+        //test the situation described above:
+        WriteableScope testScope1 = WriteableScope.create(symtab.unnamedPackage);
+        testScope1.enter(firstSymbol);
+
+        WriteableScope dupped1 = testScope1.dup();
+
+        dupped1.enter(otherSymbol);
+        dble.invoke(dupped1);
+
+        if (testScope1.dupUnshared().findFirst(first) != firstSymbol) {
+            System.err.println("cannot find the Symbol in the dupUnshared scope (1)");
+            errors++;
+        }
+
+        //also check a situation where the clashing Symbol is removed from the dupped scope:
+        WriteableScope testScope2 = WriteableScope.create(symtab.unnamedPackage);
+        testScope2.enter(firstSymbol);
+
+        WriteableScope dupped2 = testScope2.dup();
+
+        dupped2.enter(otherSymbol);
+        dble.invoke(dupped2);
+        dupped2.remove(otherSymbol);
+
+        if (testScope2.dupUnshared().findFirst(first) != firstSymbol) {
+            System.err.println("cannot find the Symbol in the dupUnshared scope (2)");
+            errors++;
+        }
+    }
+
+    int MAX_TRIES = 100; // max tries to find a hash clash before giving up.
+
+}