Refactor the complete ObjectHolder logic!

[mensinda]

OK, first of all, this PR ended up being a lot more involved (read bigger) than anticipated. At the same time, I don't think that it would be a good idea to split this PR, since doing this refactoring half-baked can get really complicated really fast. With this current, admittedly huge, PR, there is a clear before and after state.

Still, sorry for this big PR...

---

This PR is a complete overhaul of the ObjectHolder / holderify / unholder mechanism:

Now all objects that can be held by an ObjectHolder must be derived from InterpreterHeldObject. Additionally, ObjectHolder now directly inherits from InterpreterObject. Non-Object holders can be differentiated by inheriting from MesonInterpreterObject. They must not directly inherit from InterpreterObject.

This setup ensures that it can always be checked whether an object is present in its "held" or "raw" state. The dummy MesonInterpreterObject class ensures that non-ObjectHolders (like MesonMain) are not affected by this change.

---

The next big change in this PR is a clear separation where ObjectHolders and the plain objects are used. Additionally, InterpreterBase is now the sole instance in charge of holderifying and unholderifying objects. It is also the only class that should now deal with ObjectHolders. Called functions and methods will now always receive the "raw" objects instead of ObjectHolders. Additionally, interpreter functions and methods now must not return ObjectHolders (as in, doing so is now a hard error).

Code example before:

class Interpreter:
    # stuff ...

    def func_do_stuff(self, node, args, kwargs):
        exe1 = args[0]
        dep1 = args[1]
        if isinstance(exe1, ExecutableHolder):
            exe1 = exe1.held_object
        dep1 = getattr(dep1, 'held_object', dep1)  # We really had code like this...
        assert isinstance(exe1, build.Executable)
        assert isinstance(dep1, Dependency)
        # Do some more stuff...
        lib = build.SharedLibrary(exe1, dep1)
        return SharedLibraryHolder(lib)  # Returning an ObjectHolder is now an error

Code example after:

class Interpreter:
    def __init__(self, ...) -> None:
        self.holder_map.update({
            build.SharedLibrary: SharedLibraryHolder
        }

    def func_do_stuff(self, node, args, kwargs):
        exe1 = args[0]
        dep1 = args[1]
        assert isinstance(exe1, build.Executable)
        assert isinstance(dep1, Dependency)
        return build.SharedLibrary(exe1, dep1)  # let InterpreterBase take care of holderifying this

[lgtm-com]

This pull request introduces 1 alert and fixes 2 when merging 45f7c51 into 1086305 - view on LGTM.com

new alerts:

• 1 for Unreachable code

fixed alerts:

• 2 for Unused import

[dcbaker]

I like the ideas, and in general I'm in favor for this, but I do think that InterpreterHeldObject is looking at the problem backwards. The Build layer should be unaware of details of the Interpreter, just like the various *.Holder classes are supposed to abstract the Build layer in the interpreter. I think it would be better to have a BuildObject base class in the build module, and have the interpreter holders act on that instead of the other way around.

There's also a bunch of conflicts that are going to pop up from my qt rework, which fixes most of the Generator and GeneratedList cludge that we have right now.

[jpakkane]

What dcbaker said. I always get confused which way around the ownership/inheritance arrows go in UML diagrams but build objects should not even know that there is such a thing as an interpreter. That is what the Holder in the variable names originally mean. They are "interpreter things" that "hold" "build-level things".

[mensinda]

The main reason why I want something like InterpreterHeldObject is to tell mypy that only certain objects (generally all Meson objects that are not interpreter related) can be held by an ObjectHolder. That's why InterpreterHeldObject is also in mesonlib and not any interpreter package. Would be renaming it to MesonBaseObject or MesonBase be acceptable (it is currently an empty class anyway in mesonlib anyway)?

There's also a bunch of conflicts that are going to pop up from my qt rework, which fixes most of the Generator and GeneratedList cludge that we have right now.

Yeah, I know... But with such a big PR, conflicts will be unavoidable.

[mensinda]

Anyway, looks like I have some bad commits in my history. I will clean this up once the Qt module changes have landed.

[jpakkane]

The main reason why I want something like InterpreterHeldObject is to tell mypy that only certain objects (generally all Meson objects that are not interpreter related) can be held by an ObjectHolder

On the other hand this is too loose. The real limitation is that an ExecutableHolder can only hold an Executable. Dunno if that is doable with Mypy, though.

Alternatively can't you specify that type to be an union of Executable, SharedLibrary etc?

[dcbaker]

@jpakkane yeah, you can do that with mypy. We actually already do that, you use a Generic with specializations:

T = typing.TypeVar('T')

class Holder(typing.Generic[T]):
   pass

class ExecutableHolder(Holder[build.Executable]):
  pass

But you can constrain TypeVar as well, which might be another way to handle this "what can a Holder hold?" problem

Holdable = typing.TypeVar('Holdable', BuildObject, str, covariant=True)

class Holder(typing.Generic[Holdable]):
   pass

class ExecutableHolder(Holder[build.Executable]):  # this is fine, assuming build.Executable = class Executable(BuildObject)
  pass

class BytesHolder(Holder[bytes]):  # mypy error
  pass

[mensinda]

The other problem is that ObjectHolder is not limited to build.Target objects. For instance, mesonlib.File can now also be held by an ObjectHolder. With this change, basically all objects (except str, int, ...) returned by the interpreter need a corresponding ObjectHolder.

The current constraint of ObjectHolder (see interpreterbase.baseobjects) is T.TypeVar('InterpreterObjectTypeVar', bound=InterpreterHeldObject). InterpreterHeldObject is defined in mesonlib.universal. I don't see which other existing class we could use instead to cover all possible held objects...

[dcbaker]

You wouldn't use bound=, you'd have to do something like TypeVar(T, str, int, list, dict, build.Target, ..., covariant=True) (I think, I always get covariance and contravariance confused). Essentially TypeVar(T, bound=SomeClass) == TypeVar(T, SomeClass, covariant=True)

[mensinda]

@dcbaker So, we should list all possible base classes in the T.TypeVar directly instead of introducing a new one? I can test if this works.

[dcbaker]

Yes, that should work. Though having a shared base class would make that easier, and I like the idea of all build objects having a shared base. There's another option as well, though I haven't tested it. the abc.ABCMeta class can have "virtual subclasses" so you could (potentially do something like):

class Holdable(metaclass=abc.ABCMeta):
     pass

@Holdable.register
class Target
    pass

[jpakkane]

Though having a shared base class would make that easier, and I like the idea of all build objects having a shared base.

I'm not a fan of overuse of inheritance, which this thing is approaching. A union type better describes the property of "this can be any of these unrelated types A, B and C". Sadly Python's syntax for that is not the greatest because it is basically a duck typed language.

[mensinda]

Well, the shared base class is what I was trying to achieve with InterpreterHeldObject (though I admit that the name is misleading and should just have been something like HoldableObject).

From the current options, I would say I like the abc.ABCMeta approach the most (assuming that it works). Honestly, I am not a fan of adding all the types to the TypeVar, even if it is just base types. Just search for InterpreterHeldObject) in vscode and you will get around 30 hits (not including isinstance checks)... Maintaining this centrally in a TypeVar will be a pain. So I would prefer a decentralized solution (where the best one looks to be abc.ABCMeta).

[dcbaker]

I'm fine with a base class, but that should be part of build and not part of the mesonlib or the interpreter, at least, that's my take. Maybe others have other opinions.

[mensinda]

Also, I forgot to mention that this PR turns off the type annotations for the ast module, since what the AstInterpreter does is fundamentally incompatible with InterpreterBase...

[dcbaker]

Would it help if we had holders for everything, btw? I've really wanted to add a ListHolder, StringHolder, NumberHolder, BoolHolder, and DictHolder for a long time now...

[mensinda]

This was also my plan add the beginning of the refactoring, but for this we need the fundamentals from this PR. This way we can also extract the custom logic for these basic types from InterpreterBase. Also on where to put the base class: It can basically only be mesonlib because mesonlib.File is also a held object. And mesonlib cant depend on build. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[dcbaker]

Honest question, is there any reason File couldn't move to build? I've honestly thougtht that at least File and FileMode probably belong in build.

[mensinda]

OK, the abc.ABCMeta approach won't work because of python/mypy#2922 (although actually running the code still works),

Also, maybe I am doing something wrong, but using TypeVar('T', build.Target, mesonlib.File, ..., covariant=True) does also not work (also tried contravariant=True). Thus, I am going forward with a new abstract base class in build.py, unless we can come up with a better alternative.

[jpakkane]

It's not really nice that one needs to add code and inheritance levels just to work around the limitations of some tool like this. :(

[mensinda]

Renamed InterpreterHeldObject --> HoldableObject.

Sorry @dcbaker, moving File and/or HoldableObject to build.py is just impossible. Here is just one problem: build.py depends on dependencies, however, dependencies would now also depend on build.py for HoldableObject. Same problem with programs.py.

I am going to leave HoldableObject in mesonlib for now, however, we could make a new module for it if you really don't want to have it in mesonlib.

[dcbaker]

@mensinda I guess then let's go ahead and rebase and merge this.

[mensinda]

Yeah, let's hope that the CI stays green.

[lgtm-com]

This pull request introduces 1 alert and fixes 2 when merging 1ff6c83 into 79fec1c - view on LGTM.com

new alerts:

• 1 for Unreachable code

fixed alerts:

• 2 for Unused import