Copilot commented on code in PR #51294:
URL: https://github.com/apache/arrow/pull/51294#discussion_r3982451049
##########
python/pyarrow/tests/test_misc.py:
##########
@@ -270,3 +270,50 @@ def test_extension_type_constructor_errors(klass):
msg = f"Do not call {klass.__name__}'s constructor directly, use .*
instead."
with pytest.raises(TypeError, match=msg):
klass()
+
+
[email protected]
+def test_public_callables_reject_none_without_crashing():
+ # GH-51293: a typed Cython parameter that is not declared "not None"
+ # lets None reach code that dereferences it, killing the interpreter
+ # instead of raising. Passing None to any public callable must produce
+ # a Python exception, never a fatal signal.
+ code = """if 1:
+ import importlib, inspect
+ mods = ["pyarrow", "pyarrow.compute", "pyarrow.dataset",
+ "pyarrow.parquet", "pyarrow.fs", "pyarrow.ipc",
+ "pyarrow.csv", "pyarrow.json", "pyarrow.feather"]
+ names = []
+ for mn in mods:
+ try:
+ m = importlib.import_module(mn)
+ except Exception:
+ continue
+ for n in dir(m):
+ if n.startswith("_"):
+ continue
+ try:
+ obj = getattr(m, n)
+ except Exception:
+ continue
+ if callable(obj) and not inspect.isclass(obj):
+ names.append((mn, n))
+ names.sort()
+ for mn, n in names:
+ print("%s.%s" % (mn, n), flush=True)
+ try:
+ getattr(importlib.import_module(mn), n)(None)
+ except BaseException:
+ pass
+ print("DONE", flush=True)
+ """
+ res = subprocess.run([sys.executable, "-c", code],
+ capture_output=True, text=True)
+ lines = res.stdout.splitlines()
+ if not lines or lines[-1] != "DONE":
+ culprit = lines[-1] if lines else "<no output>"
Review Comment:
This subprocess check can miss crashes that happen after printing "DONE"
(e.g., during interpreter shutdown), and it can hang the test suite
indefinitely if a callable blocks. Consider asserting `returncode == 0` and
adding a timeout to `subprocess.run`.
##########
python/pyarrow/tests/test_misc.py:
##########
@@ -270,3 +270,50 @@ def test_extension_type_constructor_errors(klass):
msg = f"Do not call {klass.__name__}'s constructor directly, use .*
instead."
with pytest.raises(TypeError, match=msg):
klass()
+
+
[email protected]
+def test_public_callables_reject_none_without_crashing():
+ # GH-51293: a typed Cython parameter that is not declared "not None"
+ # lets None reach code that dereferences it, killing the interpreter
+ # instead of raising. Passing None to any public callable must produce
+ # a Python exception, never a fatal signal.
Review Comment:
The comment says passing `None` to any public callable “must produce a
Python exception”, but this test doesn’t assert that (and some public APIs
legitimately accept `None`). Consider rewording to the actual invariant being
tested: no interpreter termination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
