Skip to content

chore: enforce type annotations on all functions via ruff ANN rules#2339

Open
maxisbey wants to merge 5 commits intomainfrom
enforce-type-annotations
Open

chore: enforce type annotations on all functions via ruff ANN rules#2339
maxisbey wants to merge 5 commits intomainfrom
enforce-type-annotations

Conversation

@maxisbey
Copy link
Contributor

Motivation

Pyright strict mode already enforces argument types (via reportMissingParameterType), but it infers return types silently — you can write def foo(): with no return annotation and pyright won't complain. This PR closes that gap by enabling Ruff's ANN rules so every function must have explicit type hints.

Changes

Configuration (pyproject.toml)

[tool.ruff.lint]
select = ["ANN", ...]  # flake8-annotations
ignore = ["ANN401", ...]  # `Any` is sometimes correct; pyright handles real misuse

[tool.ruff.lint.flake8-annotations]
allow-star-arg-any = true  # *args: Any, **kwargs: Any is valid for pass-through wrappers
  • ANN401 ignored — banning Any is too aggressive; we legitimately use it in places and pyright strict catches genuine misuse.
  • allow-star-arg-any = true*args: Any, **kwargs: Any is a common valid pattern.
  • Per-file ignores for test_func_metadata.py and test_server.py where untyped params are intentional (testing schema inference on unannotated signatures).
  • README snippets exempt (via tests/test_examples.py) — short doc examples shouldn't need full annotations.

Code changes

  • Ruff's unsafe-fix auto-added -> None or inferred return types to ~910 functions (the vast majority were trivial -> None on test functions).
  • Manually annotated ~100 functions that Ruff couldn't infer — mostly @asynccontextmanager generators (yielding stream tuples), pytest fixtures yielding test resources, ASGI handlers, and a few property/classmethod returns.

Impact

The diff is large (149 files) but mechanical. The meaningful parts are:

  • pyproject.toml config (the enforcement)
  • ~20 functions in src/mcp/ that now have explicit return types (e.g., sse_client -> AsyncGenerator[...], Context.session -> ServerSession)
  • 2 # noqa: ANN comments for intentional no-annotation cases

AI Disclaimer

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

No bugs found, but this is a 149-file PR with design decisions (which ANN rules to ignore, per-file exemptions, config choices) that warrant a human sign-off.

Extended reasoning...

Overview

This PR enables Ruff's ANN rules in pyproject.toml to enforce explicit type annotations on all functions. The vast majority of the ~1000 function changes are trivial -> None additions to test functions, __init__ methods, and async handlers. About ~20 functions in src/mcp/ received meaningful return type annotations (e.g., sse_client -> AsyncGenerator[...], Context.session -> ServerSession, _create_platform_compatible_process -> Process | FallbackProcess). Two # noqa: ANN comments are added for intentionally untyped test parameters.

Security risks

None. The PR makes no behavioral changes — only type annotations are added or modified. Auth-related files receive only -> None annotations on __init__ and handler methods.

Level of scrutiny

The code changes themselves are low-risk and mechanical. However, the pyproject.toml configuration changes represent design decisions: which rules to select (ANN), which to ignore (ANN401), which files to exempt (test_func_metadata.py, test_server.py), and the allow-star-arg-any setting. These are reasonable choices but should be confirmed by a maintainer.

Other factors

One minor annotation concern: handle_sse in server.py:929 is annotated -> Response but takes raw ASGI parameters (scope, receive, send), making it an ASGI callable whose return value is ignored — -> None would be more accurate. This is harmless but worth noting. The PR includes an AI disclaimer, indicating it was largely auto-generated. The sheer scope (149 files) and embedded design decisions make this worth a quick human review even though the changes are individually trivial.

elicit_with_validation,
)
from mcp.server.lowlevel.helper_types import ReadResourceContents
from mcp.server.session import ServerSession
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Nit: ServerSession (line 17) is imported at runtime but only used in the return type annotation on line 228 (-> ServerSession). Since this file has from __future__ import annotations, all annotations are lazy strings and don't need runtime imports. Move ServerSession under the existing if TYPE_CHECKING: block (line 19) for consistency with how MCPServer is already handled on line 20.

Extended reasoning...

What the issue is

The PR adds from mcp.server.session import ServerSession as a top-level runtime import at line 17 of context.py. However, ServerSession is only used in one place: the return type annotation -> ServerSession on the session property at line 228. This file has from __future__ import annotations at line 1, which means all annotations are evaluated lazily as strings and do not require the referenced types to be available at runtime.

The inconsistency

The same file already follows the correct pattern for type-only imports. On lines 19-20, MCPServer is imported under if TYPE_CHECKING: precisely because it is also only used in type annotations (e.g., _mcp_server: MCPServer | None on line 58 and the return type on line 74). The new ServerSession import should follow this identical pattern.

Step-by-step proof

  1. Line 1: from __future__ import annotations — enables PEP 563 lazy annotation evaluation.
  2. Line 17: from mcp.server.session import ServerSession — runtime import added by this PR.
  3. Line 228: def session(self) -> ServerSession: — the only usage of ServerSession in the file, and it is a return type annotation.
  4. Because of (1), the annotation -> ServerSession is stored as the string "ServerSession" at class definition time. Python never resolves it to the actual class at runtime unless get_type_hints() is called explicitly.
  5. Therefore, the import at line 17 is not needed at runtime and should be placed under if TYPE_CHECKING: on line 19.

Impact

This is a minor style/consistency issue. There is no circular import today, and the unnecessary runtime import has negligible performance cost. However, it introduces an inconsistency within the same file (where MCPServer correctly uses TYPE_CHECKING) and could become a circular import issue in the future if session.py ever imports from mcpserver.

Fix

Move from mcp.server.session import ServerSession from line 17 into the existing if TYPE_CHECKING: block at line 19, alongside the MCPServer import.

Enable the `ANN` (flake8-annotations) rule set to require return type
and argument annotations on all function definitions. Pyright strict
already enforces argument types but infers return types silently; this
closes the gap so every function has an explicit return annotation.

Configuration choices:
- `ANN401` (any-type) is ignored: `Any` is sometimes the right answer
  and pyright strict already catches genuine misuse.
- `allow-star-arg-any = true`: `*args: Any, **kwargs: Any` is a common
  valid pattern for pass-through wrappers.
- Per-file ignores for `test_func_metadata.py` and `test_server.py` where
  untyped parameters are intentional (testing schema inference on
  unannotated signatures).
- README code snippets (via pytest-examples) exempt — short doc
  examples shouldn't need full annotations.

Auto-fix added `-> None` or inferred return types to ~910 functions.
The remaining ~100 were annotated manually — mostly
`@asynccontextmanager` generators, pytest fixtures, and ASGI handlers
that Ruff couldn't infer.
- Switch AsyncGenerator[None, None] to AsyncIterator[None] in 4 example
  snippets to match codebase convention for @asynccontextmanager lifespans
- Regenerate README.v2.md to sync snippet changes from eb3d0d0
Align with ecosystem norms per review feedback:
- Add mypy-init-return = true to skip -> None on __init__ when args are typed
- Exempt tests/** from ANN rules (near-universal practice)
- Revert test file annotations and __init__-only changes

Shrinks diff from 149 to ~47 files, keeping the valuable src/ return
type annotations that close the pyright gap.
@maxisbey maxisbey force-pushed the enforce-type-annotations branch from d6b3ae4 to cfde91d Compare March 24, 2026 23:10
Replaces mypy-init-return (which only exempts __init__ when args are
typed) with a blanket ANN204 ignore. Special methods have well-known
return types and pyright infers them correctly.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — mechanical type annotation enforcement with correct return types throughout.

Extended reasoning...

Overview

This PR enables Ruff's ANN rules in pyproject.toml to enforce explicit type annotations on all functions. It touches 47 files (149 total including tests), but the changes are almost entirely mechanical: ~910 auto-added -> None annotations and ~100 manually annotated return types on @asynccontextmanager generators, ASGI handlers, properties, and decorators. The meaningful config changes are well-reasoned (ANN204 ignored for dunder methods, ANN401 ignored since pyright strict handles Any misuse, tests exempted).

Security risks

None. This PR adds type annotations only — no logic changes, no new dependencies, no changes to auth/crypto/permissions code. The annotations are hints that do not affect runtime behavior.

Level of scrutiny

Low scrutiny warranted. The changes are mechanical and type-only. I verified the manually annotated return types in src/mcp/ (transport functions, server methods, context properties) and they are correct. The handle_sse -> Response annotation is valid (the function returns Response()). The _create_platform_compatible_process -> Process | FallbackProcess is accurate. The AsyncGenerator[tuple[...], None] annotations on transport context managers are technically correct.

Other factors

Two nits were found by the bug hunting system: (1) ServerSession should be under TYPE_CHECKING in context.py (I already posted this as an inline comment), and (2) AsyncGenerator vs AsyncIterator inconsistency between transport and lifespan functions. Both are pure style/consistency issues with zero functional impact. Neither warrants blocking the PR — they can be addressed in a follow-up if desired.

Comment on lines +39 to +41
) -> AsyncGenerator[
tuple[MemoryObjectReceiveStream[SessionMessage | Exception], MemoryObjectSendStream[SessionMessage]], None
]:
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Nit: The transport @asynccontextmanager functions (sse_client, stdio_client, stdio_server, websocket_client, websocket_server, connect_sse) are annotated with AsyncGenerator[T, None], while the lifespan @asynccontextmanager functions in the same PR series (commit 8b7399c) use AsyncIterator[T]. Both are technically correct, but using AsyncIterator for transports would match the lifespan convention and align with the contextlib typeshed stubs.

Extended reasoning...

What the inconsistency is

This PR manually annotates transport @asynccontextmanager functions with AsyncGenerator[T, None] as their return type (e.g., sse_client, stdio_client, stdio_server, websocket_client, websocket_server, connect_sse). Meanwhile, commit 8b7399c in the same PR series standardized lifespan @asynccontextmanager functions to use AsyncIterator[T] (visible in the example files like streamable_starlette_mount.py, streamable_http_basic_mounting.py, etc.).

Why it matters (minimally)

Both AsyncGenerator[T, None] and AsyncIterator[T] are valid return types for @asynccontextmanager. AsyncGenerator[T, None] is a subtype of AsyncIterator[T], so either works correctly at runtime and passes type checking. However, the contextlib typeshed stubs declare @asynccontextmanager as accepting Callable[..., AsyncIterator[T]], making AsyncIterator the more conventional choice.

Step-by-step illustration

  1. In src/mcp/server/stdio.py:34, this PR adds -> AsyncGenerator[tuple[...], None] to stdio_server().
  2. In examples/snippets/servers/streamable_starlette_mount.py:35, commit 8b7399c adds -> AsyncIterator[None] to lifespan().
  3. Both functions follow the identical pattern: decorated with @asynccontextmanager, containing a single yield.
  4. The only difference is the return type annotation choice: AsyncGenerator vs AsyncIterator.

Addressing the counter-argument

A reasonable counter-argument is that transport functions and lifespan functions are different categories, and some pre-existing transport functions (e.g., streamable_http_client) already used AsyncGenerator. This is a fair point — each category is internally consistent. However, since these annotations were all manually added as part of the same effort (per the PR description: "Manually annotated ~100 functions"), adopting a single convention across all @asynccontextmanager functions would be cleaner.

Impact and fix

This is purely a style/consistency nit with zero functional impact. To fix, change AsyncGenerator[T, None] to AsyncIterator[T] in the six transport functions, matching the lifespan convention.

README doc snippets are illustrative and shouldn't require full type
annotations. Lost this when reverting tests/** in d6b3ae4.

@classmethod
async def from_content(cls, content: str, deps: Deps):
async def from_content(cls, content: str, deps: Deps) -> Self:
Copy link
Contributor

Choose a reason for hiding this comment

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

🟣 Pre-existing: from typing import Self (line 18) causes ImportError on Python 3.10, but pyproject.toml requires >=3.10. typing.Self was added in Python 3.11. This PR adds a new -> Self return annotation on from_content(), deepening the dependency on the broken import. Fix: import Self from typing_extensions instead, matching the pattern used in SDK source (e.g., src/mcp/shared/session.py).

Extended reasoning...

What the bug is

The example file examples/mcpserver/memory.py imports Self from typing on line 18:

from typing import Annotated, Self, TypeVar

However, typing.Self was only introduced in Python 3.11 (PEP 673). The project declares requires-python = ">=3.10" in pyproject.toml, meaning this file will raise ImportError when run on Python 3.10.

The specific code path

Any attempt to import or run examples/mcpserver/memory.py on Python 3.10 will fail immediately at line 18 with:

ImportError: cannot import name 'Self' from 'typing' (python3.10)

Self is used in two places in the file: as a parameter type annotation in merge_with(self, other: Self, deps: Deps) (line 132), and as a return type annotation in from_content(cls, content: str, deps: Deps) -> Self (line 93). The merge_with usage existed before this PR; the -> Self return annotation on from_content() was added by this PR.

Why existing code doesn't prevent it

The SDK source files correctly import Self from typing_extensions (e.g., src/mcp/shared/session.py:12 and src/mcp/client/session_group.py:19), which backports Self to Python 3.10+. However, this example file does not follow the same pattern. Since this is a standalone example with PEP 723 script dependencies (not part of the pyright include paths), it was not caught by type checking.

Step-by-step proof

  1. pyproject.toml line 6: requires-python = ">=3.10" — the project supports Python 3.10.
  2. examples/mcpserver/memory.py line 18: from typing import Annotated, Self, TypeVar — imports Self from typing.
  3. Python 3.10's typing module does not export Self (added in 3.11).
  4. Running python3.10 examples/mcpserver/memory.py produces ImportError: cannot import name 'Self' from 'typing'.
  5. The PR diff shows line 93 changed from async def from_content(cls, content: str, deps: Deps): to async def from_content(cls, content: str, deps: Deps) -> Self:, adding a new usage of the broken import.

Impact

This is an example file, not part of the SDK's core library. The practical impact is limited — users running this example on Python 3.10 would hit an immediate import error. The example also has heavy external dependencies (pydantic-ai, asyncpg, numpy, pgvector) that may themselves require Python 3.11+. Nevertheless, the fix is trivial and would bring the example in line with SDK conventions.

Fix

Change the import on line 18 from:

from typing import Annotated, Self, TypeVar

to:

from typing import Annotated, TypeVar
from typing_extensions import Self

This matches the pattern used throughout the SDK source code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant