Skip to content

Conversation

@AlexWaygood
Copy link
Member

Summary

This PR is best reviewed with the "hide whitespace changes" option enabled on GitHub.

This PR fixes a bug where ty would consider all types as having an __mro__ attribute, when in reality this is only true for instances of type. The bug is fixed by lifting our special-casing for the __mro__ attribute out of Type::member() and into TypeInferenceBuilder::infer_attribute_load(). This means that we now only infer precise types for the __mro__ attribute for literal attribute accesses, whereas we previously also inferred this precise type for implicit attribute accesses. I think that's okay, however: we only really need the precise inference of __mro__ for our internal tests. Calling iter_mro() at a higher level (rather than in the guts of Type::member()) also appears to have the effect that we properly propagate the specialisation of a generic class down through the entire MRO when inferring the type of someclass.__mro__, resulting in more intuitive types being revealed in our mro.md test.

The type displayed in the tooltip for some autocomplete suggestions also becomes less precise, but that also seems fine. I don't think users of autocompletion need a type that's so precise there; the less precise type seems like it's probably less noisy.

Fixes astral-sh/ty#986

Test Plan

Existing mdtests updated and TODOs removed

@AlexWaygood AlexWaygood added bug Something isn't working ty Multi-file analysis & type inference labels Oct 20, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 20, 2025

CodSpeed Performance Report

Merging #20995 will improve performances by 6.23%

Comparing alex/mro-attr (7c5ad22) with main (3c7f56f)

Summary

⚡ 1 improvement
✅ 20 untouched
⏩ 30 skipped1

Benchmarks breakdown

Mode Benchmark BASE HEAD Change
Simulation ty_micro[many_enum_members] 90.2 ms 84.9 ms +6.23%

Footnotes

  1. 30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2025

mypy_primer results

Changes were detected when running on open source projects
beartype (https://github.com/beartype/beartype)
+ beartype/_util/hint/pep/proposal/pep484/pep484generic.py:418:18: warning[possibly-missing-attribute] Attribute `__mro__` may be missing on object of type `None | type`
- Found 514 diagnostics
+ Found 515 diagnostics

sympy (https://github.com/sympy/sympy)
+ sympy/core/function.py:186:39: error[invalid-argument-type] Argument to bound method `__init__` is incorrect: Expected `Iterable[Unknown]`, found `~AlwaysFalsy`
+ sympy/core/function.py:188:29: error[invalid-argument-type] Argument to function `as_int` is incorrect: Expected `SupportsIndex`, found `~None`
- Found 13696 diagnostics
+ Found 13698 diagnostics
No memory usage changes detected ✅

@sharkdp
Copy link
Contributor

sharkdp commented Oct 20, 2025

This PR fixes a bug where ty would consider all types as having an __mro__ attribute, when in reality this is only true for instances of type. The bug is fixed by lifting our special-casing for the __mro__ attribute out of Type::member() and into TypeInferenceBuilder::infer_attribute_load(). This means that we now only infer precise types for the __mro__ attribute for literal attribute accesses, whereas we previously also inferred this precise type for implicit attribute accesses.

It seems like this could have been fixed by moving that special handling to class_member instead?

Special-casing things in type inference (instead of core Type operations) is something I would wish we would do less of, not more. It's probably not very relevant for .__mro__, but in general, there are a lot of disadvantages to specializing things in type inference. The whole attribute access "machinery" with the descriptor protocol etc. will be overwritten, unions/intersections won't be handled, etc.

The type displayed in the tooltip for some autocomplete suggestions also becomes less precise, but that also seems fine. I don't think users of autocompletion need a type that's so precise there

They probably don't need it, but I found it kind of nice?

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2025

Diagnostic diff on typing conformance tests

Changes were detected when running ty on typing conformance tests
--- old-output.txt	2025-10-27 11:15:58.398773069 +0000
+++ new-output.txt	2025-10-27 11:16:01.520786835 +0000
@@ -867,11 +867,9 @@
 specialtypes_type.py:56:7: error[invalid-argument-type] Argument to function `func4` is incorrect: Expected `type[BasicUser] | type[ProUser]`, found `<class 'TeamUser'>`
 specialtypes_type.py:76:17: error[invalid-type-form] type[...] must have exactly one type argument
 specialtypes_type.py:84:5: error[type-assertion-failure] Argument does not have asserted type `type[Any]`
-specialtypes_type.py:98:5: error[type-assertion-failure] Argument does not have asserted type `tuple[type, ...]`
 specialtypes_type.py:99:17: error[unresolved-attribute] Object of type `type` has no attribute `unknown`
 specialtypes_type.py:100:17: error[unresolved-attribute] Object of type `type` has no attribute `unknown`
 specialtypes_type.py:102:5: error[type-assertion-failure] Argument does not have asserted type `tuple[type, ...]`
-specialtypes_type.py:106:5: error[type-assertion-failure] Argument does not have asserted type `tuple[type, ...]`
 specialtypes_type.py:107:17: error[unresolved-attribute] Object of type `type` has no attribute `unknown`
 specialtypes_type.py:108:17: error[unresolved-attribute] Object of type `type` has no attribute `unknown`
 specialtypes_type.py:110:5: error[type-assertion-failure] Argument does not have asserted type `tuple[type, ...]`
@@ -948,5 +946,5 @@
 typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
 typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on TypedDict `Movie`: Unknown key "title"
 typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 950 diagnostics
+Found 948 diagnostics
 WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details.

@dcreager
Copy link
Member

Special-casing things in type inference (instead of core Type operations) is something I would wish we would do less of, not more.

+1

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 21, 2025

Special-casing things in type inference (instead of core Type operations) is something I would wish we would do less of, not more.

In general, I absolutely agree. In this specific case, though, I worry that ty's inferred MRO for a class is often pretty different to a class's actual MRO at runtime, because of various fictions typeshed would (for very good reasons) have us believe. For example, we infer the MRO of tuple on main as tuple[<class 'tuple[Unknown, ...]'>, <class 'Sequence[_T_co@tuple]'>, <class 'Reversible[_T_co@tuple]'>, <class 'Collection[_T_co@tuple]'>, <class 'Iterable[_T_co@tuple]'>, <class 'Container[_T_co@tuple]'>, typing.Protocol, typing.Generic, <class 'object'>]. But at runtime:

>>> tuple.__mro__
(<class 'tuple'>, <class 'object'>)

Not only is typeshed's MRO for tuple much longer than the actual one at runtime, the MRO that we display also contains generic aliases in it, which can never appear in an MRO at runtime -- only actual class objects can appear in MROs at runtime. The lengthy MRO typeshed gives us is a white lie from typeshed, because otherwise there'd be no way to get us or other type checkers to understand that tuple should be considered a subtype of Sequence (Sequence is not a Protocol, and that's an intentional design decision in the type system). But I think displaying these frankly inaccurate MROs in things like autocomplete tooltips could honestly be pretty confusing for users, and having them be propagated through all type inference by baking the special case into Type::member() could have unexpected results in far-off places; to me it seems much better to keep the special case localized to actual attribute expressions in the source code. As the codspeed report shows, keeping the special case localized also seems to result in performance improvements on some microbenchmarks!

Honestly, after writing this all out, I'm not even sure whether we should special-case inference of the __mro__ attribute at all. Maybe our MRO tests should instead use a custom ty_extensions.reveal_mro() helper instead? Precise inference for the __mro__ attribute here wasn't ever implemented because it was something we felt users needed; it was implemented so that we'd have an easy way to write isolated unit tests for our MRO inference logic.

@sharkdp
Copy link
Contributor

sharkdp commented Oct 21, 2025

Honestly, after writing this all out, I'm not even sure whether we should special-case inference of the __mro__ attribute at all. Maybe our MRO tests should instead use a custom ty_extensions.reveal_mro() helper instead?

I got the same feeling when reading your comment and was glad to find that paragraph at the end 👍 👍

@AlexWaygood AlexWaygood force-pushed the alex/mro-attr branch 3 times, most recently from 8774382 to 0c1b397 Compare October 26, 2025 13:53
Copy link
Member Author

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Most of the changes to the tests are now a result of switching over to the custom reveal_mro assertions, so I'm annotating the changes that show how the semantics are improved as a result of this PR:

Comment on lines 167 to +197
if hasattr(DoesNotExist, "__mro__"):
# TODO: this should be `Unknown & <Protocol with members '__mro__'>` or similar
# (The second part of the intersection is incorrectly simplified to `object` due to https://github.com/astral-sh/ty/issues/986)
reveal_type(DoesNotExist) # revealed: Unknown
reveal_type(DoesNotExist) # revealed: Unknown & <Protocol with members '__mro__'>
Copy link
Member Author

Choose a reason for hiding this comment

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

this demonstrates the improved semantics from this PR

Comment on lines -332 to +333
# TODO: this should cause us to emit a diagnostic and reveal `Unknown` (function objects don't have an `__mro__` attribute),
# but the fact that we don't isn't actually a `NamedTuple` bug (https://github.com/astral-sh/ty/issues/986)
reveal_type(typing.NamedTuple.__mro__) # revealed: tuple[<class 'FunctionType'>, <class 'object'>]
# error: [unresolved-attribute]
reveal_type(typing.NamedTuple.__mro__) # revealed: Unknown
Copy link
Member Author

Choose a reason for hiding this comment

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

this demonstrates the improved semantics from this PR

scope-simple-long-identifier,main.py,0,1
tstring-completions,main.py,0,1
ty-extensions-lower-stdlib,main.py,0,7
ty-extensions-lower-stdlib,main.py,0,8
Copy link
Member Author

Choose a reason for hiding this comment

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

this change is because there is now another ty_extensions function (reveal_mro) that currently has a higher rank than typing.reveal_type in autocomplete suggestions, but would have a lower rank in an ideal world:

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 26, 2025

The beartype primer hit is clearly a false negative going away. The sympy primer hits are a bit more complicated, but also look like true positives to me. The issue is that at line 177, we infer the type of nargs as object | Any: Any is the type of nargs before the for loop, and object is the type assigned to it from inside the for loop. object | Any just simplifies to object.

The conformance suite diff is also good: two false positives going away.

This PR should be ready for another round of review now.

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.

Thank you! This looks great now.

@AlexWaygood AlexWaygood enabled auto-merge (squash) October 27, 2025 11:14
@AlexWaygood AlexWaygood merged commit db0e921 into main Oct 27, 2025
40 checks passed
@AlexWaygood AlexWaygood deleted the alex/mro-attr branch October 27, 2025 11:19
dcreager added a commit that referenced this pull request Oct 27, 2025
* origin/main:
  Respect `--output-format` with `--watch` (#21097)
  [`pyflakes`] Revert to stable behavior if imports for module lie in alternate branches for `F401` (#20878)
  Fix finding keyword range for clause header after statement ending with semicolon (#21067)
  [ty] Fix bug where ty would think all types had an `__mro__` attribute (#20995)
  Restore `indent.py` (#21094)
  [`flake8-django`] Apply `DJ001` to annotated fields (#20907)
  Clearer error message when `line-length` goes beyond threshold (#21072)
  Update upload and download artifacts github actions (#21083)
  Update dependency mdformat-mkdocs to v4.4.2 (#21088)
  Update cargo-bins/cargo-binstall action to v1.15.9 (#21086)
  Update Rust crate clap to v4.5.50 (#21090)
  Update Rust crate get-size2 to v0.7.1 (#21091)
  Update Rust crate bstr to v1.12.1 (#21089)
  Add missing docstring sections to the numpy list (#20931)
  [`pydoclint`] Fix false positive on explicit exception re-raising (`DOC501`, `DOC502`) (#21011)
  [ty] Use constructor parameter types as type context (#21054)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ty incorrectly thinks all objects have an __mro__ attribute

4 participants