Don't special-case class instances in binary expression inference #15161

dcreager · 2024-12-27T15:54:49Z

Just like in #15045 for unary expressions: In binary expressions, we were only looking for dunder expressions for Type::Instance types. We had some special cases for coercing the various Literal types into their corresponding Instance types before doing the lookup. But we can side-step all of that by using the existing Type::to_meta_type and Type::to_instance methods.

Closes #14549

crates/red_knot_python_semantic/src/types/infer.rs

crates/red_knot_python_semantic/resources/mdtest/binary/integers.md

crates/red_knot_python_semantic/src/types/infer.rs

codspeed-hq · 2024-12-27T16:05:52Z

CodSpeed Performance Report

Merging #15161 will degrade performances by 4.5%

_{Comparing dcreager/infer-binary (7149f42) with main (d45c1ee)}

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`dcreager/infer-binary`	Change
👁	`red_knot_check_file[cold]`	83.7 ms	87.7 ms	-4.5%

crates/red_knot_workspace/tests/check.rs

crates/red_knot_python_semantic/resources/mdtest/binary/classes.md

github-actions · 2024-12-27T20:03:15Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

carljm

Looks great, thank you! Just one substantive comment.

carljm · 2024-12-29T00:29:13Z

crates/red_knot_python_semantic/resources/mdtest/binary/classes.md

+class A: ...
+class B: ...
+
+reveal_type(A | B)  # revealed: Literal[A, B]


I don't think Literal[A, B] is the right type to assign to the value expression A | B.

In a type expression, A | B spells the type Type::Union[Type::Instance(A), Type::Instance(B)] -- that is, the type consisting of all instances of A (or any subclass) and all instances of B (or any subclass).

The value expression A if flag else B would correctly be assigned the type Literal[A, B], since its runtime value must either be the class object A or the class object B.

But the runtime value of the value expression A | B is not either of those, so its type can't be Literal[A, B]. Its runtime value is an instance of types.UnionType, which can't be treated as if it were a class object at all. So the type we should assign to it is Type::Instance(types.UnionType).

Suggested change

reveal_type(A | B) # revealed: Literal[A, B]

reveal_type(A | B) # revealed: UnionType

If we did any special-casing here, the correct special casing would be to create a custom type representation for types.UnionType so we can record the fact that its __args__ attribute (in this case) has the type tuple[Literal[A], Literal[B]]. But I don't think this is worth doing.

Reverted the special case, this is now returning UnionType again as before

crates/red_knot_python_semantic/resources/mdtest/binary/custom.md

carljm · 2024-12-29T00:34:30Z

crates/red_knot_python_semantic/resources/mdtest/binary/custom.md

+reveal_type(f // f)  # revealed: Unknown
+```
+
+## Subclass


This feels very closely related to the ## Classes section above -- maybe group them together instead of having the intervening function literals section?

carljm · 2024-12-29T00:36:47Z

crates/red_knot_python_semantic/src/types/infer.rs

+            (Type::ClassLiteral(_), Type::ClassLiteral(_), ast::Operator::BitOr)
+                if Program::get(self.db()).python_version(self.db()) >= PythonVersion::PY310 =>
+            {
+                Some(UnionType::from_elements(self.db(), [left_ty, right_ty]))
+            }
+


This is the clause that I think we don't want; I expect typeshed will natively give us the correct type of types.UnionType?

Removed by the revert mentioned above

crates/red_knot_workspace/tests/check.rs

carljm · 2024-12-29T00:50:49Z

It's odd that this PR would cause a regression. The profiles on CodSpeed suggest that we are fetching some ingredient more often. I suspect that the cause is that improving completeness here means we now return a more complex type in lots of cases that would previously have just returned Unknown. Since this is just a natural consequence of better completeness, I'll go ahead and acknowledge the regression.

* main: Add all PEP-585 names to UP006 rule (#5454) [`flake8-simplify`] More precise inference for dictionaries (`SIM300`) (#15164) `@no_type_check` support (#15122) Visit PEP 764 inline `TypedDict`s' keys as non-type-expressions (#15073) [red-knot] Add diagnostic for invalid unpacking (#15086) [`flake8-use-pathlib`] Catch redundant joins in `PTH201` and avoid syntax errors (#15177) Update Rust crate glob to v0.3.2 (#15185) Update astral-sh/setup-uv action to v5 (#15193) Update dependency mdformat-mkdocs to v4.1.1 (#15192) Update Rust crate serde_with to v3.12.0 (#15191) Update NPM Development dependencies (#15190) Update pre-commit hook rhysd/actionlint to v1.7.5 (#15189) Update Rust crate syn to v2.0.93 (#15188) Update Rust crate serde to v1.0.217 (#15187) Update Rust crate quote to v1.0.38 (#15186) Update Rust crate compact_str to v0.8.1 (#15184) [`flake8-type-checking`] Disable TC006 & TC007 in stub files (#15179) Test explicit shadowing involving `def`s (#15174) Fix typo in `NameImport.qualified_name` docstring (#15170) [airflow]: extend names moved from core to provider (AIR303) (#15159)

This reverts commit a52486f.

* main: (60 commits) [`ruff`] Dataclass enums (`RUF049`) (#15299) Better error message when `--config` is given a table key and a non-inline-table value (#15266) Update pre-commit dependencies (#15289) Don't fix in ecosystem check (#15267) Update Rust crate itertools to 0.14.0 (#15287) Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290) Update Rust crate clearscreen to v4 (#15288) Update Rust crate insta to v1.42.0 (#15286) Update NPM Development dependencies (#15285) Update dependency uuid to v11.0.4 (#15284) Update dependency ruff to v0.8.6 (#15283) Update Rust crate syn to v2.0.95 (#15282) Update Rust crate matchit to v0.8.6 (#15281) Update Rust crate bstr to v1.11.3 (#15280) [red-knot] Future-proof `Type::is_disjoint_from()` (#15262) [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261) [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270) Allow assigning ellipsis literal as parameter default value (#14982) [red-knot] fix control flow for assignment expressions in elif tests (#15274) [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273) ...

carljm

Looks great, thanks for the thorough tests!

* main: Use uv consistently throughout the documentation (#15302) [red-knot] Eagerly normalize `type[]` types (#15272) [`pyupgrade`] Split `UP007` to two individual rules for `Union` and `Optional` (`UP007`, `UP045`) (#15313) [red-knot] Improve symbol-lookup tracing (#14907) [red-knot] improve type shrinking coverage in red-knot property tests (#15297) [`flake8-return`] Recognize functions returning `Never` as non-returning (`RET503`) (#15298) [`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601) Avoid treating newline-separated sections as sub-sections (#15311) Remove call when removing final argument from `format` (#15309) Don't enforce `object-without-hash-method` in stubs (#15310) Don't special-case class instances in binary expression inference (#15161) Upgrade zizmor to the latest version in CI (#15300)

dcreager added 5 commits December 27, 2024 09:45

Regroup literal cases together

c8c99f6

Add Todo and Never cases

58ada96

General Instance fallback

029932d

Add a slew of tests

3b1622f

Linter

33409ba

dcreager requested review from carljm, MichaReiser, AlexWaygood and sharkdp as code owners December 27, 2024 15:54

AlexWaygood added the red-knot Multi-file analysis & type inference label Dec 27, 2024

Add mis-typed test

9fb3013

dcreager commented Dec 27, 2024

View reviewed changes

dcreager added 2 commits December 27, 2024 14:51

More recursive class definitions cause panics

f4a65c2

Add test case for class union

a5f0dad

dcreager commented Dec 27, 2024

View reviewed changes

crates/red_knot_workspace/tests/check.rs Show resolved Hide resolved

crates/red_knot_python_semantic/resources/mdtest/binary/classes.md Show resolved Hide resolved

dcreager added 2 commits December 27, 2024 15:14

Add test case for class union < 3.10

5239fa9

Special-case ClassLiteral | ClassLiteral

a52486f

carljm reviewed Dec 29, 2024

View reviewed changes

dcreager added 4 commits December 30, 2024 08:45

Revert "Special-case ClassLiteral | ClassLiteral"

fe73357

This reverts commit a52486f.

Move subclass tests to be closer to similar class tests

a70fc31

Add reflected dunder test cases

0acdbe4

MichaReiser removed their request for review December 30, 2024 14:25

carljm approved these changes Jan 6, 2025

View reviewed changes

dcreager merged commit 5e9259c into main Jan 6, 2025
21 checks passed

dcreager deleted the dcreager/infer-binary branch January 6, 2025 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't special-case class instances in binary expression inference #15161

Don't special-case class instances in binary expression inference #15161

dcreager commented Dec 27, 2024

codspeed-hq bot commented Dec 27, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

carljm left a comment

carljm Dec 29, 2024

dcreager Dec 30, 2024

carljm Dec 29, 2024

dcreager Dec 30, 2024

carljm Dec 29, 2024

dcreager Dec 30, 2024

carljm commented Dec 29, 2024

carljm left a comment

	reveal_type(A \| B) # revealed: Literal[A, B]
	reveal_type(A \| B) # revealed: UnionType

Don't special-case class instances in binary expression inference #15161

Don't special-case class instances in binary expression inference #15161

Conversation

dcreager commented Dec 27, 2024

codspeed-hq bot commented Dec 27, 2024 • edited Loading

CodSpeed Performance Report

Merging #15161 will degrade performances by 4.5%

Summary

Benchmarks breakdown

github-actions bot commented Dec 27, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

carljm left a comment

Choose a reason for hiding this comment

carljm Dec 29, 2024

Choose a reason for hiding this comment

dcreager Dec 30, 2024

Choose a reason for hiding this comment

carljm Dec 29, 2024

Choose a reason for hiding this comment

dcreager Dec 30, 2024

Choose a reason for hiding this comment

carljm Dec 29, 2024

Choose a reason for hiding this comment

dcreager Dec 30, 2024

Choose a reason for hiding this comment

carljm commented Dec 29, 2024

carljm left a comment

Choose a reason for hiding this comment

codspeed-hq bot commented Dec 27, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

`ruff-ecosystem` results