Conversation
|
30b7e60 to
a02a96a
Compare
carljm
left a comment
There was a problem hiding this comment.
This is great! Will make debugging so much nicer.
| todo_type!().into() | ||
| } | ||
| Type::Todo => Type::Todo.into(), | ||
| Type::Todo(_) => todo_type!().into(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Okay yes, this is what I understood after your first comment. I'll address this in a follow-up and fix these tests accordingly.
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.
de3f90d to
8f66ee3
Compare
Summary
Adds meta information to
Type::Todo, allowing developers to easily trace back the origin of a particular@Todotype they encounter.Instead of
Type::Todo, we now write eithertype_todo!()which creates a@Todo[path/to/source.rs:123]type with file and line information, or usingtype_todo!("PEP 604 unions not supported"), which creates a variant with a custom message.Type::Todonow contains aTodoTypefield. In release mode, this is just a zero-sized struct, in order not to create any overhead. In debug mode, this is anenumthat contains the meta information.TypeimplementsCopy, which means thatTodoTypealso needs to be copyable. This limits the design space. We could internTodoType, but I discarded this option, as it would require us to have access to the salsa DB everywhere we want to useType::Todo. And it would have made the macro invocations less ergonomic (requiring us to passdb).So for now, the meta information is simply a
&'static str/u32for the file/line variant, or a&'static strfor 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
Typeincreases from 16 to 24 bytes, but only in debug mode.Test Plan
Type::Todos that were revealed in the tests@Todoin release mode and@Todo(function parameter type)in debug mode.