Immutability Fix including heap types breaking SCCs#83
Open
xFrednet wants to merge 1 commit intoimmutable-mainfrom
Open
Immutability Fix including heap types breaking SCCs#83xFrednet wants to merge 1 commit intoimmutable-mainfrom
xFrednet wants to merge 1 commit intoimmutable-mainfrom
Conversation
Collaborator
|
Should we add a test to cover the case that needs this? |
Collaborator
Author
|
Yeah, I need to adjust the traversable but not reachable type for that. But your right |
This used to trigger an assert:
```
from immutable import freeze,set_freezable
class Node:
pass
Node.a = Node()
Node.b = Node()
Node.c = Node()
freeze(Node)
```
Credit to David for finding it!
xFrednet
force-pushed
the
fix-heap-type-cycles
branch
from
49cbe62 to
9025a92
Compare
Collaborator
Author
|
So, this added a bunch of additional warnings to the output, but I think this is a better solution than my optimistic fix earlier. |
mjp41
reviewed
Mar 14, 2026
Comment on lines
+364
to
+369
| // `tp_traverse` of heap types *should* include a | ||
| // `Py_VISIT(Py_TYPE(self));` since around Python 2.7 but | ||
| // there are still plenty of types that don't. LLMs currently | ||
| // also don't do this consistently. So, instead of visiting the | ||
| // type directly we throw it on to the DFS stack to check the | ||
| // correct behavior on back traversal. |
Collaborator
There was a problem hiding this comment.
I am worried about double visiting and having an incorrect RC. In the case of using a tp_traverse, we could check if it does visit the type? If it doesn't raise a warning, and add it manually.
| * | ||
| * If the type is part of an SCC we may end up with a higher SCC-RC | ||
| * since this can only account for one internal edge. But this will | ||
| * just cause a memory leak instead of crashing. |
Collaborator
There was a problem hiding this comment.
I'm a bit concerned by this. I think ultimately tp_reachable should have the correct semantics. We are adding it.
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
2 participants
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.
This used to let the SCC RC drop to zero or even negative or trigger an assert:
Credit to David for finding it!