fix(index): the "lenient" Requires-Python policy was implemented as its exact opposite - #8
Open
jonyoder wants to merge 1 commit into
Open
fix(index): the "lenient" Requires-Python policy was implemented as its exact opposite#8jonyoder wants to merge 1 commit into
jonyoder wants to merge 1 commit into
Conversation
…ts exact opposite
Found by an independent correctness review against the production PyPI snapshot,
and verified here before fixing.
THE DEFECT. An unparseable Requires-Python is deliberately non-fatal: the comment
said so, pip agrees (it catches InvalidSpecifier and treats the candidate as
compatible), and the value stored to express it was `version.Specifiers{}`. But
`Specifiers.Check` iterates its specifier groups and returns false when there are
none — so the empty set admits NOTHING. "Over-admits a candidate" was in fact
"rejects every interpreter." types.go's "Zero value means unconstrained" was
simply false.
⚠️ THE FAR BIGGER BLAST RADIUS IS THE ABSENT CASE, not the unparseable one. The
parsing branch is guarded by `raw.RequiresPython != ""`, so a version that
declares no Requires-Python at all never enters it and keeps the same empty
zero value. In a production PyPI snapshot that is 2,027,153 versions. A consumer
trusting the documented meaning and calling Check directly would reject roughly a
quarter of the corpus as incompatible with every interpreter.
⚠️ THE EXISTING TEST DEFENDED THE DEFECT. TestRSFIndexUnparseableRequiresPythonIsLenient
asserted `RequiresPython.String() == ""` and stopped there — it asked what the
value RENDERS AS and never what it ADMITS. So the inverted policy shipped with a
passing test named after the property it violated. This is the fourth time in this
family of repositories that an implementation and a test written from the same
misunderstanding have agreed with each other. The test now asserts the policy
across five interpreters, and I verified it fails without the fix; a separate test
asserts the opposite direction (a real, parseable constraint must still EXCLUDE an
interpreter outside it) and I verified that one fails if leniency is bought by
making the accessor unconditionally true. Neither is vacuous.
THE FIX is a PackageMetadata.SupportsPython accessor that treats an empty
specifier set as unconstrained, plus a warning on the field itself not to call
Check directly. Deliberately NOT fixed by changing the library: an empty specifier
set admitting everything is what the reference implementation does
(`Version("3.11") in SpecifierSet("")` is True, measured) and `andCheck` already
returns true for an empty GROUP, so go-python-packaging is inconsistent with
itself and arguably wrong — but flipping `Check` from reject-all to admit-all is a
FAIL-OPEN change to a published library, which wants a human decision rather than
an unattended one. Filed there; this fix does not depend on the outcome.
Also verified while confirming this, and NOT changed: PPM never hits the trap,
because src/requirements/filter.go guards `specs == ""` and returns true before
calling Check. That workaround existing in the only other consumer is itself
evidence that "empty means unconstrained" is the semantics callers expect and
currently have to implement themselves.
Refs rstudio/package-manager#19437
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
An unparseable — or absent —
Requires-Pythonnow actually admits every interpreter, via anew
PackageMetadata.SupportsPythonaccessor. The field carries a warning not to callCheckon it directly.The defect
The policy was right and the implementation was its exact inverse.
An unparseable
Requires-Pythonis deliberately non-fatal — the comment said so, and pipagrees (it catches
InvalidSpecifierand treats the candidate as compatible). The valuestored to express that was
version.Specifiers{}. ButSpecifiers.Checkiterates itsspecifier groups and returns
falsewhen there are none, so the empty set admitsnothing. "Over-admits a candidate" was in fact "rejects every interpreter", and
types.go's "Zero value means unconstrained" was simply false.Measured on the production snapshot:
RequiresPython.Check(3.11) == falseforaiohttp-compress 0.2.1(unparseable constraint).The parsing branch is guarded by
raw.RequiresPython != "", so a version declaring noRequires-Pythonnever enters it and keeps the same empty zero value. In the productionsnapshot that is 2,027,153 versions — a consumer trusting the documented meaning and
calling
Checkdirectly rejects roughly a quarter of the corpus as incompatible with everyinterpreter. Measured:
falcon 0.1.8rc1, which declares none, also returnsfalsefor 3.11.TestRSFIndexUnparseableRequiresPythonIsLenientassertedRequiresPython.String() == ""andstopped. It asked what the value renders as and never what it admits — so an inverted
policy shipped with a passing test named after the property it violated.
This is the fourth time in this family of repositories that an implementation and a test
written from the same misunderstanding have agreed with each other.
Both directions are now pinned, and I verified each fails independently:
>=3.9constraint still excludes 3.8truealwaysThe third one matters: without it, "fix the leniency" could be satisfied by an accessor that
never rejects anything, which would be a worse bug than the one being fixed.
Why this is fixed in the caller and not in the library
An empty specifier set admitting everything is what the reference implementation does —
measured, not assumed:
And
go-python-packaging's ownandCheckalready returnstruefor an empty group, so thelibrary is inconsistent with itself: one empty group is vacuously true, zero groups is false.
By that reading the library is wrong.
But flipping
Checkfrom reject-all to admit-all is a fail-open change to a publishedlibrary, and that is not a call to make unattended — a consumer using it as a gate would
silently start admitting everything. Filed upstream for a human decision. This fix does not
depend on the outcome; if the library changes,
SupportsPythonbecomes redundant rather thanwrong.
Worth noting: PPM never hits the trap, because
src/requirements/filter.goguardsspecs == ""and returnstruebefore callingCheck. That workaround existing in the onlyother consumer is itself evidence that "empty means unconstrained" is what callers expect
and currently each have to implement themselves.
Scope
This is F1 of 15 findings from the review. The rest are filed separately — including a
nondeterministic dependency answer for versions whose keys are PEP 440-equal but spelled
differently, pre-release candidates selected as "highest" against PEP 440's default for 13,335
packages, and a transitive parse failure destroying an entire walk. None are fixed here.
go test -race ./...green,gofmtclean,golangci-lint0 issues.