Derive GenericTypeVisitable for RegionConstraint _correctly_ - #160914
Derive GenericTypeVisitable for RegionConstraint _correctly_#160914ada4a wants to merge 5 commits into
GenericTypeVisitable for RegionConstraint _correctly_#160914Conversation
This is motivated by rust-lang#160164, which added the derive which wouldn't actually work, due to recusrive trait bounds (more on this in a later commit). This change will make it so that these errors are caught in rustc CI.
Becuase its implementations must uphold a soundness-critical invariant.
rustbot
added
the
S-waiting-on-review
rustbot
added
the
T-compiler
|
rustbot has assigned @JonathanBrouwer. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
It doesn't need to be unsafe. If the bounds are wrong it won't compile (at least in r-a). |
|
What about a case like this? #[derive(GenericTypeVisitable)]
struct Foo {
#[generic_type_visitable(unsafe(bounds()))]
contains_self: (Box<Self>, Bar),
}
struct Bar;AFAICT |
87bfdc9 to
fc37746
Compare
|
No, because then calling |
64616ec to
ec68072
Compare
|
The newly added complexity of the derive macro makes me wonder if it deserves some ui tests now.. Not sure where they would go though |
|
Note that only making the derive not-no-op won't make forgetting to derive it a CI failure, since rustc does not use it for anything. |
|
Yes, that will be covered by a future PR. I thought I'd get this one merged now, so that the fixed derive already gets into the source code -- otherwise the list of ra-ap crate versions I need to catch up on will only continue to rise over time 😅 |
This comment has been minimized.
This comment has been minimized.
ec68072 to
958b222
Compare
|
Apparently, a backtick in an |
I think this does deserve some tests. |
| /// | ||
| /// Therefore, it is advised to instead derive this using the derive | ||
| /// macro located in `rustc_macros`. | ||
| pub unsafe trait GenericTypeVisitable<V> { |
There was a problem hiding this comment.
Is the correctness of this trait actually relied on for soundness anywhere? If not this doesn't need to be unsafe
| /// ```ignore (would need to import GenericTypeVisitable to get this to compile) | ||
| /// #[derive(GenericTypeVisitable)] | ||
| /// struct Foo { | ||
| /// #[generic_type_visitable(bounds())] |
There was a problem hiding this comment.
How hard would it be to automatically detect bounds that would overflow the trait solver in the proc macro and remove them, rather than having to manually specify the bounds?
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
5 participants
The derive added in #160164 was incorrect -- it resulted in an overflow during trait solving. This is because
#[derive(GenericTypeVisitable)]automatically adds a: GenericTypeVisitablebound to every field of a type -- in this case,Box<[RegionConstraint<I>]>: GenericTypeVisitable<V>.To fix this, I added a
#[generic_type_visitable(bounds(..))]attribute to the derive macro, which allows overriding the added bounds.I also made the derive macro no longer a no-op in rustc, so that errors like this can be caught on r-l/r CI in the future.
cc @ChayimFriedman2