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

Rule request: abstractmethod appears on normal class #12861

Open
Skylion007 opened this issue Aug 13, 2024 · 3 comments · May be fixed by #14688
Open

Rule request: abstractmethod appears on normal class #12861

Skylion007 opened this issue Aug 13, 2024 · 3 comments · May be fixed by #14688
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule

Comments

@Skylion007
Copy link
Contributor

Skylion007 commented Aug 13, 2024

the case where an abstractmethod decorator is used on a class that does not subclass abc.ABC uses the ABC metaclass, is probably a bug. It would be great to have a rule to detect the obvious case where it is used on the wrong type of class (when all ancestors of the class in the same file for instance).

If there is a rule that already implements this, let me know. :)

Here is a real example from PyTorch:

class RemoteCacheBackend:
    """
    A backend implementation for accessing a remote/distributed cache.
    """

    def __init__(self, cache_id: str) -> None:
        pass

    @abstractmethod
    def get(self, key: str) -> Optional[object]:
        pass

    @abstractmethod
    def put(self, key: str, data: bytes) -> None:
        pass
@MichaReiser
Copy link
Member

@AlexWaygood any thoughts on this?

@AlexWaygood
Copy link
Member

I think this is a good idea for a rule. I agree that if a class that doesn't have ABCMeta (or a subclass of ABCMeta) as its metaclass, but has @abstractmethod-decorated methods, that's probably a bug. Here's a few things we'll need to keep in mind, however:

  1. We need to recognise as valid any class that has metaclass=X, or any class that inherits from a class that has metaclass=X, where X is either abc.ABCMeta or a subclass of abc.ABCMeta.

  2. For a class like the example @Skylion007 gives, that doesn't inherit from any classes or have a custom metaclass, it's easy to see that it doesn't have ABCMeta as its metaclass. But we'd need multifile analysis to be able to accurately determine whether this class has ABCMeta as its metaclass, since it depends on whether ForeignSuperclass has ABCMeta as its metaclass or not:

    from other_module import ForeignSuperclass
    
    class Klass(ForeignSuperclass): pass

    What should we do in cases like this? I vote for not emitting the error if a class inherits from any classes that come from external modules, since this is a common pattern, and I think there will be too many false positives otherwise.

  3. Stub files need to be excluded from this rule. Legacy code that was written before the introduction of the abc module often has classes that are de-facto abstract base classes, even though they don't make any use of the abc module. When writing type stubs for legacy code like this, you'll often want to add @abstractmethod to the stub methods of these classes, so that type checkers understand them to be abstract methods that need to be overridden in subclasses and emit appropriate errors. However, you won't want to lie about the metaclass in the stub file and pretend that the class has abc.ABCMeta as the metaclass, since this can cause spurious type-checker errors about metaclass conflicts in some situations. Adding @abstractmethod to a method on a class that does not have ABCMeta as the metaclass is fine in a stub file, since type checkers will still recognise the method as being abstract; the metaclass only makes a difference to runtime semantics, and stub files are never executed at runtime.

@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule accepted Ready for implementation labels Aug 14, 2024
@opk12
Copy link

opk12 commented Sep 5, 2024

in fact the @abc.abstractmethod docs says

Using this decorator requires that the class’s metaclass is ABCMeta or is derived from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants