fix(core): stop key in cache falling back to the legacy sequence protocol - #2558
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): stop key in cache falling back to the legacy sequence protocol#2558LeSingh1 wants to merge 1 commit into
key in cache falling back to the legacy sequence protocol#2558LeSingh1 wants to merge 1 commit into
Conversation
…otocol
ProgramCacheResource documents, at length, that it deliberately has no
__contains__:
There is intentionally no ``__contains__``: the obvious
``if key in cache: data = cache[key]`` idiom is racy across processes
... and exposing ``__contains__`` invites that pattern.
But the class defines __getitem__ and neither __iter__ nor __contains__, so
CPython falls back to the legacy sequence-iteration protocol: `key in cache`
becomes `cache[0], cache[1], ...` compared against the *values*.
On the shipped backends that surfaces as a baffling error from a lookup the
user never wrote:
b"k" in InMemoryProgramCache()
TypeError: cache keys must be bytes or str, got int
and `list(cache)` / `dict(cache)` fail the same way. Worse, ProgramCacheResource
is public and meant to be subclassed. For a backend whose __getitem__ accepts
integers -- a list-backed cache, say -- the fallback answers silently and
inverted:
b"v" in cache -> True # b"v" is a VALUE
b"k" in cache -> False # b"k" IS a key
Set ``__iter__ = None``, the standard way to opt out of that protocol. `in`
and iteration now raise the plain
``TypeError: argument of type 'InMemoryProgramCache' is not iterable``,
which points at the real mistake, and the documented design actually holds.
Everything else is untouched: __getitem__, get(), len(), clear(),
update() with a mapping or with pairs, and the context-manager form all
behave exactly as before. update() iterates its argument, not self.
Contributor
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.
Problem
ProgramCacheResourcedocuments, at length, that it deliberately has no__contains__:The class defines
__getitem__and neither__iter__nor__contains__. CPython then falls back to the legacy sequence-iteration protocol:key in cachebecomescache[0],cache[1], … compared against the values.On the shipped backends that surfaces as an error about a lookup the user never wrote:
Worse:
ProgramCacheResourceis public (exported fromcuda.core.utils) and exists to be subclassed. For a backend whose__getitem__tolerates integers — a list-backed cache, say — the fallback answers silently and inverted. Verified against a minimal subclass of the real ABC:So the one idiom the docstring set out to prevent is available, returns wrong answers, and does so without any error.
Fix
__iter__ = None— the standard way to opt a class out of the legacy protocol.inand iteration now raise:which points at the real mistake, and the documented design actually holds. The docstring note gains one sentence saying so.
Everything else is untouched —
__getitem__,get(),len(),clear(),update()with a mapping and with pairs, and the context-manager form all behave exactly as before.update()iterates its argument, notself, so it is unaffected.__contains__remains non-abstract, as the existingtest_program_cache_resource_requires_core_methodsasserts.Tests
Added
test_program_cache_is_not_iterable_and_rejects_in, parametrized over both shipped backends:in,iter(), andlist()each raiseTypeError, whilecache[k],get()(hit and miss) andlen()keep working.Verification I could and could not do
_abc.py/_in_memory.py/_file_stream.pyimport only stdlib pluscuda.core._module.ObjectCode, so I loaded them by path with a three-line stub for that symbol and ran the behaviour before and after:ruff checkcompared against anupstream/mainbaseline of_abc.py: no new findings (2 pre-existingUP038under my local ruff 0.12.11, unchanged; the repo pins v0.15.9, where that rule no longer exists).ruff format --checkandpython -m py_compileclean.pytest cuda_core/tests/test_program_cache.pyitself — it importscuda.core, which is not importable here (no CUDA driver, no built extension modules). The stub run exercises the same code path. Please treat CI as the first real run.Related
Touches
_abc.pyonly; independent of #2553 (_in_memory.py/_file_stream.pyconstructors) and #2555 (_keys.py).