fix metadata arglists for python defaults (#285)

* fix(metadata): preserve pyarglists defaults

* fix(metadata): add python import arglists

* test(metadata): cover python arglist docs

* fix(metadata): degrade gracefully on hostile default reprs

py-default->jvm stringified opaque Python defaults with str(py-str x),
which (1) was unbounded for huge reprs, (2) only handled a top-level
opaque so a collection default (e.g. a tuple of classes) leaked raw
pointers, and (3) threw when a default's repr raised, dropping the var
from the namespace.

safe-py-str truncates long reprs and falls back to "<unprintable>" when
repr/str raises; py-default->jvm now falls back to a str() of the whole
default whenever any nested value is opaque.

Recursive- and hanging-repr cases are left out: the former is a native
crash in libpython-clj's stringify path, the latter needs a timeout.

Author: jsavyasachi

src/libpython_clj2/metadata.clj

@@ -18,6 +18,7 @@
 
 
 (def builtins (import-module "builtins"))
+(def py-str (get-attr builtins "str"))
 (def inspect (import-module "inspect"))
 (def argspec (get-attr inspect "getfullargspec"))
 (def py-source (get-attr inspect "getsource"))
@@ -68,16 +69,49 @@
     (catch Exception _
       nil)))
 
+(def ^:private default-repr-max-len 200)
+
+(defn- safe-py-str [x]
+  (let [s (try (str (py-str x))
+               (catch Throwable _ "<unprintable>"))]
+    (if (> (count s) default-repr-max-len)
+      (str (subs s 0 default-repr-max-len) "...")
+      s)))
+
+(defn- opaque? [v]
+  (and (map? v) (contains? v :type) (contains? v :value)))
+
+(defn- py-default->jvm [x]
+  (let [jvm-val (->jvm x)]
+    ;; nested opaques have no JVM form and leak pointers - stringify instead
+    (if (some opaque? (tree-seq coll? seq jvm-val))
+      (safe-py-str x)
+      jvm-val)))
+
+(defn- py-defaults->jvm [defaults]
+  (when (->jvm defaults)
+    (->> defaults
+         (map py-default->jvm)
+         (into []))))
+
+(defn- py-kwonlydefaults->jvm [kwonlydefaults]
+  (when (->jvm kwonlydefaults)
+    (->> (call-attr kwonlydefaults "items")
+         (map (fn [entry]
+                (let [[k v] (seq entry)]
+                  [(->jvm k) (py-default->jvm v)])))
+         (into {}))))
+
 (defn py-fn-argspec [f]
   (if-let [spec (try (when-not (pyclass? f)
                        (argspec f))
                      (catch Throwable e nil))]
     {:args           (->jvm (get-attr spec "args"))
      :varargs        (->jvm (get-attr spec "varargs"))
      :varkw          (->jvm (get-attr spec "varkw"))
-     :defaults       (->jvm (get-attr spec "defaults"))
+     :defaults       (py-defaults->jvm (get-attr spec "defaults"))
      :kwonlyargs     (->jvm (get-attr spec "kwonlyargs"))
-     :kwonlydefaults (->jvm (get-attr spec "kwonlydefaults"))
+     :kwonlydefaults (py-kwonlydefaults->jvm (get-attr spec "kwonlydefaults"))
      :annotations    (->jvm (get-attr spec "annotations"))}
     (py-fn-argspec (get-attr f "__init__"))))
 
@@ -132,22 +166,16 @@
                               (map symbol)
                               (into []))
 
-         ;;These sometimes have actual python symbols in them so we can't use them
-         ;; or-map          (->> (concat
-         ;;                       (interleave kw-default-args defaults)
-         ;;                       (flatten (seq kwonlydefaults)))
-         ;;                      (partition-all 2)
-         ;;                      (map vec)
-         ;;                      (map (fn [[k v]] [(symbol k) v]))
-         ;;                      (into {}))
+         ;; Preserve the default values that inspect returned.  These may be nil
+         ;; or non-keyword JVM representations of Python values.
          as-varkw    (when (not (nil? varkw))
                        {:as (symbol varkw)})
          default-map (->> (concat
                            (interleave kw-default-args defaults)
                            (flatten (seq kwonlydefaults)))
                           (partition-all 2)
                           (map vec)
-                          (map (fn [[k v]] [(symbol k) (keyword k)]))
+                          (map (fn [[k v]] [(symbol k) v]))
                           (into {}))
 
          kwargs-map (merge default-map

src/libpython_clj2/python.clj

@@ -381,6 +381,14 @@ user> (py/py. np linspace 2 3 :num 10)
      #'~varname))
 
 
+(defn ^:no-doc py-var-metadata [var-name var-data]
+  (try
+    (let [metadata-fn (requiring-resolve 'libpython-clj2.metadata/py-fn-metadata)]
+      (select-keys (metadata-fn var-name var-data {}) [:doc :arglists]))
+    (catch Throwable _
+      {:doc (get-attr var-data "__doc__")})))
+
+
 (defmacro from-import
   "Support for the from a import b,c style of importing modules and symbols in python.
   Documentation is included."
@@ -390,7 +398,7 @@ user> (py/py. np linspace 2 3 :num 10)
        ~@(map (fn [varname]
                 `(let [~'var-data (get-attr ~'mod-data ~(name varname))]
                    (def ~varname ~'var-data)
-                   (alter-meta! #'~varname assoc :doc (get-attr ~'var-data "__doc__"))
+                   (alter-meta! #'~varname merge (py-var-metadata ~(name varname) ~'var-data))
                    #'~varname))
               (concat [item] args)))))
 

test/libpython_clj2/metadata_test.clj

@@ -0,0 +1,92 @@
+(ns libpython-clj2.metadata-test
+  (:require [clojure.test :refer :all]
+            [clojure.string :as str]
+            [libpython-clj2.python :as py]
+            [libpython-clj2.metadata :as metadata]))
+
+(deftest pyarglists-preserves-default-values
+  (let [argspec {:args ["top" "topdown" "onerror"]
+                 :varargs nil
+                 :varkw nil
+                 :defaults ["." true nil]
+                 :kwonlyargs ["follow_symlinks" "dir_fd"]
+                 :kwonlydefaults (array-map "follow_symlinks" false
+                                            "dir_fd" nil)}]
+    (is (= '([& [{top "."
+                  topdown true
+                  onerror nil
+                  follow_symlinks false
+                  dir_fd nil}]]
+             [& [{top "."
+                  topdown true
+                  follow_symlinks false
+                  dir_fd nil}]]
+             [& [{top "."
+                  follow_symlinks false
+                  dir_fd nil}]]
+             [& [{follow_symlinks false
+                  dir_fd nil}]])
+           (metadata/pyarglists argspec)))))
+
+(deftest py-fn-argspec-stringifies-python-object-defaults
+  (let [testcode (py/import-module "testcode")
+        default-type-fn (py/get-attr testcode "default_type_fn")]
+    (is (= '([& [{dtype "<class 'int'>"}]]
+             [])
+           (-> default-type-fn
+               metadata/py-fn-argspec
+               metadata/pyarglists)))))
+
+(deftest py-fn-argspec-stringifies-kwonly-python-object-defaults
+  (let [testcode (py/import-module "testcode")
+        kw-default-type-fn (py/get-attr testcode "kw_default_type_fn")]
+    (is (= '([& [{dtype "<class 'int'>"}]])
+           (-> kw-default-type-fn
+               metadata/py-fn-argspec
+               metadata/pyarglists)))))
+
+(defn- tc [n] (py/get-attr (py/import-module "testcode") n))
+
+(defn- default-of [n sym]
+  (->> (-> (tc n) metadata/py-fn-argspec metadata/pyarglists first)
+       (tree-seq coll? seq) (filter map?) first sym))
+
+(deftest py-default-class-object
+  (is (= "<class 'int'>" (default-of "f_class" 'x))))
+
+(deftest py-default-bad-repr-preserves-var
+  (is (= '([& [{x "<unprintable>"}]] [])
+         (-> (tc "f_badstr") metadata/py-fn-argspec metadata/pyarglists))))
+
+(deftest py-default-custom-repr
+  (is (= (apply str (repeat 40 "x")) (default-of "f_weird" 'x))))
+
+(deftest py-default-partial
+  (is (= "functools.partial(<class 'int'>, 0)" (default-of "f_partial" 'x))))
+
+(deftest py-default-nested-opaque-no-pointer-leak
+  (is (= "(<class 'int'>, <class 'str'>)" (default-of "f_nested_opaque" 'x))))
+
+(deftest py-default-lambda
+  (is (str/starts-with? (default-of "f_lambda" 'x) "<function <lambda> at 0x")))
+
+(deftest py-default-sentinel
+  (is (str/starts-with? (default-of "f_sentinel" 'x) "<object object at 0x")))
+
+(deftest py-default-huge-truncated
+  (let [s (default-of "f_huge" 'model)]
+    (is (<= (count s) 203))
+    (is (str/ends-with? s "..."))))
+
+(deftest py-default-huge-kwonly-truncated
+  (let [s (default-of "f_kw_huge" 'model)]
+    (is (<= (count s) 203))
+    (is (str/ends-with? s "..."))))
+
+(deftest py-default-mixed
+  (let [m (->> (-> (tc "f_mixed") metadata/py-fn-argspec metadata/pyarglists first)
+               (tree-seq coll? seq) (filter map?) first)]
+    (is (= 1 (m 'b)))
+    (is (= "<class 'int'>" (m 'c)))
+    (is (str/starts-with? (m 'd) "<object object at 0x"))
+    (is (= 2 (m 'e)))))

test/libpython_clj2/python_test.clj

@@ -8,6 +8,7 @@
             [tech.v3.datatype.ffi :as dt-ffi]
             [tech.v3.tensor :as dtt]
             [clojure.test :refer :all]
+            [clojure.repl :refer [doc]]
             libpython-clj2.python.bridge-as-python)
   (:import [java.io StringWriter]
            [java.util Map List]
@@ -153,6 +154,31 @@
                   a)
                 vec)))))
 
+(py/from-import testcode defaults_fn)
+
+(deftest from-import-adds-arglists-metadata
+  (is (= '([& [{top "."
+                topdown true
+                onerror nil
+                follow_symlinks false
+                dir_fd nil}]]
+           [& [{top "."
+                topdown true
+                follow_symlinks false
+                dir_fd nil}]]
+           [& [{top "."
+                follow_symlinks false
+                dir_fd nil}]]
+           [& [{follow_symlinks false
+                dir_fd nil}]])
+         (:arglists (meta #'defaults_fn)))))
+
+(deftest from-import-doc-renders-arglists
+  (let [doc-output (with-out-str (doc defaults_fn))]
+    (is (re-find #"defaults_fn" doc-output))
+    (is (re-find #"\[\[& \[\{top \"\.\"" doc-output))
+    (is (re-find #"Function with Python defaults for metadata tests\." doc-output))))
+
 (deftest aspy-iter
   (let [testcode-module (py/import-module "testcode")]
     (is (= [1 2 3 4 5]

testcode/__init__.py

@@ -1,3 +1,6 @@
+import functools
+
+
 class WithObjClass:
     def __init__(self, suppress, fn_list):
         self.suppress = suppress
@@ -49,9 +52,89 @@ def complex_fn(a, b, c: str = 5, *args, d=10, **kwargs):
     return {"a": a, "b": b, "c": c, "args": args, "d": d, "kwargs": kwargs}
 
 
+def defaults_fn(top=".", topdown=True, onerror=None, *, follow_symlinks=False, dir_fd=None):
+    """Function with Python defaults for metadata tests."""
+    return top, topdown, onerror, follow_symlinks, dir_fd
+
+
+def default_type_fn(dtype=int):
+    """Function with a Python object default for metadata tests."""
+    return dtype
+
+
+def kw_default_type_fn(*, dtype=int):
+    """Function with a keyword-only Python object default for metadata tests."""
+    return dtype
+
+
 complex_fn_testcases = {
     "complex_fn(1, 2, c=10, d=10, e=10)": complex_fn(1, 2, c=10, d=10, e=10),
     "complex_fn(1, 2, 10, 11, 12, d=10, e=10)": complex_fn(
         1, 2, 10, 11, 12, d=10, e=10
     ),
 }
+
+
+class BadStr:
+    def __repr__(self):
+        raise ValueError("boom repr")
+    def __str__(self):
+        raise ValueError("boom str")
+
+
+class WeirdStr:
+    def __repr__(self):
+        return "x" * 40
+
+
+class HugeReprModel:
+    def __init__(self, n_layers=300):
+        self.n_layers = n_layers
+
+    def __repr__(self):
+        lines = ["GnarlyNet("]
+        for i in range(self.n_layers):
+            lines.append(f"  (layer{i}): Linear(in_features=4096, out_features=4096, bias=True)")
+            lines.append(f"  (act{i}): GELU(approximate='none')")
+            lines.append(f"  (drop{i}): Dropout(p=0.1, inplace=False)")
+        lines.append(")")
+        return "\n".join(lines)
+    __str__ = __repr__
+
+
+_bad = BadStr()
+_weird = WeirdStr()
+_sentinel = object()
+_partial = functools.partial(int, 0)
+_huge = HugeReprModel(300)
+
+
+def f_class(x=int):
+    return x
+
+def f_lambda(x=lambda a: a):
+    return x
+
+def f_badstr(x=_bad):
+    return x
+
+def f_weird(x=_weird):
+    return x
+
+def f_sentinel(x=_sentinel):
+    return x
+
+def f_partial(x=_partial):
+    return x
+
+def f_nested_opaque(x=(int, str)):
+    return x
+
+def f_huge(model=_huge):
+    return model
+
+def f_kw_huge(*, model=_huge):
+    return model
+
+def f_mixed(a, b=1, c=int, *, d=_sentinel, e=2):
+    return (a, b, c, d, e)