Skip to content

How should subtyping work with respect to properties? #1364

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

Closed
AlexWaygood opened this issue Mar 6, 2023 · 12 comments
Closed

How should subtyping work with respect to properties? #1364

AlexWaygood opened this issue Mar 6, 2023 · 12 comments
Labels
topic: other Other topics not covered

Comments

@AlexWaygood
Copy link
Member

Consider the following snippet:

from abc import abstractmethod
from typing import Protocol

class HasX(Protocol):
    @property
    @abstractmethod
    def x(self) -> int: ...

class Nominal(HasX):
    @property
    def x(self) -> int:
        raise AttributeError

class Structural:
    @property
    def x(self) -> int:
        raise AttributeError

def needs_obj_with_x(obj: HasX) -> None:
    print(obj.x)

needs_obj_with_x(Nominal())
needs_obj_with_x(Structural())

This snippet will fail at runtime, as accessing the x attribute on either an instance of Nominal or an instance of Structural will result in AttributeError being raised. However, neither mypy nor pyright sees any issue with this code, which surprises me. I would have expected a type checker to complain about the Nominal.x property and the Structural.x property, since they are both annotated as returning int yet neither of them will ever return an int. I would have expected a type checker to demand that the x property on both Nominal and Structural should either have at least one return statement (that returns an int), or should be explicitly decorated with @abstractmethod.

Is this a missing feature from mypy and pyright? Or is there a reason for this not to be implemented?

(I haven't checked the behaviour of other type checkers.)

@AlexWaygood AlexWaygood added the topic: other Other topics not covered label Mar 6, 2023
@erictraut
Copy link
Collaborator

This is working as designed. If a function is annotated to return int, that means that every code path that returns a value to the caller must provide a value that is compatible with int. Some code paths may raise an exception, but a raised exception is not considered a "return value" since it is not returned to the caller. In your example, there are no returns that violate the return type annotation, so these functions type check without any errors. This has nothing to do with properties or protocols. You'll see the same behavior in other typed languages like TypeScript.

You asserted that "This snippet will fail at runtime". I'm not sure what you mean by "fail". The code raises an exception, but that appears to be the intent of the code, so I don't consider it a failure. As you know, static type checkers don't verify exception logic or enforce exception constraints.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 6, 2023

On your example, there are no returns that violate the return type annotation, so these functions type check without any errors. This has nothing to do with properties or protocols.

Indeed, but there are also no return statements at all. Mypy will emit an [empty-body] error on this snippet due to the lack of a return statement. I find this to be very useful for maintaining type safety:

class Foo:
    def x(self) -> int: ...

I'm curious about why this should be considered any different, as there is no return statement here either; yet mypy emits no error:

class Foo:
    @property
    def x(self) -> int:
        raise AttributeError

(It doesn't appear as though pyright has an equivalent check to mypy's empty-body error code, though I could be wrong.)

You'll see the same behavior in other typed languages like TypeScript.

Good to know, thank you! I have little experience with other languages, which was part of the reason why I was curious if others had any insight here that I was missing, and why I posted this issue.

You asserted that "This snippet will fail at runtime". I'm not sure what you mean by "fail". The code raises an exception, but that appears to be the intent of the code, so I don't consider it a failure.

The needs_obj_with_x function has been designed so that type checkers will only allow an object obj to be passed into the function if an x attribute can be safely accessed from obj. That seems to me to be the whole point of structural subtyping.

I would argue it would be useful for a type checker to complain about missing return statements in Nominal.x and Structural.x. This could help alert a programmer to the fact that instances of Nominal and Structural don't have well-defined x properties, and that accessing the x attribute of instances of either won't be safe at runtime.

@hmc-cs-mdrissi
Copy link

hmc-cs-mdrissi commented Mar 6, 2023

My main issue from pure static typing view (ignoring runtime implications of checking) is inconsistency with NoReturn/normal methods. NoReturn roughly means either every code path is an exception or it runs forever. NoReturn is compatible with every time. When you have a function that’s callable[…, X] for any type X you can always use NoReturn. Languages that are much stricter about type systems similarly are happy to allow you to have a function that always errors have signature like return int.

This also treats properties as a special, but same effect happens with normal methods.

class HasBark(Protocol):
  def bark(self) -> str:
    …

class A:
  def bark(self) -> str:
    return NotImplementedError(“fail”)

X: HasBark = A

type checks fine for both pyright and mypy. I’d consider not implemented error closest equivalent for methods to an attribute error for attributes.

@AlexWaygood
Copy link
Member Author

I’d consider not implemented error closest equivalent for methods to an attribute error for attributes.

I agree with that -- though in my opinion, unconditionally raising NotImplementedError from a method without explicitly decorating it with @abstractmethod should probably be considered an antipattern in 2023. Personally, I'd be okay with type checkers complaining about methods that unconditionally raise NotImplementedError and aren't decorated with @abstractmethod, though it would probably lead to many false positives being emitted on legacy code.

@hmc-cs-mdrissi
Copy link

hmc-cs-mdrissi commented Mar 6, 2023

Should it be allowed (no type error) if method was marked abstract? In a more realistic example it’s likely the code would instead be closer,

def use_bark(obj: HasBark) -> None:
  obj.bark()
  …

def foo(obj: A):
  use_a(obj) # Relies on A instance matching HasBark.

Here it’s common usage for actual object passed to be subtype that would implement method.

This partly is similar to how type[T] and abstract types are treated in mypy and pyright differently by default. Pyright behavior of allow abstract types to match type[T] is preferable to me as I do write a fair bit of code that doesn’t care if type is directly instantiable.

Other aspect is these error types don’t show up in type signature at all so would be a property of function you can never see from a stub. I’m not sure these rules can be described in .pyi files besides just leaving method/attribute out which feels more confusing.

edit: Fixed code formatting.

@erictraut
Copy link
Collaborator

erictraut commented Mar 6, 2023

Mypy will emit an [empty-body] error on this snippet due to the lack of a return statement

I think mypy got that one wrong. Pyright interprets ... as a code placeholder — a precedent used in type stubs, protocols, and elsewhere. Return types should not be enforced in this case, IMO. If you replace ... with pass, then it should result in a type violation because that is effectively the same as return None. Mypy only recently started to enforce the return type annotation for functions with .... Previously, it didn't enforce return types for either ... or pass. Pyright enforces return types for pass but not ....

I think you're proposing that if a type checker can prove that exceptions are raised on all code paths, it should report a type violation if the return type annotation is not NoReturn. Is that right? I disagree. As I asserted above, I consider the current behavior to be correct. I'd be very reluctant to change the behavior here because I think it would be disruptive to existing typed code bases.

The needs_obj_with_x function has been designed so that type checkers will only allow an object obj to be passed into the function if an x attribute can be safely accessed from obj.

A property is a function, and developers can implement this function any way they see fit. If they decide to explicitly raise an exception along some (or even all) code paths, that's a valid implementation of a property. The type checker has done its job by verifying that such a property exists and that all returned values will conform to the specified return type.

If you want to enforce the convention in your code base that property implementations are not allowed to raise exceptions on some or all code paths, that sounds more like a linter rule, not the concern of a type checker.

@AlexWaygood
Copy link
Member Author

Mypy will emit an [empty-body] error on this snippet due to the lack of a return statement

I think mypy got that one wrong.

I'm a fan of mypy's behaviour here; I think we'll just have to agree to disagree on this point.

I think you're proposing that if a type checker can prove that exceptions are raised on all code paths, it should report a type violation if the return type annotation is not NoReturn. Is that right?

Yes, that is correct.

I disagree. As I asserted above, I consider the current behavior to be correct [...] The type checker has done its job by verifying that such a property exists and that all returned values will conform to the specified return type.

Understood -- and, to be clear, I don't consider it a bug that type checkers don't currently emit an error on the code in my initial post. More of a "missing feature" -- but one that I at least would find quite valuable.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 6, 2023

@hmc-cs-mdrissi:

Should it be allowed (no type error) if method was marked abstract?

In my opinion, yes, because as soon as the property/method has been marked as abstract, the type checker will start disallowing dangerous calls. Here's mypy's behaviour if we add @abstractmethod to Nominal.x and Structural.x:

from abc import abstractmethod
from typing import Protocol

class HasX(Protocol):
    @property
    @abstractmethod
    def x(self) -> int: ...

class Nominal(HasX):
    @property
    @abstractmethod
    def x(self) -> int:
        raise AttributeError

class Structural:
    @property
    @abstractmethod
    def x(self) -> int:
        raise AttributeError

def needs_obj_with_x(obj: HasX) -> None:
    print(obj.x)

obj1 = Nominal()  # error: Cannot instantiate abstract class "Nominal" with abstract attribute "x"  [abstract]
obj2 = Structural()  # error: Cannot instantiate abstract class "Structural" with abstract attribute "x"  [abstract]
needs_obj_with_x(obj1)
needs_obj_with_x(obj2)  

@carljm
Copy link
Member

carljm commented Mar 6, 2023

I think mypy got that one wrong. Pyright interprets ... as a code placeholder — a precedent used in type stubs, protocols, and elsewhere. Return types should not be enforced in this case, IMO. If you replace ... with pass, then it should result in a type violation because that is effectively the same as return None.

I don't think that ... should be handled differently from pass by a type checker -- there is no difference in the runtime behavior. The use of ... as "placeholder where body isn't needed" is merely a convention. Rather, there are certain contexts in which method bodies should be ignored by a type checker (protocols and type stubs, as mentioned), and a type checker should disable return type checking within those contexts only. (I would also probably find it acceptable for a type checker to error if a method body other than ... is defined in those contexts.) But IMO using ... as method body outside of those contexts, in a method annotated to return a type other than None, should be a type error.

@carljm
Copy link
Member

carljm commented Mar 6, 2023

A property is a function, and developers can implement this function any way they see fit. If they decide to explicitly raise an exception along some (or even all) code paths, that's a valid implementation of a property. The type checker has done its job by verifying that such a property exists and that all returned values will conform to the specified return type.

If you want to enforce the convention in your code base that property implementations are not allowed to raise exceptions on some or all code paths, that sounds more like a linter rule, not the concern of a type checker.

I agree with this. NoReturn is the bottom type: it is a subtype of all types. So in the same way that if B is a subclass of A, it is fine for a method annotated to return A to in fact return an instance of B, it is also fine for a method annotated to return A to never return anything. It may be an imprecise annotation (in the same way that annotating A and returning B is), but it's not an incorrect annotation.

The proposal in the OP would effectively require the use of the abstractmethod decorator on any method that is in spirit an abstract method (because the base implementation is not supposed to be used, and subclasses are supposed to override.) I think this is too opinionated and too backwards incompatible.

@hmc-cs-mdrissi
Copy link

Mypy's abstract error produces a lot of false positives in code that does more runtime introspection.

from typing import TypeVar
from abc import ABC, abstractmethod

T = TypeVar("T")


def print_type(x: type[T]) -> None:
    print(x.__name__)

class Foo(ABC):
    @abstractmethod
    def bar(self):
        ...

print_type(Foo)

is entirely safe at runtime, but mypy flags it with an error main.py:15: error: Only concrete class can be given where "Type[Foo]" is expected [type-abstract]. The assumption abstractness is not compatible with type produces mostly noisy errors for my codebase mainly due to using a good amount of runtime type reflection, so it's rule I fully disable. print_type here is a toy example, but more realistic one is I heavily use and maintain internal library similar to pydantic/cattrs that takes abstract types in a lot of places and relies on them for deriving serialization behavior.

I think I've covered my view in full. I consider abstract/notimplemented/attributeerror to be separate from whether method/attribute is defined. Errors are fully valid implementations and depending on usage may even be safe. Using errors to mean undefined also interact awkwardly with subtyping relations. If other type checkers were to introduce rule like this, I'd prefer the rule be separate error code to disable similar to how mypy rule had a long issue debate over abstract vs concrete type usage.

@AlexWaygood
Copy link
Member Author

It sounds like the consensus is pretty clearly against me here. Thanks everybody for giving their opinions, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: other Other topics not covered
Projects
None yet
Development

No branches or pull requests

4 participants