Immutability Fix including heap types breaking SCCs#83
Immutability Fix including heap types breaking SCCs#83xFrednet wants to merge 1 commit intoimmutable-mainfrom
Conversation
|
Should we add a test to cover the case that needs this? |
|
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
|
So, this added a bunch of additional warnings to the output, but I think this is a better solution than my optimistic fix earlier. |
| // `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. |
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.
There was a problem hiding this comment.
This PR emits a warning on back traversal. I considered hacking around this and doing a more active check if the type is visited. But I think that may add more overhead for buggy code.
This now adds it to the DFS stack to warn and traverse if the type was not visited. This is not perfect, as the created SCC may now know about all internal refs and see them as external ones, but that just causes a memory leak.
| * | ||
| * 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. |
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.
There was a problem hiding this comment.
I completely agree that tp_reachable should visit the type correctly, the code also assumes this. This marker is used by traverse_via_tp_traverse for the case that we fall back on tp_traverse.
2 participants
This used to let the SCC RC drop to zero or even negative or trigger an assert:
Credit to David for finding it!