fix: import three names used only in string annotations, and correct a false packaging claim (#642) - #666
Conversation
8f737e1 to
fb6a0d0
Compare
NEEDS CHANGES → addressed, pushed as
|
…a false packaging claim (#642) Three names appeared in string annotations that their module never imported, and they were mypy's complete set of ``name-defined`` findings for the package: * ``pcapkit/utilities/logging.py:350`` used ``Any``; added to the ``TYPE_CHECKING`` block beside ``IO``, ``Optional`` and ``Union``. * ``pcapkit/protocols/schema/internet/ipv6_route.py:136`` used ``Protocol`` and ``:271`` used ``Optional``; added, ``Protocol`` as ``ProtocolBase as Protocol``. Twenty-one sibling schema modules already spell it that way, in the same ``payload:`` stub; this makes twenty-two. mypy 2.3.1 over ``pcapkit``: 3 ``name-defined`` errors before, 0 after; 115 total errors before, 112 after, so nothing else moved. ``MANIFEST.in:14-17`` asserted that ``include README.md`` was "the only thing that puts it in an sdist" and that an sdist without it "cannot be installed at all". Both halves are false, and the file had contradicted itself since #631 wrote the correct mechanism seven lines below without correcting this. Deleting the lines and rebuilding gives a byte-identical sdist listing -- empty diff -- that installs with exit 0: ``setuptools/command/sdist.py:59-60`` ships the README unconditionally and ``setuptools/dist.py:460``'s default ``license_files`` glob ships ``LICENSE``. Of the original three ``include`` lines only ``CHANGELOG.md`` is load-bearing. The comment now says that. New ``tests/project/test_annotation_names.py`` resolves every string annotation in the package -- following a nested forward reference such as ``'list["Nested"]'``, while treating ``Literal`` members and ``Annotated`` metadata as the values they are -- against the names its own module binds. It reports the same three findings as mypy on the unfixed tree and none after. That module named ``ast.TypeAlias`` and ``ast.TypeVar`` directly, and both are PEP 695 nodes added in Python 3.12, so *every* test in it raised ``AttributeError`` on the 3.10 and 3.11 matrix jobs -- ``bound_names`` walks every node of every file, so the attribute is reached whatever a test does. Both are now resolved once at module scope through ``getattr(ast, ..., ())``, leaving the ``isinstance`` branches otherwise untouched: ``isinstance(x, ())`` is always False, so the branches stay live on 3.12+ and are simply unreachable below it. Chosen over a ``sys.version_info`` comparison because it writes no version number down at all -- a comparison states 3.12 next to the attribute it guards, and the two can then drift -- and over a per-node ``getattr`` because a module-level constant lifts the lookup out of a loop that runs on every node of every file. ``ast.TypeVar`` is the branch that earns its keep: it carries its name as a bare ``str`` and emits no ``ast.Name`` node, so forcing ``_TYPE_VAR`` to ``()`` on 3.14.7 turns ``T`` and ``U`` into false findings. ``ast.TypeAlias`` is defensive by comparison -- its name *is* an ``ast.Name`` in ``Store`` context, which the preceding branch already catches -- and is left as it stands rather than removed. A new ``test_a_pep695_type_parameter_is_in_scope`` pins both the guards and the behaviour, skipped below 3.12 because its fixture source cannot parse there. Measured on real interpreters rather than simulated. 3.10.21 and 3.11.15: 5 failed, exit 1 -> 5 passed, 1 skipped, exit 0. 3.14.7: all 6 pass, exit 0. 133 passed over ``tests/project`` and ``tests/utilities/test_logging.py``, exit 0, subtests unchanged at 487. No changelog entry on this branch. Per the rule that no code branch touches ``CHANGELOG.md`` or anything under ``docs/source/changelog/``, this change's entry -- and the wording correction the ``MANIFEST.in`` claim implies for the #619 entry, plus the missing ``(#570)`` and ``(#577)`` citations -- go to the shared changelog pull request #657 instead.
fb6a0d0 to
9f23972
Compare
The changelog changes #666 was carrying on its own branch, moved here so that #666 touches only `MANIFEST.in`, `pcapkit/protocols/schema/internet/ipv6_route.py`, `pcapkit/utilities/logging.py` and `tests/project/test_annotation_names.py`. #666 was the last open branch still editing `CHANGELOG.md` itself; it no longer does. Four pieces, not one, because #666 had amended existing entries as well as needing a new one. A new **Fixed** bullet for #642: three names used in string annotations that their own module never imported -- `Any` in `pcapkit/utilities/logging.py`, and `Protocol` and `Optional` in `pcapkit/protocols/schema/internet/ipv6_route.py`. The bullet names the `typing.cast` case specifically, because that is the one no running test can catch: `cast` never evaluates its first argument. It states plainly that nothing resolves at runtime that did not before, since `TYPE_CHECKING` is `False` when the interpreter runs, so that the entry is not read as a runtime fix. mypy 2.3.1's before/after is quoted as the measurement -- three `name-defined` errors to none, 115 total to 112 -- and the new `tests/project/test_annotation_names.py` is described as what pins the invariant without a type checker installed. Three missing citations recovered: `(#570)` on the L2TPv3 worked-example line, `(#577)` on the `register_extractor_engine` keyword line, and `(#619)` on the README rename entry. And the #619 entry's packaging claim corrected in place. It asserted that `include README.md` in `MANIFEST.in` was "the only thing that puts the README in a source distribution" and that an sdist without it "cannot be installed". Both halves are false: setuptools' own `sdist` command ships the README before `MANIFEST.in` is read at all, so dropping the line leaves the listing byte-identical at 861 entries, and `setup.py` reads the file from wherever it is executing, which for a `pip` install of an sdist is the unpacked sdist. #666 corrects the same claim in the `MANIFEST.in` comment, so the two stay in step. No `:pep:` role, though the new bullet discusses PEP 695: `util/changelog_md.py` converts only double-backtick literals and the `:rfc:` role, and raises `ResidualMarkupError` on anything else, exactly as the #661 entry hit with `:obj:`. Plain prose instead. 50 lines added to the two files, 11 reflowed. `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited; `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests, exit 0 -- the same counts the previous commit on this branch reported, so nothing else moved. Committed from a detached HEAD and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree at a stale f846523 and could not be taken here. Written against b2ec64b and rebased onto d47ada0, the branch having taken #641's entry and the #652/#650 boundary correction in the meantime. Both files conflicted, both at the append point rather than in substance -- #641's bullet and this one land in the same place at the end of **Fixed** -- so the resolution keeps both, #641's first. `CHANGELOG.md` was not hand-resolved: it is generated, so it was regenerated from the resolved entry file and `--check` re-run, which is the only resolution that cannot drift. Refs #642
|
GOOD TO GO Cross-review of What it checked and how
What it disputed, recorded rather than folded awayThe The comparative claim the number supports — identical listing with and without the line, empty Not verified, stated rather than implied
Note on this revision's scope changeThe changelog changes this branch was carrying have been moved to the shared changelog pull request #657 ( The |
Fixes #642
1. Three unimported names in string annotations
mypy 2.3.1 over
pcapkit, before and after — these three were the completename-definedset, and still are:pcapkit/utilities/logging.py:350usedAny; added to theTYPE_CHECKINGblock besideIO,Optional,Union.pcapkit/protocols/schema/internet/ipv6_route.py:136usedProtocoland:271usedOptional; added,ProtocolspelledProtocolBase as Protocolto match the twenty-one sibling schema modules that already import it that way, in the samepayload:stub shape. This makes twenty-two. (The PR originally said "nine"; that came from a truncated grep and the cross-review caught it. Re-counted: 21 onmain, 22 with this change.)The issue's claim that
logging.py:350is a live failure is half wrong, and the half that is wrong is the fixMeasured, on the worktree tree (
pcapkit.__file__printed and asserted,__editable__*finders stripped fromsys.meta_path):Refinement from the cross-review, and it cuts against this section: under plain
get_type_hintsthe fix changes nothing, as below. But this project's own Sphinx machinery —docs/source/conf.py'sbind_type_checking_names, which executes every module'sTYPE_CHECKINGblock viasphinx_autodoc_typehints._resolver.resolve_type_guarded_importsprecisely so autodoc'sget_type_hintscalls succeed — does evaluate those names. Under that mechanism the fix is load-bearing: on the branch_writes_to's hints resolve to{'candidate': Handler, 'stream': typing.Any, 'return': bool}, while onmainthe same mechanism still raisesNameError: name 'Any' is not definedfor_writes_toalone (the other four resolve, sinceOptionalandUnionare in the block). It never surfaces in a real docs build only because_writes_tois private and noautofunction/automoduledirective indocs/source/pcapkit/utilities/logging.rstdocuments it. So the "type-checker-visible only" conclusion holds for this package as it stands, but partly by accident — document or rename that function and it becomes a live docs-build failure.The
NameErroris real. But addingAnyto theTYPE_CHECKINGblock does not fix it, and cannot. After the fix, that call raises identically — becauseTYPE_CHECKINGisFalseat runtime, so aTYPE_CHECKING-only import is absent from the module namespace either way. The decisive evidence is the siblings:Four of those five fail on
OptionalandUnion, which are imported in the block. Identical output on an unmodifiedmaintree. So_writes_tois not qualitatively different from its neighbours: unresolvable-at-runtime annotations are a property of theTYPE_CHECKINGidiom this package uses in all 496 modules, not a defect of line 350.What line 350 did have, and what is fixed here, is a genuine type-checker-visible defect: it used a name imported nowhere at all, not even for type checking. That is why mypy flagged it and not the other four.
Not done here, and offered as a decision rather than taken unilaterally: making
get_type_hintsactually work would mean importing the typing names unconditionally. That fixes 5 functions in 1 of 496 modules, leaves 495 unchanged, and makes one module inconsistent with the package's universal idiom. It is a repository-wide style decision, not a missing import. Say the word and it is a one-line follow-up.All three names are therefore type-checker-visible only. Nothing here is described as a live bug.
The
unreachableatipv6_route.py:264is not relatedIt is adjacent by two lines and independent.
addresses: 'bytes' = ListField(...)at:211declares the attribute asbytes, so mypy narrowsbuffer = self.addressesand readsif not isinstance(buffer, bytes):at:228as statically false — making the whole block, endingreturn selfat:269, dead to mypy. At runtime the branch is live and load-bearing: theNOTEat:229records thatself.addressesreally is alist[bytes]when the schema was built viamake, and #556 is the issue where treating it asbytesraised.So it is a false positive of the declared-type convention that the file's own
# mypy: disable-error-code=assignmentheader exists to accommodate. Fixing it properly means widening the declared type to'bytes | list[bytes]', which changes the schema's public type surface and ripples into the__init__stub — a real change deserving its own review, deliberately not folded in here. It survives the fix unchanged, now reported at:265after the one-line insertion:2.
MANIFEST.in— re-measured from scratch, claim confirmed falseRe-measured independently rather than taken on trust, on setuptools 84.0.0, by
git archive-ing to a clean scratch tree and building both ways:diffemptypip install --no-deps→ exit 0include CHANGELOG.mddeletedCHANGELOG.mdgone, nothing elseREADME.md,LICENSE,CHANGELOG.mdandCITATION.cffare all still in the sdist built without those lines. What ships them:setuptools/command/sdist.py:59-60—README_EXTENSIONS = ['', '.rst', '.txt', '.md']feedingREADMES, appended unconditionally bysetuptools/_distutils/command/sdist.py:277-296beforeMANIFEST.inis read. The.mdentry is setuptools' own addition over the distutils default, which is exactly what made the Markdown rename stop mattering.setuptools/dist.py:460— defaultlicense_filesglob['LICEN[CS]E*', 'COPYING*', 'NOTICE*', 'AUTHORS*'], used becausepyproject.tomldeclares nolicense-files. It emitsadding license file 'LICENSE'in a build with the line deleted.One correction to the issue's own reasoning. It says
setup.py's unguarded read happens "from the working tree during the build, not from an unpacked sdist". That is wrong: duringpip installof an sdist,setup.pyexecutes inside the unpacked sdist, so the failure mode is reachable in principle. It just cannot be triggered by deleting the line, because the README is in the sdist regardless. The correction says this accurately rather than repeating the issue's version.Only
CHANGELOG.mdis load-bearing, confirmed by the third build. The twoincludelines are kept, now documented as an explicit statement of intent that does not depend on a setuptools default — a weaker reason than the one the comment used to give, which is the point of rewriting rather than deleting it.MANIFEST.in:47is corrected too: it justified therecursive-include docs/source/changelogline as "the same shape asinclude README.md, and for the same reason" — the same false premise, one propagation further. It now namesinclude CHANGELOG.md.The rewrite is comment-only: rebuilt with the edited file, the sdist listing
diffs empty against the baseline, 862 lines.3. Changelog: the self-contradiction, and the citations
docs/source/changelog/1.5.0.rst:961-965carried the identical false claim, contradicted by the#631entry ~250 lines below. Corrected in place.Matching the precedent.
375e9d411(#638) corrected the#630entry by doing both: editing the wrong prose in place and adding a new* **Fixed**bullet describing the correction. This PR does only the in-place half, deliberately — a new bullet for this correction belongs on #657, the long-lived shared changelog branch, and will go there as its own commit.Citations. The base rate has drifted since the issue was filed: re-measured on "does the bullet contain any
(#nnn)anywhere", the file now has 80 top-level bullets, 75 cited, 5 uncited — not the issue's 76/71/5, because four cited bullets have landed since. The five uncited are exactly the ones the issue lists. Three are fixed::944-979README landing page(#619)2221c2d8f docs(readme): trim to a landing page and convert to Markdown (#619):181-214code=class keyword(#570)1c5833e00 feat(protocols): opt-in code= registration with enum-type inference for Protocol/ProtocolBase (#514) (#570):343-350StreamEOFError/register_extractor_engine(#577)9c240a60e test(utilities,foundation): pin the quiet StreamEOFError convention and register_extractor_engine's keyword (#577)Two are deliberately left uncited, because no number could be recovered and a wrong number is worse than an admitted gap:
:136-139(Probe/CipherSuite/IntegritySuiteoffNamedTuple) — the exact-content commit is26d72fa14, which carries no PR number.#406is inferable from its position between(#405)and(#407)and frompr/406being the one missing ref in that range, but that is circumstantial and a guess does not belong in a published changelog.:336-338(format='text'→dictdumper.Text) — exact-content commit892c2100c, no PR number, and its neighbourscf06ff971/73bf00308have none either, consistent with a rebase-merge that never got a squash number. Nothing found.#619appears once elsewhere, at:1228, as a back-reference from the#631entry ("#619did not address it") — a reference, not a citation, and left alone.CHANGELOG.mdregenerated withutil/changelog_md.py, never hand-edited.--checkexits 0. Its diff is exactly three lines, one per edited bullet, so no pre-existing drift was reintroduced.Evidence
Exit codes read from files;
.venv/bin/python3.14.7.The new test fails on
mainand passes here.tests/project/test_annotation_names.pyresolves every string annotation in the package — includingtyping.cast's first argument, which is whereipv6_route.py:271hid, sincecastnever evaluates it — against the names its own module binds anywhere,TYPE_CHECKINGblocks included. Copied onto an unmodifiedmaintree:Name for name and line for line the same three mypy reports, with no mypy dependency and in 2s. It carries its own self-check: a module with a planted unresolvable name is written to a scratch directory and must be reported, because a source-analysis guard that silently found nothing would look exactly like success.
tests/project+tests/utilities/test_logging.py: 127 passed before, 129 passed after (the +2 are the new test's two methods), bothEXIT=0, subtests unchanged at 487.tests/utilities/test_logging.py tests/protocols/schema tests/protocols/internet/test_ipv6_extension_unit.py: 121 passed,EXIT=0.Coverage. Both changed lines are inside
if TYPE_CHECKING:blocks, which CPython never executes and coverage excludes, so no statement count moves. Measured over the selection above:ipv6_route.py100% (78 stmts, 0 miss, 12 branches, 0 partial);logging.py95% (78 stmts, 2 miss) with both misses at:314and:321, pre-existing and nowhere near line 46.Conflict note
docs/source/changelog/1.5.0.rstis also touched by #657 (docs/changelog-1.5.0), the long-lived shared changelog PR, so this may conflict. #657 merges last. These changes are corrections to existing prose and citations on existing bullets, not new bullets, which is why they are here; the new bullet describing these two fixes goes to #657.Corrections after cross-review
A cross-review on a different model (Sonnet) re-derived every measurement here independently. It confirmed the mypy counts (3 → 0
name-defined, 115 → 112 total), theget_type_hintsreasoning, theProtocolBase as Protocolchoice overtyping.Protocol, the absence of any circular-import hazard (traced throughbind_type_checking_names' own per-module walk: 462 modules, 0 failures), theunreachableanalysis, all three setuptools citations to the line, the three sdist builds, the changelog bullet counts (80/75/5 before, 80/78/2 after, independently derived), and that the new test fails onmainwith exactly the three findings. Four things changed as a result:get_type_hintssection understated the fix. The Sphinx-build refinement above is new, and it partly vindicates the issue's "live failure" framing in the one context that matters for this repo.'list["Nested"]'parses the inner name as anast.Constant, invisible to a walk overast.Name— so an unresolvable name one level of quoting down went unreported. Now followed, withLiteralmembers andAnnotatedmetadata deliberately not followed, because those are values and following them false-positives on the twenty-oddLiteral["big", "little"]annotations this package already has. Three new tests pin all of it.bound_namesis scope-blind, so a local variable namedOptionalanywhere in a file masks a genuinely missingOptionalimport in that file. Real, and I implemented the narrowing — module and class scope only, which is whatget_type_hintsactually consults. It then false-positived on real code:pcapkit/toolkit/scapy.py:208casts to'IPv6ExtHdrFragment', imported six lines above inside the same function, which is correct because acaststring resolves lexically — and mypy accordingly does not flag it. Telling the two apart means tracking a binding set per scope and knowing which construct resolves where, i.e. reimplementing the type checker. So the trade is made the other way on purpose: no false positives, one documented false negative. The docstring now records the whole episode, andtest_a_cast_to_a_function_local_import_is_not_a_findingpins it so that reintroducing the narrowing fails a test rather than a real module.The reviewer also found positive evidence that declining to cite
#406on theNamedTuplebullet was right rather than merely cautious: issue #406 exists and is about a scapy-engine bug, unrelated to that bullet. Citing it would have been actively wrong.One number it could not reconcile: it measured 863 total sdist entries where this PR says 862. Both of the conclusions that matter — empty diff between baseline and modified, exactly one entry lost when
include CHANGELOG.mdgoes — reproduced exactly. Recorded rather than papered over.Re-verified after these changes:
tests/project/test_annotation_names.pyEXIT=0, 5 passed (was 2); on unmodifiedmain,EXIT=1with the same three findings.tests/project+tests/utilities/test_logging.py: 127 → 132 passed, bothEXIT=0, subtests unchanged at 487.