Derive GenericTypeVisitable for RegionConstraint _correctly_ - #160914
Derive GenericTypeVisitable for RegionConstraint _correctly_#160914ada4a wants to merge 5 commits into
GenericTypeVisitable for RegionConstraint _correctly_#160914Conversation
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. |
| /// ```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?
There was a problem hiding this comment.
I mean.. that would require us to filter out the fields whose types contain Self, unless it's a type like (Self, Bar), in which case we'd need to filter out Self but not Bar, etc. etc. I'd argue that this is pretty difficult to do in general, as proc-macros don't have access to name resolution.
Given that this is also my literal first time writing proc macro code, I'd rather keep it simple (if a bit annoying for the user) than risk unsoundness 😅
There was a problem hiding this comment.
One last argument is that this is the first time such a manual bound was even necessary, and that's with GenericTypeVisitable having existed for some while. So my hope is that cases like this will only come up relatively rarely.
There was a problem hiding this comment.
IMO that's just not worth it. Other proc macros in the ecosystem also just require specifying bounds.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
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.
958b222 to
d75b165
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Ah, sorry, forgot about the |
|
Oh, apparently rustc-dev-guide has a section on |
There was a problem hiding this comment.
@bors r+ rollup
This looks reasonable, thanks! <3
As a follow up you:
- Should make a rustc-dev-guide PR explaining the new
boundsoption (Feel free to assign me for review if you feel like the PR benefits from review) - Could add a
ui-fulldepstest for this macro
rust-bors
Bot
added
S-waiting-on-bors
… r=JonathanBrouwer Derive `GenericTypeVisitable` for `RegionConstraint` _correctly_ The derive added in rust-lang#160164 was incorrect -- it resulted in an overflow during trait solving. This is because `#[derive(GenericTypeVisitable)]` automatically adds a `: GenericTypeVisitable` bound 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. Best reviewed commit-by-commit. cc @ChayimFriedman2
…uwer Rollup of 7 pull requests Successful merges: - #157513 (Reject non-constructor self types in const-arg tuple-call lowering) - #159887 (compiletest: forward disable-minification from bootstrap) - #160949 (Add floating point inline ASM support for SPARC) - #156160 (feat: add symmetric PartialEq impls for Vec, &[T], &mut [T] versus Cow<'_, [T]>) - #160302 (target_features: sse (or at least avx2) is incompatible with soft-float ABI) - #160914 (Derive `GenericTypeVisitable` for `RegionConstraint` _correctly_) - #161341 (be more permissive wrt overflow and and improve diagnostics)
5 participants
View all comments
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.
Best reviewed commit-by-commit.
cc @ChayimFriedman2