-
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
Point at associated type for some obligations #65288
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
447f3ff
to
c808d66
Compare
This comment has been minimized.
This comment has been minimized.
I don't think I'm the best reviewer for this, but I'm not sure who is. |
c808d66
to
4c3d9fa
Compare
Maybe @nikomatsakis? |
This comment has been minimized.
This comment has been minimized.
@bors try |
Point at associated type for some obligations Partially address #57663.
☀️ Try build successful - checks-azure |
@rust-timer build afb10f4 |
Queued afb10f4 with parent 000d90b, future comparison URL. |
Finished benchmarking try commit afb10f4, comparison URL. |
Perf looks clean |
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 nice @estebank! I have some minor suggestions around refactoring.
src/test/ui/associated-types/point-at-type-on-obligation-failure-2.stderr
Outdated
Show resolved
Hide resolved
cbd994b
to
9824ba6
Compare
9824ba6
to
8d4714a
Compare
8d4714a
to
580a93e
Compare
0a7f7b4
to
3a4cacd
Compare
@nikomatsakis let me know if there are any outstanding changes you'd like me to do. |
let obligations = self.nominal_obligations(trait_ref.def_id, trait_ref.substs); | ||
|
||
let cause = self.cause(traits::MiscObligation); | ||
let param_env = self.param_env; | ||
|
||
let item = &self.item; | ||
let extend_cause_with_original_assoc_item_obligation = | |
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.
Heh. Personally I'd have preferred to see this be a separate method, but I can accept this =) I do like the comments!
@bors r+ |
📌 Commit 3a4cacd has been approved by |
⌛ Testing commit 3a4cacd with merge e39dd255a4db06727866034e3a5288ddbf2b8fc4... |
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
💔 Test failed - checks-azure |
It's a network error. |
…omatsakis Point at associated type for some obligations Partially address rust-lang#57663.
Point at associated type for some obligations Partially address #57663.
☀️ Test successful - checks-azure |
@@ -47,8 +50,9 @@ pub fn trait_obligations<'a, 'tcx>( | |||
body_id: hir::HirId, | |||
trait_ref: &ty::TraitRef<'tcx>, | |||
span: Span, | |||
item: Option<&'tcx hir::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.
I don't think passing around references to the HIR fits ty::wf
, this can just be an Option<DefId>
and should probably be named something more suggestive.
let obligations = self.nominal_obligations(trait_ref.def_id, trait_ref.substs); | ||
|
||
let cause = self.cause(traits::MiscObligation); | ||
let param_env = self.param_env; | ||
|
||
let item = &self.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.
No need to borrow a Copy
field.
Partially address #57663.