[red-knot] Invalid assignments to attributes by sharkdp · Pull Request #15613 · astral-sh/ruff

sharkdp · 2025-01-20T10:46:26Z

Summary

Raise "invalid-assignment" diagnostics for incorrect assignments to attributes, for example:

class C:
    var: str = "a"

C.var = 1  # error: "Object of type `Literal[1]` is not assignable to `str`"

closes #15456

Test Plan

Updated test assertions
New test for assignments to module-attributes

sharkdp · 2025-01-20T12:48:26Z

crates/red_knot_python_semantic/src/types/infer.rs

+    fn infer_target<F>(&mut self, target: &ast::Expr, value: &ast::Expr, to_assigned_ty: F)
+    where
+        F: Fn(&'db dyn Db, Type<'db>) -> Type<'db>,


I went through three or four different iterations of this API here. I'm still not satisfied. If we want to pass in the assigned_ty directly, we need to call infer_standalone_expression at the call site. But we are only allowed to do that if the target is not a ast::Expr::Name(_), so we need to basically copy the whole match statement below to the call site. The duplication maybe not so bad, but I could some expression types are not yet handled here (see pre-existing TODO), and then we would need to remember to update it everywhere.

Maybe my struggle to incorporate this here is also a sign that I'm doing something wrong?

I think it's fine. An altenrative, to avoid monomorphization is to have a Target enum with a Name, and another variant but naming kind of gets awkward.

What's the reason that we only infer the value's type for name? Is it because the inference for non-names happens elsewhere and it, therefore, avoids doubl-inference?

What's the reason that we only infer the value's type for name? Is it because the inference for non-names happens elsewhere and it, therefore, avoids doubl-inference?

We only call infer_standalone_expression(value) for non-Names. For Names, we call infer_definition, which (somewhere down the line) calls infer_standalone_expression(value) itself.

So yes, if we would call infer_standalone_expression(value) for all targets, we would get double-inference (results e.g. in duplicated diagnostics).

sharkdp · 2025-01-20T12:50:12Z

crates/red_knot_python_semantic/src/types/infer.rs

                    }
-                }

-                Type::Never


Is it important that we return Never here? I wasn't able to construct a Python example that would get access to this type...?

It was changed to use Never in b1ce8a3, which was prompted by #13981 (comment)

You can't get access to this type in Python, the only reason we need a type for it is the "type pulling" test, which is a proxy for "what should we show if you hover over this expression in an IDE". Never is my best answer for that (I think it's a better answer than None), but I don't think it matters much.

You can't get access to this type in Python, the only reason we need a type for it is the "type pulling" test, which is a proxy for "what should we show if you hover over this expression in an IDE". Never is my best answer for that (I think it's a better answer than None), but I don't think it matters much.

Sorry, I should have phrased my question better: given that we "don't think it matters much", is it okay if we change the type of the obj.attr expression in an assignment like obj.attr = value from Never to the type that we would get for obj.attr in a Load context? I'm inclined to say that this is much more helpful for a user in the IDE, which is why I thought there must have been a reason why Never was chosen instead.

To get this merged, I'm going to assume that it's okay to do this (let me know if not!).

In a case like obj.attr = value, I think it's fine if hover on obj.attr shows the type of value. If it shows the type of obj.attr prior to the assignment, I think that's not good. (I haven't had a chance to fully review this PR yet so I don't know yet which it is.)

In a case like obj.attr = value, I think it's fine if hover on obj.attr shows the type of value. If it shows the type of obj.attr prior to the assignment, I think that's not good.

Okay. In that case, this will need another iteration (it's the latter).

I just realized that until we add narrowing on assignments to attributes, there's no difference in the obj.attr = value case. But in the obj = value, or particularly the obj: annotation = value case, it could be a totally different type, and I don't think it's sensible for hover on that assignment target to show its type prior to the assignment.

But this change only applies to attribute expressions, so for now at least I think it's fine as-is.

Raise "invalid-assignment" diagnostics for incorrect assignments to attributes, for example: ```py class C: c: str = "a" C.c = 1 ```

AlexWaygood · 2025-01-21T12:06:36Z

crates/red_knot_python_semantic/resources/mdtest/attributes.md

 C.pure_instance_variable = "overwritten on class"

-# TODO: this should be an error (incompatible types in assignment)
+# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to `str`"


I wonder if this might be a slightly friendlier error message for users?

Suggested change

# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to `str`"

# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to attribute with public type `str`"

looks like a pre-existing issue, though, so feel free to ignore this for this PR

I'll open a follow up PR to discuss this.

AlexWaygood · 2025-01-21T12:07:28Z

crates/red_knot_python_semantic/resources/mdtest/attributes.md

+# error: [invalid-assignment] "Object of type `int` is not assignable to `str`"
+for mod.global_symbol in IntIterable():
+    pass


carljm

Very nice! A few thoughts, but nothing that I think requires immediate follow-up.

carljm · 2025-01-22T16:23:29Z

crates/red_knot_python_semantic/src/types/infer.rs

                let value_ty = self.infer_expression(value);

-                if let Type::Instance(instance) = value_ty {
+                let symbol = if let Type::Instance(instance) = value_ty {


note for future: I suspect that we should allow member access to signal error conditions, rather than doing this special-casing based on the specific type out here in type inference

carljm · 2025-01-22T16:35:51Z

crates/red_knot_python_semantic/src/types/infer.rs

+                    ..
+                },
+            ) => {
+                let attribute_expr_ty = self.infer_attribute_expression(lhs_expr);


In future, if we add narrowing of attribute types, there could be a distinction between "infer the local type of this attribute expression" and "get the declared type of this attribute", and we would want the latter here, not the former. Which might also obsolete the change in this PR to the inferred type of Store attribute expressions. But for now, there is no difference, so it's convenient to just use infer_attribute_expression for both.

carljm · 2025-01-22T16:37:12Z

crates/red_knot_python_semantic/src/types/infer.rs

                    }
-                }

-                Type::Never


But this change only applies to attribute expressions, so for now at least I think it's fine as-is.

carljm · 2025-01-22T16:39:51Z

crates/red_knot_python_semantic/resources/mdtest/attributes.md

 reveal_type(Foo.__class__)  # revealed: Literal[type]
 ```

+## Module attributes


Should we also have a test for multi-level attribute assignment, either mod.Class.attr = ... or Class.NestedClass.attr = ...?

## Summary Add a new test for attribute accesses in case of nested modules / classes. Resolves this comment: #15613 (comment) ## Test Plan New MD test.

sharkdp added the ty Multi-file analysis & type inference label Jan 20, 2025

This comment was marked as resolved.

Sign in to view

sharkdp force-pushed the david/attribute-stores branch from 51cde27 to 92b828f Compare January 20, 2025 12:14

sharkdp marked this pull request as ready for review January 20, 2025 12:15

sharkdp requested review from AlexWaygood, MichaReiser and carljm as code owners January 20, 2025 12:15

sharkdp marked this pull request as draft January 20, 2025 12:18

sharkdp force-pushed the david/attribute-stores branch 2 times, most recently from 47c5ba1 to 07c51c8 Compare January 20, 2025 12:45

sharkdp commented Jan 20, 2025

View reviewed changes

sharkdp marked this pull request as ready for review January 20, 2025 12:50

sharkdp added 3 commits January 21, 2025 12:06

[red-knot] Invalid assignments to attributes

8101cd5

Raise "invalid-assignment" diagnostics for incorrect assignments to attributes, for example: ```py class C: c: str = "a" C.c = 1 ```

Also check assignment target in unpack assignments and for loops

e502b93

… unpackings

818f4c1

sharkdp force-pushed the david/attribute-stores branch from 3facc1d to 818f4c1 Compare January 21, 2025 11:07

AlexWaygood approved these changes Jan 21, 2025

View reviewed changes

sharkdp merged commit f349dab into main Jan 22, 2025
21 checks passed

sharkdp deleted the david/attribute-stores branch January 22, 2025 09:42

sharkdp mentioned this pull request Jan 22, 2025

[red-knot] Improved error message for attribute-assignments #15668

Merged

carljm reviewed Jan 22, 2025

View reviewed changes

sharkdp mentioned this pull request Jan 23, 2025

[red-knot] Add test for nested attribute access #15684

Merged

sharkdp added a commit that referenced this pull request Jan 23, 2025

[red-knot] Add test for nested attribute access (#15684)

1ecd978

## Summary Add a new test for attribute accesses in case of nested modules / classes. Resolves this comment: #15613 (comment) ## Test Plan New MD test.

sharkdp mentioned this pull request Jan 27, 2025

[red-knot] instance attributes #14164

Closed

18 tasks

	# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to `str`"
	# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to attribute with public type `str`"

Comments

Conversation

sharkdp commented Jan 20, 2025

Summary

Test Plan

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

4 participants

MichaReiser Jan 20, 2025 •

edited

Loading