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

Metaclass custom or: Alternative syntax for unions requires Python 3.10 or newer #542

Closed
Zajozor opened this issue Oct 29, 2020 · 17 comments
Labels
waiting for user response Requires more information from user

Comments

@Zajozor
Copy link

Zajozor commented Oct 29, 2020

This was originally reported in #513 , however it ended up as being closed due to fixing a different problem related to type hints. As requested by @erictraut I'm reopening it here.

Environment data

  • Version: Pylance language server 2020.10.3 (pyright b881f28d)
  • OS and version: MacOS 10.15.7
  • Python version: Python 3.7.7
  • Django version: 2.2.2
  • Djangorestframework version: 3.9.4

Expected behaviour

Pylance should find out that the | operator is supported for the classes.

Actual behaviour

Pylance reports the use of the | operator as invalid usage of alternative union syntax with python <3.10.

Logs

No relevant logs

Additional information

We are using the Django Rest Framework and defining custom permission classes for a view:

Both permission classes which contain the | between them inherit from this class

@six.add_metaclass(BasePermissionMetaclass)
class BasePermission(object):
    """
    A base class from which all permission classes should inherit.
    """

    def has_permission(self, request, view):
        """
        Return `True` if permission is granted, `False` otherwise.
        """
        return True

    def has_object_permission(self, request, view, obj):
        """
        Return `True` if permission is granted, `False` otherwise.
        """
        return True

Further related code can be found at https://github.com/encode/django-rest-framework/blob/master/rest_framework/permissions.py#L97 and near that.

And the BasePermissionMetaclass inherits from OperationHolderMixin which supports the __or__ operation.

Please let me know if further information is required.

@jakebailey
Copy link
Member

jakebailey commented Oct 29, 2020

These stubs are sourced from TypedDjango: https://github.com/typeddjango/django-stubs

It's likely a fix would need to happen there.

@jakebailey
Copy link
Member

Scratch that, I missed that this is a third party django extension.

@jakebailey
Copy link
Member

jakebailey commented Oct 29, 2020

If you just use a regular metaclass, not the "six" version, does it work?

class BasePermission(object, metaclass=BasePermissionMetaclass):
    ...

That decorator behavior isn't likely to fit in the type system.

@jakebailey jakebailey added the waiting for user response Requires more information from user label Oct 29, 2020
@github-actions github-actions bot removed the triage label Oct 29, 2020
@Zajozor
Copy link
Author

Zajozor commented Oct 29, 2020

@jakebailey Yup, I can confirm that rewriting to:

# @six.add_metaclass(BasePermissionMetaclass)
class BasePermission(object, metaclass=BasePermissionMetaclass):
   ...

does remove the error reported by Pylance.

Sorry, but what does this mean in the context of this issue, can it be fixed here? Or somewhere else?

@jakebailey
Copy link
Member

The behavior of that six library is to provide compatibility between Python 2 and 3, but behaves in dynamic ways that a static checker can't understand. In this case, it's a decorator that appears to modify a class to emulate the behavior of specifying a metaclass, but we can't tell that the library is going to do this. We typically don't handle this sort of behavior as it doesn't scale to special case every library's special behavior.

@jakebailey
Copy link
Member

jakebailey commented Oct 29, 2020

@Zajozor Does this fix work for you, avoiding this six decorator? Or are you still intending on keeping it?

@erictraut
Copy link
Contributor

If you want to stick with the six decorator in the implementation, you (or the library author) could create a corresponding type stub file that defines it in a way that works with static type checkers.

@Zajozor
Copy link
Author

Zajozor commented Oct 30, 2020

Hi, thanks a lot for your time.

@jakebailey It's a third party library (Django Rest Framework), so no, it does not really work for me to rewrite it because of this.
Fortunately, upon a further research I found a repo with stubs for DRF - https://github.com/typeddjango/djangorestframework-stubs and using that + specifying a custom stub path in pylance the case is solved :) 🎉

One last question though, if I have two classes and use | on them while they do not support it (screenshot):
CleanShot 2020-10-30 at 07 56 57@2x

I would maybe expect the error message to tell me something like "MetaClass does not support |" instead of complaining about union type expression support in a place which is not related to typing.

It's definitely not a blocker, but i'd appreciate if you gave it a thought.

@NielsKorschinsky
Copy link

Hello,
I'm having the same issue like @Zajozor . I'm using the | in two different spots, where one does work while the other gets the Version error.
Parameter: doesnt work varx: int | None = 360
Vardeclaration: does work def __init__(self, .... , varx: int | None, ....)

Could you give me a heads-up on this topic?

@erictraut
Copy link
Contributor

@NielsKorschinsky, I'm not able to repro the problem based on your description above. Could you please extract a small, minimal piece of code that exhibits the issue you're seeing?

@NielsKorschinsky
Copy link

Of course, sure.
Working:

request_timeout: int | None = 60
    """timeout for api-requests"""

Not working:

class tesing():
    def __init__(self, varx: int | None) -> None:
        pass

Error message:
Alternative syntax for unions requires Python 3.10 or newerPylance

My version:

py --version
Python 3.8.2

@NielsKorschinsky
Copy link

I just did some more tests. It seems like the import:
from __future__ import annotations resolves this issue.

If i do not add this line in the code posted below, I'm reciving errors on every |sign, no matter where i placed it with the error message posted in my post above. Once i add it, everything works fine.
This error-message is missleading and should be optimized.

from __future__ import annotations
class tesing():

    varz: int | None

    def __init__(self, varx: int | None) -> None:
        vary: int | None
        pass

    def mymethod(self, varx: int | None) -> None:
        pass

@erictraut
Copy link
Contributor

Yes, I had assumed that you were using from __future__ import annotations. Without this, the use of | within type annotations will result in runtime exceptions in Python 3.9 or older, which is why pylance is flagging it in these cases.

I see the error in both of your examples above when from __future__ import annotations is missing, and neither error when it's present. That's the expected behavior.

Is that inconsistent with what you're seeing?

Screen Shot 2020-11-19 at 6 33 31 AM

Screen Shot 2020-11-19 at 6 32 54 AM

@erictraut
Copy link
Contributor

Incidentally, this has nothing to do with this bug report since this case doesn't involve custom metaclasses. If you still think there's a problem here, please open a new bug report so we can track it separately.

@NielsKorschinsky
Copy link

Your screenshots show the exact same behavior as it is within my code.
I just notice I've read Python 3.10 as Python 3.1 - resulting in my confusion. I wasnt aware python 3.10 is out yet.
Thanks for your help.
Shall I delete my comments?

@erictraut
Copy link
Contributor

OK, good to hear. No need to delete your comments. Others may find them useful.

Python 3.10 is under development. Pre-release (alpha) versions are available. We're just trying to stay ahead of the curve and implement new type-related functionality in preparation for the eventual release of 3.10.

@judej
Copy link
Contributor

judej commented Dec 29, 2020

Closing old issue. Please reopen if necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for user response Requires more information from user
Projects
None yet
Development

No branches or pull requests

5 participants