Repository navigation
Correct the reported signatures of several public callables - #1197
Conversation
|
@sobolevn, this one touches the type stubs, so a look from you would be welcome if you have time. Ten declarations gain a |
|
|
||
| def test_structmeta_text_signature_is_valid(): | ||
| """`StructMeta` has a `__signature__` getset, so only the text form is checkable""" | ||
| text_signature = msgspec.StructMeta.__text_signature__ |
There was a problem hiding this comment.
I would prefer to not test __text_signature__, it is an implementation detail. Only test the inspect.signature() result.
There was a problem hiding this comment.
That is what the suite does: fifteen callables are asserted through inspect.signature, and __text_signature__ appears in one test, for StructMeta. It is the one declaration inspect.signature cannot reach:
>>> inspect.signature(msgspec.StructMeta)
TypeError: unexpected object <attribute '__signature__' of 'msgspec._core.StructMeta' objects> in __signature__ attribute
>>> help(msgspec.StructMeta) # on main
class StructMeta(builtins.type)
| StructMeta(name, bases, namespace, /, *, **struct_config)
pydoc catches that TypeError and falls back to __text_signature__, so the broken line still reaches users. Of fourteen corrected declarations, thirteen do show through inspect.signature; this is the one that does not.
So it is the attribute, the help() output that renders it, or no coverage for that line. I took the attribute as the least indirect, and I am happy to switch.
| ) -> None: ... | ||
| def __rich_repr__(self) -> list[tuple[str, Any]]: ... | ||
| def __replace__(self, **changes: Any) -> Self: ... | ||
| def __rich_repr__(self, /) -> list[tuple[str, Any]]: ... |
There was a problem hiding this comment.
Why do we treat self this way? Should it be a regular parameter unless there are other pos-only parameters?
There was a problem hiding this comment.
Well, it's technically more correct I guess since self can only be passed positionally? Passing it as self=... doesn't work. I'm not sure it's necessary, but it's not technically wrong (just a bit odd, because you usually don't pass self explicitly at all since you don't do Struct.__replace__(<struct instance>)).
There was a problem hiding this comment.
It really depends on the exact method / class:
>>> class Unset(enum.Enum):
... def __bool__(self):
... print(self)
... return False
...
>>> Unset.__bool__(self=1)
1
FalseSome allow this, C methods usually don't 😅
There was a problem hiding this comment.
Oh, interesting. StructMeta doesn't allow it, which is what I'd expected. Hm. Well, we shouldn't lie about this then. Maybe just get rid of it, since it's not really beneficial?
There was a problem hiding this comment.
All five reject it:
>>> msgspec.Struct.__replace__(self=Example(1))
TypeError: unbound method _StructMixin.__replace__() needs an argument
>>> msgspec.UnsetType.__bool__(self=msgspec.UNSET)
TypeError: descriptor '__bool__' of 'msgspec.UnsetType' object needs an argument
Struct.__rich_repr__, Meta.__rich_repr__ and Raw.copy say the same. Your Unset.__bool__(self=1) is a Python level function, where self is ordinary parameter; these five are C level.
self= is not dead as a keyword on __replace__ though. It just never means the instance:
>>> class WithSelf(msgspec.Struct):
... self: int
>>> WithSelf(1).__replace__(self=7)
WithSelf(self=7)
def __replace__(self, /, **changes) says exactly that. Without the slash, mypy accepts Struct.__replace__(self=instance), which raises at runtime.
@provinzkraut that is where the slash earns its keep. On the other four it rules out a call nobody writes, so I am fine dropping those if you prefer. Either way Struct.__replace__.__text_signature__ is None here and on main, so the slash rests on call behavior alone.
Entries for msgspec#1194, msgspec#1196, msgspec#1197, msgspec#1199 and msgspec#1209 are missing from the changelog of the 0.22.0 release. Of the changes merged after msgspec#1211 moved the unreleased entries into the 0.22.0 section, only msgspec#1207 added its own entry. This adds the missing entries next to the related entries in that section, and marks msgspec#1196 as a breaking change: objects that expose `__dataclass_fields__` only through instance attribute access are no longer encoded as dataclasses. Co-authored-by: Tseluiko Aleksandr <[email protected]>
inspect.signatureandhelp()are built from a text signature: the first line of a C-level callable's docstring, before the--separator. That line has to parse as a Python parameter list, and it has to match how the callable is really called. Fourteen of those lines in_core.cfail one or the other, and between them they cover sixteen callables.Three do not parse.
msgspec.RawdeclaresRaw(msg="", /), which is two adjacent C string literals concatenating toRaw(msg=, /).msgspec.StructMetadeclares*, **struct_config.msgspec.structs.replacedeclaresreplace(struct, / **changes), missing a comma. ForRaw,inspect.signatureraisesValueError: <class 'msgspec.Raw'> builtin has invalid signatureon every supported version, and the API docs renderclass msgspec.Rawwith no parameters.replacebehaves that way from Python 3.12 onwards: up to 3.11 the stray/was stripped along with the rest of the non-Python syntax, leaving something that parsed, and from 3.12 it is kept.StructMetahas a__signature__getset, so its string only reachedhelp()anddocs/api.rst, which hard-codes the same text.Four more parse, but describe the wrong calling convention. Keyword arguments are rejected at runtime by
msgspec.msgpack.Ext,msgspec.structs.asdict,msgspec.structs.astupleandmsgspec.structs.force_setattr, yet their parameters are marked as positional-or-keyword. The stubs carry the same omission, somsgspec.structs.asdict(struct=p)passes type checking and raisesTypeErrorwhen run.The remaining seven begin with a bare
selfrather than CPython's$selfmarker, so the instance becomes a parameter of the bound method:inspect.signature(encoder.encode)gives(self, obj, /), and binding a single argument to that fails althoughencoder.encode(obj)is the correct call. Nine bound methods are affected:encode,encode_intoandencode_linesonjson.Encoder,encodeandencode_intoonmsgpack.Encoder,decodeanddecode_linesonjson.Decoder,decodeonmsgpack.Decoder, andRaw.copy. InRaw.copythatselfcarries no trailing/either, so it was reported as keyword-addressable on top of being reported at all.Ten stub declarations gain the matching
/.The unit tests assert the exact reported signature of fifteen callables against a literal, and compile the
StructMetatext signature. Five negative checks intests/typing/basic_typing_examples.pypin the tightened stubs in that file's existing idiom.stubtestcouples four of the ten stub edits to the C side by itself; theExt.__init__edit is pinned by nothing else, and the fiveselfedits have no observable effect to pin. On current main, fifteen of the sixteen unit cases fail on Python 3.11 and all sixteen on 3.14, and mypy, pyright and pyrefly each flag all five negative checks as unused ignores.Not addressed here: four decoder strings give
type='Any'as a default, where the real default istyping.Anyrather than that string. A text signature cannot carry that value. A default written as a name is evaluated, and the result is kept only if it is one of a handful of scalar types, sotype=Anyandtype=typing.Anyalike make the whole signature invalid andinspect.signaturethen raises for the callable. Correcting this one means going outside the text signature.This continues #1116, which corrected nine such strings.