-
Notifications
You must be signed in to change notification settings - Fork 13.5k
impl trait
in bindings (feature: impl-trait-existential-types)
#53542
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
Conversation
Seems to be blocked on #53469 at the moment. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I don't see anything in the errors. All I see is some diagnostic message changes. Can you bless those and give me some more info? |
@oli-obk You're right. I was too cursory in looking at those. Are the changes a bad thing necessarily? I'm not sure how I even caused them. Anyway, do please elaborate on how I can give you more info. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/hir/lowering.rs
Outdated
@@ -1224,7 +1254,7 @@ impl<'a> LoweringContext<'a> { | |||
t.span, | |||
E0562, | |||
"`impl Trait` not allowed outside of function \ | |||
and inherent method return types" | |||
and inherent method return types or bindings" |
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.
this caused the changes ;)
☔ The latest upstream changes (presumably #53384) made this pull request unmergeable. Please resolve the merge conflicts. |
@oli-obk @eddyb Feedback would be greatly appreciated at this point. I get this error now:
The test file is simply: fn main() {
let x: impl PartialEq<i32> = 123_i32;
} Note that if instead of the following line in code: let c_ty = self.fcx.inh.infcx.canonicalize_response(&o_ty); I do this: let c_ty = self.fcx.inh.infcx.canonicalize_response(&revealed_ty); then things work, although |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I think the error you were getting was related to the initializer value of |
also: your rebase has gone very wrong |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_typeck/check/mod.rs
Outdated
self.fcx.tables.borrow_mut().user_provided_tys_mut().insert(ty.hir_id, c_ty); | ||
|
||
Some(o_ty) | ||
Some(revealed_ty) |
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.
I think you need to track both o_ty
and revealed_ty
here, and add a special check so that assigning to the variable is only allowed with revealed_ty
and reading from the variable is only allowed with o_ty
.
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.
Probably very related to what the assign
method on self
does.
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.
Good idea. It occurred to me before in fact, but I was hoping there may be a simpler way... I think you're right in that it's necessary though.
@oli-obk New WIP version pushed. The problem seems to be that |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Okay, this is my declaration that I'm officially giving up on this, at least until someone provides in @rust-lang/lang provides can provide proper mentorship. (I thank @oli-obk for his efforts to guide me so far, but I'm afraid I need more low-level guidance. MIR officially makes everything here an unholy mess, which I don't see any way around.) e.g. The current main obstacle is this error:
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #54000) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc_typeck/check/mod.rs
Outdated
debug!("visit_local: ty.hir_id={:?} o_ty={:?} c_ty={:?}", ty.hir_id, o_ty, 1); | ||
|
||
let revealed_ty = if self.fcx.tcx.features().impl_trait_in_bindings { | ||
self.fcx.require_type_is_sized(o_ty, ty.span, traits::SizedReturnType); |
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.
Is this line needed/right, @oli-obk? If so, is traits::SizedReturnType
the right value of argument to pass?
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 line is needed. not sure about the third arg, what other values are possible?
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.
@cramertj Advice on this, specifically the value of the third arg?
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.
This looks correct to me.
Edit: scratch that-- after looking further, I don't think we need this? @oli-obk why do we need to force that the type of the local is sized? Is this just to prevent unsized locals? I'd think that whatever existing protections we have against unsized rvalues would be sufficient there (especially since we're now working to allow unsized rvalues).
@oli-obk Latest version looking pretty good now. Just three failures, and I think two are nothing to do with me (could be happening on nightly?). Do you have any idea about the source of the |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@Aaronepower This is no longer blocked. |
@cramertj All tests passing. LGTM! |
@@ -0,0 +1,35 @@ | |||
error[E0434]: can't capture dynamic environment in a fn item |
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.
Nit: these error messages are pretty misleading. Can you open an issue about improving these?
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.
I agree, it could definitely be clearer. I figured out what it meant immediately, but probably only because I've been working on this for a while. Will open an issue now.
Awesome! Looks good apart from the one error message, but we can tackle that in a followup. @bors r+ |
📌 Commit 16cf404 has been approved by |
@cramertj Okay great, thanks! |
`impl trait` in bindings (feature: impl-trait-existential-types) This PR enables `impl Trait` syntax (opaque types) to be used in bindings, e.g. * `let foo: impl Clone = 1;` * `static foo: impl Clone = 2;` * `const foo: impl Clone = 3;` This is part of [RFC 2071](https://github.com/rust-lang/rfcs/blob/master/text/2071-impl-trait-existential-types.md) ([tracking issue](#34511)), but exists behind the separate feature gate `impl_trait_in_bindings`. CC @cramertj @oli-obk @eddyb @Centril @varkor
☀️ Test successful - status-appveyor, status-travis |
cc @nikomatsakis I believe this PR caused #54593, can you take a look at the NLL changes? |
@@ -883,6 +891,23 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { | |||
) | |||
} | |||
|
|||
fn sub_types_or_anon( |
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.
Is "anon" here supposed to be opaque
?
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.
Yes, it should be.
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.
Anyway this PR is already merged, so best to open a new one with these changes.
fn eq_opaque_type_and_type( | ||
&mut self, | ||
revealed_ty: Ty<'tcx>, | ||
anon_ty: Ty<'tcx>, |
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.
Similar here.
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.
I think anon_ty
as a parameter name fits in with the naming scheme elsewhere, but not sure...
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.
That naming scheme probably got missed by the anon -> opaque mass rename.
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.
Probably yep.
None => return Ok(()), | ||
}; | ||
|
||
// Finally, if we instantiated the anon types successfully, we |
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.
And in this comment.
This PR enables
impl Trait
syntax (opaque types) to be used in bindings, e.g.let foo: impl Clone = 1;
static foo: impl Clone = 2;
const foo: impl Clone = 3;
This is part of RFC 2071 (tracking issue), but exists behind the separate feature gate
impl_trait_in_bindings
.CC @cramertj @oli-obk @eddyb @Centril @varkor