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

dcreager · 2024-12-27T15:55:50Z

crates/red_knot_python_semantic/src/types/infer.rs

+            (Type::BooleanLiteral(b1), Type::BooleanLiteral(b2), ast::Operator::BitOr) => {
+                Some(Type::BooleanLiteral(b1 | b2))
+            }


I reordered these so that all of the special cases for literals appeared together

dcreager · 2024-12-27T15:56:06Z

crates/red_knot_python_semantic/src/types/infer.rs

+            (todo @ Type::Todo(_), _, _) | (_, todo @ Type::Todo(_), _) => Some(todo),
+            (Type::Never, _, _) | (_, Type::Never, _) => Some(Type::Never),


Had to add clauses for our other gradual type forms

dcreager · 2024-12-27T15:57:20Z

crates/red_knot_python_semantic/src/types/infer.rs

-            (Type::Instance(_), Type::IntLiteral(_), op) => self.infer_binary_expression_type(
-                left_ty,
-                KnownClass::Int.to_instance(self.db()),
-                op,
-            ),
-
-            (Type::IntLiteral(_), Type::Instance(_), op) => self.infer_binary_expression_type(
-                KnownClass::Int.to_instance(self.db()),
-                right_ty,
-                op,
-            ),
-
-            (Type::Instance(_), Type::Tuple(_), op) => self.infer_binary_expression_type(
-                left_ty,
-                KnownClass::Tuple.to_instance(self.db()),
-                op,
-            ),
-
-            (Type::Tuple(_), Type::Instance(_), op) => self.infer_binary_expression_type(
-                KnownClass::Tuple.to_instance(self.db()),
-                right_ty,
-                op,
-            ),


These are now all handled by the to_meta_type calls down below. The new approach is more complete: before we handled when each literal type appeared in an expression with an Instance, but not with other (different) literal types.

dcreager · 2024-12-27T16:00:22Z

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

+
+# TODO: We don't currently verify that the actual parameter to int.__add__ matches the declared
+# formal parameter type.
+reveal_type(2 + "f")  # revealed: int


This test produced a @Todo type before, since our literal coercions weren't complete

dcreager · 2024-12-27T16:01:46Z

crates/red_knot_python_semantic/src/types/infer.rs

+                Type::FunctionLiteral(_)
+                | Type::ModuleLiteral(_)
+                | Type::ClassLiteral(_)
+                | Type::SubclassOf(_)
+                | Type::Instance(_)
+                | Type::KnownInstance(_)
+                | Type::Union(_)
+                | Type::Intersection(_)


I've expanded this out into a list of all of the coercable types, just like @AlexWaygood requested in #15045 (comment)

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

CodSpeed Performance Report

Merging #15161 will degrade performances by 4.76%

_{Comparing dcreager/infer-binary (0acdbe4) with main (04d5381)}

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`dcreager/infer-binary`	Change
👁	`red_knot_check_file[cold]`	83.5 ms	87.6 ms	-4.76%

dcreager · 2024-12-27T19:58:12Z

crates/red_knot_workspace/tests/check.rs

-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, false),
+    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, true),
+    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_27.py", true, true),


The test failures were because we're now noticing the recursiveness of some additional class definitions (#14672)

class Tree(list[Tree | Leaf]): ...

We now see into the BitOr of two ClassLiterals, and in most cases, can see via the typeshed that the result is a UnionType. But in these two fixtures, there's a recursive reference inside that union, which we can't yet handle.

Maybe eliminating the special case and just returning types.UnionType instead, as commented above, will help these cases?

Oh, perhaps not; I see now that this comment was made before you added that special case.

dcreager · 2024-12-27T19:59:16Z

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

+class A: ...
+class B: ...
+
+reveal_type(A | B)  # revealed: UnionType


This comes from the typeshed. We might consider special-casing the BitOr of two ClassLiterals to return a more specific Type::Union

(Note that this is not a type annotation, so none of the logic in Type::in_type_expression applies)

I went ahead and added this special case, but retaining the behavior that it's only available in Python ≥3.10

Oh, I commented on this in my review; I think the added special case is not correct, the type from typeshed is :)

👍 Sounds good! I wasn't sure about which way to go, so I added the special case in a separate commit that would be easier to revert

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

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -23 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

python/typeshed (+0 -23 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

- stdlib/builtins.pyi:1022:12: UP006 Use `collections.abc.MutableSequence` instead of `MutableSequence` for type annotation
- stdlib/builtins.pyi:1077:12: UP006 Use `collections.abc.MutableMapping` instead of `MutableMapping` for type annotation
- stdlib/builtins.pyi:1237:13: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:1381:17: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1388:17: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1398:17: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1407:17: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1417:17: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1870:14: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1871:13: UP006 Use `collections.abc.Mapping` instead of `Mapping` for type annotation
- stdlib/builtins.pyi:1872:15: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:2077:52: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:2078:54: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:2109:32: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:210:85: UP006 Use `collections.abc.MutableMapping` instead of `MutableMapping` for type annotation
- stdlib/builtins.pyi:2111:32: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:2115:52: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:2116:54: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:438:11: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:622:13: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:722:17: UP006 Use `collections.abc.MutableSequence` instead of `MutableSequence` for type annotation
- stdlib/builtins.pyi:839:18: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation
- stdlib/builtins.pyi:968:13: UP006 Use `collections.abc.Sequence` instead of `Sequence` for type annotation

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
UP006	23	0	23	0	0

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

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

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

+reveal_type(No() & Yes())  # revealed: Unknown
+# error: [unsupported-operator] "Operator `//` is unsupported between objects of type `No` and `Yes`"
+reveal_type(No() // Yes())  # revealed: Unknown
+```


It feels like we are missing the Yes() <op> No() case here; is there a reason you didn't include that?

I wasn't sure if that would be needed, since No is not a subclass of Yes. So it doesn't matter whether it implements __radd__, we're always going to use Yes.__add__, and so that would give the same result as the Yes() <op> Yes() case.

So I think what I want is another stanza, where Sub and No both implemented the reflected methods, to show that we correctly use the Sub reflections since it's a subclass, but don't pick up the No reflections since it's not.

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

carljm · 2024-12-29T00:38:21Z

crates/red_knot_workspace/tests/check.rs

-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, false),
+    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, true),
+    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_27.py", true, true),


Maybe eliminating the special case and just returning types.UnionType instead, as commented above, will help these cases?

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.

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

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

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

dcreager Dec 27, 2024

dcreager Dec 27, 2024

dcreager Dec 27, 2024

dcreager Dec 27, 2024

dcreager Dec 27, 2024

codspeed-hq bot commented Dec 27, 2024 •

edited

Loading

dcreager Dec 27, 2024

carljm Dec 29, 2024

carljm Dec 29, 2024

dcreager Dec 27, 2024

dcreager Dec 27, 2024

dcreager Dec 27, 2024

carljm Dec 29, 2024

dcreager Dec 30, 2024

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 Dec 29, 2024

dcreager Dec 30, 2024

carljm Dec 29, 2024

carljm commented Dec 29, 2024

		(todo @ Type::Todo(_), _, _) \| (_, todo @ Type::Todo(_), _) => Some(todo),
		(Type::Never, _, _) \| (_, Type::Never, _) => Some(Type::Never),

	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

Are you sure you want to change the base?

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

Conversation

dcreager commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Merging #15161 will degrade performances by 4.76%

Summary

Benchmarks breakdown

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Dec 29, 2024

codspeed-hq bot commented Dec 27, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

`ruff-ecosystem` results