Skip to content

Fix __index fallback/getter priority and cross-class member pointer registration#245

Draft
kunitoki with Copilot wants to merge 9 commits into
masterfrom
copilot/try-call-parent-newindex-fix
Draft

Fix __index fallback/getter priority and cross-class member pointer registration#245
kunitoki with Copilot wants to merge 9 commits into
masterfrom
copilot/try-call-parent-newindex-fix

Conversation

Copilot AI commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Extends the __newindex setter-priority fix (Issue 242) to __index getters: a registered getter anywhere in the hierarchy now takes priority over a derived-class __index fallback. Also fixes two bugs introduced in the WIP commit.

__index getter priority (Issue 242 parity)

try_call_parent_index is split into two passes — extensible getters first, fallbacks second — mirroring the __newindex fix:

getGlobalNamespace(L)
    .beginClass<Base>("Base")
        .addProperty("x", &Base::getX, &Base::setX)
    .endClass()
    .deriveClass<Middle, Base>("Middle")
        .addIndexMetaMethod(&Middle::onIndex)   // catch-all fallback
    .endClass();

// Before: obj.x reads via Middle::onIndex instead of Base::getX
// After:  obj.x correctly calls Base::getX

Cross-class member pointer registration

push_class_property_getter/setter overloads used C both as the explicit registration class and as the deduced member-owner class. When registering a base-class member on a derived-class, deduction failed and the generic data-member overload won, producing compile errors. Fixed by splitting into independent C (registration class) and B (member-owner class) template parameters.

// Now compiles correctly — base-class member pointers on a derived registration
deriveClass<Derived, Base>("Derived")
    .addProperty("x", &Base::getX, &Base::setX)

__index fallback priority — preserved cases

The WIP reordering moved the fallback too aggressively, breaking two legitimate semantics. Restored:

  • Static __index: static fallback must shadow registered static property getters (StaticIndexFallbackCanShadowExistingStaticProperty).
  • allowOverridingMethods: instance fallback must precede class-table Lua methods so Lua-side overrides via __newindex take effect.

For plain instance __index (no allowOverridingMethods), fallback is still consulted only after all registered getters across the full hierarchy.

Copilot AI changed the title [WIP] Fix order of checking for setters and newindex fallbacks Fix newindex setter/fallback priority across the inheritance chain Jun 18, 2026
Copilot AI requested a review from kunitoki June 18, 2026 17:01
@kunitoki

Copy link
Copy Markdown
Owner

@copilot We should do the same for the __index fallback ?

…x fallback priority semantics

- Decouple registration class C from member-owner class B in 4 getter and 4
  setter push_class_property_getter/setter overloads so that base-class member
  function pointers work correctly when registering properties on a derived class
  (e.g. addProperty("x", &Base::getX, &Base::setX) on Class<Derived>).

- Restore __index fallback priority inside index_metamethod for two cases that
  the previous WIP commit broke:
    1. Static __index: static fallback must be consulted before registered static
       property getters (StaticIndexFallbackCanShadowExistingStaticProperty).
    2. Instance __index with allowOverridingMethods: the instance fallback must be
       consulted before class-table Lua methods so Lua-side overrides take effect
       (IndexFallbackMetaMethodFreeFunctorOnClassOverwriteProperty).
  For plain instance __index (no allowOverridingMethods), the fallback is still
  consulted only after all registered getters across the full hierarchy (Issue 242
  fix remains intact).

All 1080 tests pass.
Copilot AI changed the title Fix newindex setter/fallback priority across the inheritance chain Fix __index fallback/getter priority and cross-class member pointer registration Jun 18, 2026
Comment thread Source/LuaBridge/detail/CFunctions.h

@kunitoki kunitoki left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot we need a static_assert(std::is_same_v<C, B> || std:: derived_from <C, B>) in these methods (push_class_property_getter and push_class_property_setter) now taking C and B

Copilot AI commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@copilot We should do the same for the __index fallback ?

Yes — try_call_parent_index_fallbacks now scans parent metatables for __index fallbacks in a second pass (after try_call_parent_index_extensibles has already checked all registered getters), mirroring the two-pass approach used for __newindex. This was implemented in 7294db0.

Copilot AI requested a review from kunitoki June 18, 2026 22:24
Copilot AI requested a review from kunitoki June 18, 2026 22:40
Repository owner deleted a comment from Copilot AI Jun 18, 2026
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.

2 participants