Issue 46976 #47083
Issue 46976 #47083
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -1009,7 +1010,7 @@ impl<'a> LoweringContext<'a> { | |||
| hir::TyPath(hir::QPath::Resolved(None, P(hir::Path { | |||
| span, | |||
| def: Def::TyParam(DefId::local(def_index)), | |||
| segments: vec![].into(), | |||
| segments: hir_vec![hir::PathSegment::from_name(name)], | |||
There was a problem hiding this comment.
I'm intrigued about the implications elsewhere in the compiler of having a path with a single path segment of impl Bound1 + Bound2, including that it'd be different from impl Bound2 + Bound1 (but that is already an issue elsewhere, IIRC) and the high likelihood of code not expecting this.
That being said, the code looks otherwise correct. Deferring to @cramertj and @arielb1, @nikomatsakis as they have more context, but it LGTM.
There was a problem hiding this comment.
@estebank It's also interesting because there can be multiple different types with the same name (since fn foo(_: impl Into<u8>, _: impl Into<u8>) { ... } desugars to something like fn foo<"impl Into<u8>": Into<u8>, "impl Into<u8>": Into<u8>>(...) { ... }). I think it should be fine, though.
There was a problem hiding this comment.
That is a good point as well. If there isn't already, I can add a compile-pass test for the fn f(impl Trait, impl Trait) with two distinct types to the two arguments. That'll make sure it works.
|
This is a bit messy because I did it on my phone https://play.rust-lang.org/?gist=fa9a03daaccf45929044537529eb2f46&version=nightly This does work on current nightly rustc, but breaks clippy (Path must have at least one segment). There is obviously code expecting no empty paths, which argument position impl trait produces without this patch. I guess the question is whether the synthetic path to the synthetic type name/generic parameter is more or less harmful than the empty path. I personally have convinced myself that having the path is the correct solution (though to be fully honest I wasn't 100% sure when filling the PR). |
kennytm
added
the
S-waiting-on-review
|
@bors r+ This seems fine to me. |
|
📌 Commit 077ba8a has been approved by |
|
I do sort of want to change the path to some form of enum or something else, but that seems like an orthogonal thing. |
Issue 46976 ICE is due to an empty path segments, so I set the path to be the same as the in band ty params symbol. (I think this is how regular generics end up being handled?) Pinging @cramertj, this is your code I'm editing here.
|
☀️ Test successful - status-appveyor, status-travis |
ICE is due to an empty path segments, so I set the path to be the same as the in band ty params symbol. (I think this is how regular generics end up being handled?)
Pinging @cramertj, this is your code I'm editing here.