Add more typing to debugpy and switch to 'standard' type checking mode#1637
Add more typing to debugpy and switch to 'standard' type checking mode#1637rchiodo wants to merge 27 commits into
Conversation
…odo/type_standard # Conflicts: # CONTRIBUTING.md # src/debugpy/adapter/clients.py # src/debugpy/adapter/launchers.py # src/debugpy/adapter/servers.py # src/debugpy/common/sockets.py # src/debugpy/server/api.py # src/debugpy/server/cli.py # tests/requirements.txt
|
🔒 Automated review in progress — @heejaechang is auto-reviewing this PR. |
|
Mostly a solid typing pass, but a few runtime-behavior changes rode in with it and two are genuine bugs ( |
- messaging.Message.__call__: fix dead no-arg fast path (args.count -> len(args)) - messaging.OutgoingRequest.wait_for_response: honor raise_if_failed=False by returning the error body instead of asserting; return type now MessageDict|Exception - messaging: complete AssociableMessageDict migration; associate_with is now a real method backed by a shared associated_dicts list instead of a setattr closure - server/api._settrace: only latch called on success so a failed settrace no longer permanently blocks configure(); drop no-op except/finally - adapter/servers.authenticate: fail closed when authorize result is an Exception - common/util.Observable.observers: revert class default to immutable tuple to avoid shared-mutable footgun; typed and callers reassign instead of in-place += - common/json._converter: document the int/float narrowing from numbers.Number - add unit tests for wait_for_response(raise_if_failed=False), Message.__call__() no-arg, and the configure() already-running guard incl. the settrace-failure case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Nice PR — the risky runtime changes (settrace/configure gate, wait_for_response semantics, no-arg Message.call, fail-closed auth) are backed by targeted tests, and several concerns a reviewer might raise are already handled with inline rationale. Approving; one small defensive-guard note left inline. |
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Restore fail-fast behavior in spawn_debuggee: for a real debug launch, assert servers.listener is not None instead of silently skipping the port/adapterAccessToken setup, which would spawn a debuggee unable to connect back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Overall this is a solid, well-tested typing pass. A few non-blocking notes below about failing-fast vs. substituting degraded fallback values on the newly-added |
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Address review feedback on the typing pass: replace silently-degraded None fallbacks with fail-fast asserts, and make ThreadSafeSingleton.assert_locked side-effect-free.
- singleton.py: assert_locked now checks lock._is_owned() instead of acquire()/release(), avoiding RLock recursion-count mutation and an unbalanced release() under python -O.
- servers.py: assert pydevdSystemInfo result is non-exception (pid/ppid always assigned before use); assert listener in inject() rather than connecting to an empty address.
- clients.py: assert servers.listener in attach_request and the client listener in notify_of_subprocess instead of substituting ("", 0)/None.
- debuggee.py: assert process in wait_for_exit rather than reporting a bogus clean exit (code 0).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| body = request.wait_for_response(raise_if_failed=False) | ||
| assert isinstance(body, messaging.MessageHandlingError) | ||
| assert body is request.response.body | ||
| assert "pause not supported" in str(body) |
There was a problem hiding this comment.
assert "pause not supported" in str(body) is a partial substring check. MessageHandlingError.__str__ returns the bare reason, so assert it exactly: assert str(body) == "pause not supported". Per the repo's tests-no-partial-asserts rule.
| monkeypatch.setattr(api.pydevd, "settrace", failing_settrace) | ||
|
|
||
| with pytest.raises(RuntimeError, match="settrace failed"): | ||
| api._settrace() |
There was a problem hiding this comment.
pytest.raises(RuntimeError, match="settrace failed") uses substring/regex matching. The message is controlled by the test itself, so assert it exactly:
with pytest.raises(RuntimeError) as exc_info:
api._settrace()
assert str(exc_info.value) == "settrace failed"Per the repo's tests-no-partial-asserts rule.
| monkeypatch.setattr(api.pydevd, "settrace", lambda *args, **kwargs: None) | ||
|
|
||
| api._settrace() | ||
| assert api._settrace.called is True |
There was a problem hiding this comment.
pytest.raises(RuntimeError, match="already running") matches the superstring "debug adapter is already running". Assert the exact message instead:
with pytest.raises(RuntimeError) as exc_info:
api.configure()
assert str(exc_info.value) == "debug adapter is already running"Per the repo's tests-no-partial-asserts rule.
|
|
||
| def configure(properties=None, **kwargs): | ||
| if getattr(_settrace, "called"): | ||
| raise RuntimeError("debug adapter is already running") |
There was a problem hiding this comment.
_settrace.called is a process-global latch that is only ever set True and never cleared. The new configure() guard therefore permanently raises "debug adapter is already running" for the interpreter's lifetime, so any legitimate reconfigure/re-attach after disconnect in a long-lived process is now blocked (previously a second configure() proceeded). Confirm whether re-attach-after-disconnect is supported — if so, clear the latch on disconnect/teardown; if not, document the one-shot contract (the tests only cover first-call behavior).
|
Overall the added typing and the |
3 participants
This is entirely necessary but I was using this to test Pylance. Seems like a good thing to be more strictly typed though.