Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow inheritance of _init from more than one level up #289

Merged
merged 5 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## 1.x.x (upcoming)

## Fixes

- In `pl.class`, `_init` can now be inherited from grandparent (or older ancestor) classes. (#289)

## 1.8.0 (2020-08-05)

### New features
Expand Down
29 changes: 14 additions & 15 deletions lua/pl/class.lua
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,23 @@ local compat
-- this trickery is necessary to prevent the inheritance of 'super' and
-- the resulting recursive call problems.
local function call_ctor (c,obj,...)
-- nice alias for the base class ctor
local base = rawget(c,'_base')
if base then
local parent_ctor = rawget(base,'_init')
while not parent_ctor do
base = rawget(base,'_base')
if not base then break end
parent_ctor = rawget(base,'_init')
local init = rawget(c,'_init')
local parent_with_init = rawget(c,'_parent_with_init')

if parent_with_init then
if not init then -- inheriting an init
init = rawget(parent_with_init, '_init')
parent_with_init = rawget(parent_with_init, '_parent_with_init')
end
if parent_ctor then
if parent_with_init then -- super() points to one above whereever _init came from
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Currently i think there is a nasty edge case here where if you erroneously call super() from an inherited _init, but that inherited _init does not legitimately have a super _init to call, you go into an infinite loop. This could be fixed if there were an else here that rawset 'super' to nil or error()ed.

rawset(obj,'super',function(obj,...)
call_ctor(base,obj,...)
call_ctor(parent_with_init,obj,...)
end)
end
else
rawset(obj,'super',nil)
Copy link
Contributor Author

@mcclure mcclure Aug 28, 2020

Choose a reason for hiding this comment

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

… The problem in the createK test is that when K() invokes call_ctor, it sets the "super" field to do a call_ctor(H._init).
call_ctor(H._init) immediately reaches the if parent_with_init then branch. It does not take it-- correctly-- because H has no superclass with an _init.
However, because the if parent_with_init then branch is skipped, self.super is unchanged... and it still points to function() call_ctor(H._init) end!
So we go into an infinite loop of call_ctor->super->init->call_ctor->super->init.
The only fix for this I can figure (I tried a few things) is to add an else clause here that rawsets self.super to nil.
That way, K._init's self.super is cleaned up before H._init is called, and the createK() test fails with "super is null" as desired.

[EDIT: This comment is obsolete, I came up with a better solution, see comment below]
However: I don't like this fix.

The problem is that this adds a rawset() to every object construction (a superfluous one, since we also rawset 'super' to nil after calling init!), but it only helps in a very specific error case (you wrote an _init that calls super() where it shouldn't and then you inherited THAT _init and invoked it via super()-- this is the only path that misbehaves). And the "fix" only results in getting a different kind of error. So this is taking on a global performance cost for better error reporting in an edge case.

If this were my project, I would throw away fbf33c1 and merge 84ad32b. I don't think the createK "fix" is worth the inefficiency.

The pain of this though is I think in LuaJIT on some platforms, stack overflows might result in a crash instead of a nice perror-catchable error.
(I have not run the createK() test in luajit because run.lua requires LuaFileSystem and I have not yet worked out how to get LuaRocks set up with command-line luajit.)

Okay I guess I just typed a lot about this but it is kind of a nuanced issue :(

[There was another paragraph here but I deleted it in the morning after doing some more testing.]

end
local res = c._init(obj,...)
local res = init(obj,...)
rawset(obj,'super',nil)
return res
end
Expand Down Expand Up @@ -146,6 +147,7 @@ local function _class(base,c_arg,c)
c.__index = c
setmetatable(c,mt)
if not plain then
if base and rawget(base,'_init') then c._parent_with_init = base end -- For super and inherited init
c._init = nil
end

Expand All @@ -160,15 +162,12 @@ local function _class(base,c_arg,c)
if not obj then obj = {} end
setmetatable(obj,c)

if rawget(c,'_init') then -- explicit constructor
if rawget(c,'_init') or rawget(c,'_parent_with_init') then -- constructor exists
local res = call_ctor(c,obj,...)
if res then -- _if_ a ctor returns a value, it becomes the object...
obj = res
setmetatable(obj,c)
end
elseif base and rawget(base,'_init') then -- default constructor
-- make sure that any stuff from the base class is initialized!
call_ctor(base,obj,...)
end

if base and rawget(base,'_post_init') then
Expand Down
75 changes: 75 additions & 0 deletions tests/test-class.lua
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ function B:foo ()
self.eee = 1
end

function B:foo2 ()
self.g = 8
end

asserteq(B(),{a=1,b=2})

-- can continue this chain
Expand All @@ -47,6 +51,77 @@ c = C()
c:foo()

asserteq(c,{a=1,b=2,c=3,eee=1})

-- test indirect inherit

D = class(C)

E = class(D)

function E:_init ()
self:super()
self.e = 4
end

function E:foo ()
-- recommended way to call inherited version of method...
self.eeee = 5
C.foo(self)
end

F = class(E)

function F:_init ()
self:super()
self.f = 6
end

f = F()
f:foo()
f:foo2() -- Test : invocation inherits this function from all the way up in B

asserteq(f,{a=1,b=2,c=3,eee=1,e=4,eeee=5,f=6,g=8})

-- Test that inappropriate calls to super() fail gracefully

G = class() -- Class with no init

H = class(G) -- Class with an init that wrongly calls super()

function H:_init()
self:super() -- Notice: G has no _init
end

I = class(H) -- Inherits the init with a bad super
J = class(I) -- Grandparent-inits the init with a bad super

K = class(J) -- Has an init, which calls the init with a bad super

function K:_init()
self:super()
end

local function createG()
return G()
end

local function createH() -- Wrapper function for pcall
return H()
end

local function createJ()
return J()
end

local function createK()
return K()
end

assert(pcall(createG)) -- Should succeed
assert(not pcall(createH)) -- These three should fail
assert(not pcall(createJ))
assert(not pcall(createK))
Copy link
Contributor Author

@mcclure mcclure Aug 28, 2020

Choose a reason for hiding this comment

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

These asserts test a series of similar things.
createG: This asserts G, a normal class with no _init, can be constructed.
createH: H inherits G, and defines an _init which calls :super(). The assert expects this should fail, with an error, because there is no such super-_init.
createJ: J inherits I which inherits H, and inherits H's _init, the bad one that calls :super() when it shouldn't. The assert expects this should fail too.
createK: K inherits J. K has an _init which calls super(), which will wind up calling H's _init, which is the one that calls :super() when it shouldn't. The assert expects this should fail.

All four asserts pass (meaning, the last three functions fail with an error), regardless of whether you are running 84ad32b or fbf33c1. The problem is which error. createH and createJ fail with "called nil function", which is good, because super is nil. But unless you include the fix in fbf33c1 createK fails with a stack overflow error.

Here's why:


--- class methods!
assert(c:is_a(C))
assert(c:is_a(B))
Expand Down