Skip to content

Add more typing to debugpy and switch to 'standard' type checking mode - #1637

Open
Rich Chiodo (rchiodo) wants to merge 41 commits into
microsoft:mainfrom
rchiodo:rchiodo/type_standard
Open

Add more typing to debugpy and switch to 'standard' type checking mode#1637
Rich Chiodo (rchiodo) wants to merge 41 commits into
microsoft:mainfrom
rchiodo:rchiodo/type_standard

Conversation

@rchiodo

@rchiodo Rich Chiodo (rchiodo) commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

This PR adds type annotations across debugpy and switches Pyright to standard type-checking mode (typeCheckingMode = "standard"), with # type: ignore comments disabled repository-wide (enableTypeIgnoreComments = false). It was originally used to exercise Pylance, but the stricter typing is a worthwhile improvement on its own.

The vast majority of the diff is pure type annotations. However, a handful of non-annotation changes were needed (or surfaced by the stricter checking) and are called out below so reviewers do not have to infer them from the diff.

Non-annotation (behavioral) changes

  • adapter/clients.pylaunch/attach handling now preserves request_options; out-of-order configurationDone requests fail explicitly; and an evaluate request that cannot be propagated to the debug server now receives a failure response instead of hanging.
  • adapter/launchers.py – for a debug launch, spawn_debuggee now asserts servers.listener is not None before reading the address/token, restoring fail-fast behavior (and narrowing the Optional) instead of silently skipping connection info.
  • common/messaging.py
    • Message.__call__ no longer uses assert for control flow: it explicitly raises when the payload is an Exception, so behavior is identical under python -O.
    • Request.respond(None) now models a successful empty response as an empty MessageDict (instead of None), matching how incoming empty responses are already parsed, so Response.body is always a MessageDict or an Exception.
    • OutgoingRequest disables the inherited _parse/_handle via explicit class-level attributes.
  • common/singleton.pyThreadSafeSingleton.assert_locked is no longer a @staticmethod and uses a side-effect-free lock._is_owned() check instead of acquire(blocking=False)/release(), avoiding an RLock recursion-count leak (and an unbalanced release() under python -O).
  • common/json.py_converter narrows accepted numeric types from numbers.Number to int/float only (DAP number payloads are int/float; Decimal/complex/Fraction never appear).
  • common/util.pyObservable.observers keeps an immutable () class default (typed Sequence) and the += call sites were switched to reassignment, avoiding mutation of a shared class attribute.
  • server/api.py – an empty adapter-endpoints read now raises the intended EOFError instead of falling through to json.loads and surfacing JSONDecodeError.

Focused regression tests were added for the behavioral changes (see tests/debugpy/common/test_messaging.py, tests/debugpy/common/test_singleton.py, tests/debugpy/common/test_json.py, and tests/debugpy/server/test_api.py).

Suppression policy

Because enableTypeIgnoreComments = false, # type: ignore has no effect and must not be used. Where a diagnostic must be suppressed (correct code the checker cannot verify, e.g. access to a private-but-stable runtime API), a narrow # pyright: ignore[reportSpecificRule] is used instead. This convention is now documented in CONTRIBUTING.md.

@rchiodo
Rich Chiodo (rchiodo) requested a review from a team as a code owner July 24, 2024 21:34
Comment thread src/debugpy/adapter/clients.py Outdated
Comment thread src/debugpy/adapter/clients.py Outdated
Comment thread src/debugpy/common/messaging.py Outdated
Comment thread src/debugpy/common/messaging.py Outdated
…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
@rchiodo
Rich Chiodo (rchiodo) requested a review from a team as a code owner July 16, 2026 19:00
@heejaechang

Heejae Chang (heejaechang) commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🔒 Automated review in progress — Heejae Chang (@heejaechang) is auto-reviewing this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

pytest-retry re-runs flaky tests' setup/call phases directly without
firing pytest_runtest_makereport, so pytest 9's tmp_path finalizer never
repopulates its tmppath_result_key stash entry after the first attempt's
teardown deletes it, raising KeyError at teardown (seen on py314 when
test_attach_pid_client was retried). Re-seed the stash key in a
pytest_runtest_setup hook, which pytest-retry runs on each retry.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@StellaHuang95

Stella Huang (StellaHuang95) commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR.

Comment thread src/debugpy/common/singleton.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

@StellaHuang95 Stella Huang (StellaHuang95) added the review-auto:approved Automated review: no blocking findings (approval posted). label Aug 6, 2026
Address review feedback: cover the owned-vs-unlocked invariant that assert_locked now enforces (_is_owned() check). Verifies attribute access succeeds when the current thread owns the lock and fails fast (AssertionError) on read/write when unlocked.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

Comment thread src/debugpy/common/messaging.py
Comment thread src/debugpy/adapter/launchers.py
Comment thread src/debugpy/common/messaging.py Outdated
Comment thread src/debugpy/common/json.py
Comment thread pyproject.toml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

- messaging.py: replace setattr-based _parse/_handle override on
  OutgoingRequest with explicit class-level typed optional attributes so
  the override stays statically visible to Pyright.
- test_messaging.py: pin the respond(None) contract (Response.body is an
  empty MessageDict, never None), matching incoming empty responses.
- test_json.py: add focused regression coverage pinning _converter to
  int/float and rejecting Decimal/complex/Fraction.
- CONTRIBUTING.md: document the standard Pyright mode and the
  repository-wide prohibition on '# type: ignore' (use targeted
  '# pyright: ignore[...]').

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread src/debugpy/server/api.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

Restores tests/debugpy/server/test_api.py (referenced by the PR
description) with focused coverage for the empty-read -> EOFError path in
debugpy.server.api.listen(): an empty read (b'') from the adapter
endpoints socket must surface an EOFError rather than falling through to
json.loads.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread src/debugpy/adapter/clients.py Outdated
Comment thread tests/debugpy/server/test_api.py
@StellaHuang95 Stella Huang (StellaHuang95) added review-auto:changes-requested Automated review: posted blocking findings to address. and removed review-auto:approved Automated review: no blocking findings (approval posted). labels Aug 8, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread src/debugpy/common/messaging.py
Comment thread src/debugpy/adapter/clients.py Outdated
Mark the class-local decorator helper static so Pyright no longer treats its wrapped callable as an instance-method self parameter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

@StellaHuang95 Stella Huang (StellaHuang95) added review-auto:approved Automated review: no blocking findings (approval posted). and removed review-auto:changes-requested Automated review: posted blocking findings to address. labels Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants