[red-knot] Meta data for `Type::Todo` #14500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

sharkdp merged 2 commits into main from david/todo-type-metadata

Nov 21, 2024

Contributor

sharkdp commented Nov 20, 2024 •

edited

Loading

Summary

Adds meta information to Type::Todo, allowing developers to easily trace back the origin of a particular @Todo type they encounter.

Instead of Type::Todo, we now write either type_todo!() which creates a @Todo[path/to/source.rs:123] type with file and line information, or using type_todo!("PEP 604 unions not supported"), which creates a variant with a custom message.

Type::Todo now contains a TodoType field. In release mode, this is just a zero-sized struct, in order not to create any overhead. In debug mode, this is an enum that contains the meta information.

Type implements Copy, which means that TodoType also needs to be copyable. This limits the design space. We could intern TodoType, but I discarded this option, as it would require us to have access to the salsa DB everywhere we want to use Type::Todo. And it would have made the macro invocations less ergonomic (requiring us to pass db).

So for now, the meta information is simply a &'static str / u32 for the file/line variant, or a &'static str for the custom message. Anything involving a chain/backtrace of several @Todos or similar is therefore currently not implemented. Also because we currently don't see any direct use cases for this, and because all of this will eventually go away.

Note that the size of Type increases from 16 to 24 bytes, but only in debug mode.

Test Plan

Observed the changes in Markdown tests.
Added custom messages for all Type::Todos that were revealed in the tests
Ran red knot in release and debug mode on the following Python file:
```
def f(x: int) -> int:
    reveal_type(x)
```
Prints @Todo in release mode and @Todo(function parameter type) in debug mode.

sharkdp added internal ty labels

sharkdp requested review from carljm, MichaReiser and AlexWaygood as code owners

November 20, 2024 20:44

Contributor

github-actions bot commented Nov 20, 2024 •

edited

Loading

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser approved these changes

View reviewed changes

Member

MichaReiser left a comment

Clever!

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

sharkdp force-pushed the david/todo-type-metadata branch from 30b7e60 to a02a96a Compare

November 20, 2024 21:41

carljm approved these changes

View reviewed changes

Contributor

carljm left a comment

This is great! Will make debugging so much nicer.

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

crates/red_knot_python_semantic/src/types.rs Outdated

    
                          }

                          Type::Todo => Type::Todo.into(),

                          Type::Todo(_) => todo_type!().into(),

Contributor

carljm Nov 20, 2024

I guess there are some judgment calls about when to propagate a Todo vs create a new one. This could propagate, like we do below in to_instance and to_meta_type, but maybe an attribute of a todo feels more like a new todo than an extension of the previous one? Not sure what will be more useful in practice.

Contributor Author

sharkdp Nov 21, 2024

I'm going to assume for now that the original todo is more interesting, so I changed the logic here to propagate the incoming todo.

crates/red_knot_python_semantic/src/types.rs

Comment on lines +3636 to +3655

+                      assert!(todo1.is_equivalent_to(&db, todo2));
+                      assert!(todo3.is_equivalent_to(&db, todo4));
+                      assert!(todo1.is_equivalent_to(&db, todo3));
+                      assert!(todo1.is_subtype_of(&db, todo2));
+                      assert!(todo2.is_subtype_of(&db, todo1));
+                      assert!(todo3.is_subtype_of(&db, todo4));
+                      assert!(todo4.is_subtype_of(&db, todo3));
+                      assert!(todo1.is_subtype_of(&db, todo3));
+                      assert!(todo3.is_subtype_of(&db, todo1));

Contributor

carljm Nov 20, 2024

I think these are all not actually correct (as discussed in Discord today), but that's orthogonal to this PR; these assertions are consistent with our current handling. So I think it makes sense to leave them as-is for this PR.

Contributor Author

sharkdp Nov 20, 2024

Oh, I was about to read that discussion again tomorrow. I thought it was unrelated to my change 😄. I can look into it (not here).

Contributor

carljm Nov 21, 2024

It is unrelated to your change, except that you added some explicit tests that Todo (which is supposed to be a dynamic type just like Any or Unknown) is equivalent to and a subtype of itself, which I don't think we had explicitly tested before, and is not really right. So that makes it related :)

Contributor Author

sharkdp Nov 21, 2024

Okay yes, this is what I understood after your first comment. I'll address this in a follow-up and fix these tests accordingly.

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

sharkdp added 2 commits

November 21, 2024 09:51


          [red-knot] Meta data for Type::Todo

95e11e4

Adds meta information to `Type::Todo`, allowing developers to easily
trace back the origin of a particular `@Todo` type they encounter.

Instead of `Type::Todo`, we now write either `type_todo!()` which
creates a `@Todo[path/to/source.rs:123]` type with file and line
information, or using `type_todo!("PEP 604 unions not supported")`,
which creates a variant with a custom message.

`Type::Todo` now contains a `TodoType` field. In release mode, this is
just a zero-sized struct, in order not to create any overhead. In debug
mode, this is an `enum` that contains the meta information.

`Type` implements `Copy`, which means that `TodoType` also needs to be
copyable. This limits the design space. We could intern `TodoType`, but
I discarded this option, as it would require us to have access to the
salsa DB everywhere we want to use `Type::Todo`. And it would have made
the macro invocations less ergonomic (requiring us to pass `db`).

So for now, the meta information is simply a `&'static str` / u32 for
the file/line variant, or a `&'static str` for the custom message.
Anything involving a chain/backtrace of several `@Todo`s or similar is
therefore currently not implemented. Also because we currently don't see
any direct use cases for this, and because all of this will eventually
go away.

Note that the size of `Type` increases from 16 to 24 bytes, but only in
debug mode.


          Review findings

8f66ee3

sharkdp force-pushed the david/todo-type-metadata branch from de3f90d to 8f66ee3 Compare

November 21, 2024 08:52

sharkdp merged commit 47f39ed into main

20 checks passed

sharkdp deleted the david/todo-type-metadata branch

November 21, 2024 08:59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels