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

EnforceOverrides doesn't work for magic methods #36

Closed
JoostFromMicrosoft opened this issue Jan 28, 2020 · 7 comments
Closed

EnforceOverrides doesn't work for magic methods #36

JoostFromMicrosoft opened this issue Jan 28, 2020 · 7 comments
Assignees

Comments

@JoostFromMicrosoft
Copy link

I tried marking an __enter__ method as final, but this doesn't work:

class MultiResourceManager(ExitStack, EnforceOverrides):
    @overrides
    @final
    def __enter__(self):
        with ExitStack() as stack:
            stack.push(self)
            self.acquire_resources()
            stack.pop_all()
        return self

    @abstractmethod
    def acquire_resources(self) -> None:
        pass

class OverridesEnter(MultiResourceManager):
    def __enter__(self):
        # this should fail!
        pass

The EnforceOverridesMeta class explicitly excludes names that start with double underscores (see enforce.py lines 10-11). I suspect the intention of this is to exclude names like __module__ or __qualname__. However, those could be more appropriately filtered by checking callable(value) instead.

The updated code would be:

class EnforceOverridesMeta(ABCMeta):
    def __new__(mcls, name, bases, namespace, **kwargs):
        cls = super().__new__(mcls, name, bases, namespace, **kwargs)
        for name, value in namespace.items():
            # Actually checking the direct parent should be enough,
            # otherwise the error would have emerged during the parent class checking
            if not callable(value): # <--- this line is different
                continue
            value = mcls.handle_special_value(value)
            is_override = getattr(value, '__override__', False)
            for base in bases:
                base_class_method = getattr(base, name, False)
                if not base_class_method or not callable(base_class_method):
                    continue
                assert is_override, \
                    'Method %s overrides but does not have @overrides decorator' % (name)
                # `__finalized__` is added by `@final` decorator
                assert not getattr(base_class_method, '__finalized__', False), \
                    'Method %s is finalized in %s, it cannot be overridden' % (base_class_method, base)
        return cls
    
    @staticmethod
    def handle_special_value(value):
        if isinstance(value, classmethod):
            value = value.__get__(None, dict)
        elif isinstance(value, property):
            value = value.fget
        return value

Relevant scenarios:

  • Like above: preventing override of a magic method such as __enter__ to ensure a behavior / safety by marking it final.
  • If __init__ is part of the interface (e.g. for factory functions operating on types), marking it as final.
  • Enforcing @overrides markers on magic functions to make it explicit when such functions are being overriden.

Note that the change as proposed above is not backwards compatible. To maintain backwards compatibility the existing class could be maintained and an additional class could be added with the new behavior (e.g. EnforceOverridesAll).

@mkorpela
Copy link
Owner

mkorpela commented Feb 9, 2020

Backwards incompatibility here is a real puzzle. I have to enumerate all the magic methods to be certain that this does not break anything.
For instance implementing eq or any other magic method without @overrides will then break.

@JoostFromMicrosoft
Copy link
Author

It might be easier to just add it as a new class.

As an aside, I had to make a few more tweaks to get it to work with properties (since properties aren't callable). This is the version I have now; changed lines marked:

class EnforceOverridesAllMeta(ABCMeta):
    def __new__(mcls, name, bases, namespace, **kwargs):
        cls = super().__new__(mcls, name, bases, namespace, **kwargs)
        for name, value in namespace.items():
            # Actually checking the direct parent should be enough,
            # otherwise the error would have emerged during the parent class checking
            value = mcls.handle_special_value(value) # NOTE: this line moved above the if
            if not callable(value): # NOTE: this line is different
                continue
            is_override = getattr(value, '__override__', False)
            for base in bases:
                base_class_method = getattr(base, name, False)
                base_class_method = mcls.handle_special_value(base_class_method) # NOTE: this line added
                if not base_class_method or not callable(base_class_method):
                    continue
                assert is_override, \
                    'Method %s overrides but does not have @overrides decorator' % (name)
                # `__finalized__` is added by `@final` decorator
                assert not getattr(base_class_method, '__finalized__', False), \
                    'Method %s is finalized in %s, it cannot be overridden' % (base_class_method, base)
        return cls

    @staticmethod
    def handle_special_value(value):
        if isinstance(value, classmethod):
            value = value.__get__(None, dict)
        elif isinstance(value, property):
            value = value.fget
        return value

@apirogov
Copy link

apirogov commented Aug 4, 2022

I'd love to have the improved version... would like to mark __init__ and some properties as final!

So will this be included officially in some form?

@mkorpela
Copy link
Owner

mkorpela commented Aug 5, 2022

@apirogov I try to find time to do this :)

@mkorpela mkorpela self-assigned this Aug 5, 2022
@apirogov
Copy link

Another issue is that EnforceOverrides is triggered by inner classes and thinks they are methods. Is this a bug?

@mkorpela
Copy link
Owner

mkorpela commented Oct 9, 2022

@apirogov could you make another issue for the case you explained?

@mkorpela
Copy link
Owner

mkorpela commented Oct 9, 2022

Fixed this issue in #105

@mkorpela mkorpela closed this as completed Oct 9, 2022
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

3 participants