Fix singleton class lifecycle cleanup#776
Conversation
st0012
force-pushed
the
fix-singleton-lifecycle
branch
3 times, most recently
from
54bc228 to
5dc2e67
Compare
st0012
force-pushed
the
fix-singleton-lifecycle
branch
from
5dc2e67 to
1ba8c9c
Compare
st0012
force-pushed
the
fix-singleton-lifecycle
branch
from
1ba8c9c to
613bc62
Compare
st0012
force-pushed
the
fix-singleton-lifecycle
branch
from
613bc62 to
7bf434b
Compare
st0012
force-pushed
the
fix-singleton-lifecycle
branch
9 times, most recently
from
d83e97c to
f5dff8a
Compare
st0012
force-pushed
the
fix-singleton-lifecycle
branch
from
f5dff8a to
572acd3
Compare
| /// Ancestor chain is stale, but the declaration should not be pruned just | ||
| /// because it is currently waiting for new document data to be merged. | ||
| RecomputeAncestors, | ||
| /// A local anchor was detached. Remove only if no graph-level anchors remain. |
There was a problem hiding this comment.
I think it's worth expanding on what this means. I don't know what a local anchor is supposed to be.
| /// Declaration-specific modes processed by the unified invalidation worklist. | ||
| enum DeclarationInvalidationReason { | ||
| /// Definitions or structural ownership changed. The declaration may be | ||
| /// removed, or it may survive and need ancestor relinearization. |
There was a problem hiding this comment.
Does the need ancestor relinearization belong here? If it needs re-linearization, wouldn't the reason be RecomputeAncestors?
More generally, I think DeclarationInvalidationReason can be better documented with examples as it's not immediately obvious how this is used and what triggers each scenario.
| for singleton_id in attached_singletons { | ||
| items.push(InvalidationItem::declaration_changed(singleton_id)); | ||
| } |
There was a problem hiding this comment.
Can't we push to items directly above? Why do we need to accumulate in a vector and then loop to push into items?
Also, items.extend(attached_singletons) is more efficient.
| && let Some(declaration) = self.declarations.get_mut(&target_id) | ||
| { | ||
| declaration.remove_constant_reference(ref_id); | ||
| declarations_to_prune.push(target_id); |
There was a problem hiding this comment.
If I'm following the logic correctly, this means that we're enqueueing all declarations for possible pruning. Is that understanding correct?
This is a inefficient because it only really applies to singleton classes.
Consider this for example:
class Foo; end
FooFor Foo, it does not matter if the reference is removed. The declaration can never be pruned unless the definition is removed. Only Foo::<Foo> might possibly get pruned by the removal of constant references.
| for def_id in &missed_def_ids { | ||
| declaration.remove_definition(def_id); | ||
| } | ||
| fn detach_definitions_from_any_declaration( |
There was a problem hiding this comment.
Just to confirm my understanding here: it seems that we're taking orphan definitions and looping through them to try to remove their ID from being registered in any declaration. In what scenario can this happen?
If a definition ID got registered in a declaration at any point, it means we managed to connect them correctly. If an incremental change comes it that would make the definition orphan, then I would expect that iteration to follow definition_id_to_declaration_id and successfully remove itself from the declaration.
So I'm not sure what conditions can lead to:
- We correctly register a definition ID in a declaration
- The definition becomes orphan, but we haven't removed the ID from the declaration
- Now we need to loop through all of them to cleanup
This feels like a scenario that shouldn't ever happen. It might be a bug elsewhere in the invalidation algorithm.
| if let Some(namespace) = declaration.as_namespace() { | ||
| let has_reference_anchor = match namespace { | ||
| Namespace::SingletonClass(_) => { | ||
| if self.singleton_class_needed_for_extend(namespace) { | ||
| return false; | ||
| } | ||
|
|
||
| !namespace.references().is_empty() | ||
| } | ||
| Namespace::Todo(_) => false, | ||
| Namespace::Class(_) | Namespace::Module(_) => !namespace.references().is_empty(), | ||
| }; | ||
|
|
||
| // Synthetic namespaces such as TODO parents and singleton classes can | ||
| // have no definitions while still owning surviving members or being | ||
| // referenced. Pruning is allowed only after every graph-level anchor | ||
| // is gone: definitions, members, references (Class/Module/Singleton), | ||
| // an attached singleton class, and — for singleton classes — an | ||
| // `extend` on the attached object (handled by the early return above). | ||
| // TODO parents are not kept alive by references alone. | ||
| return namespace.definitions().is_empty() | ||
| && namespace.members().is_empty() | ||
| && !has_reference_anchor | ||
| && namespace.singleton_class().is_none(); | ||
| } | ||
|
|
||
| declaration.has_no_definitions() |
There was a problem hiding this comment.
This might be a better fit for a method in Declaration. Something like declaration.is_unanchored().
| let Some(namespace) = self.declarations.get_mut(&decl_id).and_then(|d| d.as_namespace_mut()) else { | ||
| return; | ||
| }; | ||
| fn detach_edges_and_queue_prune(&mut self, decl_id: DeclarationId, queue: &mut Vec<InvalidationItem>) { |
2 participants
Summary
Fixes singleton class lifecycle cleanup during document deletion and re-indexing. Singleton classes are now pruned only when they are no longer anchored by definitions, members, constant references, or surviving
extendmixins on their attached object.The cleanup path is split into focused steps for method references, constant references, unmapped definitions, raw definition entries, and declaration pruning. Removability is checked through the graph so singleton classes can account for external anchors, while TODO parents are not kept alive by unresolved references alone.
The invalidation worklist now keeps declaration lifecycle modes under
InvalidationItem::Declaration { id, reason }, so declaration-specific invalidation reasons stay localized instead of widening the top-level work item enum.