[GR-12264] Report side-effects in KEY_INFO

PullRequest: graalpython/256

Author: timfel

doc/INTEROP.md

@@ -145,9 +145,12 @@ Returns true for None only.
 
 ###### HAS_SIZE
 According to the Truffle interop contract answering `true` to `HAS_SIZE` implies
-that indexed element access is available. Thus, we answer `true` here only for
-(sub-)instances of `tuple`, `list`, `array.array`, `bytearray`, `bytes`, `str`,
-and `range`.
+that indexed element access is available. However, we cannot fully guarantee
+this. We may answer `true` here when the object has both a `__len__` field and a
+`__getitem__` field. If the object's length is reported >0, we also try to read
+the item `0` and if that fails, we answer `false`. If the object reports it's
+empty, we cannot know if a read with an index will actually work, but we'll
+report `true`.
 
 ###### GET_SIZE
 Calls `__len__`. Just because `GET_SIZE` returns something positive does not
@@ -159,27 +162,35 @@ knowing what the `__getitem__` method does with an integer argument. Use
 Returns true for those values that can be unboxed using the `UNBOX` message.
 
 ###### KEY_INFO
-This will lookup the key using `READ`, assume it is readable and writable, and
-check `IS_EXECUTABLE`.
+This will lookup the key using the Python MRO. It will check if it's readable
+and writable, and also check if it has side-effects based on wether it is an
+inherited descriptor (i.e., an object with `__get__`, `__set__`, and/or
+`__delete__`). If the owner of the key is mutable (the owner being the class the
+key is inherited from or the object itself) then `REMOVABLE` and `MODIFABLE` are
+true. If the object itself is mutable, `INSERTABLE` will also be true. Finally,
+if the attribute is a function or it is *not* a descriptor and has a `__call__`,
+we declare it `INOCABLE`. We don't do this for descriptors, because we would
+have to run the `__get__` method and this message should not have side-effects.
 
 ###### HAS_KEYS
-Returns true for any boxed Python object, so small integers, booleans, or floats
-usually don't return true.
+Always returns true.
 
 ###### KEYS
-This returns the direct attributes of the receiver object, which would usually
-be available through `__getattribute__`. 
-
-The `KEYS` message requires the returned object to have only `java.lang.String`
-items. If the object responds to `keys`, `values`, `items`, and `__getitem__`,
-we assume it is Mapping, and we present the result of the `keys` method in
-combination with the attributes if, and only if, all keys are strings. This is
-roughly parallel to how `READ` and `WRITE` would be handled for string keys.
+This returns the all attributes of the receiver object that would usually be
+available through `__getattribute__`, i.e., both inherited and direct
+attributes.
+
+If the object responds to `keys`, `values`, `items`, and `__getitem__`, we
+assume it is Mapping, and we present the String result of the `keys` method in
+combination with the attributes, prefixed with `[` if, and only if, the request
+asked for _internal_ keys also. The `KEYS` message requires the returned object
+to have only `java.lang.String` items, so inlo String keys are added to the
+result set. The `[` prefix ensures that in our handling of `READ` and `WRITE`
+messages we also treat them as mapping entries, not attributes.
 
 It's still possible that none of the keys can be `READ`: the `READ` message uses
 Python semantics for lookup, which means that an inherited descriptor with a
-`__get__` method may still come before the object's keys and do anything
-(including raising an `AttributeError`).
+`__get__` method or the `__getitem__` method may still intercept actual access.
 
 ###### IS_POINTER
 Returns true if the object is a Python function defined in a Python C extension

graalpython/com.oracle.graal.python.cext/include/truffle.h

@@ -54,6 +54,9 @@ void *truffle_assign_managed(void *dst, void *managed);
 void *truffle_deref_handle_for_managed(void *managed);
 bool truffle_cannot_be_handle(void *nativeHandle);
 
+// wrapping functions
+void *truffle_decorate_function(void *function, void *wrapper);
+
 /*
  * All function below here are deprecated and will be removed in a future release.
  * Use the equivalent functions from <polyglot.h> instead.

graalpython/com.oracle.graal.python.tck/src/com/oracle/graal/python/tck/PythonProvider.java

@@ -165,7 +165,7 @@ public Collection<? extends Snippet> createExpressions(Context context) {
 
         // dictionaries
         addExpressionSnippet(context, snippets, "dict", "lambda: { 'x': 4 }", OBJECT);
-        addExpressionSnippet(context, snippets, "dict", "lambda: { 'a': 1, 'b': 2, 'c': 3 }", OBJECT, new PDictMemberVerifier(arr("a", "b", "c"), arr("x", "y", "z")));
+        addExpressionSnippet(context, snippets, "dict", "lambda: { 'a': 1, 'b': 2, 'c': 3 }", OBJECT, new PDictMemberVerifier(arr("get", "keys", "update"), arr("a", "b", "c")));
 
         // @formatter:on
         return snippets;

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/debug/PythonDebugTest.java

@@ -41,7 +41,9 @@
 package com.oracle.graal.python.test.debug;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 import java.util.Arrays;
 import java.util.HashMap;
@@ -326,6 +328,76 @@ public void testReenterArgumentsAndValues() throws Throwable {
         }
     }
 
+    @Test
+    public void testGettersSetters() throws Throwable {
+        final Source source = Source.newBuilder("python", "" +
+                        "class GetterOnly:\n" +
+                        "  def __get__(self):\n" +
+                        "    return 42\n" +
+                        "\n" +
+                        "class P:\n" +
+                        "  def __init__(self):\n" +
+                        "    self.__x = None\n" +
+                        "    self.__y = None\n" +
+                        "    self.__nx = 0\n" +
+                        "    self.__ny = 0\n" +
+                        "\n" +
+                        "  @property\n" +
+                        "  def x(self):\n" +
+                        "    self.__nx += 1\n" +
+                        "    return self.__x\n" +
+                        "\n" +
+                        "  @x.setter\n" +
+                        "  def x(self, value):\n" +
+                        "    self.__nx += 1\n" +
+                        "    self.__x = value\n" +
+                        "\n" +
+                        "  y = GetterOnly()\n" +
+                        "\n" +
+                        "p = P()\n" +
+                        "str(p)\n" +
+                        "\n", "testGettersSetters.py").buildLiteral();
+        try (DebuggerSession session = tester.startSession()) {
+            session.suspendNextExecution();
+            tester.startEval(source);
+            expectSuspended((SuspendedEvent event) -> {
+                DebugStackFrame frame = event.getTopStackFrame();
+                assertEquals(1, frame.getSourceSection().getStartLine());
+                event.prepareStepOver(7);
+            });
+            expectSuspended((SuspendedEvent event) -> {
+                DebugStackFrame frame = event.getTopStackFrame();
+                assertEquals("p = P()", frame.getSourceSection().getCharacters().toString());
+                event.prepareStepOver(1);
+            });
+            expectSuspended((SuspendedEvent event) -> {
+                DebugValue p = session.getTopScope("python").getDeclaredValue("p");
+                DebugValue x = p.getProperty("x");
+                assertTrue(x.hasReadSideEffects());
+                assertTrue(x.hasWriteSideEffects());
+                assertTrue(x.isReadable());
+                assertTrue(x.isWritable());
+                DebugValue nx = p.getProperty("__nx");
+                assertEquals(0, nx.as(Number.class).intValue());
+                assertEquals("None", x.as(String.class));
+                assertEquals(1, nx.as(Number.class).intValue());
+                x.set(42);
+                assertEquals(2, nx.as(Number.class).intValue());
+                assertEquals("42", x.as(String.class));
+                assertEquals(3, nx.as(Number.class).intValue());
+                DebugValue y = p.getProperty("y");
+                assertTrue(y.hasReadSideEffects());
+                assertFalse(y.hasWriteSideEffects());
+                assertTrue(y.isReadable());
+                assertTrue(y.isWritable());
+                DebugValue ny = p.getProperty("__ny");
+                assertEquals(0, ny.as(Number.class).intValue());
+                y.set(24);
+                assertEquals("24", y.as(String.class));
+            });
+        }
+    }
+
     private void expectSuspended(SuspendedCallback callback) {
         tester.expectSuspended(callback);
     }

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/interop/JavaInteropTest.java

@@ -46,7 +46,8 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
-import java.util.Set;
+import java.util.Arrays;
+import java.util.List;
 
 import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.Context.Builder;
@@ -227,14 +228,14 @@ public void accessSuitePy() throws IOException {
 
         Value libraries = suite.getMember("libraries");
         assertNotNull("libraries found", libraries);
-        final Set<String> suiteKeys = suite.getMemberKeys();
+        final List<Object> suiteKeys = Arrays.asList(suite.invokeMember("keys").as(Object[].class));
         assertTrue("Libraries found among keys: " + suiteKeys, suiteKeys.contains("libraries"));
 
         Value dacapo = null;
-        for (String k : libraries.getMemberKeys()) {
+        for (Object k : libraries.invokeMember("keys").as(List.class)) {
             System.err.println("k " + k);
             if ("DACAPO".equals(k)) {
-                dacapo = libraries.getMember(k);
+                dacapo = libraries.getMember((String) k);
             }
         }
         assertNotNull("Dacapo found", dacapo);

graalpython/com.oracle.graal.python.test/src/tests/test_interop.py

@@ -47,6 +47,10 @@ def test_import():
         import python_cext
         assert imported_cext is python_cext
 
+    class GetterOnly():
+        def __get__(self, instance, owner):
+            pass
+
     class CustomObject():
         field = 42
 
@@ -56,6 +60,16 @@ def __getitem__(self, item):
         def __len__(self):
             return 21
 
+        getter = GetterOnly()
+
+        @property
+        def setter(self):
+            pass
+
+        @setter.setter
+        def setter_setter(self):
+            pass
+
     class CustomMutable(CustomObject):
         _items = {}
 
@@ -195,11 +209,31 @@ def test_has_keys():
 
     def test_keys():
         o = CustomObject()
-        assert len(polyglot.__keys__(o)) == 0
+        inherited_keys = len(polyglot.__keys__(o))
         o.my_field = 1
-        assert len(polyglot.__keys__(o)) == 1
+        assert len(polyglot.__keys__(o)) == 1 + inherited_keys
         assert "my_field" in polyglot.__keys__(o)
 
+    def test_key_info():
+        o = CustomObject()
+        o.my_field = 1
+        o.test_exec = lambda: False
+        readable_invokable_insertable = polyglot.__key_info__(o, "__init__")
+
+        readable_invokable_modifiable_insertable = polyglot.__key_info__(o, "__len__")
+        assert readable_invokable_modifiable_insertable == polyglot.__key_info__(o, "test_exec")
+
+        readable_modifiable_insertable = polyglot.__key_info__(o, "field")
+        assert readable_modifiable_insertable == polyglot.__key_info__(o, "my_field")
+
+        assert readable_invokable_insertable != readable_modifiable_insertable
+        assert readable_invokable_insertable != readable_invokable_modifiable_insertable
+        assert readable_modifiable_insertable != readable_invokable_modifiable_insertable
+
+        sideeffects_get = polyglot.__key_info__(o, "getter")
+        sideeffects_get_set = polyglot.__key_info__(o, "setter")
+        assert sideeffects_get != sideeffects_get_set
+
     def test_host_lookup():
         import java
         try:

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/PythonLanguage.java

@@ -38,6 +38,7 @@
 import com.oracle.graal.python.builtins.objects.PNone;
 import com.oracle.graal.python.builtins.objects.PythonAbstractObject;
 import com.oracle.graal.python.builtins.objects.code.PCode;
+import com.oracle.graal.python.builtins.objects.dict.PDict;
 import com.oracle.graal.python.builtins.objects.function.PArguments;
 import com.oracle.graal.python.builtins.objects.function.PBuiltinFunction;
 import com.oracle.graal.python.builtins.objects.function.PFunction;
@@ -62,6 +63,7 @@
 import com.oracle.graal.python.runtime.PythonOptions;
 import com.oracle.graal.python.runtime.PythonParser.ParserMode;
 import com.oracle.graal.python.runtime.exception.PException;
+import com.oracle.graal.python.runtime.interop.InteropMap;
 import com.oracle.truffle.api.Assumption;
 import com.oracle.truffle.api.CallTarget;
 import com.oracle.truffle.api.CompilerAsserts;
@@ -338,6 +340,7 @@ protected boolean isVisible(PythonContext context, Object value) {
     }
 
     @Override
+    @TruffleBoundary
     protected Iterable<Scope> findLocalScopes(PythonContext context, Node node, Frame frame) {
         ArrayList<Scope> scopes = new ArrayList<>();
         for (Scope s : super.findLocalScopes(context, node, frame)) {
@@ -346,7 +349,7 @@ protected Iterable<Scope> findLocalScopes(PythonContext context, Node node, Fram
         if (frame != null) {
             PythonObject globals = PArguments.getGlobalsSafe(frame);
             if (globals != null) {
-                scopes.add(Scope.newBuilder("globals()", globals).build());
+                scopes.add(Scope.newBuilder("globals()", scopeFromObject(globals)).build());
             }
             Frame generatorFrame = PArguments.getGeneratorFrameSafe(frame);
             if (generatorFrame != null) {
@@ -358,13 +361,21 @@ protected Iterable<Scope> findLocalScopes(PythonContext context, Node node, Fram
         return scopes;
     }
 
+    private static InteropMap scopeFromObject(PythonObject globals) {
+        if (globals instanceof PDict) {
+            return InteropMap.fromPDict((PDict) globals);
+        } else {
+            return InteropMap.fromPythonObject(globals);
+        }
+    }
+
     @Override
     protected Iterable<Scope> findTopScopes(PythonContext context) {
         ArrayList<Scope> scopes = new ArrayList<>();
         if (context.getBuiltins() != null) {
             // false during initialization
-            scopes.add(Scope.newBuilder("__main__", context.getMainModule()).build());
-            scopes.add(Scope.newBuilder("builtins", context.getBuiltins()).build());
+            scopes.add(Scope.newBuilder("__main__", scopeFromObject(context.getMainModule())).build());
+            scopes.add(Scope.newBuilder("builtins", scopeFromObject(context.getBuiltins())).build());
         }
         return scopes;
     }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/InteropModuleBuiltins.java

@@ -54,9 +54,14 @@
 import com.oracle.graal.python.builtins.PythonBuiltins;
 import com.oracle.graal.python.builtins.objects.PNone;
 import com.oracle.graal.python.builtins.objects.function.PythonCallable;
+import com.oracle.graal.python.nodes.SpecialAttributeNames;
 import com.oracle.graal.python.nodes.function.PythonBuiltinNode;
+import com.oracle.graal.python.runtime.PythonContext;
+import com.oracle.graal.python.runtime.PythonCore;
+import com.oracle.graal.python.runtime.PythonOptions;
 import com.oracle.graal.python.runtime.exception.PythonErrorType;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.TruffleFile;
 import com.oracle.truffle.api.TruffleLanguage.Env;
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Fallback;
@@ -84,6 +89,23 @@ protected List<com.oracle.truffle.api.dsl.NodeFactory<? extends PythonBuiltinNod
         return InteropModuleBuiltinsFactory.getFactories();
     }
 
+    @Override
+    public void initialize(PythonCore core) {
+        super.initialize(core);
+
+        PythonContext context = core.getContext();
+        Env env = context.getEnv();
+        String coreHome = PythonOptions.getOption(context, PythonOptions.CoreHome);
+        try {
+            TruffleFile coreDir = env.getTruffleFile(coreHome);
+            TruffleFile docDir = coreDir.resolveSibling("doc");
+            if (docDir.exists() || (docDir = coreDir.getParent().resolveSibling("doc")).exists()) {
+                builtinConstants.put(SpecialAttributeNames.__DOC__, new String(docDir.resolve("INTEROP.md").readAllBytes()));
+            }
+        } catch (SecurityException | IOException e) {
+        }
+    }
+
     @Builtin(name = "import_value", minNumOfPositionalArgs = 1, keywordArguments = {"name"})
     @GenerateNodeFactory
     public abstract static class ImportNode extends PythonBuiltinNode {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/SysModuleBuiltins.java

@@ -138,14 +138,14 @@ public void initialize(PythonCore core) {
                         true,  // dont_write_bytecode
                         false, // hash_randomization
                         false, // ignore_environment
-                        PythonOptions.getOption(core.getContext(), PythonOptions.InspectFlag).booleanValue(), // inspect
-                        PythonOptions.getOption(core.getContext(), PythonOptions.InspectFlag).booleanValue(), // interactive
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.InspectFlag), // inspect
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.InspectFlag), // interactive
                         false, // isolated
-                        PythonOptions.getOption(core.getContext(), PythonOptions.NoSiteFlag).booleanValue(), // no_site
-                        PythonOptions.getOption(core.getContext(), PythonOptions.NoUserSiteFlag).booleanValue(), // no_user_site
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.NoSiteFlag), // no_site
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.NoUserSiteFlag), // no_user_site
                         false, // optimize
-                        PythonOptions.getOption(core.getContext(), PythonOptions.QuietFlag).booleanValue(), // quiet
-                        PythonOptions.getOption(core.getContext(), PythonOptions.VerboseFlag).booleanValue(), // verbose
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.QuietFlag), // quiet
+                        PythonOptions.getFlag(core.getContext(), PythonOptions.VerboseFlag), // verbose
         }));
         builtinConstants.put("graal_python_core_home", PythonOptions.getOption(core.getContext(), PythonOptions.CoreHome));
         builtinConstants.put("graal_python_stdlib_home", PythonOptions.getOption(core.getContext(), PythonOptions.StdLibHome));