Class-Commons icon indicating copy to clipboard operation
Class-Commons copied to clipboard

Spec clarification regarding metamethods

Open airstruck opened this issue 10 years ago • 10 comments

Can you clarify the meaning of this line in the spec:

It SHOULD NOT set any special functions, like metamethods

I don't know if my implementation violates this, since I use __call to instantiate my classes. Here is my class micro-library, base.lua:

return {
    extend = function (self, subtype)
        return setmetatable(subtype or {}, {
            __index = self, 
            __call = function (self, ...)
                local instance = setmetatable({}, { __index = self }) 
                return instance, instance:constructor(...)
            end 
        })
    end,
    constructor = function () end
}

And here is my class commons implementation:

if _G.common_class == false then return end

local Base = require 'base'

_G.common = {
    class = function (name, class, superclass)
        local c = Base.extend(superclass or Base, class)
        c.constructor = class.init
        return c
    end,
    instance = function (class, ...)
        return (class(...))
    end
}

This passes all the tests, but I worry that the __call metamethods on returned classes violate the spec. Am I misunderstanding the spec, or do I need to wrap my returned classes in an extra table or do something else in order not to violate the spec?

airstruck avatar Jul 08 '15 17:07 airstruck

Ah yes, that is worded poorly perhaps, it is meant to say no metamethods are set as a result of class members existing. That is, your library does what it normally does to a class, but doesn't use the class contents to generate other behaviour.

A good example might be slither, when it normally finds a method called cmp, it defines the __lt metamethod (and friends), but it is not supposed to (and doesn't) do that when using the class commons interface.

bartbes avatar Jul 08 '15 17:07 bartbes

Got it, thanks for clearing that up!

airstruck avatar Jul 08 '15 18:07 airstruck

By the way, it does explicitly mention the constructor as being allowed.

bartbes avatar Jul 09 '15 15:07 bartbes

It does mention the init constructor, but the constructor and the method that instantiates the class are not necessarily the same. For example in kikito's middleclass, the constructor is named initialize and the instantiator is new. In my case, the constructor is constructor and the instantiator is the __call metamethod. The instantiator calls the constructor, and the user supplies the constructor, but the user doesn't supply the instantiator... it's supplied by the library.

I'm not trying to split hairs here, just thinking out loud. I can't think of a better way to word the spec, but it did confuse me, probably just because my instantiator happens to be a metamethod.

What you wrote above does help clarify it, that the implementation "doesn't use the class contents to generate other behaviour." Actually I'm not too clear on why it would matter as long as a library using the class commons interface didn't rely on that added behavior, so a brief explanation of why this is important probably wouldn't hurt either.

airstruck avatar Jul 09 '15 19:07 airstruck

It's mostly a "least surprise" thing. As mentioned, slither treats __cmp__ specially, where other libs don't, so if I happen to have a method with that name (of course it was named so that would be uncommon), it would act differently with slither than it would with the other libs.

bartbes avatar Jul 09 '15 22:07 bartbes

Yeah, I see what you mean, I'm just thinking you could write in the implementor spec "add whatever members you want to the returned table, as long as class/superclass members can be resolved."

Then in the user spec at the bottom, write something like "any attempt to do anything with those returned tables other than access class members and pass them to instance is undefined behavior."

Just an idea, might make it easier on implementors and probably not much different for users.

airstruck avatar Jul 10 '15 00:07 airstruck

Here's an example of a case where it might make sense to be able to add extra behavior via metamethods. Say I want to use a simple class commons shim as a fallback in a library I'm creating. The name argument can't be omitted, but it is also not necessary for the shim to operate. It would be kind of silly if the shim just threw that argument out, so I add a __tostring so that name won't go to waste. Here's the shim:

if (_G.common_class == false) or _G.common then return end

_G.common = {
    class = function (name, class, superclass)
        local description = (name or 'anonymous') .. ' class'

        return setmetatable(class, {
            __index = superclass,
            __tostring = function () return description end
        })
    end,

    instance = function (class, ...)
        local description = tostring(class) .. ' instance'
        local instance = setmetatable({}, {
            __index = class,
            __tostring = function () return description end 
        })

        if class.init then class.init(instance, ...) end

        return instance
    end
}

Now, I can't see any harm in this, but as far as I can tell the spec says I shouldn't add this __tostring metamethod. But if that's the case, I can't do anything useful with that name argument, so my common.class function has a useless first argument. This might be alright for a normal implementation, but for a shim it doesn't make much sense.

airstruck avatar Jul 10 '15 18:07 airstruck

Here's another example that's a bit more involved regarding the __cmp__ case specifically.

Say I'm working on a game, and I'm using a networking library that deserializes data sent from a peer. Serialized data is sent across the wire, and the networking library gives me tables. The library can optionally create instances of a user-defined class rather than simple tables. The library also uses class commons, so the implementation behind the user-defined class can be slither, or 30log, or middleclass, or whatever I choose.

Now say I want to use slither, because I like the __cmp__ functionality for comparing vectors, and my user-defined base class takes advantage of that. Under the current implementation, that functionality is missing from slither when the slither classes are created through the class commons interface. In other words, the class commons interface to slither doesn't give me "normal" slither classes, it gives me slither-like classes with some functionality removed. In this case the functionality that was removed was the functionality I wanted in the first place.

This partly defeats the purpose of being able to have the networking library use whatever class library I want. If I want it to use slither classes, then I probably want full-featured slither classes, not a feature-restricted version.

airstruck avatar Jul 10 '15 18:07 airstruck

Well, according to the wording rfc (2119), "SHOULD NOT" allows for deviations from the spec. But if I understand what you're saying correctly, you think we should make it something like "may choose not to"?

bartbes avatar Jul 14 '15 16:07 bartbes

It's just my opinion, but the way I see it, the implementor spec should require that class/superclass members can be resolved in the returned table, but not make any recommendion about additional members one way or the other. The user spec should recommend (or require) that libraries using the interface should only access members of the returned table that were class/superclass members, and should not do anything else with the returned tables besides pass them in to common API functions.

In my opinion, implementors choosing not to provide some additional behavior under the class commons interface that would normally be provided without it seems likely to do more harm that good, since end users will most likely not be entirely familiar with the spec and will not understand why some functionality is missing.

In general, I think it's best for a spec like the implementor spec, that attempts to find common ground among disparate implementations, to try to stick to saying what's required, and stay away from saying what's prohibited. The restrictions seem more appropriate in the user spec.

Take all of that with a grain of salt, though; you've been looking at this particular spec a lot longer than I have ;)

airstruck avatar Jul 14 '15 18:07 airstruck