Import wildcard

[alanjds]

google#263 opened on Feb 21, 2017 by @claudiu-coman

@trotterdylan thanks for suggesting doing everything in Go! Once I got familiar with the code, I managed to write the same functionality with so fewer lines. :D
I intentionally simplified the Python test since I will now be moving detailed checks to module_test.go. I'll start writing the test for LoadMembers.
I think it makes sense now to close google#254 in favor of this approach.

[alanjds]

Comment by claudiu-coman
Thursday Feb 23, 2017 at 00:33 GMT

---

I added the Go tests, waiting for review.

[alanjds]

Comment by claudiu-coman
Saturday Feb 25, 2017 at 12:42 GMT

---

@trotterdylan implemented all your suggestions. Thanks a lot for all the help, it's my first time coding in Go, but I'm learning.

The last travis job failed inexplicably. Tests passed on linux, but failed on osx, in apparently a part of the code I did not touch. If it's not flakiness and it's because of my code, how can I investigate it?

There are 2 more things I might have to do in terms of functionality.
First one is the Exception message when all is not iterable. Both my implementation and Python raise TypeError. However, the exception message from my code (if all is an int) is "'int' object is not iterable", rather than "'int' object does not support indexing". This is because the Python code does not call an iterator over the list of members, it uses a loop with an index to get each item in the sequence, generating an error in PySequence_GetItem. Should I try to replicate the exact Python behaviour, or is this good enough, considering we raise the same exception class?

The second one is the exception raised if the frame Globals is not a dictionary. In Python you get a SystemError. Wondering if it's worth doing.

[alanjds]

Comment by trotterdylan
Saturday Mar 04, 2017 at 18:45 GMT

---

Sorry for the delay in responding. I've been occupied with other things.

Re: Travis failure: This looks like unrelated flakiness. I'm not sure what's up with that but don't worry about it for the purposes of this PR.

Re: PySequence_GetItem: Good find. It looks like there's no specification that __all__ must be a list (or a tuple I guess), but that certainly seems to be the case for all modules I've seen. Whenever __all__ is referenced in documentation, it always seems to referred to as a list, e.g.: https://docs.python.org/2/tutorial/modules.html#importing-from-a-package

Given that CPython implicitly requires a "sequence" (i.e. a tuple or a list) we should probably require the same. From experimentation, it seems like it calls getitem on increasing values of an index until it gets an IndexError. So __all__ can in fact be a class with custom __getitem__ behavior, e.g.:

test.py:

class Foo(list):
    def __getitem__(self, n):
        raise Exception('wut')

__all__ = Foo()

$ python
>>> from test import *
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "test.py", line 3, in __getitem__
    raise Exception('wut')
Exception: wut

It's very awkward behavior IMO, but we should replicate it as best we can.

Re: Globals being not a dict: I don't think this can happen in our case because we're pulling globals from f.Globals() which returns a *Dict.

[alanjds]

Comment by claudiu-coman
Monday Mar 20, 2017 at 22:23 GMT

---

Sorry for not responding. I was away on a business trip, but I can start working on this again tomorrow.
I will study the exceptions more closely. Before I left I did some research and I think grumpy never raises that exception (it's not in the code). So maybe a better approach to raising that exact message in LoadMembers would be to:

• enable support for "'int' object does not support indexing" in the grumpy sequence code
• make LoadMembers use similar primitives as the original Python source code
  This should ensure the code throws the correct exception and it should make it easy for other parts of the code that use sequences to raise the same exception without having to hardcode it.
  Wdyt? I may be wrong as I don't know the code that well, but this is the first impression I got.

Meanwhile, I will do more investigation then come up with a solution.

[alanjds]

Comment by trotterdylan
Monday Mar 20, 2017 at 22:59 GMT

---

CPython and Grumpy are a bit different in how getitem is implemented. Here's the CPython code: https://github.com/python/cpython/blob/2.7/Objects/abstract.c#L135

The difference stems from the fact that CPython has two different __getitem__ slots, one for "sequences" and one for "mappings". If tp_as_mapping is set then it uses the mp_subscript slot which is similar to Grumpy's GetItem slot. For "sequence" types, tp_as_sequence will be set and the sq_item slot will be used, which takes a Py_ssize_t param instead of a generic *PyObject.

I suspect this is an optimization and I'm not convinced we need to make our implementation this complicated right now. If you just call GetItem() directly by passing increasing integer objects in, the behavior will be very close. The only difference is we'll get an 'int' object has no attribute '__getitem__' instead of the "does not support indexing" error. Both are TypeErrors, so this does not seem very problematic.

[alanjds]

Comment by trotterdylan
Wednesday Mar 22, 2017 at 19:28 GMT

---

Just a heads up: I've made some fairly significant changes to the import code that will conflict with your changes. It's probably worth reviewing google@b9a0c8a to get a sense of how this affects you. Let me know if you have any questions.

[alanjds]

Comment by claudiu-coman
Thursday Mar 23, 2017 at 23:18 GMT

---

Thanks for letting me know!

[alanjds]

Closing in favor of #80, the same code from @claudiu-coman refactored for the updated import machinery. Thanks for the PR. It was REALLY useful.