Skip to content

Register base classes before derived classes with a topological sort - #53

Open
cobriensr wants to merge 1 commit into
godotengine:masterfrom
cobriensr:fix-class-registration-order
Open

Register base classes before derived classes with a topological sort#53
cobriensr wants to merge 1 commit into
godotengine:masterfrom
cobriensr:fix-class-registration-order

Conversation

@cobriensr

@cobriensr cobriensr commented Jul 18, 2026

Copy link
Copy Markdown

Problem

The entry-point generator orders registered [GodotClass] types so that base classes register before derived classes and this is necessary because registration is eager: GodotRegistry.RegisterClassCore calls classdb_register_extension_class… with the parent's name immediately, so if a class is registered before its base, the engine rejects it (unknown parent) and the whole C# extension fails to load.

But the ordering in EntryPointGenerator was not a topological sort, but a declaration-order-dependent heuristic: it built the type map incrementally inside the same loop and only accumulated a "derived count" for base types already seen, then OrderByDescending'd that count. So the emitted order depends on source/file order.

Concrete failure: three classes Bat : Enemy, Enemy : Mob, Mob : Node declared derived-first (e.g. alphabetical file order) are emitted as Enemy, Mob, Bat -> Enemy before its base Mob -> the engine rejects Enemy -> the extension doesn't load. Nothing about the code is wrong, only the file naming.

Fix

Replace the heuristic with a real topological order: build the complete name -> spec map first, then order by each type's transitive registered-ancestor depth with a stable sort:

int GetRegisteredAncestorDepth(GodotRegistrationSpec spec)
{
    int depth = 0;
    string baseTypeName = spec.FullyQualifiedBaseSymbolName;
    while (specByTypeName.TryGetValue(baseTypeName, out var baseSpec))
    {
        depth++;
        baseTypeName = baseSpec.FullyQualifiedBaseSymbolName;
    }
    return depth;
}
return specs.OrderBy(GetRegisteredAncestorDepth).ToImmutableArray();

A base type always has a strictly lower depth than any type deriving from it, so base classes always come first; OrderBy is stable, so unrelated (equal-depth) classes keep their declaration order. (C# forbids inheritance cycles, so the walk always terminates.)

Verification

  • New regression test (EntryPointGeneratorTests): an inheritance chain declared derived-first. Fail-before: the old generator emitted Enemy, Mob, Bat (derived before base). Pass-after: Mob, Enemy, Bat.
  • The existing MainWithInheritance snapshot's order changed slightly (a sibling moved so equal-depth types are now in declaration order) — still every base before its derived; regenerated to match.
  • Godot.SourceGeneration.Tests: 22/22.

I found this issue while auditing the source generator for adoption-blocking bugs and this issue was not previously filed.

The entry-point generator ordered registered classes so that base classes
register before derived ones, but the ordering was a declaration-order-dependent
heuristic (it built the type map incrementally and only accumulated a count for
base types already seen), not a topological sort. When classes were declared
derived-first, a class could be emitted before its base. Because registration is
eager -- GodotRegistry.RegisterClassCore calls classdb_register_extension_class
with the parent name immediately -- the engine then rejects the class for an
unknown parent and the whole C# extension fails to load, based on nothing but
source/file order.

Replace the heuristic with a real topological order: build the complete
name->spec map first, then order by each class's transitive registered-ancestor
depth using a stable sort (so unrelated classes keep their declaration order). A
base type always has a strictly lower depth than any type deriving from it, so
base classes are always registered first.

Add a regression test with an inheritance chain declared derived-first, which the
previous heuristic ordered incorrectly.

Co-Authored-By: Claude Opus 4.8 (1M context) <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