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 34 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. |
|
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>
|
Overall the added typing and the |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…numeration - components.py: cast to the TypeVar T instead of the runtime ype parameter (reportInvalidTypeForm). - log.py: extend the protocol/guard to require both name and version so pkg.version is valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/common/messaging.py:L726. 📍 src/debugpy/common/messaging.py:726 |
…l__ and Response - Message.__call__ now raises exception payloads explicitly instead of relying on an assert, so behavior is consistent when assertions are stripped under python -O. - Request.respond normalizes a None body for successful empty responses to an empty payload, so Response.body is always a MessageDict or Exception (fixes standard type checking and matches how incoming empty responses are parsed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fixed in d9532bb (re: src/debugpy/common/messaging.py:726, empty successful responses). Request.respond now normalizes a None body for a successful no-body response into an empty payload (_payload(None)) before constructing the Response, so Response.body is always a MessageDict or an Exception. This makes standard type checking pass (Response.init keeps its MessageDict | Exception contract) and matches how incoming empty responses are already parsed (Response._parse fills a missing body with an empty dict), so the outgoing and incoming paths are now consistent. The emitted JSON is unchanged — an empty/None body is still omitted from the wire message. |
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
The configurationDone_request guard was changed from a silently ineffective request.cant_handle(...) into a fail-fast raise. Add targeted coverage for receiving configurationDone before a start request and after startup has already begun, verifying a failure response is sent and startup side effects do not run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
…ards The _start_message_handler handle() only ever receives launch/attach requests (enforced by the assert on entry). Typing it as messaging.Message forced isinstance(request, messaging.Request) fallbacks that, with assertions disabled (python -O), could let a non-request message skip initialization/response and leave the client waiting. Type the parameter as messaging.Request and remove the now-redundant guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
sock_io.read() returns b'' at EOF, not None, so the previous is None guard never fired and an empty adapter handshake fell through to json.loads producing a JSONDecodeError. Use a non-shadowing name and check ot data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
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.