Add augmented assignment inference for `-=` operator by charliermarsh · Pull Request #13981 · astral-sh/ruff

charliermarsh · 2024-10-29T17:16:03Z

Summary

charliermarsh · 2024-10-29T17:16:28Z

crates/red_knot_python_semantic/src/semantic_index/builder.rs

+                    self.mark_symbol_used(symbol);
+                    let use_id = self.current_ast_ids().record_use(expr);
+                    self.current_use_def_map_mut().record_use(symbol, use_id);
+                }


Per Discord: the use shouldn't see the definition, so we had to change the order here. (This hasn't mattered up until augmented assignments.)

charliermarsh · 2024-10-29T17:16:46Z

crates/red_knot_python_semantic/src/types/infer.rs

+                    ),
+                );
+                Type::Unknown
+            })


The logic here is meant to mimic:

if hasattr(a, "__isub__"): _value = a.__isub__(b) if _value is not NotImplemented: a = _value else: a = a - b del _value else: a = a - b

As per https://snarky.ca/unravelling-augmented-arithmetic-assignment/.

However, I don't really know how to handle the NotImplemented case, if at all -- i.e., how we can determine whether to use __isub__ or fall back to a - b. Can we even detect that from the type system?

The convention for dunder methods is that if calling the dunder would return NotImplemented, it's annotated "as if it were an illegal call". So int.__sub__ is annotated as "not accepting" float in typeshed, even though actually calling it with a float "works fine" (it just returns NotImplemented, allowing the operation to fallback to float.__rsub__):

ruff/crates/red_knot_vendored/vendor/typeshed/stdlib/builtins.pyi

Line 273 in 60a2dc5

def __sub__(self, value: int, /) -> int: ...

>>> (42).__sub__(42.0) NotImplemented

Currently, though, we don't do any checking of argument types passed into functions, so we can't implement the "fallback to the other method" logic fully right now. There's lots of TODO comments in our tests for binary arithmetic about this currently.

Some example TODOs for you:

ruff/crates/red_knot_python_semantic/resources/mdtest/binary/instances.md

Lines 284 to 291 in 60a2dc5

# TODO should be complex, need to check arg type and fall back to `rhs.__radd__`

reveal_type(3.14 + 3j) # revealed: float

# TODO should be float, need to check arg type and fall back to `rhs.__radd__`

reveal_type(42 + 4.2) # revealed: int

# TODO should be complex, need to check arg type and fall back to `rhs.__radd__`

reveal_type(3 + 3j) # revealed: int

The way these dunder methods are typically annotated is that their parameter type should reflect the type of argument for which the method will return a value (not Notimplemented), and any other type will return NotImplemented. So we can detect this from the type system, but currently red-knot can't, because we don't yet have call signature checking (it's something I want to work on this week.)

I'll comment more details above.

Oops, didn't see Alex's review before submitting mine.

charliermarsh · 2024-10-29T17:17:01Z

crates/red_knot_python_semantic/src/types/infer.rs

+                            "Operator `{op}` is unsupported for type `{}`",
+                            target_type.display(self.db),
+                        ),
+                    );


Should this... fall back to a - b?

This relates to your question below about NotImplemented. If there is an __isub__ method, but it doesn't accept the value_type we try to pass it, then we should fall back to __sub__. For any other call error (e.g. __isub__ is set to some non-callable type for some reason) we should just emit a diagnostic and be done here. This will require some more complex matching on the specific error kind we get back from the call -- but this is all a TODO until we have call signature checking.

I would like to add a test for this case (__isub__ exists but doesn't accept the type we pass to it) with a TODO comment in the tests for the right result, to help us remember that this needs fixing.

crates/red_knot_python_semantic/src/types/infer.rs

carljm

Looks good! Mostly just would like to add a few more tests.

carljm · 2024-10-29T17:39:44Z

crates/red_knot_python_semantic/src/types/infer.rs

+                    ),
+                );
+                Type::Unknown
+            })


The way these dunder methods are typically annotated is that their parameter type should reflect the type of argument for which the method will return a value (not Notimplemented), and any other type will return NotImplemented. So we can detect this from the type system, but currently red-knot can't, because we don't yet have call signature checking (it's something I want to work on this week.)

I'll comment more details above.

carljm · 2024-10-29T17:45:52Z

crates/red_knot_python_semantic/src/types/infer.rs

+                            "Operator `{op}` is unsupported for type `{}`",
+                            target_type.display(self.db),
+                        ),
+                    );


This relates to your question below about NotImplemented. If there is an __isub__ method, but it doesn't accept the value_type we try to pass it, then we should fall back to __sub__. For any other call error (e.g. __isub__ is set to some non-callable type for some reason) we should just emit a diagnostic and be done here. This will require some more complex matching on the specific error kind we get back from the call -- but this is all a TODO until we have call signature checking.

I would like to add a test for this case (__isub__ exists but doesn't accept the type we pass to it) with a TODO comment in the tests for the right result, to help us remember that this needs fixing.

crates/red_knot_python_semantic/src/types/infer.rs

carljm · 2024-10-29T17:56:14Z

Oh, just noticed the corpus_no_panic test failure. This is the test that verifies that all expressions get a type. I think the problem here is that when we use infer_name_load and infer_attribute_load to get the type of the LHS for a Name or Attribute, we never actually store the type on the expression node (infer_expression normally takes care of that.)

But we don't want to store the assume-Load type on the node. So I think we also need to call infer_expression on the LHS node regardless, even if we are also calling infer_name_load or infer_attribute_load on it, just to get the None type stored on it that we currently infer for Store expressions.

charliermarsh · 2024-10-29T22:34:31Z

Still getting that same failure after calling self.infer_expression (at least locally), will return to it later and see what I can figure out.

charliermarsh · 2024-10-29T22:36:39Z

Oh I think it's because we end up visiting the attribute twice, maybe...? Something like that? Because infer_attribute_load calls self.infer_expression?

carljm · 2024-10-29T22:47:46Z

Ah yeah -- if you look at the error you're getting, I'll bet before it was no entry found for key and now it's a failure of the assertion on line 1900? That is because we end up calling infer_expression twice for the same expression, which we try to maintain an invariant that we don't do.

carljm · 2024-10-29T22:55:17Z

One option for how to handle this is to extract that code at the end of self.infer_expression (that stores a type into the expression map and asserts there wasn't one there already) into a method, like fn store_expression_type(...), and then instead of calling self.infer_expression(target) along with self.infer_attribute_load(target), instead you'd just manually store a None (or like I mentioned, Never is probably better but then you should also update it in infer_attribute_expression and infer_name_expression for consistency) for the target node. This means we end up duplicating the knowledge of what type to store for a Store context expression... there's probably a way to refactor away that duplication.

github-actions · 2024-10-30T02:23:21Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)

pypa/setuptools (error)

ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pypa/setuptools (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence

AlexWaygood

Nice

AlexWaygood · 2024-10-30T08:09:45Z

crates/red_knot_python_semantic/src/types/infer.rs

+        // TODO(charlie): Add remaining branches for different types of augmented assignments.
+        if let (Operator::Sub, Type::Instance(class)) = (*op, target_type) {
+            let class_member = class.class_member(self.db, "__isub__");
+            let call = class_member.call(self.db, &[value_type]);


You need to explicitly pass in the self argument here because we've accessed the method on the class rather than the instance, so you're getting the original function object rather than a method object from the class.class_member call

Suggested change

let call = class_member.call(self.db, &[value_type]);

let call = class_member.call(self.db, &[target_type, value_type]);

This will matter when we start checking that function calls are being passed arguments of the correct types

AlexWaygood · 2024-10-30T08:13:25Z

crates/red_knot_python_semantic/src/types/infer.rs

+        let member_ty = value_ty.member(self.db, &Name::new(&attr.id));
+
+        member_ty


nit

Suggested change

let member_ty = value_ty.member(self.db, &Name::new(&attr.id));

member_ty

value_ty.member(self.db, &Name::new(&attr.id))

## Summary See: #13981 (comment)

charliermarsh requested review from AlexWaygood, MichaReiser and carljm as code owners October 29, 2024 17:16

charliermarsh commented Oct 29, 2024

View reviewed changes

charliermarsh added the ty Multi-file analysis & type inference label Oct 29, 2024

charliermarsh commented Oct 29, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Show resolved Hide resolved

charliermarsh commented Oct 29, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Show resolved Hide resolved

carljm approved these changes Oct 29, 2024

View reviewed changes

charliermarsh force-pushed the charlie/aug branch from b989561 to 91006d0 Compare October 29, 2024 22:32

charliermarsh force-pushed the charlie/aug branch from 91006d0 to 7168bc3 Compare October 30, 2024 02:07

Add augmented assignment inference for -= operator

7361a48

charliermarsh force-pushed the charlie/aug branch from 7168bc3 to 7361a48 Compare October 30, 2024 02:09

charliermarsh merged commit c6b8215 into main Oct 30, 2024

charliermarsh deleted the charlie/aug branch October 30, 2024 02:14

charliermarsh mentioned this pull request Oct 30, 2024

Use Never instead of None for stores #13984

Merged

AlexWaygood reviewed Oct 30, 2024

View reviewed changes

This was referenced Oct 30, 2024

Add remaining augmented assignment dunders #13985

Merged

Use binary semantics when __iadd__ et al are unbound #13987

Merged

charliermarsh added a commit that referenced this pull request Oct 30, 2024

Use Never instead of None for stores (#13984)

b1ce8a3

## Summary See: #13981 (comment)

AlexWaygood mentioned this pull request Jan 20, 2025

[red-knot] Invalid assignments to attributes #15613

Merged

	# TODO should be complex, need to check arg type and fall back to `rhs.__radd__`
	reveal_type(3.14 + 3j) # revealed: float

	# TODO should be float, need to check arg type and fall back to `rhs.__radd__`
	reveal_type(42 + 4.2) # revealed: int

	# TODO should be complex, need to check arg type and fall back to `rhs.__radd__`
	reveal_type(3 + 3j) # revealed: int

	let call = class_member.call(self.db, &[value_type]);
	let call = class_member.call(self.db, &[target_type, value_type]);

		let member_ty = value_ty.member(self.db, &Name::new(&attr.id));

		member_ty

Comments

Conversation

charliermarsh commented Oct 29, 2024

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm commented Oct 29, 2024

Uh oh!

charliermarsh commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliermarsh commented Oct 29, 2024

Uh oh!

carljm commented Oct 29, 2024

Uh oh!

carljm commented Oct 29, 2024

Uh oh!

github-actions bot commented Oct 30, 2024

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charliermarsh commented Oct 29, 2024 •

edited

Loading

`ruff-ecosystem` results