Skip to content

Run stubtest in the type-checking CI job - #1116

Open
Siyet wants to merge 2 commits into
mainfrom
add-stubtest-ci
Open

Run stubtest in the type-checking CI job#1116
Siyet wants to merge 2 commits into
mainfrom
add-stubtest-ci

Conversation

@Siyet

@Siyet Siyet commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Closes #1056.

Adds a stubtest step to the test-typing job so stub/runtime drift is caught in CI instead of by hand (as in #1043, #1062). mypy is already in the test-typing group, so no new dependency.

Root-cause fixes (instead of allowlisting)

  • Positional-only __text_signature__. json/msgpack encode/decode and the Encoder/Decoder methods take their first arg positional-only at runtime, but their C __text_signature__ omitted the /, so inspect.signature reported them as positional-or-keyword and stubtest flagged 10 mismatches. Added the / in _core.c so introspection matches runtime; behaviour is unchanged (keyword calls already raised). This removes all 10 pos-only allowlist entries.
  • StructMeta @disjoint_base. StructMeta is a disjoint base at runtime; marked it @disjoint_base in the stub (the stub already imports from typing_extensions, so no new floor of consequence). Removes that allowlist entry.

Other stub fixes

  • structs.pyi was missing __all__.
  • json.formats stub marked buf positional-only (/), but the runtime accepts it as a keyword, so the / is dropped to match (this is the one callable that really takes a keyword).

Allowlist

What is left in tests/typing/stubtest_allowlist.txt is 10 structural false positives stubtest cannot see through: the per-class __init__ synthesized in C for every Struct/inspect type, the custom __new__ on Meta/Ext/UnsetType, Struct.__init_subclass__ (config kwargs via dataclass_transform, surfaces on 3.14+), and the private _utils module.

Run with --ignore-unused-allowlist. The test-typing job runs stubtest on a single Python resolved from python-version-file: pyproject.toml (currently 3.14, floats up over time), not a pinned matrix, so a single superset allowlist with --ignore-unused is robust to the runner Python moving. Verified green on 3.12, 3.14, 3.15.

@Siyet
Siyet temporarily deployed to docs-preview July 4, 2026 14:17 — with GitHub Actions Inactive
@codspeed-hq

codspeed-hq Bot commented Jul 4, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 10.44%

⚡ 2 improved benchmarks
✅ 137 untouched benchmarks
⏩ 135 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation test_decode_type[arm-json-None] 907.1 µs 820.3 µs +10.58%
Simulation test_pickle_load[arm] 1.7 ms 1.6 ms +10.29%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing add-stubtest-ci (9b87ebd) with main (ceb01c1)

Open in CodSpeed

Footnotes

  1. 135 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Siyet
Siyet requested a review from sobolevn July 4, 2026 14:43
Comment thread tests/typing/stubtest_allowlist.txt Outdated
# mark "/") are correct. json.format is the one callable that really accepts a
# keyword, and its stub is fixed instead of allowlisted.
msgspec\.json\.encode
msgspec\.json\.decode

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is typed as:

def decode(
    buf: Buffer | str,
    /,
    *,
    type: type[_T],
    strict: bool = True,
    dec_hook: _DecHookSig = None,
) -> _T: ...

It's real signature:

>>> msgspec.json.decode('{}')
{}
>>> msgspec.json.decode(buf='{}')
Traceback (most recent call last):
  File "<python-input-5>", line 1, in <module>
    msgspec.json.decode(buf='{}')
    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
TypeError: Missing 1 required arguments

It's __text_signature__:

>>> inspect.signature(msgspec.json.decode)
<Signature (buf, *, type='Any', strict=True, dec_hook=None)>
>>> msgspec.json.decode.__text_signature__
"(buf, *, type='Any', strict=True, dec_hook=None)"

The correct fix would be to add / to the text signature:

"json_decode(buf, /, *, type='Any', strict=True, dec_hook=None)\n"

And remove these entries from the stubtest ignore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in d4560b2: added the / to the __text_signature__ for json/msgpack encode/decode and the Encoder/Decoder methods, so inspect.signature reports them positional-only and stubtest agrees. Dropped all 10 pos-only entries from the allowlist. Behaviour is unchanged (they already rejected keyword calls), only the introspection metadata was wrong.

Comment thread tests/typing/stubtest_allowlist.txt Outdated
msgspec\.Struct\.__init_subclass__

# StructMeta is a disjoint base at runtime. Marking it @disjoint_base in the
# stub is deferred (see #1056) pending a typing_extensions compat decision.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand :)
What compat decision are you talking about specifically?
That not all type checkers support @disjoint_base?

In this case it would be just a regular (T) -> T function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, my "compat decision" was just the worry that disjoint_base is new enough (typing_extensions 4.13) that pulling it into the stub raises the minimum type-checker/typeshed floor: an older mypy would error on the import rather than treat it as identity. But the stub already imports Buffer/Self/dataclass_transform from typing_extensions, so the floor is modern anyway and the bump is small. Added @disjoint_base to StructMeta in d4560b2 and dropped the allowlist entry. mypy and pyrefly are both happy with it.

@sobolevn

sobolevn commented Jul 5, 2026

Copy link
Copy Markdown
Member

These go in tests/typing/stubtest_allowlist.txt. Run with --ignore-unused-allowlist since the exact finding set shifts across Python versions (verified green on 3.12, 3.14, 3.15).

If this is the case, you can use per-version ignores. See https://github.com/typeddjango/django-stubs/blob/master/scripts/stubtest.sh

@Siyet
Siyet temporarily deployed to docs-preview July 13, 2026 09:29 — with GitHub Actions Inactive
@Siyet

Siyet commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@sobolevn On per-version allowlists: django-stubs runs stubtest across a pinned matrix of Python versions, so a common + per-version split (each checked for unused entries) fits there. msgspec's test-typing job runs stubtest on a single Python resolved from python-version-file: pyproject.toml (requires-python >=3.10), which currently lands on 3.14 and floats up as new stables release. With one floating version a single superset allowlist + --ignore-unused-allowlist is simpler and doesn't break when the runner's default Python moves. After the two fixes above the allowlist is down to 10 structural entries, only one of which is version-dependent (Struct.__init_subclass__, 3.14+).

If you'd rather keep the unused-entry safety that --ignore-unused gives up, the single-file alternative is to pin the stubtest step to a specific Python and drop the flag: strict mode already passes on 3.14 with this allowlist. That pins the interpreter for one step and would trip local runs on other versions, so I left it out, but happy to go that way if you prefer.

@Siyet

Siyet commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Heads up on the red CI here: the failure is in the test-typing stubtest step, but it is not from this PR. It is a pre-existing mypy error in _json_schema.py introduced by #1028 (none_member: Type | None passed to to_schema), which nothing caught before because the msgspec source is not type-checked in CI, only tests/typing is. This PR 's stubtest step is the first thing to run mypy over the package, so it surfaces it, which is the point.

Fixed separately in #1121. Once that lands I will rebase this and CI goes green.

Siyet added 2 commits July 13, 2026 13:09
Add the positional-only "/" to the encode/decode __text_signature__
strings in _core.c so introspection matches the runtime (these already
reject keyword calls), letting stubtest drop the 10 pos-only allowlist
entries. Mark StructMeta @disjoint_base in the stub to drop that entry
too. The remaining allowlist keeps --ignore-unused-allowlist since
stubtest runs on a single floating Python version, not a matrix.
@Siyet
Siyet force-pushed the add-stubtest-ci branch from d4560b2 to 9b87ebd Compare July 13, 2026 10:09
@Siyet Siyet mentioned this pull request Jul 13, 2026
sobolevn pushed a commit to adriangb/msgspec that referenced this pull request Jul 13, 2026
Follow-up to msgspec#1028.

msgspec#1028 introduced `none_member: mi.Type | None` in the union branch of
`_json_schema.py` and guarded the `self.to_schema(none_member)` calls
with a separate `has_none` boolean. mypy can't narrow `none_member`
through the flag, so it errors:

```
_json_schema.py:363: error: Argument 1 to "to_schema" of "_SchemaGenerator" has incompatible type "Type | None"; expected "Type"  [arg-type]
```

This is not caught by current CI because the msgspec source is not
type-checked there (only `tests/typing` is); msgspec#1116 (adding stubtest) is
what surfaces it. Fixing it here so msgspec#1116 can go green.

The fix drops the redundant `has_none` flag and checks `none_member is
not None` at the call sites, which gives mypy the narrowing. No runtime
behavior change (`has_none` and `none_member` were always set together):
`tests/unit/test_schema.py` still passes (115), and stubtest with mypy
2.2.0 is clean.

Co-authored-by: Siyet <Siyet@users.noreply.github.com>
Comment thread .justfile
env-run "test" "pyrefly check tests/typing"
) (
env-run "test"
"python -m mypy.stubtest msgspec --allowlist tests/typing/stubtest_allowlist.txt --ignore-unused-allowlist"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still advocate for the version-based allowlists, because otherwise we would have --ignore-unused-allowlist option enabled, which can hide problems :(

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.

Consider using stubtest to test the stubs

2 participants