fix metadata arglists for python defaults

[jsavyasachi]

Fixes#284.
Fixes#283.

Changes

• Preserve values returned in argspec defaults when generating pyarglists.
• Stringify Python-object defaults, such as <class 'int'>, instead of exposing pointer metadata.
• Add :arglists metadata for vars created by py/from-import, so Clojure doc output can render the signature before the docstring.
• Add regression coverage for positional defaults, keyword-only defaults, Python-object defaults, and rendered clojure.repl/doc output.

Verification

env PATH=/private/tmp/libpython-clj-x86-python-shim:$PATH clojure -M:test -n libpython-clj2.metadata-test
env PATH=/private/tmp/libpython-clj-x86-python-shim:$PATH clojure -M:test -n libpython-clj2.python-test
env PATH=/private/tmp/libpython-clj-x86-python-shim:$PATH clojure -M:test

Note: the PATH shim was needed locally because Java is x86_64 while the default Homebrew Python is arm64.

[cnuernber]

This looks good to me.

@jjtolton - If you have a minute could you take a look?

[jjtolton]

Initial impression, big improvement in dev experience. Doing a little adversarial testing to make sure nothing breaks with weird or opaque types.

[jjtolton]

@jsavyasachi Overall I think this is a great direction. However, there are some edge cases to consider.

1. Things like pytorch models have really nasty reprs, and there's currently no truncation or limit.

2. nested opaques leak raw pointers:

3. if there is a bad repr, the var is silently dropped from the ns and not interned.

4. if someone writes a broken repr that hangs or is infinitely recursive, that will blow up in the metadata. It's also a weird potential attack vector that exposes an exec while poopulating the metadata, but at that point you are running an unsandboxed repl against untrusted python code there are a lot of other potential attack vectors so I'm not going to worry about that.

Technically #4 is actually a bug in the way libpython-clj handles RecursionErrors when stringifying, the recursive repr should result in a stackoverflow and be a special case of (#3) but it's a landmine currently.

In general, I think this is a good direction -- the only thing missing is some graceful degradation for the above cases that allows a developer to diagnose and debug without the repl experience being unceremoniously degraded by surprisingy metadata handling.

Python adversarial testing file

importfunctoolsclassBadStr:
"""repr/str both raise -- triggers the throw path in py-default->jvm."""def__repr__(self):
raiseValueError('boom repr')
def__str__(self):
raiseValueError('boom str')
classWeirdStr:
def__repr__(self):
return'x'*40classHugeReprModel:
"""Simulates a PyTorch-style model whose repr is enormous (many nested layers)."""def__init__(self, n_layers=300):
self.n_layers=n_layersdef__repr__(self):
lines= ["GnarlyNet("]
foriinrange(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__classRecursiveRepr:
"""repr recurses without bound -> Python raises RecursionError."""def__repr__(self):
returnrepr(self)
__str__=__repr__classHangRepr:
"""repr never returns -> str(x) blocks forever while holding the GIL."""def__repr__(self):
whileTrue:
pass__str__=__repr___bad=BadStr()
_weird=WeirdStr()
_sentinel=object()
_partial=functools.partial(int, 0)
_recursive=RecursiveRepr()
_hang=HangRepr()
_huge=HugeReprModel(300)
deff_class(x=int):
"""Default is a Python class object."""returnxdeff_lambda(x=lambdaa: a):
"""Default is a lambda."""returnxdeff_badstr(x=_bad):
"""Default's str()/repr() raise."""returnxdeff_weird(x=_weird):
"""Default has a custom repr."""returnxdeff_sentinel(x=_sentinel):
"""Default is a bare sentinel object()."""returnxdeff_partial(x=_partial):
"""Default is a functools.partial."""returnxdeff_nested_opaque(x=(int, str)):
"""Default is a tuple of opaque class objects (nested case)."""returnxdeff_huge(model=_huge):
"""Default is an object with a massive repr -- the 'detonate the arglists' case."""returnmodeldeff_kw_huge(*, model=_huge):
"""Keyword-only variant of the massive-repr default."""returnmodeldeff_recursive(x=_recursive):
"""Default's repr recurses infinitely -> RecursionError."""returnxdeff_hang(x=_hang):
"""Default's repr never returns -> str() hangs forever."""returnxdeff_mixed(a, b=1, c=int, *, d=_sentinel, e=2):
"""Mixed positional/keyword-only with a couple of opaque defaults."""return (a, b, c, d, e)

[jjtolton]

See comments.

[jsavyasachi]

Thanks for the adversarial pass. I ported your file into testcode as the fixture. What's fixed:

• Huge reprs: truncated at 200 chars, so a big model default can't blow up the arglists.
• repr/str that raises (BadStr): falls back to <unprintable>, and the var is interned instead of being silently dropped.
• Nested opaques (f_nested_opaque): this one was a real leak, a tuple of classes was emitting raw Pointer objects. It now renders (<class 'int'>, <class 'str'>).

Two I left out of this PR:

• f_recursive: reproduced it as a native crash (SIGILL) in the stringify path, below where a Clojure catch can reach. That's the RecursionError handling you flagged, should this be a separate issue we open?
• f_hang: can't be guarded without a timeout, so out of scope I hope?

Still learning, so happy to hear your thoughts and make further changes if necessary!

[jjtolton]

Thanks for the adversarial pass. I ported your file into testcode as the fixture. What's fixed:

• Huge reprs: truncated at 200 chars, so a big model default can't blow up the arglists.

• repr/str that raises (BadStr): falls back to <unprintable>, and the var is interned instead of being silently dropped.

• Nested opaques (f_nested_opaque): this one was a real leak, a tuple of classes was emitting raw Pointer objects. It now renders (<class 'int'>, <class 'str'>).

Two I left out of this PR:

• f_recursive: reproduced it as a native crash (SIGILL) in the stringify path, below where a Clojure catch can reach. That's the RecursionError handling you flagged, should this be a separate issue we open?

• f_hang: can't be guarded without a timeout, so out of scope I hope?

Still learning, so happy to hear your thoughts and make further changes if necessary!

Great work and much appreciated! I will take a look today.

I agree the last two are out of scope (unless you wanted an extra challenge) as f_recursive is a bug within libpython-clj and f_hang would require some creative workaround like a short watchdog timer (metadata ingestion should be taking microseconds at most, anything that lasts more than a millisecond should be reaped with a fallback). Ofc I don't actually know how possible that is without killing the repl, so there would be some design work involved.

but that's defense in depth against someone hanging a repr, so I would not call it high priority.

the goal of course is to make sure no one blames libpython-clj for poorly written python code 😇

[cnuernber]

Thanks @jsavyasachi and @jjtolton for this work - it looks sufficient to me to move forward.

[jjtolton]

Agreed. Looks good. There's one extreme pathological edge case if someone put in a massive repr, it gets fully materialized before the truncation occurs, but that can be deferred.