-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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.
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
Permit T::Item
based on bounds that appear in where clauses
#22512
Permit T::Item
based on bounds that appear in where clauses
#22512
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
👍 This doesn't seem to conflict with #22172 and looks like great progress forward. |
pub default: Option<Ty<'tcx>>, | ||
pub object_lifetime_default: Option<ObjectLifetimeDefault>, | ||
|
||
// FIXME #22436 -- once #22436 lands, we can remove this | ||
pub region_bounds: Vec<ty::Region>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like #22436 has landed, could we remove this before landing this patch? if you don't have time I would be happy to help.
6baf735
to
0aff005
Compare
rebased, addressed @jroesch's point |
@bors r=eddyb 0aff005 |
@eddyb I'm interpreting that as r+...let me know if that is incorrect |
@bors r- saw some local test failures |
0aff005
to
26752b2
Compare
@bors r=eddyb 26752b2 |
@bors r- I'm confused by my local git state. Not sure that I tested what I thought I tested. Holding off on the review status until I know. |
Sorry for the confusion, I read through the patch and it looks good - but I didn't feel qualified enough for a proper review. Somehow I forgot to expand on that. |
r? @nick29581 |
(make check passes locally) |
@@ -253,11 +376,146 @@ impl<'a, 'tcx> AstConv<'tcx> for CollectCtxt<'a, 'tcx> { | |||
} | |||
} | |||
|
|||
fn get_enum_variant_types<'a, 'tcx>(ccx: &CollectCtxt<'a, 'tcx>, | |||
/// Interface used to find the bounds on a type parameter from within | |||
/// an `ItemCtxt`. This allows us to multiple kinds of sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing verb in 'to multiple'
r+ with nits addressed |
26752b2
to
5280904
Compare
Addressed nits, rebased. |
@bors r=nrc 5280904 |
Needs a rebase. |
(Has nontrivial merge conflicts which need code changes -- removed from rollup) |
Needs rebase |
5280904
to
1fb2580
Compare
@bors r+ 1fb2580 |
@bors: p=5 |
☔ Merge conflict |
global context. Have this `ItemCtxt` carry a (currently unused) pointer to the in-scope generics.
rather than poking through the `TypeParameterDef` directly.
report are not *necessary* cycles, but we'll work on refactoring them over time. This overlaps with the cycle detection that astconv already does: I left that code in because it gives a more targeted error message, though perhaps less helpful in that it doesn't give the full details of the cycle.
and act more generically.
exclusively stored in the where clauses.
to pass in the appropriate ast::generics etc
compiling something I expect to succeed, and this lets me get stacktraces and also abort compilation faster.
1fb2580
to
1ef3598
Compare
@bors: force |
…ds, r=nikomatsakis This is a fix for #20300 though as a side-sweep it fixes a number of stack overflows because it integrates cycle detection into the conversion process. I didn't go through and retest everything. The tricky part here is that in some cases we have to go find the information we need from the AST -- we can't use the converted form of the where-clauses because we often have to handle something like `T::Item` *while converting the where-clauses themselves*. Since this is also not a fixed-point process we can't just try and keep trying to find the best order. So instead I modified the `AstConv` interface to allow you to request the bounds for a type parameter; we'll then do a secondary scan of the where-clauses to figure out what we need. This may create a cycle in some cases, but we have code to catch that. Another approach that is NOT taken by this PR would be to "convert" `T::Item` into a form that does not specify what trait it's using. This then kind of defers the problem of picking the trait till later. That might be a good idea, but it would make normalization and everything else much harder, so I'm holding off on that (and hoping to find a better way for handling things like `i32::T`). This PR also removes "most of" the `bounds` struct from `TypeParameterDef`. Still a little ways to go before `ParamBounds` can be removed entirely -- it's used for supertraits, for example (though those really ought to be converted, I think, to a call to `get_type_parameter_bounds` on `Self` from within the trait definition). cc @jroesch Fixes #20300
Can this be the source of bug #22841? My code also broke on a different error that I haven't reported as a bug because I'm not sure if it is: In this case, the trait
Is this really a recursive type? |
This is a fix for #20300 though as a side-sweep it fixes a number of stack overflows because it integrates cycle detection into the conversion process. I didn't go through and retest everything.
The tricky part here is that in some cases we have to go find the information we need from the AST -- we can't use the converted form of the where-clauses because we often have to handle something like
T::Item
while converting the where-clauses themselves. Since this is also not a fixed-point process we can't just try and keep trying to find the best order. So instead I modified theAstConv
interface to allow you to request the bounds for a type parameter; we'll then do a secondary scan of the where-clauses to figure out what we need. This may create a cycle in some cases, but we have code to catch that.Another approach that is NOT taken by this PR would be to "convert"
T::Item
into a form that does not specify what trait it's using. This then kind of defers the problem of picking the trait till later. That might be a good idea, but it would make normalization and everything else much harder, so I'm holding off on that (and hoping to find a better way for handling things likei32::T
).This PR also removes "most of" the
bounds
struct fromTypeParameterDef
. Still a little ways to go beforeParamBounds
can be removed entirely -- it's used for supertraits, for example (though those really ought to be converted, I think, to a call toget_type_parameter_bounds
onSelf
from within the trait definition).cc @jroesch
Fixes #20300