Skip to content

Fix flyweight_with_metaclass sharing instances for different arguments - #495

Open
Darkslayer3324j wants to merge 1 commit into
faif:masterfrom
Darkslayer3324j:fix/flyweight-metaclass-key-collision
Open

Darkslayer3324j wants to merge 1 commit into
faif:masterfrom
Darkslayer3324j:fix/flyweight-metaclass-key-collision

Conversation

@Darkslayer3324j

Copy link
Copy Markdown

Problem

FlyweightMeta._serialize_params builds the pool key with "".join(map(str, args)) + str(kwargs) + cls.__name__. There is no separator and the type of each argument is lost, so different calls collide and the second one silently gets the first one's instance:

>>> Card2("1", "0") is Card2("10")
True      # should be False
>>> Card2(1) is Card2("1")
True      # should be False

The reverse also happens: Card2(a=1, b=2) and Card2(b=2, a=1) are not shared, because str(kwargs) depends on insertion order.

Fix

Use repr((cls.__name__, args, sorted(kwargs.items()))) as the key. It keeps argument boundaries and types, and is independent of keyword order. The key is still a string, so nothing else about the pool changes (the __main__ demo still passes).

Tests

patterns/structural/flyweight_with_metaclass.py had no tests (42% covered). I added tests/structural/test_flyweight_with_metaclass.py with cases for sharing, non-sharing, argument boundaries, argument types and kwarg order. Three of the five fail on master and all pass with the change. The repo's lint steps (flake8, isort, black --check, mypy, codespell) are clean and pytest tests patterns passes (136 passed).

Written with AI assistance (Claude Code); I reproduced the collisions above and ran the checks.

The pool key concatenated str(arg) with no separator, so Card2('1', '0') and
Card2('10') (or Card2(1) and Card2('1')) got the same key and the second call
returned the first call's instance. Use repr of the class name, args and
sorted kwargs instead, which also makes the key independent of kwarg order.
Add tests, since the module had none.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant