[GR-18338] skip NO_VALUE keys in PythonObject storage

PullRequest: graalpython/668

Author: cosminbasca

graalpython/com.oracle.graal.python.test/src/tests/test_class-set-attrib.py

@@ -39,87 +39,114 @@
 
 counter = 0
 
+
 class Foo:
-  def __setattr__(self, key, value):
-    global counter
-    counter = counter + 1
-    object.__setattr__(self, key, value)
-  def __delattr__(self, key):
-    global counter
-    counter = counter + 10
-    object.__delattr__(self, key)
+    def __setattr__(self, key, value):
+        global counter
+        counter = counter + 1
+        object.__setattr__(self, key, value)
+
+    def __delattr__(self, key):
+        global counter
+        counter = counter + 10
+        object.__delattr__(self, key)
+
 
 def test_call():
-  global counter
-  counter = 0
-  f = Foo()
-  f.a = 1
-  Foo.b = 123
-  assert counter == 1, "setting attrib on class should not call its own __setattr__"
-  del f.a
-  del Foo.b
-  assert counter == 11, "deleting attrib on class should not call its own __delattr__"
+    global counter
+    counter = 0
+    f = Foo()
+    f.a = 1
+    Foo.b = 123
+    assert counter == 1, "setting attrib on class should not call its own __setattr__"
+    del f.a
+    del Foo.b
+    assert counter == 11, "deleting attrib on class should not call its own __delattr__"
+
 
 class AClass:
-  pass
+    pass
+
 
 class BClass(AClass):
-  pass
+    pass
+
 
 class CClass(BClass):
-  pass
+    pass
+
 
 class DClass(BClass):
-  pass
+    pass
+
 
 def custom_set(self, key, value):
-  object.__setattr__(self, key, value + 10 if isinstance(value, int) else value)
+    object.__setattr__(self, key, value + 10 if isinstance(value, int) else value)
+
 
 def custom_get(self, key):
-  value = object.__getattribute__(self, key)
-  return value + 100 if isinstance(value, int) else value
+    value = object.__getattribute__(self, key)
+    return value + 100 if isinstance(value, int) else value
+
 
 def test_assignments():
-  object = CClass()
-  # writing to BClass changes the result, writing to DClass doesn't
-  targets = (AClass, BClass, DClass, CClass, object)
-  results = (0, 1, 1, 3, 4)
-  for i in range(0, len(targets)):
-    targets[i].foo = i
-    assert object.foo == results[i], "normal %d" %i
-  # make sure that a custom __getattribute__ is used
-  BClass.__getattribute__ = custom_get
-  for i in range(0, len(targets)):
-    targets[i].bar = i
-    assert object.bar == results[i] + 100, "custom get %d" % i
-  # check correct lookups when deleting attributes
-  for i in reversed(range(0, len(targets))):
-    assert object.bar == results[i] + 100, "delete %d" % i
-    del targets[i].bar
-  # make sure a custom __setattr__ is used
-  BClass.__setattr__ = custom_set
-  object.baz = 9
-  assert object.baz == 119, "custom set"
+    object = CClass()
+    # writing to BClass changes the result, writing to DClass doesn't
+    targets = (AClass, BClass, DClass, CClass, object)
+    results = (0, 1, 1, 3, 4)
+    for i in range(0, len(targets)):
+        targets[i].foo = i
+        assert object.foo == results[i], "normal %d" % i
+    # make sure that a custom __getattribute__ is used
+    BClass.__getattribute__ = custom_get
+    for i in range(0, len(targets)):
+        targets[i].bar = i
+        assert object.bar == results[i] + 100, "custom get %d" % i
+    # check correct lookups when deleting attributes
+    for i in reversed(range(0, len(targets))):
+        assert object.bar == results[i] + 100, "delete %d" % i
+        del targets[i].bar
+    # make sure a custom __setattr__ is used
+    BClass.__setattr__ = custom_set
+    object.baz = 9
+    assert object.baz == 119, "custom set"
 
 
 def test_setattr_via_decorator():
-  def setdec(func):
-    setattr(func, 'SPECIAL_ATTR', {'a': 1, 'b': 2})
-    return func
+    def setdec(func):
+        setattr(func, 'SPECIAL_ATTR', {'a': 1, 'b': 2})
+        return func
 
-  @setdec
-  def f():
-    return 1
+    @setdec
+    def f():
+        return 1
 
-  assert hasattr(f, 'SPECIAL_ATTR')
-  assert getattr(f, 'SPECIAL_ATTR') == {'a': 1, 'b': 2}
+    assert hasattr(f, 'SPECIAL_ATTR')
+    assert getattr(f, 'SPECIAL_ATTR') == {'a': 1, 'b': 2}
 
+    class MyClass(object):
+        @setdec
+        def f(self):
+            return 1
 
-  class MyClass(object):
-    @setdec
-    def f(self):
-      return 1
+    m = MyClass()
+    assert hasattr(m.f, 'SPECIAL_ATTR')
+    assert getattr(m.f, 'SPECIAL_ATTR') == {'a': 1, 'b': 2}
+
+
+def test_deepcopy_attribute_removal():
+    from copy import deepcopy
+
+    class A:
+        def __init__(self):
+            self.a = "a"
+
+        def add_rem_attr(self):
+            self.b = "b"
+            del self.b
+
+    i1 = A()
+    assert i1.__dict__ == deepcopy(i1).__dict__
 
-  m = MyClass()
-  assert hasattr(m.f, 'SPECIAL_ATTR')
-  assert getattr(m.f, 'SPECIAL_ATTR') == {'a': 1, 'b': 2}
+    i1.add_rem_attr()
+    assert i1.__dict__ == deepcopy(i1).__dict__

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/DynamicObjectStorage.java

@@ -41,8 +41,13 @@
 package com.oracle.graal.python.builtins.objects.common;
 
 import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.function.Predicate;
 
+import com.oracle.graal.python.builtins.objects.PNone;
 import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage;
+import com.oracle.truffle.api.CompilerAsserts;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.object.Layout;
@@ -105,17 +110,20 @@ public boolean remove(Object key, Equivalence eq) {
         return result;
     }
 
+    protected Iterable<Object> getKeysIterable() {
+        return store.getShape().getKeys();
+    }
+
     @Override
     public Iterable<Object> keys() {
-        return wrapJavaIterable(store.getShape().getKeys());
+        return wrapJavaIterable(getKeysIterable());
     }
 
     @Override
     @TruffleBoundary
     public Iterable<Object> values() {
         ArrayList<Object> entries = new ArrayList<>(store.size());
-        Shape shape = store.getShape();
-        for (Object key : shape.getKeys()) {
+        for (Object key : getKeysIterable()) {
             entries.add(store.get(key));
         }
         return wrapJavaIterable(entries);
@@ -125,8 +133,7 @@ public Iterable<Object> values() {
     @TruffleBoundary
     public Iterable<DictEntry> entries() {
         ArrayList<DictEntry> entries = new ArrayList<>(store.size());
-        Shape shape = store.getShape();
-        for (Object key : shape.getKeys()) {
+        for (Object key : getKeysIterable()) {
             entries.add(new DictEntry(key, store.get(key)));
         }
         return wrapJavaIterable(entries);
@@ -143,6 +150,67 @@ public DynamicObject getStore() {
         return store;
     }
 
+    private static class FilterIterator<T> implements Iterator<T> {
+        private final Iterator<T> iterator;
+        private final Predicate<T> predicate;
+        private T value;
+        private boolean valueIsSet = false;
+
+        private FilterIterator(Iterator<T> iterator, Predicate<T> predicate) {
+            this.iterator = iterator;
+            this.predicate = predicate;
+        }
+
+        @Override
+        public boolean hasNext() {
+            return valueIsSet || consumeUntilNext();
+        }
+
+        @Override
+        public T next() {
+            if (valueIsSet || consumeUntilNext()) {
+                valueIsSet = false;
+                return value;
+            }
+            // no more vals matching were found
+            throw new NoSuchElementException();
+        }
+
+        @TruffleBoundary
+        private boolean consumeUntilNext() {
+            while (iterator.hasNext()) {
+                T next = iterator.next();
+                if (predicate.test(next)) {
+                    value = next;
+                    valueIsSet = true;
+                    return true;
+                }
+            }
+            return false;
+        }
+    }
+
+    private static class NoValuePredicate<T> implements Predicate<T> {
+
+        private final DynamicObject store;
+
+        private NoValuePredicate(DynamicObject store) {
+            this.store = store;
+        }
+
+        @Override
+        public boolean test(T t) {
+            return store.get(t) != PNone.NO_VALUE;
+        }
+    }
+
+    private static class NoValueFilterIterator extends FilterIterator<Object> {
+
+        private NoValueFilterIterator(DynamicObject store) {
+            super(store.getShape().getKeys().iterator(), new NoValuePredicate<>(store));
+        }
+    }
+
     public static final class FastDictStorage extends DynamicObjectStorage {
         public FastDictStorage() {
         }
@@ -159,8 +227,42 @@ public HashingStorage copy(Equivalence eq) {
     }
 
     public static final class PythonObjectDictStorage extends DynamicObjectStorage {
+        private int size = -1;
+        private Shape shape;
+
         public PythonObjectDictStorage(DynamicObject store) {
             super(store);
+            shape = store.getShape();
+        }
+
+        @Override
+        protected Iterable<Object> getKeysIterable() {
+            return new Iterable<Object>() {
+                @Override
+                public Iterator<Object> iterator() {
+                    return new NoValueFilterIterator(getStore());
+                }
+            };
+        }
+
+        @Override
+        public int length() {
+            CompilerAsserts.neverPartOfCompilation();
+            if (shape != getStore().getShape()) {
+                size = 0;
+                for (@SuppressWarnings("unused")
+                Object ignored : getKeysIterable()) {
+                    size += 1;
+                }
+                shape = getStore().getShape();
+            }
+            return size;
+        }
+
+        @Override
+        public boolean hasKey(Object key, Equivalence eq) {
+            CompilerAsserts.neverPartOfCompilation();
+            return super.hasKey(key, eq) && getStore().get(key, PNone.NO_VALUE) != PNone.NO_VALUE;
         }
 
         @Override