Skip to content

Close the gaps in the IcePy stub/module consistency check - #6609

Closed
pepone wants to merge 4 commits into
mainfrom
fix/icepy-stub-check-gaps
Closed

Close the gaps in the IcePy stub/module consistency check#6609
pepone wants to merge 4 commits into
mainfrom
fix/icepy-stub-check-gaps

Conversation

@pepone

@pepone pepone commented Aug 14, 2026

Copy link
Copy Markdown
Member

Closes #6443.

Closes all six gaps recorded there, in scripts/checkIcePyStub.py, plus nine docstring additions to the stub and four constructor docstrings in the C sources:

  1. Attributes checked — the stub's 26 annotated class attributes (with the string literal below each as their docstring) are now compared against the module's getset descriptor docstrings, prose and declared type alike.
  2. Reflow-tolerant comparisonnormalizeProse() folds hard line wrapping, so rewrapping a paragraph or a bullet does not read as drift, while blank lines, indentation changes, lists, and section underlines still have to match. The two wrap-only attribute divergences the issue found pass without any stub or C++ edit.
  3. Deleted stub prose caught — a description the module ships that the stub does not document is now reported. This direction had nine live hits: stringVersion, intVersion, createProperties, stringToIdentity, identityToString, getProcessLogger, setProcessLogger, loadSlice, and compileSlice are documented in the module but were bare in the stub; their docstrings are copied into the stub. A tp_doc of "IcePy.<name>" — the C sources' placeholder for an undocumented class — counts as no description.
  4. Explicit dunder signatures checked — a __init__ slot carries the wrapper CPython generates, and its docstring describes object's generic one, so there was nothing in the module to hold the stub to. Communicator, ObjectPrx, Operation and Properties now spell their constructor in tp_doc, the way a C type documents one, and the check compares the stub's own __init__ declaration against that line. None of the four is re-exported by Ice/IcePyTypes.py, so no published page changes.
  5. Overload collapse detected — a name the stub declares more than once is reported and dropped from the comparison, instead of silently comparing only the last definition.
  6. Reverse sweep — every public name in vars(IcePy) must be declared by the stub, and every member in vars() of each class must be declared by the stub class or one of its stub bases. Dunders that vars(object) supplies need not be repeated, but a slot object does not have — ExecutorCall.__call__ — is required, since omitting it would make the type checker reject a legitimate call.

Review follow-ups

Three rounds of review are folded in, in 8c0f0ab7f7, e368ddc874 and b29a33c343:

  • None is CPython's only for __hash__ — the exemption no longer swallows every None-valued class member.
  • normalizeProse() folds prose only — list items, field lists, directives, section underlines, and indented literal or preformatted blocks keep their own lines, so loadSlice's five option bullets are not collapsed into one paragraph. A marker counts only where reST would begin a block — after a blank line, or at a new indentation — so a wrapped continuation that merely opens with one (30901. For pre-releases…) stays part of its paragraph. A list item's own wrapping folds, since its text continues on the following lines indented past the marker; a nested list is still read as a list, and a field list or directive is left alone because what follows it is an indented body, not the same sentence carrying on. A section adornment is recognized by its shape — a repeated punctuation character — rather than a hand-picked set that missed ::::::.
  • __mro__ dereference guarded — a stub class whose module counterpart is a function is reported as a difference instead of crashing with an AttributeError traceback.
  • @property accepted, and still checked — a read-only getset can be declared @property without the check demanding a callable signature from IcePy, and a property or attribute whose IcePy docstring does open with a signature line is reported, since the stub would otherwise promise an attribute for something the caller has to call.
  • Module scope verified, not just suppressed — a name the stub declares at module scope now has to exist in the module (TypeVar and its kind excepted, since they are there for the type checker alone), and standing a value where IcePy defines a class — Logger: Any — is reported, because it type-checks anything and takes every member of that class out of the comparison with it.
  • Attribute annotations compared — the C sources open a data member's description by naming its type, so the annotation and that name are held to each other, modulo the module a type is named through: Ice.EndpointInfo is IcePy.EndpointInfo, and the C sources use both spellings.
  • Duplicates reported, not half-compared — a name declared more than once is dropped from the comparison after being reported, so an @overload pair no longer also produces a spurious mismatch against its last definition.
  • Normalized diff output — the printed diff is built from the same normalized prose the comparison used, so a real divergence inside a rewrapped docstring is not buried under wrap-only noise.

Verification

Each branch was red/green probed against a fresh IcePy build. The mutation battery, run against the current head:

Mutation applied to the stub Result
semantic edit in a method docstring EndpointInfo.type: descriptions differ
semantic edit in an attribute docstring EndpointInfo.compress: descriptions differ
deleted attribute docstring EndpointInfo.compress: IcePy ships a description, but the stub does not document it
arity drift loadSlice: signatures differ
wrong Properties.__init__ signature Properties: signatures differ
stub-only name NotInIcePy: declared in the stub, but IcePy does not define it
invented module constant INVENTED_CONSTANT: declared in the stub, but IcePy does not define it
class replaced by Logger: Any Logger: IcePy defines a class, but the stub declares it as a value
deleted method EndpointInfo.datagram: IcePy defines it, but the stub does not declare it
duplicate def intVersion: the stub declares it more than once
@property over a callable EndpointInfo.type: the stub declares it as a value, but IcePy documents it as callable
wrong attribute annotation EndpointInfo.underlying: the stub's type and the one IcePy documents differ
type prefix dropped from the module's description EndpointInfo.compress: IcePy's description does not open by naming the type
reflow only, same words green
bullet rewrapped, same words green
read-only getset declared @property green
Ice.EndpointInfo vs EndpointInfo green

Ruff, clang-format, Pyright, and the checker itself are green against a fresh build; Pyright reports no new diagnostic.

No changelog fragment, matching #6426 (tooling; no user-facing behavior change).

Copilot AI balanced review requested due to automatic review settings August 14, 2026 10:53
@pepone pepone added this to the 3.8.3 milestone Aug 14, 2026
@pepone pepone added the python label Aug 14, 2026

Copilot AI left a comment

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.

Pull request overview

Extends IcePy’s consistency checker to detect missing declarations, attributes, duplicate definitions, and documentation drift.

Changes:

  • Adds bidirectional module/stub validation and attribute checks.
  • Adds reflow-tolerant prose comparison.
  • Documents nine previously bare stub functions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
scripts/checkIcePyStub.py Expands consistency validation.
python/python/IcePy-stubs/__init__.pyi Adds missing function documentation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/checkIcePyStub.py Outdated
Comment thread scripts/checkIcePyStub.py Outdated

@externl externl left a comment

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.

  • shipped() dereferences obj.__mro__ unguarded (checkIcePyStub.py:137), so a stub class whose module counterpart is a function crashes with an AttributeError traceback instead of a diagnostic — reverseProblems guards the identical case at :199.
  • A read-only @property in the stub is rejected: signatureOf renders any def as name(...) and then requires the C docstring to open with it, which a getset descriptor never does. The EndpointInfo getsets really are read-only, so the natural stub correction is blocked by the check.
  • A duplicate def is reported and still half-compared against the last definition (the docstring at :68 says otherwise), so an @overload pair produces a spurious second error.
  • The printed diff is the raw text while the comparison is normalized, so a real divergence in a rewrapped docstring gets buried under wrap-only noise; diffing the normalized strings would fix it.

@pepone

pepone commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review. Addressed all four points in 8c0f0ab7f7:

  • Guarded the MRO traversal when the runtime object is not a class.
  • Recognized @property declarations and stopped requiring callable signatures for them.
  • Removed duplicate declarations from the comparison after reporting them.
  • Generated description diffs from the normalized prose used for comparison.

I also added focused regression coverage and reran Ruff, Pyright, and the IcePy stub consistency check successfully.

@pepone
pepone requested a review from externl August 14, 2026 17:35

@externl externl left a comment

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.

All four earlier points are fixed — I rebuilt IcePy and exercised each one. Two new things came out of that:

  • normalizeProse regressed reflow tolerance. LIST_ITEM = (?:[-+*]|#\.|\d+[.)])\s matches any wrapped line beginning with digits-then-period, so it refuses to fold it as a continuation. intVersion's docstring contains "…the returned value is 30901. For pre-releases…"; today the wrap happens to fall after 30901., but moving that wrap point — without changing a single word — produces intVersion: descriptions differ. That's the opposite of headline claim 2.
  • The @property fix trades a false rejection for a false acceptance. isProperty passes None as the stub signature (:113), so both signature-comparison branches are skipped entirely rather than checked differently. Marking EndpointInfo.type as a @property while IcePy ships type() -> int is now silently green, and pyright would then reject info.type(). Requiring that split() found no signature line when isProperty would keep the fix without the hole.

Also worth a pass: the description still covers only the first commit, so @property support, the normalized diff output, the duplicate suppression, the __mro__ guard and the normalizeProse rewrite are all undocumented.

For what it's worth, nothing became permissive — stub-only names, module-only names, arity drift, deleted docstrings and semantic edits inside attribute docstrings all still fire, and a reflow-only rewrap still passes.

@pepone
pepone requested a review from externl September 1, 2026 12:46
@pepone

pepone commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Both points fixed in e368ddc874, and the description now covers every commit.

  • normalizeProse treats a marker as structural only where reST would begin a block — after a blank line or at a new indentation — so intVersion's 30901. continuation folds again and rewrapping a paragraph reads as no change wherever the wrap falls. Section underlines still attach to the line above, and loadSlice's bullets stay distinct.
  • Properties and annotated attributes now carry a flag instead of a None signature, and IcePy opening their docstring with a signature line is reported. EndpointInfo.type as a @property against IcePy's type() -> int is red; a read-only getset declared @property stays green.

Copilot AI left a comment

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread scripts/checkIcePyStub.py Outdated
continue

continuation = indent == joinIndent
underline = not stripped.strip("-=~^\"'`#*+_")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in b29a33c343. A section adornment is now recognized by its shape — a single punctuation character repeated (ADORNMENT) — instead of a hand-picked character set, so Heading / :::::: stays structural.

Comment thread scripts/checkIcePyStub.py
pythonSupplied = (
value is None
and member == "__hash__"
or isinstance(value, types.WrapperDescriptorType)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in b29a33c343. You are right that the four __init__ slots were being swallowed — Properties, Communicator, ObjectPrx and Operation are all wrapper_descriptor, so their stub signatures were unchecked, leaving item 4 of #6443 open.

Rather than exempting less, the module now provides the missing source: those four types spell their constructor in tp_doc, the way a C type documents one, and the check holds the stub's own __init__ declaration to that line. A wrong Properties.__init__ signature is now reported as Properties: signatures differ. None of the four is re-exported by Ice/IcePyTypes.py, so no published page changes.

@externl externl left a comment

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.

A few gaps still open, all in scripts/checkIcePyStub.py:

  • if pythonSupplied: continue (line 310) drops every __init__ before signature comparison, so item 4 of #6443 stays open — a wrong Properties.__init__ signature keeps the check green.
  • topLevel (line 128) only suppresses, never verifies. A module constant the stub invents is never checked to exist, and Logger: Any in place of class Logger: silently drops all its member comparisons.
  • Attribute annotations are compared to nothing (line 122) — only the docstring is. underlying: EndpointInfo | None at python/python/IcePy-stubs/__init__.pyi:384 already disagrees with its own docstring and the getset doc, and the check passes.
  • joinIndent is cleared after a structural line (line 249), so rewrapping a bullet's wording reports descriptions differ with no word changed.

🤖 Reviewed with Claude Code

@pepone

pepone commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

All four fixed in b29a33c343, and the description now covers the widened scope.

  • __init__ signatures. You were right that item 4 was still open, and the exemption was not the only problem: there was nothing in the module to compare against, since the wrapper on a tp_init slot documents object's generic __init__. So the module now supplies it — Communicator, ObjectPrx, Operation and Properties spell their constructor in tp_doc, the way a C type documents one, and the check holds the stub's own __init__ declaration to that line. A wrong Properties.__init__ signature reports Properties: signatures differ. None of the four is re-exported by Ice/IcePyTypes.py, so nothing published moves. This is the only C++ in the PR.
  • topLevel. Module-scope names now go through the same existence check as everything else, so an invented constant is reported; TypeVar and its kind are excepted, since they are there for the type checker alone. Logger: Any in place of class Logger: reports IcePy defines a class, but the stub declares it as a value.
  • Attribute annotations. The C sources open a data member's description by naming its type, so the annotation and that name are now held to each other — underlying: dict[str, complex] is red. One correction on the example, though: Ice.EndpointInfo is IcePy.EndpointInfo (IcePyTypes.py re-exports the C type unchanged), so underlying's annotation and its docstring name the same class today. The difference is which spelling the C sources use — EndpointInfo.cpp qualifies it, ConnectionInfo.cpp does not — so the comparison ignores the qualifier rather than forcing a C++ edit.
  • joinIndent after a list item. A list item's own wrapping folds now: its text continues on the following lines indented past the marker, so rewrapping a bullet is no more a change than rewrapping a paragraph. That first indented line is still read as a marker if it is one, which is what a nested list looks like, and field lists and directives are left alone because what follows them is an indented body, not the same sentence carrying on.

Copilot's two also landed: the section-adornment set is now recognized by shape, so :::::: is not folded into prose.

The mutation battery in the description is the current one — 13 red cases, 4 green, including bullet-rewrap and Ice.-qualifier equivalence.

The checker now compares the stub's 26 annotated attributes against the
module's getset docstrings, tolerates paragraph reflow (equal-indent lines
join; blank lines, indentation changes, and section underlines still have
to match), reports descriptions the module ships that the stub lacks,
reports duplicate stub declarations instead of half-comparing them, exempts
only documentation CPython itself generates (slot wrappers, members
inherited from object, the __hash__ = None of an unhashable type) instead
of every dunder by name, and sweeps the module's own inventory in reverse
so a public member the stub omits is reported.

The reverse-prose check surfaced nine top-level functions the module
documents but the stub did not; their docstrings are copied into the stub.
normalizeProse treated any line opening with a list, field, or directive
marker as structural, including one that was only a wrapped continuation.
intVersion's "...the returned value is 30901. For pre-releases..." would
read as an enumerated item the moment the wrap moved, reporting drift for
a paragraph whose words had not changed. A marker now counts only where
reST would begin a block -- after a blank line, or at a new indentation --
while a section underline still attaches to the line above it.

Declaring a stub member @Property left its signature None, which skipped
both signature comparisons rather than changing them. A stub could mark a
member an attribute while IcePy documented it as callable, and the check
stayed green even though pyright would then reject the call. Properties
and annotated attributes now carry a flag, and IcePy shipping a signature
line for either is reported.

Claude-Session: https://claude.ai/code/session_0164LsXFe8qNNDfchEUjoCCN
Four gaps were left, and closing the first one needs the module's help.

A __init__ slot carries the wrapper CPython generates, whose docstring
describes object's generic one, so exempting it as CPython-supplied left
the signatures the stub declares for Communicator, ObjectPrx, Operation
and Properties unverified -- item 4 of #6443, which this claimed to close.
Those four types now spell their constructor in tp_doc, the way a C type
documents a constructor, and the check holds the stub's __init__ to that
line. None of the four is re-exported by Ice, so nothing published moves.

A name declared at module scope only suppressed the reverse sweep, so the
stub could invent a constant that no module attribute backs. Those names
now go through the same existence check as everything else, TypeVar and
its kind excepted, since they exist for the type checker alone. Standing
a value where IcePy defines a class -- `Logger: Any` -- is reported too:
it type-checks anything, and takes every member of the class out of the
comparison with it.

An attribute's annotation was compared to nothing. The C sources open a
data member's description by naming its type, so the two are now held to
each other, modulo the module a type is named through: Ice re-exports
IcePy's C types unchanged, so Ice.EndpointInfo and EndpointInfo are the
same class and both spellings appear.

Finally, normalizeProse folds a list item's own wrapping, so rewrapping a
bullet is no longer drift, and recognizes a section adornment by its shape
rather than a hand-picked character set that missed "::::::".

Claude-Session: https://claude.ai/code/session_0164LsXFe8qNNDfchEUjoCCN
@pepone
pepone force-pushed the fix/icepy-stub-check-gaps branch from b29a33c to 8ab72ee Compare September 1, 2026 14:54
@pepone

pepone commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Closing this, the script is getting to complex for what is worth, @externl is working on an alternative approach in #6695.

@pepone pepone closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: gaps in the IcePy stub/module consistency check

3 participants