[red-knot] Consolidate all gradual types into single Type variant by dcreager · Pull Request #15386 · astral-sh/ruff

dcreager · 2025-01-09T20:25:00Z

One nit: I think we need to consider Any and Unknown and Todo as all (gradually) equivalent to each other, and thus type & Any and type & Unknown and type & Todo as also equivalent. The distinction between Any vs Unknown vs Todo is entirely about provenance/debugging, there is no type level distinction. (And I've been wondering if the Any vs Unknown distinction is really worth it.)

The thought here is that most places want to treat Any, Unknown, and Todo identically. So this PR simplifies things by having a single Type::Any variant, and moves the provenance part into a new AnyType type. If you need to treat e.g. Todo differently, you still can by pattern-matching into the AnyType. But if you don't, you can just use Type::Any(_).

(This would also allow us to (more easily) distinguish "unknown via an unannotated value" from "unknown because of a typing error" should we want to do that in the future)

sharkdp

I like it! (haven't reviewed each and every change, but I guess it's more or less mechanical)

crates/red_knot_python_semantic/src/types.rs

github-actions · 2025-01-09T20:36:07Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

AlexWaygood · 2025-01-09T21:05:54Z

Brilliant idea! Is it possibly worth calling the variant and struct Type::Gradual and GradualType rather than Type::Any and AnyType? Type::Any(AnyType::Unknown) reads a bit strangely to me

AlexWaygood · 2025-01-09T21:12:21Z

Also: all roads lead to Rome

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/class_base.rs

dcreager

Brilliant idea! Is it possibly worth calling the variant and struct Type::Gradual and GradualType rather than Type::Any and AnyType? Type::Any(AnyType::Unknown) reads a bit strangely to me

Done

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/class_base.rs

dcreager

but I guess it's more or less mechanical

That is a good callout. I did try to make this a pure refactoring. For instance, there are a couple of places where we were doing something only for a Todo, but not for Any or Unknown. Instead of inspecting and possibly changing those in this PR, I've left them as-is so that this PR does not combine a refactoring with a logic change.

dcreager · 2025-01-09T21:30:10Z

crates/red_knot_python_semantic/src/types/infer.rs

-                                todo @ Type::Todo(_) => return Ok(todo),
+                                todo @ Type::Any(AnyType::Todo(_)) => return Ok(todo),


* main: [red-knot] Move `UnionBuilder` tests to Markdown (#15374)

crates/red_knot_python_semantic/src/types.rs

Co-authored-by: Alex Waygood <[email protected]>

carljm

This is excellent!!

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/class_base.rs

carljm

This makes handling these in match statements so much nicer!

carljm · 2025-01-09T21:59:16Z

crates/red_knot_python_semantic/src/types.rs

@@ -1085,9 +1082,16 @@ impl<'db> Type<'db> {
    pub(crate) fn is_same_gradual_form(self, other: Type<'db>) -> bool {


Side note on seeing this method, wondering why we need it, and going to look: I wonder if maybe we shouldn't allow Any/Unknown/Todo to coexist in the same union/intersection, and instead should just define a hierarchy of preference, e.g. Todo takes top priority, Any second, and Unknown third?

Definitely not something for this PR, just musing.

crates/red_knot_python_semantic/src/types.rs

Co-authored-by: Carl Meyer <[email protected]> Co-authored-by: Alex Waygood <[email protected]>

Consolidate all gradual types into single Type variant

8880a3e

dcreager requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners January 9, 2025 20:25

dcreager changed the title ~~Consolidate all gradual types into single Type variant~~ [red-knot] Consolidate all gradual types into single Type variant Jan 9, 2025

dcreager added the ty Multi-file analysis & type inference label Jan 9, 2025

Fix test cases

9d49bd8

sharkdp approved these changes Jan 9, 2025

View reviewed changes

MichaReiser reviewed Jan 9, 2025

View reviewed changes

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

AlexWaygood approved these changes Jan 9, 2025

View reviewed changes

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

crates/red_knot_python_semantic/src/types/class_base.rs Outdated Show resolved Hide resolved

dcreager added 2 commits January 9, 2025 16:26

Rename Any → Gradual

cb070cb

Rename annotated_any → any

c88b497

dcreager commented Jan 9, 2025

View reviewed changes

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

crates/red_knot_python_semantic/src/types/class_base.rs Outdated Show resolved Hide resolved

dcreager commented Jan 9, 2025

View reviewed changes

dcreager added 2 commits January 9, 2025 16:33

GradualType::Annotated → Any

8d12d1a

Merge branch 'main' into dcreager/consolidate-any

cf2a10c

* main: [red-knot] Move `UnionBuilder` tests to Markdown (#15374)

AlexWaygood approved these changes Jan 9, 2025

View reviewed changes

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

Update crates/red_knot_python_semantic/src/types.rs

bea6c3f

Co-authored-by: Alex Waygood <[email protected]>

carljm approved these changes Jan 9, 2025

View reviewed changes

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

Gradual -> Dynamic

f580910

AlexWaygood approved these changes Jan 9, 2025

View reviewed changes

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

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

crates/red_knot_python_semantic/src/types/class_base.rs Outdated Show resolved Hide resolved

carljm approved these changes Jan 9, 2025

View reviewed changes

dcreager and others added 3 commits January 9, 2025 21:06

Apply suggestions from code review

03c5811

Co-authored-by: Carl Meyer <[email protected]> Co-authored-by: Alex Waygood <[email protected]>

Consolidate on $crate

e12f63b

Last few gradual → dynamic

1a1491f

dcreager merged commit baf0683 into main Jan 10, 2025
21 checks passed

dcreager deleted the dcreager/consolidate-any branch January 10, 2025 02:32

AlexWaygood mentioned this pull request Jan 10, 2025

[red-knot] Support assert_type #15194

Merged

		todo @ Type::Todo(_) => return Ok(todo),
		todo @ Type::Any(AnyType::Todo(_)) => return Ok(todo),

		@@ -1085,9 +1082,16 @@ impl<'db> Type<'db> {
		pub(crate) fn is_same_gradual_form(self, other: Type<'db>) -> bool {

Comments

Conversation

dcreager commented Jan 9, 2025

Uh oh!

sharkdp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

AlexWaygood commented Jan 9, 2025

Uh oh!

AlexWaygood commented Jan 9, 2025

Uh oh!

Uh oh!

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

dcreager Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Jan 9, 2025 •

edited

Loading

`ruff-ecosystem` results