Skip to content

[red-knot] Check assignability of bound methods to callables#17430

Merged
dhruvmanila merged 1 commit intomainfrom
dhruv/bound-method-assignability
Apr 16, 2025
Merged

[red-knot] Check assignability of bound methods to callables#17430
dhruvmanila merged 1 commit intomainfrom
dhruv/bound-method-assignability

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Apr 16, 2025

Summary

This is similar to #17095, it adds assignability check for bound methods to callables.

Test Plan

Add test cases to for assignability; specifically it uses gradual types because otherwise it would just delegate to is_subtype_of.

@dhruvmanila dhruvmanila added the ty Multi-file analysis & type inference label Apr 16, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2025

mypy_primer results

Changes were detected when running on open source projects 
pyinstrument (https://github.com/joerick/pyinstrument)
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/pyinstrument/test/test_stack_sampler.py:107:9: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method StackSampler.subscribe(target: Unknown | @Todo(Inference of subscript on special form), *, desired_interval: int | float, use_timing_thread: bool | None = None, use_async_context: bool) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/pyinstrument/test/test_stack_sampler.py:110:9: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method StackSampler.subscribe(target: Unknown | @Todo(Inference of subscript on special form), *, desired_interval: int | float, use_timing_thread: bool | None = None, use_async_context: bool) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/pyinstrument/test/test_stack_sampler.py:125:19: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method StackSampler.unsubscribe(target: Unknown | @Todo(Inference of subscript on special form)) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/pyinstrument/test/test_stack_sampler.py:126:19: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method StackSampler.unsubscribe(target: Unknown | @Todo(Inference of subscript on special form)) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/pyinstrument/test/test_profiler.py:136:17: Argument to this function is incorrect: Expected `(...) -> Unknown | @Todo(Support for `typing.TypeVar` instances in type expressions)`, found `bound method ClassWithMethods.long_method() -> Unknown`
- error[lint:invalid-assignment] /tmp/mypy_primer/projects/pyinstrument/test/fake_time_util.py:28:5: Object of type `@Todo(map_with_boundness: intersections with negative contributions) | (bound method FakeClock.get_time() -> Unknown)` is not assignable to attribute `timer_func` of type `(() -> int | float) | None`
- Found 275 diagnostics
+ Found 269 diagnostics

rich (https://github.com/Textualize/rich)
- error[lint:invalid-assignment] /tmp/mypy_primer/projects/rich/rich/cells.py:30:1: Object of type `(bound method frozenset.issuperset(s: @Todo(generics), /) -> bool) | @Todo(instance attribute on class with dynamic base)` is not assignable to `(str, /) -> bool`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/rich/rich/text.py:737:29: Argument to this function is incorrect: Expected `(...) -> Unknown | @Todo(Support for `typing.TypeVar` instances in type expressions)`, found `bound method Console.get_style(name: str | Style, *, default: Style | str | None = None) -> Style`
- Found 560 diagnostics
+ Found 558 diagnostics

werkzeug (https://github.com/pallets/werkzeug)
- error[lint:invalid-assignment] /tmp/mypy_primer/projects/werkzeug/src/werkzeug/formparser.py:376:21: Object of type `bound method list.append(object: Unknown | @Todo(Support for `typing.TypeVar` instances in type expressions), /) -> None` is not assignable to `(bytes, /) -> Any`
- error[lint:invalid-assignment] /tmp/mypy_primer/projects/werkzeug/src/werkzeug/test.py:98:9: Object of type `(bound method BytesIO.write(buffer: @Todo(Support for `typing.TypeAlias`), /) -> int) | @Todo(instance attribute on class with dynamic base)` is not assignable to `(bytes, /) -> int`
+ warning[lint:unused-ignore-comment] /tmp/mypy_primer/projects/werkzeug/src/werkzeug/local.py:370:50: Unused blanket `type: ignore` directive
- Found 687 diagnostics
+ Found 686 diagnostics

scrapy (https://github.com/scrapy/scrapy)
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_contracts.py:570:49: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method DemoSpider.returns_request(response) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_contracts.py:573:38: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method DemoSpider.returns_request(response) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_contracts.py:584:50: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method DemoSpider.returns_request(response) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_contracts.py:587:38: Argument to this function is incorrect: Expected `(...) -> Unknown`, found `bound method DemoSpider.returns_request(response) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_utils_python.py:228:30: Argument to this function is incorrect: Expected `(...) -> Any`, found `bound method A.method(a, b, c) -> Unknown`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/tests/test_utils_python.py:235:30: Argument to this function is incorrect: Expected `(...) -> Any`, found `bound method Literal[" "].join(**kwargs: @Todo(todo signature **kwargs)) -> @Todo(return type of overloaded function)`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/scrapy/scrapy/core/scraper.py:207:57: Argument to this function is incorrect: Expected `(...) -> Any`, found `Unknown & ~AlwaysFalsy | @Todo(Inference of subscript on special form) & ~AlwaysFalsy | Unknown & @Todo(Inference of subscript on special form) & ~AlwaysFalsy | (bound method Spider._parse(response: Response, **kwargs: Any) -> Any)`
- Found 1583 diagnostics
+ Found 1576 diagnostics

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 16, 2025

(Checking the ecosystem diff...)

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 16, 2025

They look mostly correct as most of them expected a gradual callable type and received bound methods. Earlier, this would be delegated to is_subtype_of which would see that it's not a fully static type and thus return an early false.

There's this additional warning about unused suppression comment which I want to look closely at: https://github.com/pallets/werkzeug/blob/7868bef5d978093a8baa0784464ebe5d775ae92a/src/werkzeug/local.py#L370.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 16, 2025 •
edited
Loading

There's this additional warning about unused suppression comment which I want to look closely at: pallets/werkzeug@7868bef/src/werkzeug/local.py#L370.

This also seems correct because otherwise (without this PR) we get the following diagnostic:

red-knot: Return type does not match returned value: Expected `(...) -> Any`, found `bound method Any.i_op(other: Any) -> @Todo(generics)` [lint:invalid-return-type]

which seems like a clear false positive that's now is fixed and thus does not require a type: ignore comment but I'd prefer if @sharkdp would take a second look.

It seems like another win for a thorough descriptor protocol implementation from @sharkdp!

carljm
carljm approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

sharkdp
sharkdp approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

As discussed on Discord, the ecosystem change looks good to me — thank you!

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 16, 2025

As discussed on Discord, the ecosystem change looks good to me — thank you!

For context:

So what i_op.__get__(obj, type(obj)) does is that it takes this i_op function, and uses the fact that functions are descriptors, calls the __get__ method on the function object with an instance argument of obj. And what the function descriptor does in that case is create a bound-method object

[...] it binds to Any

@dhruvmanila dhruvmanila merged commit 5350288 into main Apr 16, 2025
23 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/bound-method-assignability branch April 16, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@sharkdp sharkdp sharkdp approved these changes

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood

@dcreager dcreager Awaiting requested review from dcreager

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dhruvmanila @carljm @sharkdp