Import wildcard

[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 #254 in favor of this approach.

[claudiu-coman]

I added the Go tests, waiting for review.

[trotterdylan]

This looks great! Thanks for working on it. I like how simple it is. I have a few suggestions.

[trotterdylan]

It's simpler to use the dict field of the Object struct: module.dict

[trotterdylan]

Can you use seqForEach from seq.go here?

[trotterdylan]

memberName may not be a string so this unsafe conversion is not .. safe :)

It's worth looking at the CPython source to see what happens with non-string members in __all__ or in the globals dict.

[claudiu-coman]

So I read the source code and it seems to raise several errors:

• lines 48888 and 4893 seem to be unexpected errors, don't think we have to map these
• line 4895 if the module doesn't contain either one of all and dict. It doesn't seem like it's possible in the Go code
• line 4909 if all or dict (whichever gets chosen) is not iterable. Throws IndexError
• line 4924 if the member name is not a string, or the module does not contain the member
• lines 4926 and 4928 throw errors if locals is malformed, the member name is already validated from above.

Looking over the exceptions, there are already a few covered ones, without changing the code:

• IndexError when trying to iterate over a non-iterable object
• AttributeError when the module does not have a requested member

So, what I should focus on is:

• TypeError is the member name is not actually a string
• SystemError if the Globals (locals) is malformed. I don't know how to replicate that in real-life though, so I couldn't test in the shell, I just went through the code.

WDYT?

[trotterdylan]

if filterF != nil && filterF(memberName) {

[trotterdylan]

Maybe stash the globals dict in a local variable at the top of the function.

[trotterdylan]

I recommend using the the runInvokeTestCase pattern used in other tests if at all possible. It takes care of a number of things like frame creation and handling exceptions. You could return the globals dictionary that gets populated and check that against your expectation. Something like:

fun := wrapFuncForTest(f *Frame, m *Object) (*Dict, *BaseException) {
  raised := LoadMembers(f, m)
  if raised != nil {
    return nil, raised
  }
  return f.Globals(), nil
}
cases := []invokeTestCase{
  {args: fooAllDefinedModule, want: newTestDict(var1, val1, var2, val2)},
}
for _, cas := range cases {
  if err := runInvokeTestCase(fun, &cas); err != "" {
    t.Error(err)
  }
}

[trotterdylan]

Would be good to test some of the corner cases like when __all__ is not iterable.

[claudiu-coman]

@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.

[trotterdylan]

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.

[claudiu-coman]

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.

[trotterdylan]

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.

[trotterdylan]

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 b9a0c8a to get a sense of how this affects you. Let me know if you have any questions.

[claudiu-coman]

Thanks for letting me know!