Skip to content

Refactor all member access to go through checkmember.py (meta issue) #7724

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

Open
ilevkivskyi opened this issue Oct 16, 2019 · 3 comments · May be fixed by #19180
Open

Refactor all member access to go through checkmember.py (meta issue) #7724

ilevkivskyi opened this issue Oct 16, 2019 · 3 comments · May be fixed by #19180
Labels
meta Issues tracking a broad area of work needs discussion priority-0-high refactoring Changing mypy's internals

Comments

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Oct 16, 2019

There are bunch of places where we duplicate some parts of code for special logic in member access (such as properties, class methods, descriptors, __getattr__(), etc):

  • The checkmember.py itself
  • Override checks for variables
  • Override checks for methods
  • Multiple inheritance checks
  • Checks for special methods via check_op() and has_member()
  • Protocol implementation check

Here are some issues that will be solved (or significantly simplified) by refactoring this to go through the same place (probably checkmember.py):

Also I think having member access cleaned-up, centralized, and documented will open the way to finally solving #708 (currently the oldest high priority issue), and to having general support for descriptor protocol (for historical reasons, several things like properties are supported via some special-casing).

ilevkivskyi added a commit that referenced this issue Nov 27, 2019
Fixes #8020

There is a bunch of code/logic duplication around `bind_self()`, mostly because of #7724. This PR makes all three main code paths consistently follow the same structure:
1. `freshen_function_type_vars()`
2. `bind_self()`
3. `expand_type_by_instance(..., map_instance_to_supertype())` (a.k.a `map_type_from_supertype()`)

I briefly scrolled through other code paths, and it looks like this was last major/obvious inconsistency (although code around `__getattr__`/`__setattr__`/`__get__`/`__set__` looks a bit suspicious).
ilevkivskyi added a commit that referenced this issue Nov 29, 2019
This is a follow up for #8021, that applies the fix also to non-callable types. Currently non-callable types are still wrong:
```python
R = TypeVar('R')
T = TypeVar('T')
# Can be any decorator that makes type non-callable.
def classproperty(f: Callable[..., R]) -> R: ...

class C(Generic[T]):
    @classproperty
    def test(self) -> T: ...

x: C[int]
y: Type[C[int]]
reveal_type(x.test)  # Revealed type is 'int', OK
reveal_type(y.test)  # Revealed type is 'T' ???
```

So, #7724 strikes again. It turns out there is not only duplicated logic for attribute kinds (decorators vs normal methods), but also for callable vs non-callable types. In latter case we still need to expand the type (like in other places, e.g., `analyze_var`).
@sobolevn
Copy link
Member

sobolevn commented Jun 23, 2021

Plugin management is also really important here, because right now protocols do not support objects with custom get_attribute_hook / get_method_sig_hook items. Quick example:

# ex.py
class A(object):
    attr: str  # is set via plugin

    def method(self) -> str:  # is set via plugin
        ...

a = A()
reveal_type(a.attr)
# Revealed type is "builtins.float"
reveal_type(a.method())
# Revealed type is "builtins.int"

Plugin:

from mypy.plugin import Plugin

class MyPlugin(Plugin):
    def get_attribute_hook(self, fullname):
        if fullname == 'ex.A.attr':
            def test(ctx):
                return ctx.api.named_type('builtins.float')
            return test

    def get_method_signature_hook(self, fullname: str):
        if fullname == 'ex.A.method':
            def test(ctx):
                return ctx.default_signature.copy_modified(
                    ret_type=ctx.api.named_type('builtins.int'),
                )
            return test

def plugin(version):
    return MyPlugin

At the moment, everything works as expected.
Now, let's define a protocol and check if it works.

Protocol (problematic part):

from typing_extensions import Protocol

class Some(Protocol):
    attr: float

    def method(self) -> int:
        ...

def accepts_protocol(some: Some) -> int:
    return some.method()

accepts_protocol(a)
# ex.py:26: error: Argument 1 to "accepts_protocol" has incompatible type "A"; expected "Some"
# ex.py:26: note: Following member(s) of "A" have conflicts:
# ex.py:26: note:     attr: expected "float", got "str"
# ex.py:26: note:     Expected:
# ex.py:26: note:         def method(self) -> int
# ex.py:26: note:     Got:
# ex.py:26: note:         def method(self) -> str

find_member does not call get_attribute_hook because it does not even have chk instance.

@sobolevn
Copy link
Member

At the moment I am trying to refactor find_member to be almost an alias to analyze_member_access. It is not so-commonly used. grep:

» ag find_member
mypy/checkmember.py
139:    # TODO: This and following functions share some logic with subtypes.find_member;

mypy/join.py
15:    is_protocol_implementation, find_member
592:        return find_member('__call__', t, t, is_operator=True)

mypy/messages.py
38:    is_subtype, find_member, get_member_flags,
599:            call = find_member('__call__', original_caller_type, original_caller_type,
1922:        if not find_member(member, left, left):
1936:        supertype = find_member(member, right, left)
1938:        subtype = find_member(member, left, left)
1957:        if find_member(member, left, left):

mypy/constraints.py
324:                    call = mypy.subtypes.find_member('__call__', template, actual,
435:            inst = mypy.subtypes.find_member(member, instance, subtype)
436:            temp = mypy.subtypes.find_member(member, template, subtype)
481:            call = mypy.subtypes.find_member('__call__', self.actual, self.actual,

mypy/checker.py
61:    unify_generic_callable, find_member
4678:            call = find_member('__call__', subtype, subtype, is_operator=True)
4683:                call = find_member('__call__', supertype, subtype, is_operator=True)
4963:        iter_type = get_proper_type(find_member('__iter__', instance, instance, is_operator=True))

mypy/subtypes.py
292:            call = find_member('__call__', left, left, is_operator=True)
321:                call = find_member('__call__', right, left, is_operator=True)
408:                call = find_member('__call__', right, left, is_operator=True)
560:            supertype = get_proper_type(find_member(member, right, left))
562:            subtype = get_proper_type(find_member(member, left, left))
608:def find_member(name: str,
736:        typ = get_proper_type(find_member(member, instance, instance))
1299:            call = find_member('__call__', left, left, is_operator=True)

All things in checker.py are easy, I will just change find_member to analyze_member_access.

But I have several questions:

  1. What to do with functions that use find_member inside? Like ones in subtypes.py and constrains.py? They will require a lot of context to work properly: chk is the most complex one to pass through
  2. How will it affect performance if is_subtype (which is widely used) will call get_attribute_hook?
  3. Should find_memebr and analyze_member_access be different? Are there any details to keep in mind?

@97littleleaf11 97littleleaf11 added the meta Issues tracking a broad area of work label Dec 1, 2021
ilevkivskyi added a commit that referenced this issue Nov 15, 2022
Ref #12840 
Fixes #11871
Fixes #14089

This is an alternative implementation to two existing PRs:
#11666,
#13133. This PR treats `typing.Self`
as pure syntactic sugar, and transforms it into a type variable early
during semantic analyzis.

This way we can re-use all the existing machinery and handled edge cases
for self-types. The only new thing is self-type for _attributes_ (as
proposed in the PEP). This required handling in several places, since
attribute access is duplicated in several places (see #7724), plus
special forms (like NamedTuples and TypedDicts) and dataclasses plugin
require additional care, since they use attribute annotations in special
ways.

I don't copy all the existing tests for "old style" self-types, but only
some common use cases, possible error conditions, and relevant new edge
cases, such as e.g. special forms mentioned above, and implicit type
variable binding for callable types.
@ilevkivskyi
Copy link
Member Author

Another problem that came to my mind while thinking about this is how to handle deferrals? If e.g. is_subtype() ultimately ends up in protocol check w.r.t. to a class with not ready attribute (e.g. a tricky decorator) then we can't really return neither False nor True since both can cause spurious errors. The only way I see is to raise an error, but then each call to is_subtype() should expect this, making everything insanely complex.

ilevkivskyi added a commit that referenced this issue Mar 31, 2025
Fixes #5803
Fixes #18695
Fixes #17513
Fixes #13194
Fixes #12126

This is the first PR towards #7724,
some notes:
* I add a new generic `suppress_errors` flag to `MemberContext` mostly
as a performance optimization, but it should also be handy in the
following PRs
* I noticed some inconsistencies with how we handle variable inference
(e.g. we don't infer type at all if rvalue type is not compatible with
superclass type). After all I decided to remove some of them, as it
makes implementation of this PR simpler.
* I added a bunch of TODOs, most of those will be addressed in following
PRs.
* A while ago we agreed that an explicit `Callable[...]` annotation in
class body means how the type looks on an _instance_, but the override
check used to handle this inconsistently (I add few `reveal_type()`s to
tests to illustrate this).
ilevkivskyi added a commit that referenced this issue Apr 2, 2025
This is a second "large" PR towards
#7724. Here I actually expect a
smaller fallout than for variables, since methods are usually less
tricky, but let's see.
ilevkivskyi added a commit that referenced this issue Apr 4, 2025
This is the third "major" PR towards
#7724

This one is mostly straightforward. I tried to preserve the existing
logic about mutable overrides (to minimize fallout), as currently we
e.g. don't use the covariant mutable override error code here. In future
we can separately "synchronize" mutable override logic across variable
override, method override, and multiple inheritance code paths (as
currently all three are subtly different).
ilevkivskyi added a commit that referenced this issue May 30, 2025
Fixes #18024
Fixes #18706
Fixes #17734
Fixes #15097
Fixes #14814
Fixes #14806
Fixes #14259
Fixes #13041
Fixes #11993
Fixes #9585
Fixes #9266
Fixes #9202
Fixes #5481

This is a fourth "major" PR toward
#7724. This is one is
watershed/crux of the whole series (but to set correct expectations,
there are almost a dozen smaller follow-up/clean-up PRs in the
pipeline).

The core of the idea is to set current type-checker as part of the
global state. There are however some details:
* There are cases where we call `is_subtype()` before type-checking. For
now, I fall back to old logic in this cases. In follow up PRs we may
switch to using type-checker instances before type checking phase (this
requires some care).
* This increases typeops import cycle by a few modules, but
unfortunately this is inevitable.
* This PR increases potential for infinite recursion in protocols. To
mitigate I add: one legitimate fix for `__call__`, and one temporary
hack for `freshen_all_functions_type_vars` (to reduce performance
impact).
* Finally I change semantics for method access on class objects to match
the one in old `find_member()`. Now we will expand type by instance, so
we have something like this:
  ```python
  class B(Generic[T]):
      def foo(self, x: T) -> T: ...
  class C(B[str]): ...
  reveal_type(C.foo)  # def (self: B[str], x: str) -> str
  ```
FWIW, I am not even 100% sure this is correct, it seems to me we _may_
keep the method generic. But in any case what we do currently is
definitely wrong (we infer a _non-generic_ `def (x: T) -> T`).

---------

Co-authored-by: hauntsaninja <hauntsaninja@gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
lsrafael13 added a commit to lsrafael13/mypy that referenced this issue May 30, 2025
Implements a check to disallow assignments to self.__class__, as these can cause unsafe runtime behavior. Includes tests for __class__ and related dunder attributes.

Fixes python#7724
@lsrafael13 lsrafael13 linked a pull request May 30, 2025 that will close this issue
cdce8p pushed a commit to cdce8p/mypy that referenced this issue May 31, 2025
Fixes python#18024
Fixes python#18706
Fixes python#17734
Fixes python#15097
Fixes python#14814
Fixes python#14806
Fixes python#14259
Fixes python#13041
Fixes python#11993
Fixes python#9585
Fixes python#9266
Fixes python#9202
Fixes python#5481

This is a fourth "major" PR toward
python#7724. This is one is
watershed/crux of the whole series (but to set correct expectations,
there are almost a dozen smaller follow-up/clean-up PRs in the
pipeline).

The core of the idea is to set current type-checker as part of the
global state. There are however some details:
* There are cases where we call `is_subtype()` before type-checking. For
now, I fall back to old logic in this cases. In follow up PRs we may
switch to using type-checker instances before type checking phase (this
requires some care).
* This increases typeops import cycle by a few modules, but
unfortunately this is inevitable.
* This PR increases potential for infinite recursion in protocols. To
mitigate I add: one legitimate fix for `__call__`, and one temporary
hack for `freshen_all_functions_type_vars` (to reduce performance
impact).
* Finally I change semantics for method access on class objects to match
the one in old `find_member()`. Now we will expand type by instance, so
we have something like this:
  ```python
  class B(Generic[T]):
      def foo(self, x: T) -> T: ...
  class C(B[str]): ...
  reveal_type(C.foo)  # def (self: B[str], x: str) -> str
  ```
FWIW, I am not even 100% sure this is correct, it seems to me we _may_
keep the method generic. But in any case what we do currently is
definitely wrong (we infer a _non-generic_ `def (x: T) -> T`).

---------

Co-authored-by: hauntsaninja <hauntsaninja@gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
lsrafael13 added a commit to lsrafael13/mypy that referenced this issue Jun 1, 2025
…Visitor (Fixes python#7724)

Replaces previous incorrect call to self.chk.fail() with self.errors.report(), resolving AttributeError.
Ensures assignment to __class__ is properly reported as an error during instance attribute assignments.
lsrafael13 added a commit to lsrafael13/mypy that referenced this issue Jun 2, 2025
Added a check inside TypeChecker.visit_assignment_stmt to disallow assignments to '__class__', as such assignments are unsafe and may lead to unpredictable behavior.

The new check raises an error using self.fail when '__class__' is assigned, matching the style used in other TypeChecker checks.

This change addresses issue python#7724.
lsrafael13 added a commit to lsrafael13/mypy that referenced this issue Jun 2, 2025
Implemented the check inside TypeChecker.visit_assignment_stmt to disallow assignments to '__class__', as such assignments are unsafe.

The check correctly uses self.fail() following mypy's error reporting conventions, and is located after initial assignment processing to align with the design of TypeChecker.

Previous redundant logic inside VarAssignVisitor.visit_assignment_stmt has been removed.

This change addresses issue python#7724.
ilevkivskyi added a commit that referenced this issue Jun 9, 2025
Fixes #5136
Fixes #5491

This is a fifth "major" PR toward
#7724. Although it would be
impractical to move all the operator special-casing to `checkmember.py`,
this does two things:
* Removes known inconsistencies in operator handling
* Adds a much more complete `has_operator()` helper that can be a
starting point for future performance optimizations
ilevkivskyi added a commit that referenced this issue Jun 9, 2025
Fixes #3832
Fixes #5723
Fixes #17174
Improves #7217

This is a sixth "major" PR toward
#7724. Previously access to
"static" attributes (like type aliases, class objects) was duplicated in
four places:
* In `analyze_ref_expr()`
* In `determine_type_of_member()` (for modules as subtypes of protocols)
* In instance attribute access logic
* In class attribute logic

Most of these were somewhat incomplete and/or inconsistent, this PR
unifies all four (there is still tiny duplication because I decided to
limit the number of deferrals, i.e. preserve the existing logic in this
respect). Some notable things that are not pure refactoring:
* Previously we disabled access to type variables as class attributes.
This was inconsistent with plain references and instance attributes that
just return `Instance("typing.TypeVar")`.
* Instance access plugins were only applied on `TypeInfo`s and
`TypeAlias`es, now they are applied always.
* Previously arguments kinds were sometimes not correct for TypedDict
class objects with non-required keys.
* I tweaked `TypeOfAny` in couple places to be more logical.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues tracking a broad area of work needs discussion priority-0-high refactoring Changing mypy's internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants