Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jedi does not fetch annotations for dynamically generated class methods on JPype #1347

Closed
Thrameos opened this issue Jun 19, 2019 · 11 comments
Closed

Comments

@Thrameos
Copy link
Contributor

I am attempting get tab completion to work for the JPype module under IPython. JPype is a bridge between Java and Python. As part of the process it generates dynamic classes and methods which have no presence in terms of a Python file. The classes and methods implement the relevant Python APIs such as __doc__ and __annotations__. But no matter what I do I just get garbage outputs. Tracing down through as best I can it appears that Jedi is trying to open the source files rather than calling inspect to get type hints or even asking for the __doc__, in an effort to avoid calling Python code. The problem is there is absolutely no way to get type hints for dynamic classes produced by a CPython extension like JPype.

Can you please point me to some documentation on how to inform Jedi where the type hints are located? Or perhaps give me a some way to mark my objects as being safe to probe?

@Thrameos
Copy link
Contributor Author

Looking deeper it appears the my methods are not triggering PEP0484 anywhere. Thus my attempts to supply __annotations__ are unlikely to work. Is there some reason a compiled extension module can't use PEP0484 rather than just docstrings?

@davidhalter
Copy link
Owner

I just did a new release.

However __annotations__ is indeed not valued on builtin objects is at the moment not looked at. This could be changed. Note however that in the future there's going to be strings only in there, see also Python 3.8's from __future__ import annotations.

How do your annotations typically look like?

@Thrameos
Copy link
Contributor Author

Thanks for the reply. The system is currently pretty simple.

I have implemented a property on my methods (PyJPMethod) that calls a short piece of Python code the first time which uses Java reflection to create a dictionary. The dictionary currently just contains {"return":<type>} where type is the dynamically generated type that will be returned. The only exception is if there is only one overload in which case the dict also contains args#. If you would like a different dictionary contents that is certainly possible. I can also force the cache to be filled on creation if we must avoid any actual calls.

Note however that in the future there's going to be strings only in there, see also Python 3.8's from future import annotations.

It would be hard to make that into a string though as the dynamic class does not have any presence in Python as far as a path. It may live inside in multiple JVMs in which case there may be many instances with the same name (one for each JVM). It will always be a regular Python class with a standard __dict__. All of its contents are compiled objects written as a descriptor which themselves type (PyJPMethod). I could give a virtual home that is reachable in the form of a dynamic module, but then I would have to force the classes to point to a series of dynamic getattr paths (ie. jpype._jclass.virtual_jvm.id00732f912312.java.lang.String) I try to avoid that because it is already bad enough with my module being jpype._jclass and the classname java.lang.String. It gets rendered in exceptions everything from java.lang.String (good), String (horrible). jpype._jclass.java.lang.String (acceptable), jpype._jclass.String (weird) and depending on the exception handler.

The descriptors are sufficient to pass functionlike for the purposes of inspect. Thus inspect.signature generates the response '(*args) -> jpype._jclass.java.lang.String' or whatever is the type is, but that type is bogus because there is no java.lang.String class in module jpype._jclass. Sometimes there is more than one possible type but I don't know how to indicate it so I just give an empty dictionary.

I can provide an alternative API if required. The ideal API would be to call __returns__([<type>]) where the argument is a list of types that are being interrogated. It would then return the exact type that would be returned or None if there is no possible match in the strongly typed Java overloads.

Relevant sections of code for __annotations__

This code shows the properties that are generated for the Java method.

struct PyGetSetDef methodGetSet[] = {
  {"__self__", (getter) (&PyJPMethod::getSelf), NULL, NULL, NULL},
  {"__name__", (getter) (&PyJPMethod::getName), NULL, NULL, NULL},
  {"__qualname__", (getter) (&PyJPMethod::getQualName), NULL, NULL, NULL},
  {"__doc__", (getter) (&PyJPMethod::getDoc), (setter) (&PyJPMethod::setDoc), NULL, NULL},
  {"__annotations__", (getter) (&PyJPMethod::getAnnotations), (setter) (&PyJPMethod::setAnnotations), NULL, NULL},
  {"__defaults__", (getter) (&PyJPMethod::getNone), NULL, NULL, NULL},
  {"__kwdefaults__", (getter) (&PyJPMethod::getNone), NULL, NULL, NULL},
  {"__code__", (getter) (&PyJPMethod::getCode), NULL, NULL, NULL},
  {NULL},
};

Here is the code that generates the __annotations__ when request the first time.

def _jmethodAnnotation(method, cls, overloads):
    try:
        returns = []

        # Special handling if we have 1 overload
        if len(overloads)==1:
            ov = overloads[0]
            out = {}
            for i,p in enumerate(ov.getParameterTypes()):
                out['arg%d'%i] = JClass(p)
            out['return'] = JClass(ov.getReturnType())
            return out

        # Otherwise, we only get the return
        for ov in overloads:
            returns.append(ov.getReturnType())
        returns = set(returns)
        if len(returns) == 1:
            return {"return": JClass([i for i in returns][0])}

        # We have multiple return types, no way to make a signature
    except:
        pass
    return {}

The class also returns a docstring, but it is napolean style with customizations for Java. Having multiple overloads strung onto one class is really not something Python documentation can do, so I had to improvise to make it possible for it to get render in autodoc.

I attempted to get jedi to pick it up by adding my classes to the safe list for probing in jedi.extension.compiled. The docstring was parsed, but there is nothing for it as the return type is bogus and the style is not supported. Jedi didn't look call pep0484.get_return_type. Trying to jam that further, through hit a wall even after I moved py__annotations__ to be a method on FunctionContext and CompiledContext as the current pep0484 expects a tree and not an actual type.

Here is a short example to help make it more clear.

import jpype
import inspect

# Starts the slaved JVM
jpype.startJVM(convertStrings=False)

# Create a new dynamic class in the JVM
JString = jpype.JClass("java.lang.String")

# Create an object of that type
s = JString('foo')

# Probe it for behavior
print(s.substring.__annotations__)
print(inspect.signature(s.substring))
print(inspect.getdoc(s.substring))

Gives...

$ python3 testPy.py
{'return': <java class 'java.lang.String'>}
(*args) -> jpype._jclass.java.lang.String
Java method dispatch 'substring' for 'java.lang.String'

Virtual Methods:
  * java.lang.String substring(int)
  * java.lang.String substring(int, int)

Returns:
  java.lang.String

@davidhalter
Copy link
Owner

Today's your lucky day :) I quickly fixed it. It doesn't work for the 3.8 case, where I'm not even sure how to do that anyway, but your case should work.

Please test and have fun!

@Thrameos
Copy link
Contributor Author

Thanks for the quick turnaround.

I assume that I need to add these lines to my module to activate it.

try:
    import jedi
    import _jpype
    jedi.evaluate.compiled.access.ALLOWED_DESCRIPTOR_ACCESS += ( _jpype.PyJPMethod, )
except:
    pass

Or is there a different mechanism required?

@davidhalter
Copy link
Owner

I guess, yes :) It is a known problem. #1299 will be the ticket to probably do something like Script(..., allow_unsafe_getattr=True). Once that's done you may want to get rid of your hack.

@Thrameos
Copy link
Contributor Author

I am not sure that one would want to force allowing unsafe gettr everywhere. This solution would have the downside that since other packages such as IPython are deciding how to call Script, the problem would propagate to having to contact every maintainer and ask them to enable an unsafe option to use my module.

My types are derived from the python types thus they pass inspect.isfunction() which just checks if it is an instance of FunctionType. But the jedi list that checks for descriptor access does not check for inheritance but rather for a specific type on the list. Was it intentional to use

 if type(attr) in ALLOWED_DESCRIPTOR_ACCESS:

as opposed to something like

 if issubclass(type(attr), ALLOWED_DESCRIPTOR_ACCESS):

Barring the serious meta programming I am doing, there aren't too many compiled objects that would be trying to derive from Python base types. And anyone doing that is likely trying to pretend to be safe.

@davidhalter
Copy link
Owner

Was it intentional to use

Yes it was intentional. The only way this is safe is if you actually check the exact types. I think it was ok to catch your case as "insecure".

This solution would have the downside that since other packages such as IPython are deciding how to call Script

That's true, but they could also offer this as configuration. That's at least what people want (AFAIK). Or we could create a .jedirc.

@Thrameos
Copy link
Contributor Author

I am just worried about that case of having to document 8 different ways to set up the tab completion on IPython, PyCharm, Spyder, vim, Jupyter, Bob's Discount Python Front, etc... It would be a nightmare given I don't use all those packages (with the exception of Bob's). Things like .jedirc may work but by the same token different tools and systems have different ideas about home directories and the like, so it results in similar issues.

If the user wants to use a module like JPype that is adding new basic types via extension, and the documentation says "this will configure Jedi to accept Java types as Python native which is potentially unsafe. YOU HAVE BEEN WARNED." then they likely have already consented to calling potentially unsafe things for those limited set of classes. But then as I am trying to be as safe as I can (while using Java), there isn't really any user code that should be on that path unless they monkey patch my internal classes at which point they really didn't want safety. Having a master switch called on jedi that activates all unsafe operations (including for things other than JPype) would seem like inviting even more errors as it would mean some untested third party class gets trusted.

If you want, I can push the annotations to be pure caches, but that would be a performance hit as the annotations are only for use when editing. Alternatively I can force the user to import a module with the hacks ie. "import jpype.jedi" but I find not enough people actually read the docs for that to be very effective.

I should also note that my tracing indicates that Jedi entered my code at least once on an untrusted class. Not one that I added to the trusted list, but something that I was emulating that ended up being accepted and called. It was the meta class for the created class types.

@davidhalter
Copy link
Owner

I recently thought a lot about this stuff again and realized that it's probably pretty pointless to protect the user with these kind of measures. It is more of a safety thing. This means that subclasses of functions should be returned normally, but subclasses of properties should be "dealt" with.

The way how we deal with properties now is different, see #1299. For Interpreter completions they just get executed, for Script calls not. I figured this is ok for now and I'll wait for feedback.

Hope you are ok with this. I'm pretty sure your use cases will now be covered.

@Thrameos
Copy link
Contributor Author

Thanks for the info. I will check it out when I have the chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants