fixed byte_LOAD_ATTR

[hgluka]

Made it so byte_LOAD_ATTR properly handles __getattribute__ and __getattr__.

[freakboy3742]

The logic looks good; there's a couple of stray comments that can be cleaned up, but it seems sound.

The tests, however, need to be beefed up.

[freakboy3742]

No - the Function is only bound to the instance as part of the getattr process. The class has a single, common types.Function instance; each object instance gets a separate types.Method instance. In theory, this could be cached after first access, or at time of object construction; but Method is such a lightweight class it doesn't really matter.

[freakboy3742]

This comment can be removed, too.

[freakboy3742]

This test will pass; but it's not a good test. If you actually run this, you'll get exceptions because the y on line 58 is an unknown variable. The error will be consistent between Python and Batavia, so the test passes; but it's not fully testing like you think it is.

It also doesn't test __getattribute__, which was the major contribution of this patch.

[shaib]

I think that, in view of the current implementation and that this is Javascript, you should also add tests in the spirit of my negative example above (setting __getattr__ and __getattribute__ on the instance to verify that it doesn't work).

[hgluka]

Ah, of course, I should've checked in python before actually adding the tests :D

[freakboy3742]

These tests are better; but they're still a bit opaque, and missing a bunch of paths through the code. They also don't do any validation that attris being passed around correctly.

Another good test of __getattr__ et al would to give a demonstration of the descriptor protocol - that's an example of the code in active use.

[shaib]

This appears to be wrong -- we should be checking for __getattr__ and __getattribute__ on the class, not on the instance:

>>> class A:
...     pass
... 
>>> a = A()
>>> a.__getattr__ = lambda x:x
>>> 
>>> a.asdasdsad
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute 'asdasdsad'
>>> A.__getattr__ = lambda instance,x:x
>>> a.asdasdsad
'asdasdsad'

[hgluka]

Correct me if I'm wrong, but to do anything useful (including a decorator protocol) with user-defined __getattribute__ and __getattr__, we'd need to add __dict__ (and/or object.__getattribute__ for avoiding recursion with __getattribute__), which probably means changing the PyObject type.
For now, I definitely need to fix what @shaib pointed out, and I'll try to make the tests more robust in some other way.

[shaib]

Hi, @lucantrop, are you still working on this? mind a little collaboration?

[hgluka]

Yeah, sure, I was just going to ask for some help. In the testbed, the following code:

class A:
	pass

A.__getattribute__ = lambda instance, x: x

a = A()
print("class "+A.attr)
print("object "+a.attr)

prints:

class attr
Traceback (most recent call last):
  File "/tmp/tmp57ctpx0f", line 8, in <module>
AttributeError: 'A' object has no attribute 'attr'

which indicates that the object didn't inherit everything it should've from the class.
Also,

class A:
    def foo(self):
        return 'bar'
a = A()
print(a.foo())
print(A.foo(a))

prints

bar
Traceback (most recent call last):
  File "/tmp/tmp6tunw14q", line 6, in <module>
AttributeError: 'A' object has no attribute 'foo'

I suspect the problem lies somewhere in the make_class function in VirtualMachine.js, but, that's as far as I went into it.
On the other hand, I started implementing the object builtin, which is, as I said in a previous comment, needed in order to use __getattribute__. I'll push that part of the code when I get it working properly.

[hgluka]

It mostly works like cpython now, but there are a few fixes left to do.

[hgluka]

@freakboy3742 this is (finally 😄) finished, so you can review it again.

[freakboy3742]

👊