Class inheritance syntax doesn't work as expected

[alerque]

Hey @mcclure maybe you can answer this. It's possible I've misunderstood how pl.class() is supposed to work (it is under documented after all!), but with inheritance working properl now (thanks for that!) I'm on a mission to get rid of some of the ugly hacks I had in place in various projects.

I can get inheritance to work using the documented syntax (see foo class below and the baz subclass), but I was under the impression that another syntax should work too (see foo class and bar subclass). Unfortunately it does nothing at all.

Since that doesn't work as expected, one way I found to make subclassing convenient is using pl.tablex.update() to populate the class after defining it as an exact copy of the parent class (see foo + qiz below). Additionally I've used another syntax (see foo + zar below) that works, but creates an extra degree of separation and (as of anything later than Penlight 1.8.0) now does not have a super() method. I think this new behavior is correct –it shouldn't have one because there is a break in the inheritance since the constructor is given a plain table instead of a class— but still it leaves things in an awkward place.

Can you comment on why foo+bar doesn't work in the second example here?

local class = require("pl.class")
local tablex = require("pl.tablex")

local function subclass_hack (base, model)
    return tablex.update(class(base), model)
end

local foo = class({
        attr = "original base",
        _init = function (self, arg)
            print("Running init() from foo class with "..arg.." identifies as "..self.attr)
        end
    })

local bar = class(foo, nil, {
        attr = "subclassed",
        _init = function (self, arg)
            self:super(arg)
        end
    })

local baz = class(foo)
baz.attr = "subclassed"
function baz:_init (arg)
    self:super(arg)
end

local qiz = subclass_hack(foo, {
    attr = "subclassed",
    _init = function (self, arg)
        self:super(arg)
    end
})

local zar = class({
    _base = foo,
    attr = "subclassed",
    _init = function (self, arg)
        self._base._init(self, arg)
        -- In Penlight <= 1.8.0 this worked:
        -- self:super(arg)
    end
})

-- Base class works as expected
local a = foo("1st")

-- This does *not* work as expected, it functions as an instance of foo
local b = bar("2nd")

-- This syntax works, its just cumbersome
local c = baz("3rd")

-- This hack to do what I expected pl.class to do in the bar class
local d = qiz("4th")

-- This gets the job done, but there is no super() function available
local e = zar("5th")

The output I get is:

Running init() from foo class with 1st identifies as original base
Running init() from foo class with 2nd identifies as original base
Running init() from foo class with 3rd identifies as subclassed
Running init() from foo class with 4th identifies as subclassed
Running init() from foo class with 5th identifies as subclassed

Obviously I expected the second line to be a subclass with different properties, not just a copy of the first class.

[mcclure]

Hi,
I can't speak for the penlight project.
This said, if I look at the documentation:
https://stevedonovan.github.io/Penlight/api/libraries/pl.class.html
It says:

class (base, c_arg, c)
create a new class, derived from a given base class. Supporting two class creation syntaxes: either Name = class(base) or class.Name(base). The first form returns the class directly and does not set its _name. The second form creates a variable Name in the current environment set to the class, and also sets _name.
Parameters:
base optional base class
c_arg optional parameter to class constructor
c optional table to be used as class

There are two problems I see here. First off, the docs seem vague to me around what "c" means. I don't feel certain I know what "optional table to be used as class" means.

Second off, the doc defines base as an optional base class. By my reading, this would make it undefined behavior if the first argument to class() were something other than a class. As a user of the library, I would not expect any specific outcome if I provided a simple table as the first argument. In your sample code, when defining foo, you provide a simple table as the first argument.

Reading the source, I find that if a plain table with no metatable is passed as the first argument to class(), then rather than base being treated as a base class, the value of c is reassigned to the first argument (base) and the new base class becomes c._base (which is probably nil). This is all very interesting, but I note none of this special behavior is defined in the documentation. So that means your code is probably correct, but at the least, there is a documentation issue here which Penlight should fix.

If I rewrite the code to not use the c argument, I get:

local class = require("pl.class")

local foo = class()
foo.attr = "original base"
function foo:_init(arg)
    print(string.format("Running init() from foo class with %s identifies as %s", tostring(arg), tostring(self.attr)))
end

local bar = class(foo)
bar.attr = "subclassed 1"
function bar:_init(arg)
    self:super(arg)
end

local a = foo("1st")
local b = bar("2nd")

I get:

Pearl:class mcc$ LUA_PATH="/Users/mcc/work/g/other/Penlight/lua/?.lua" lua5.1 test.lua 
Running init() from foo class with 1st identifies as original base
Running init() from foo class with 2nd identifies as subclassed 1

Which is exactly your expected behavior.

So, speaking as one user of the library to another, my advice is: specify the class members using the normal syntax and it should work, and I would avoid the c argument unless I knew exactly what I was doing, because its semantics seem unclear.

…

This is probably not very satisfying to you since you are doing fiddling-inside-the-abstraction stuff like calling tablex.update (which I personally would probably not do, because I could not be sure whatever it is you just did would work properly in future versions of Penlight). So, let's take one more look inside _class( and see why the c argument doesn't do what you're expecting.

If we look in the implementation for _class, we see that (unless you take the undocumented path where the first argument to class() is a plain table, and assuming you didn't set any of the other special undocumented, underscored keys like _handler) basically the main steps of class() are:

• Create a new empty table c or, if the user supplies a c, use the supplied one.
• If a base is provided, do a normal lua pairs() and copy all keys out of base into c.
• Set c to be its own __index. This is a standard Lua trick, I'll return to this.
• Create an empty table mt, give it a __call (which creates a new object, sets the new object's metatable as c, and calls the constructor code), and sets mt as the metatable for c.

So basically, class(x, nil, y) takes the table y, copies the items out of x into y, gives y a metatable that allows y to be callable, and returns y. Calling y after it's been given its metatable will return a new object (let's call it Z) which itself has y as a metatable. If you access a key on Z which has not been specifically set on Z, it will check Z's metatable (which is the now-altered y) for __index, see that y.__index is a table (just a reference to y), and return y[key]. Still with me?

So if we think about these steps for a moment, it's immediately obvious why your original sample code doesn't do what you expect it to. When you construct bar, The very first two things we do are:

• Set c to your table with attr="subclassed", then
• Overwrite all keys of c with the values from base-- and base contains attr="original class"!

I don't know what c should do, but that's what it does do.

My personal opinion, I think that what Penlight is doing is basically reasonable, that your use of "c" is not appropriate for what I understand "c" to be for (that is: allowing you to set metatable keys for your newly-constructed object) and I think if there is a problem at Penlight's end it is that it needs to document c better.

[mcclure]

Follow-up—

Documenting c better might mean:

• Document the "if base is a plain table rather than a class, it's reinterpreted as c" special behavior.
• Change the text "optional table to be used as class" in the documentation to "optional table to be used as metatable"
• Document however many of these special keys, like _create and _handler, that Penlight feels like supporting.
• … it might be reasonable to include a line somewhere clarifying that when both base and c are specified, the named members of base overwrite the named members of c at the moment class() is called, because that is sort of nonobvious. You could also remove the "not override" clause from tupdate, which would cause inheritance to only copy those keys from base that c does not already possess; I can't think of a single instance where someone would intentionally supply a key for c and want it immediately thrown away by being overwritten with a member from base).

[alerque]

Thank you very much for the analysis @mcclure, that's helped me make sense of a few bits I was flat out reading wrong in the code.

However as of right now I'm even less convinced than before that the current inheritance behavior is correct.

the named members of base overwrite the named members of c at the moment class() is called

Yes that's what's happening, but doesn't that seem backwards to you? I cannot imagine a situation where I want to create a new class Bar than inherits from class Foo but the things I explicitly define in Bar get overwritten with things from Foo. The whole point of inheritance should go the other way, no?

I do agree that the nature of c as a table or metatable is completely unclear. That needs sorting out in documentation, code, or both. But even leaving it's status as meta or not out of the picture, the inheritance direction is still backwards in my mind.

Here's an interesting one. There is one line of code in _class() that calls the local function tupdate() and uses the status of the base class (not c, base; the latter may be a copy of the former by the time the update is done, but the test for plain status is done first) as a boolean flag for whether or not to override keys. This appears to be the mechanism that is clobbering input keys with keys from the base class.

What's weird about this to me is I don't understand why the nature of my base table as a Penlight class or a plain table would ever reverse the direction that I wanted class inheritance to happen! In my initial examples the 1st (foo) and 5th (zar) classes are being detected as plain, the others are not. In the 1st case of foo, there is nothing to inherit from so nothing goes wrong. In the 5th case zar the base class is being detected as plain (true enough) but that's causing a reversal in the inheritance direction. What would ever be the use case for that?

If I apply this patch, everything works as expected for all of my examples:

diff --git a/lua/pl/class.lua b/lua/pl/class.lua
index 49246ee..e402b67 100644
--- a/lua/pl/class.lua
+++ b/lua/pl/class.lua
@@ -141,7 +141,7 @@ local function _class(base,c_arg,c)
     if type(base) == 'table' then
         -- our new class is a shallow copy of the base class!
         -- but be careful not to wipe out any methods we have been given at this point!
-        tupdate(c,base,plain)
+        tupdate(c,base,true)
         c._base = base
         -- inherit the 'not found' handler, if present
         if rawget(c,'_handler') then mt.__index = c._handler end

Now I get:

Running init() from foo class with 1st identifies as original base
Running init() from foo class with 2nd identifies as subclassed
Running init() from foo class with 3rd identifies as subclassed
Running init() from foo class with 4th identifies as subclassed
Running init() from foo class with 5th identifies as subclassed

In the final example 5 (zar) the super() function still doesn't work, but that's a secondary issue.

Of course to actually fix this I would dig in and clean up a little deeper, but first help me think about why the inheritance would ever go backwards and the base class would override something set in the class being defined.

After we agree on the direction I'll review the conflation between regular and meta tables, it seems to me the docs are calling something a metatable that isn't at all.

[alerque]

Cross-linking other open issues that are related to class init: #48, #79, #142, and #288.

[mcclure]

"but first help me think about why the inheritance would ever go backwards"

It depends on what the "c" argument was originally meant to do. If "c" is meant to be "here are the fields of the new class/metatable", then no, it doesn't really make any sense. One possible usage of "c" though is providing the basic "object" that the metatable is stored in. For example, say that you are using LuaJIT, and you want the class/metatable to be a LuaJIT object. This is sort of a stretch, though. For one thing, I'm not sure this LuaJIT idea could work with all the "weird" fields like _handler that the code accesses; for another, there's not really a lot of advantage to the class/metatable being a LuaJIT object, you'd be more likely to override the _create thing and create a LuaJIT object there (because it's useful for objects to be LuaJIT, but not classes); and lastly, even in this hypothetical situation, I can't think of a reason I'd want the underlying object to have its fields overwritten by the parent class fields.

If I had to make a guess as to why the inheritance "goes backwards", I'd guess that this is literally a bug. The clue is in the not dont_override check. Consider: When does dont_override trigger? The only time dont_override is true is the very specific circumstance where:

• "base" is a plain object
• "base._base" is specified
  It isn't enough to merely set both "base" and "c", because if "base" is a plain object and c is specified, c gets overwritten by "base".

That's so weirdly specific it feels like an artifact that maybe the code used to work another way.

Incidentally, I was wrong about something above. I said that the first-argument table approach is undocumented, and that _base is undocumented. It's not, both of these two things are documented in the guide, I just didn't see it. (_create and _handler are still not documented, though). So you were using a supported path all along, sorry lol

Anyway, you have convinced me, I think given specifying fields of your class in the table is a documented and therefore clearly intended use case when the metatable fields are given as the first argument, it's reasonable for a user to assume that the table will behave the same way when it's passed as the third argument. The inheritance going the "other way" just because you specified the base class as the first argument rather than as _base is incredibly unexpected and will probably never do anything but surprise a user and introduce bugs. I think penlight should remove not dont_override or and make it so inheritance never "goes backward". (And ideally should document _create, _post_init, _class_init and _handler.)

[mcclure]

After we agree on the direction I'll review the conflation between regular and meta tables, it seems to me the docs are calling something a metatable that isn't at all.

If I understand what you're referring to, I still think this part is correct. If you haven't read it already I suggest reading this page from the lua docs, specifically the part after "The use of the __index metamethod for inheritance is so common that Lua provides a shortcut".

[alerque]

Thanks for the docs link. Lua plays a bit fast and loose with the term "meta", and I see they call some things meta that I wouldn't have. That confusing point is exacerbated by the fact that that pl.class is like inception, even it's meta tables have meta tables. Classes are started using a constructor function plus _init() plus _class_init(). Just for grins there is a magic properties version. The docs being fuzzy on what tables are what just makes the whole thing a disaster. Not exactly meta-meta-tables, but close.

I think we're on the same page now, but this is a serious can of worms. Dropping the not dont_override nonsense go away is an easy place to start. It doesn't break anything internally and clearly fixes a behavior anomaly that should never have been.

However, that doesn't actually fix my problem like I thought it would. It turns out my test code at the start of this issue isn't actually testing what I thought it was. Even when I got the output I expected, it wasn't actually getting there the right way. I started trying to fix that, and fell down a rabbit hole. Your changes to avoid problems with super() definitely fixed some cases, but they don't work for others. Also it completely breaks one case that worked before, notably defining a class from a plain table using inheritance set in the table with _base. This is a model used internally by Penlight. None of those usage broke because none of them happened use use _init(), but it did break.

In trying to come up with tests for how I think it should work, I ran across a really weird usage in pl.List. I also ran into a bunch of C stack overflow situations.

I'm headed to bed and will have to dig back in another time, but I'm posting some of the things I was messing with to a draft PR.

[alerque]

For giggles, Penlight classes are their own grandpa:

Penlight/lua/pl/class.lua

Line 152 in ce0bdba

c.__index = c

On a more serious note, this line strikes me as bogus:

Penlight/lua/pl/class.lua

Line 156 in ce0bdba

c._init = nil

Deleting it fixes some cases and breaks others so obviously something needs to be done, but whatever needs to be done I don't see why it should be happening only to some alternative syntaxes but not others.

This stems from 552807f, the commit message for which is not particularly enlightening.

[mcclure]

Also it completely breaks one case that worked before, notably defining a class from a plain table using inheritance set in the table with _base. This is a model used internally by Penlight. None of those usage broke because none of them happened use use _init(), but it did break.

This sounds wrong to me, unless I'm thinking about it wrong removing not_override should have only the effect of making all cases act like the plain-table-plus-_base case. What's the test case for that? Is the test case to just make the change and run the unit tests?

Will attempt to look at the test cases and your PR later this week or weekend.

[mcclure]

I do think that "The 'plain table plus _base' case should act like the 'inherit from a class plus specify a table' case, especially since the documentation gives you nothing to suspect the two cases are different.

[alerque]

Also it completely breaks one case that worked before, notably defining a class from a plain table using inheritance set in the table with _base. This is a model used internally by Penlight. None of those usage broke because none of them happened use use _init(), but it did break.

This sounds wrong to me, unless I'm thinking about it wrong removing not_override should have only the effect of making all cases act like the plain-table-plus-_base case. What's the test case for that? Is the test case to just make the change and run the unit tests?

These are two different topics.

Removing the not dont_override shenanigans passes all tests cleanly, it's kind of a no-brainer at this point. But applying it doesn't fix everything either. Some possible class use cases (that we wern't testing for) still don't work.

But that's not what my previous comment was about. I was referring to your PR #289 broke one thing that was working. Albeit something that was not being tested properly, but something we've roughly agreed should work above. The case I'm talking about is the plain table with a _base value for inheritance. This test works in 1.8.0 but not 1.9.0:

local class = require("pl.class")

local Foo = class({
    _init = function(self)
      print("init foo")
    end
  })

local Bar = class({
    _base = Foo,
    _init = function(self)
      self:super()
      print("init bar")
    end
  })

A = Bar() -- expect "init foo\ninitbar", get "init bar"

In many other cases your work with :super() was great, it works in more places and doesn't cause recursion in places it did before. But that case above used to work and now it doesn't.