Lookup classes in normalized builtins before trying sys.modules

[Jongy]

This logic was removed in b683c25, possibly under the assumption that all builtins exist under the builtins module - but this assumption is not correct, since some builtins like "method" are not available as builtins.method.
That's what "_normalized_builtin_types" is for (which was unused until this fix).

Resolves the immediate issue in #426, but IDK if it breaks anything else along the way 😅

Few notes:

1. Not tested, I just checked the specific issue described in inspect.isfunction(rpyc_remote_function) #426. @comrumino please tell me what you think, afterwards I'll add some tests for this case (builtins not under builtins)
2. Another possible issue (that existed before): in the builtin case, this assignment ns["__class__"] = _builtin_class is done, but it's later assigned again: ns['__class__'] = class_descriptor, where class_descriptor is a NetrefClass. I tested both cases (with the override, and without) and I couldn't tell the difference. Perhaps the flow taken in NetrefClass.__get__ is always to get the object and not its type? Need to see further. Either way, we should either skip the override, or remove the (unnecessary & misleading) first assignment. Wdyt?
3. If I understand correctly, the reason the change in b683c25 did not break all builtins, is that all those available under builtins were still using their matching local builtin. In that case, we can probably narrow down _normalized_builtin_types to contain only the builtins not accessible via builtins, like types.BuiltinFunctionType and type(None), ... and possibly we should do another "sweep" on builtins to ensure that none were forgotten?

Marking as draft, until we decide on those discussions.

[Jongy]

Okay, had some time to look at it again.

1. I ran the tests - everything passes except for test_ssl.py which fails with a seemingly unrelated error ssl.SSLError: [SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:1108); (also fails for me on master). Tested on Python 3.8.

2. During my testing, It seems that the object returned from NetrefClass.__get__ (for builtins) is the _builtin_class itself - which makes sense, since all members of _normalized_builtin_types, are... types. Now, I also admit I don't fully understand the purpose of NetrefClass. I see it was added in 8fa210d and the second assignment to ns["__class__"] is there ever since. So I think it's just fair to remove the first assignment.

3. I don't want to make any additional changes so I don't want to narrow down _normalized_builtin_types now. I know there are more builtins missed here. For example, types.AsyncGeneratorType (which we can get with async def f(): yield 7 followed by x = f()) will fail the isinstance even after this fix. Not sure how to run a complete check... (only way would be to look for all builtins in CPython source?) and since it doesn't pose a problem to me now, I guess we can wait until something comes up, if anything ever will. I'm adding it as a TODO in the code.

I added a test for this regression - ensures that builtin types that won't be found in sys.modules, will get their type correctly (fails without this fix).

[Jongy]

P.S, I added a debug print in class_factory that prints the name_pack & the resolved local class for it.

Here are the results before:

for builtins.type found <class 'type'>
for builtins.object found <class 'object'>
for builtins.bool found <class 'bool'>
for builtins.complex found <class 'complex'>
for builtins.dict found <class 'dict'>
for builtins.float found <class 'float'>
for builtins.int found <class 'int'>
for builtins.list found <class 'list'>
for builtins.slice found <class 'slice'>
for builtins.str found <class 'str'>
for builtins.tuple found <class 'tuple'>
for builtins.set found <class 'set'>
for builtins.frozenset found <class 'frozenset'>
for builtins.BaseException found <class 'BaseException'>
for builtins.Exception found <class 'Exception'>
for builtins.NoneType found None
for builtins.builtin_function_or_method found None
for builtins.generator found None
for builtins.method found None
for builtins.code found None
for builtins.frame found None
for builtins.traceback found None
for builtins.type found <class 'type'>
for builtins.function found None
for builtins.wrapper_descriptor found None
for builtins.method-wrapper found None
for builtins.list_iterator found None
for builtins.tuple_iterator found None
for builtins.set_iterator found None
for builtins.bytes found <class 'bytes'>
for builtins.bytearray found <class 'bytearray'>
for builtins.range_iterator found None
for builtins.memoryview found <class 'memoryview'>

(notice all those that were mapped to non-None are those available under builtins)

After the fix:

for builtins.type found <class 'type'>
for builtins.object found <class 'object'>
for builtins.bool found <class 'bool'>
for builtins.complex found <class 'complex'>
for builtins.dict found <class 'dict'>
for builtins.float found <class 'float'>
for builtins.int found <class 'int'>
for builtins.list found <class 'list'>
for builtins.slice found <class 'slice'>
for builtins.str found <class 'str'>
for builtins.tuple found <class 'tuple'>
for builtins.set found <class 'set'>
for builtins.frozenset found <class 'frozenset'>
for builtins.BaseException found <class 'BaseException'>
for builtins.Exception found <class 'Exception'>
for builtins.NoneType found <class 'NoneType'>
for builtins.builtin_function_or_method found <class 'builtin_function_or_method'>
for builtins.generator found <class 'generator'>
for builtins.method found <class 'method'>
for builtins.code found <class 'code'>
for builtins.frame found <class 'frame'>
for builtins.traceback found <class 'traceback'>
for builtins.type found <class 'module'>
for builtins.function found <class 'function'>
for builtins.wrapper_descriptor found <class 'wrapper_descriptor'>
for builtins.method-wrapper found <class 'method-wrapper'>
for builtins.list_iterator found <class 'list_iterator'>
for builtins.tuple_iterator found <class 'tuple_iterator'>
for builtins.set_iterator found <class 'set_iterator'>
for builtins.bytes found <class 'bytes'>
for builtins.bytearray found <class 'bytearray'>
for builtins.range_iterator found <class 'range_iterator'>
for builtins.memoryview found <class 'memoryview'>

[comrumino]

Sorry for the slow response. I read over the code and the changes seem reasonable. I'll merge this and let the CI do the test work. If the CI starts failing we can go from there.

[comrumino]

The changes seem okay in https://travis-ci.org/github/tomerfiliba-org/rpyc

The only thing of note would be to add a unit test to prove #426 is fixed. If you want the credit, I'd be glad to accept a PR for it. Otherwise, I'll get to adding a unit test when I get to it.

Again, thank you for the contribution!

[Jongy]

The changes seem okay in https://travis-ci.org/github/tomerfiliba-org/rpyc

The only thing of note would be to add a unit test to prove #426 is fixed. If you want the credit, I'd be glad to accept a PR for it. Otherwise, I'll get to adding a unit test when I get to it.

Again, thank you for the contribution!

Thanks for merging.

About the unit test - what's common to types.MethodType and NoneType is that both are not accessible via builtins. So the unit test I've added in 108ff8e was enough to my understanding (and I preferred to implement it with NoneType because that's more easily "created", comparing to a method which requires to define a class and create an instance, ...)

If you deem a specific test for the case of #426 is relevant here, I can post a PR adding one, sure.

[comrumino]

Clearly, you have put more thought into that unit test then me. I've added a comment in da188fc using your above explaination for future reference. To me, it was not obvious what the test is proving. I totally agree with your choice. I'll go ahead and close #426

[talsaiag]

thank you!