Allow inheritance of _init from more than one level up

[mcclure]

Addresses #288. My smoke test is

lua5.1 -e 'class = require("pl.class") class.A() function A:_init(x) print("A", x) end A(3) class.B(A) B(4) class.C(B) C(5) class.D(C) function D:_init(x) print("D", x) end D(6) class.E(D) E(7) class.F(E) function F:_init(x) print("F", x, "THEN") self:super(x) end F(8) class.G(F) G(9) class.H(G) function H:_init(x) print("H", x, "THEN") self:super(x) end H(10) class.I(H) I(11)'

Explanation:

Currently _init is exempted from "fat inheritance", and _init inheritance works by a strange workaround where the base class exactly one level up is checked for _init at constructor time. This appears to be done so that :super() can work properly, but it means that _init cannot be inherited from a grandparent.

This patch adds a _parent_with_init field to all relevant classes which points to the most recent ancestor with an _init implementation. This simplifies both __call and _class and might actually be a performance improvement (it removes a loop).

[mcclure]

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.

[coveralls]

Coverage decreased (-0.005%) to 84.719% when pulling dc2bb9b on mcclure:grandparent-init into f9f06de on stevedonovan:master.

[mcclure]

Hello @stevedonovan— is this repo still considering pull requests? Is there a way I could reformulate this patch, or a patch to fix the outstanding "broken super() from init()" issue I mention in the comment above, that would make this patch more likely to be accepted? Thanks.

[alerque]

@Tieske I just had to hack around this on yet another project. Is there something I can to to help this along? I'd be happy to test or code or whatever if it will help this fix (or some fix for grandparent inheritance) to land.

[mcclure]

If the "coverage decreased" is the problem— I am not sure how to use the coverage bot, but as far as I know all new added lines are covered by a test. I think the only reason coverage decreased was that my patch deletes more lines than it adds.

[Tieske]

the coverage problem can be either deleting more lines than adding, or it is a coveralls problem, in the sense that it takes only the last report it receives (windows and *nixes have different coverage so depending on who finishes last the coverage changes)

[Tieske]

as for the problem; the class system is profoundly broken for this exact issue you ran into. You cannot do inheritance while using the : operator. When using : you can only pass self, and only on first level inheritance this works, on 2nd level, you need the additional class.

So I've been thinking about rewriteing the whole Class thing in a non-fat inheritance style. Because we have seen so little reports about this issue, I think we can safely state that inheritance is 98% single. And then the extra metatable lookups will not be an issue.

[mcclure]

@Tieske :

"You cannot do inheritance while using the : operator. When using : you can only pass self, and only on first level inheritance this works, on 2nd level, you need the additional class"

I don't understand what this means. I use 2nd level inheritance all the time with the : operator and it works fine. The only thing that doesn't work is 2nd level inheritance of _init, and in my testing this patch corrects that.

I don't know if rewriting "class" is a good idea or not; personally I'd suggest do whatever is most efficient. What I would say is that rewriting "class" sounds like a task that could take time, and involve decisions and tradeoffs to be made. On the other hand accepting this patch has no downside, and you could do it immediately.

Fixing the bug I describe in this comment could also be done quickly, and I think also would not come with performance downsides (because I think the else case would only trigger when you are in the failure case the else case handles).

[alerque]

I second what @mcclure is saying here. I realize there are deeper issues with the class system, but this is a glaring issue that is clearly not functioning at all ... as in if you use the code as documented nothing happens, it's a noop. Anybody with working code has gotten there with a work around, not with the documented way of doing this. With this patch it works as documented. I've tested this on several projects and even run patched version of Penlight on one just for this. I don't understand why a low cost lightweight fix should be held back.

Overhaul the class system if you want, I have no objection. But in the mean time why not let this bit of code that works for people using the current system get released so we don't have to keep patching? This patch not getting attention was the single most prominent issue that made me request the previous maintainer of Penlight consider adding people to the maintainer crew. I offered to help then and I can still offer to help now (although time is slimmer now than a year ago when I offered) — what do we need to do to get this merged? Either in present form or with any revisions that need made...

[alerque]

This needs moving to a new/upcomming changelog entry.

[Tieske]

@mcclure can you fix the changelog entry, and the edge case you mentioned before (preferably with test cases)

[mcclure]

Thanks. I may not be able to get to this until next week, but I will get on that.

[mcclure]

Hm. Okay.

So in 84ad32b I fixed the changelog, and I added (1) a test for nth level inheritance using the : operator, because @Tieske was worried about that, and (2) a test for the "nasty edge case" I mention in my comment above. So that is all fine.

However, I also (in fbf33c1) fixed the "nasty edge case", and that did not turn out so well.

Will explain below:

[mcclure]

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:

[mcclure]

… 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.]

[mcclure]

One last note: I don't think that the super() stack overflow problem (the "nasty edge case") should be considered an impediment to merging this patch, for a simple reason: The stack overflow is present even without introducing this patch. If I take the test-class.lua from this patch and test it in 59dc58a, I can reproduce the stack overflow. I have to make a slight change to test-class.lua: I have to delete I = class(H) and change J's definition to J = class(H) (because without this PR, grandparent inheritance doesn't work, so without the change we are not testing the same thing). With that tweak, constructing J() enters an infinite loop in the pre-PR Penlight just as it does with the PR.

[EDIT: This comment is obsolete, I came up with a better solution, see comment below]
So basically, 84ad32b fixes the grandparent bug without introducing any new bugs, and you have the option of additionally merging fbf33c1 which fixes a second pre-existing bug in exchange for a small inefficiency.

[mcclure]

Okay… I apologize for the many posts :P

I think d38454b is my final submission. It avoids the second, duplicate rawset on each invocation, at the cost of an additional branch on each invocation (using parent_with_init, which is conveniently already a true value iff the second rawset is needed). I expect (but have not benchmarked) that the branch is less costly than the second rawset.

To recap, your choices are to merge 84ad32b, in which case you fix the grandparent bug with what I expect will be only a performance improvement; or merge d38454b ,in which case you additionally fix the pre-existing bad-:super bug (ie you replace a recursion loop, stack overflow and potential crash on LuaJIT, with a nice "null function" error) in exchange for one additional branch on every object construction.

[mcclure]

Hi, any updates on this? I'm sorry if I made this complicated, but I think either of 84ad32b or d38454b would be an objective improvement over the current code (and the code coverage bot is no longer complaining! 🙂).

[Tieske]

thx @mcclure !