Add tests and docs for #[derive(GenericTypeVisitable)] - #161806
Conversation
I ended up deciding not to add docs about `bounds` as it seems like a relatively minor feature of the derive, and there are docs at [1]. [1]: https://github.com/rust-lang/rust/blob/3ffb26fbf5bf232cf59e314e75ea325973f4f583/compiler/rustc_type_ir_macros/src/lib.rs#L21-L55
Just specifying `T: GenericTypeVisitable` doesn't work, as the trait has a generic: `V`, the visitor. `T: GenericTypeVisitable<__V>` is what actually works, as `__V` is the generic added to the impl generated by the derive macro. We discussed[1] different ways of making this nicer, but settled on not doing anything, as we don't expect people to need to specify any actual bounds. [1]: https://rust-lang.zulipchat.com/#narrow/channel/185405-t-compiler.2Frust-analyzer/topic/Updating.20next-solver/near/618331780 and below
|
|
rustbot
added
A-rustc-dev-guide
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? me |
| use rustc_type_ir::{GenericTypeVisitable, Interner}; | ||
| use rustc_type_ir_macros::GenericTypeVisitable; | ||
|
|
||
| #[derive(GenericTypeVisitable)] |
There was a problem hiding this comment.
It would be nice to make this into a run-pass test that uses it's main function to test the implementation of GenericTypeVisitable. Doesn't have to be for all of the types in the file if you don't feel like it, but having some coverage of the implementation is good
|
|
||
| #[derive(GenericTypeVisitable)] | ||
| struct MissingBound<T> { | ||
| // This should fail, as `T: GenericTypeVisitable<__V>` wasn't specified |
There was a problem hiding this comment.
I guess you need to split this into it's own file then
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
Other than that this looks good to me, thanks! |
4 participants
..given the added complexity from the newly-added
boundsattributeFollow-up to #160914
More details in individual commits.
cc @JonathanBrouwer (you might want to take over the review of this since you have some context already.. but as you wish)
cc @ChayimFriedman2