-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Trait-associated const fixes #25091
Trait-associated const fixes #25091
Conversation
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
Constants with values that depend on generic parameters or `Self` cause ICEs in `check_const`, and are not yet accepted via RFC, so we need to throw a proper error in these cases.
…terns. This allows some lookup of trait-associated consts during type-checking, which may be helpful for future fixes as well.
24157ef
to
98f41ff
Compare
@quantheory argh, I must again ask your apologies for the delay. I'm going over assigned PRs now and I see this one has been sitting for quite a while! |
@@ -233,7 +233,8 @@ struct ConstantExpr<'a>(&'a ast::Expr); | |||
|
|||
impl<'a> ConstantExpr<'a> { | |||
fn eq(self, other: ConstantExpr<'a>, tcx: &ty::ctxt) -> bool { | |||
match const_eval::compare_lit_exprs(tcx, self.0, other.0, None) { | |||
match const_eval::compare_lit_exprs(tcx, self.0, other.0, None, | |||
|id| {ty::node_id_item_substs(tcx, id).substs}) { |
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.
the usual thing here is not to pass a closure, but rather a Typer
-- but I'm not sure off-hand if we have a method getting the substs of an id
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.
Well, the other thing is that const_eval
has to be working already in collect
, and I'm trying to get resolution of trait-associated constants working there. We don't have ClosureTyper
and ParameterEnvironment
in play at that point, and it would be kind of funky to use them that early.
@nikomatsakis I've made the |
@nikomatsakis (Also, on your earlier point, you don't have to apologize. I'll bug you about moving faster or delegating the review if I'm feeling really impatient for some reason, but I know that you have lots of important things to do, and a real life and family and all of that. Your insight is always appreciated, though.) |
@bors r+ This seems ok as a temporary hack. |
📌 Commit 8db699d has been approved by |
…omatsakis Closes #25046 (by rejecting the code that causes the ICE) and #24946. I haven't been able to deal with the array size or recursion issues yet for associated consts, though my hope was that the change I made for range match patterns might help with array sizes, too. This PR is pretty much orthogonal to #25065.
Closes #25046 (by rejecting the code that causes the ICE) and #24946. I haven't been able to deal with the array size or recursion issues yet for associated consts, though my hope was that the change I made for range match patterns might help with array sizes, too.
This PR is pretty much orthogonal to #25065.