Ice lang item #26910
Ice lang item #26910
Conversation
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| // Test that we don't ICE when we are missing the owned_box lang item. |
There was a problem hiding this comment.
doesn't this file need an error pattern?
There was a problem hiding this comment.
it has one (admittedly it was missing on my first push :-) )
|
Hm, I'm not totally on board with returning |
|
@huonw I'm not 100% sure I agree, seems like if the compiler wants a lang item to be there and it isn't then that ought to be an error, the fact that we work around it sometimes means its still an error. However, I guess the change is only worthwhile if we use the error message from lang items in the errors, which we don't and I don't want to do. So, here is a version without the Result change. The treatment of owned_box becomes a little ad hoc, but then our treatment of owned box seems pretty ad hoc already. |
Many of the lang-item queries are essentially checking whether some type has a certain behaviour, where that behaviour is represented by some lang-item. Hence it's fine if the lang item doesn't exist, it means the type definitely doesn't have that behaviour. |
| @@ -90,6 +90,10 @@ impl LanguageItems { | |||
| } | |||
| } | |||
|
|
|||
| pub fn require_owned_box(&self) -> Result<ast::DefId, String> { | |||
There was a problem hiding this comment.
Other places that do this sort of thing just write the self.require(...) call inline, maybe it's ok to do that here too instead of introducing a special method?
There was a problem hiding this comment.
My reasoning was:
- owned_box is already somewhat special - it doesn't follow the idioms of the others
- we could have done that but didn't, which indicates that we need to do something differently
- the 'right' thing should be at least as ergonomic as the 'wrong' thing (
.owned_box())
There was a problem hiding this comment.
the 'right' thing should be at least as ergonomic as the 'wrong' thing (.owned_box())
Hm, actually, I'm not totally clear why owned_box is wrong? Doesn't it return an Option, and the ICE caused by the unwrap call?
There was a problem hiding this comment.
That's right, but there's not a great way to handle the error unless you get the error message which comes with the require version. I guess you could make up your own message. That's why I use "'right'" rather than "right" - it's not too important. But it is an error if its not there, so Result feels right.
|
(r=me with or without |
|
📌 Commit 7fb8208 has been approved by |
|
⌛ Testing commit 7fb8208 with merge b5939ae... |
|
💔 Test failed - auto-linux-64-x-android-t |
|
@bors: retry |
No description provided.