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 29 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>
|
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>
| log.reraise_exception("{0} in {1}", exc, session) | ||
|
|
||
| return Missing() | ||
| return cast(type, Missing()) |
There was a problem hiding this comment.
📍 src/debugpy/adapter/components.py:134
Standard checking reports reportInvalidTypeForm because the value parameter type cannot be used as a type expression. Cast to the type variable instead (cast(T, Missing())) or otherwise model this factory without using the runtime parameter as a type.
[verified]
| report(" {0}=={1}\n", pkg.name, pkg.version) | ||
| if has_name(pkg): | ||
| name = pkg.name | ||
| report(" {0}=={1}\n", name, pkg.version) |
There was a problem hiding this comment.
📍 src/debugpy/common/log.py:378
Standard checking reports that HasName has no version attribute. Extend the protocol and guard to require both name and version, or narrow version separately before accessing it.
[verified]
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.