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

spec: clarify the mro linearization of Any #1672

Closed
wants to merge 8 commits into from

Conversation

mikeshardmind
Copy link

@mikeshardmind mikeshardmind commented Mar 31, 2024

See discussion here: https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981

  • This matches the current behavior of mypy and pyright, which support subclassing of Any.

To summarize the rationales possible for this in terms of type theory, this would either be

  • The lower known bound of the possible type and type checkers only allowing the lower known bound
  • Type checkers picking a definition of compatibility of Any based on LSP substitution
  • Or type checkers decide to prefer known information in the face of unknown potential diamond patterns treating them as rare.

Given the relatively low priority on this outside of it being one of the only unresolved prerequisite questions for the intersection draft, I don't think we need to set in stone a reason at this point as long as the behavior intended is clear, and I have not included a reason in this changeset, preferring to leave that open to any future work in better unifying the type system's specification with theory.

If the typing council would prefer a reason to be chosen, I would advocate for the first of the three as the most flexible and forward-looking, see specific comments https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/3 and https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/7 for discussion of this approach.

See discussion here: https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981

- This matches the current behavior of mypy and pyright, which support subclassing of Any.

To summarize the rationales possible for this in terms of type theory, this would either be 

- The lower known bound of the type and type checkers only allowing the lower known bound

- Type checkers picking a definition of compatibility of Any based on LSP substitution

- Or type checkers decide to prefer known information in the face of unknown potential diamond patterns treating them as rare.

Given the relatively low priority on this outside of it being one of the only unresolved prerequisite questions for the intersection draft, I don't think we need to set in stone a reason at this point in time as long as the behavior intended is clear and agreed upon.

If you'd prefer a reason to be chosen, the lower bound is most flexible into the future, see https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/3 and https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/7 for discussion of this approach.
@erictraut
Copy link
Collaborator

This matches the current behavior of mypy and pyright, which support subclassing of Any.

I'm fine with the proposed wording, and I think it's a reasonable approach, but pyright doesn't currently match this behavior. I don't think mypy does either. I'm not sure about pyre or pytype. Could you write some test cases to verify/test the proposed behavior? That will give us a sense for the investment that will need to be made to make all the type checkers compliant with this rule. We can also use your test cases in the conformance suite.

@mikeshardmind
Copy link
Author

mikeshardmind commented Mar 31, 2024

but pyright doesn't currently match this behavior. I don't think mypy does either.

Pyright is currently matching the behavior, unless you're aware of a case in which it doesn't. I'm not sure if the actual internals match that behavior, but the outcome is correct for that wording:

Code sample in pyright playground

from typing import Any, reveal_type

class Base:
    foo: str

class AnyFirst(Any, Base):
    ...

class AnyLast(Base, Any):
    ...


reveal_type(AnyFirst().foo)
reveal_type(AnyLast().foo)
reveal_type(AnyFirst().bar)
reveal_type(AnyLast().bar)

if it wasn't following this behavior, the first reveal type would show Any (it shows str)

mypy as well

pyre doesn't support Any as a base class in typed code, either explicitly or implicitly through unknown types

I'm not sure on pytype's current state of this feature.

Could you write some test cases to verify/test the proposed behavior? That will give us a sense for the investment that will need to be made to make all the type checkers compliant with this rule. We can also use your test cases in the conformance suite.

Sure.

@mikeshardmind
Copy link
Author

Output of the

Output changed for specialtypes_any when running pytype
Old output:


New output:
File "specialtypes_any.py", line 112, in <module>: Any [assert-type]
File "specialtypes_any.py", line 114, in <module>: Any [assert-type]

pytype has differing behavior from the wording here in the case of Any as a base class obscuring a known definition.

, I'm unsure if I've missed something about how the conformance suite is intended to work, but the conformance suite did not fail pytype for this:

Relevent generated toml (pytype/specialtypes_any.toml)

conformant = "Pass"
output = """
File "specialtypes_any.py", line 112, in <module>: Any [assert-type]
File "specialtypes_any.py", line 114, in <module>: Any [assert-type]
"""

No other changes upon running the conformance suite pyright and mypy pass the new tests, pyre does not have support for subclassing Any, so not expected to work.

@erictraut
Copy link
Collaborator

It turns out that in your code sample above, pyright does follow the proposed spec. However, that's just by chance. If you change foo: str to foo = "", it no longer follows the spec.

It will be relatively straightforward to change pyright's logic to follow the proposed spec, but it will require some work.

@Daverball
Copy link
Contributor

Daverball commented Apr 1, 2024

@mikeshardmind Another compliance test case to consider is __getattr__. Currently mypy and pyright diverge there:

Mypy Playground | Pyright Playground

Interestingly, despite disagreeing about precedence here, both type checkers seem to agree that __getattr__ should not prevent structurally matching a Protocol as long as one of the base classes is unknown and there are no explicit contradictory members present.

So pyright is a little bit more consistent and awarding proper attributes the highest precedence, regardless of context and whether or not they're unknown, whereas mypy will only give proper attributes a higher precedence in structural type matching, presumably to avoid false positives in duck typing, but preferring known information in all other cases.


Upon closer inspection, the structural matching behavior is actually independent of __getattr__, both mypy and pyright seem to accept any structural match with a class deriving from Any regardless of the inheritance order and whether there are contradictory attributes, this may be due to nominal subtyping rules however, since Any is a subclass of any type. But I'm not sure how I feel about nominal subtyping taking precedence over structural compatibility in this case.

I have updated the examples in the playgrounds respectively to reflect that. So we can probably at least leave specifying that part up to a later date, but we should still consider what a plain attribute access returns in the presence of __getattr__ while inheriting from Any.

@mikeshardmind
Copy link
Author

Thank you both for those additional cases, I've added 2 more classes to be checked with __getattr__ and checks for classvars and a special dunder on each of the now 4, properly revealing that no existing type checker would be fully compliant with this wording.

docs/spec/special-types.rst Outdated Show resolved Hide resolved
assert_type(af.non_exist_attr, Any)
assert_type(af.classvar1, str)
assert_type(AnyFirst.classvar1, str)
assert_type(iter(af()), Iterator[str])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this -- why are we calling af() here? Given that there's no explicit __call__ defined above, it seems like perhaps this should not be a type error (since we assume Any is implicitly callable), but should be typed as Any?

It looks like this is intended to check the __iter__ method above, in which case I think these parens should be eliminated

Suggested change
assert_type(iter(af()), Iterator[str])
assert_type(iter(af), Iterator[str])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does raise the question of whether it's useful to also specify here the implicit __call__ on classes with Any in MRO?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the implicit __call__ should be documented or not, but the iter mistake was fixed.

assert_type(al.non_exist_attr, Any)
assert_type(al.classvar1, str)
assert_type(AnyLast.classvar1, str)
assert_type(iter(al()), Iterator[str])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_type(iter(al()), Iterator[str])
assert_type(iter(al), Iterator[str])

assert_type(af_getattr.triggers_getattr, int)
assert_type(af_getattr.classvar1, str)
assert_type(AnyFirstGetAttr.classvar1, str)
assert_type(iter(af_getattr()), Iterator[str])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_type(iter(af_getattr()), Iterator[str])
assert_type(iter(af_getattr), Iterator[str])

assert_type(al_getattr.triggers_getattr, int)
assert_type(al_getattr.classvar1, str)
assert_type(AnySubFirst.classvar1, str)
assert_type(iter(al_getattr()), Iterator[str])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_type(iter(al_getattr()), Iterator[str])
assert_type(iter(al_getattr), Iterator[str])

assert_type(full_mro_checked.triggers_getattr, int)
assert_type(full_mro_checked.classvar1, str)
assert_type(AnyLastGetAttr.classvar1, str)
assert_type(iter(full_mro_checked()), Iterator[str])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_type(iter(full_mro_checked()), Iterator[str])
assert_type(iter(full_mro_checked), Iterator[str])

conformance/tests/specialtypes_any.py Outdated Show resolved Hide resolved
conformance/tests/specialtypes_any.py Show resolved Hide resolved
Copy link
Collaborator

@erictraut erictraut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good. One small problem with assumptions about __getattr__.

conformance/tests/specialtypes_any.py Outdated Show resolved Hide resolved
assert_type(af_getattr.method1(), str)
assert_type(af_getattr.method2(), str)
assert_type(af_getattr.attr1, str)
assert_type(af_getattr.triggers_getattr, int)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. The __getattr__ method is called only if the attribute isn't present in the object's dictionary. Since Any is in the MRO (even as the last item), then all attributes are effectively "found in the object's dictionary". So this will evaluate to Any.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, one of the bases provided provides a __getattr__ definition

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But __getattr__ isn't applicable here. This method is invoked only if the attribute is not present. But in this case, it is "present" because all attributes are present on an Any base class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At runtime, this is definitely invoked, runtime doesn't see Any and do something special for __getattr__, I've modeled this on accurately typing runtime for a known implementation of __getattr__

Copy link
Member

@carljm carljm Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "modeling this on runtime" with the actual runtime Any type is meaningful here. The non_existent_attr examples don't "model runtime" either -- at runtime with the actual runtime Any type in the MRO, those examples raise AttributeError.

The point of having Any in the MRO is that for typing purposes it might represent any concrete type, which might have any actual attribute. If Any in this case represented a concrete type that provides the triggers_getattr attribute, then __getattr__ would not be invoked. So for the same reason that non_existent_attr works and produces Any in these examples, triggers_getattr should work and also produce Any, because checking the types in the MRO for the triggers_getattr attribute occurs before consulting __getattr__, and Any must be considered to provide all attributes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about going the other way on this? You could specify that a class that inherits from Any must only have a __getattr__ definition if the definition is typed to return Any

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone outline or link to the motivating case with mocks that would be broken if the spec just gave Any on these getattr cases?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be resolved that way, taking this over to discourse.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has nothing to do with whether the unknown class implements __getattr__. It has to do with whether the unknown class has an attribute called triggers_getattr. We must assume it does, in which case the presence of a __getattr__ method is irrelevant.

Consider the following:

class A:
    foo = "foo"

class B:
    def __getattr__(self, name: str) -> int:
        print("__getattr__")
        return 1

class C(A, B):
    pass

c = C()
reveal_type(c.foo) # str
print(c.foo) # "foo"

Note that B.__getattr__ is never invoked at runtime in this case, and the type checker accordingly never simulates a call to __getattr__ when evaluating the type of expression c.foo.

Now, we replace A with Any. Here again, the type checker should assume that __getattr__ will not be invoked.

class C(Any, B):
    pass

c = C()
reveal_type(c.foo) # Any

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(github isn't loading reviews in realtime right now for anyone viewing this after the fact)

conformance/tests/specialtypes_any.py Show resolved Hide resolved
conformance/tests/specialtypes_any.py Outdated Show resolved Hide resolved
@mikeshardmind
Copy link
Author

Withdrawing this. May or may not revisit this with another approach, see comments on discourse from https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/19 to https://discuss.python.org/t/take-2-rules-for-subclassing-any/47981/23

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

Successfully merging this pull request may close these issues.

6 participants