-
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
Add "type_name" support in emulate_intrinsic() #61399
Conversation
It was suggested by /u/YatoRust on Reddit that I open a PR for this, so I am.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (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. |
r? @oli-obk |
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 |
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'd love to see a test added to src/test/run-pass/ctfe
as well.
and have considered the implication of these together. Notably, |
"name_val" seem more consistent with the variable naming in the surrounding code.
Added one just now. |
Also, use core::intrinsics::type_name instead of std::intrinsics::type_name, which I don't think makes any real difference but seems like it's probably more logical.
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 |
Hmm, looks like there was a very recent change to what |
Hopefully I didn't miss anything here.
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 *think* this actually does, though!
To be clear, my PR does not include any actual built-in It just allows people who are specifically using nightly to be able to write their own Intrinsics are AFAIK all unstable forever anyways, so I don't think you currently could "stabilize" anything in this regard even if you wanted to. Additionally, I'm not sure that I see a strong relationship between the typecasty runtime dynamism of the |
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.
Additionally, I'm not sure that I see a strong relationship between the typecasty runtime dynamism of the Any trait and what type_name() provides (at compile time, making it viable for high performance debugging / logging / e.t.c. related functions and such.)
😆 the reddit post you linked to shows a typecast example
Not to worry though, I am certain this is fine as an intrinsic.
Though I am 100% behind @Centril in the notion that we shouldn't stabilize a const fn
wrapper for the type_name
intrinsic without an RFC discussing the interaction of this feature together with other const eval features.
I'm not sure how useful this PR is when it will not result in anything stabilizeable for at least a year, but I won't block it on that.
} = const_val | ||
{ | ||
// unsafe for from_utf8_unchecked() | ||
unsafe { |
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.
please get rid of str_to_immediate
, it is only used in miri's implementation of type_name
and thus not needed.
Instead, you can write
let alloc_id = self.tcx.alloc_map.lock().create_memory_alloc(data);
let name_val = Immediate::new_slice(Scalar::Ptr(ptr.into()), data.len(), self);
The code as currently written would be invalid when encountering a ConstValue::Slice
where start != 0
or end - start != data.len()
. The way type_name
is written, that doesn't happen, so... idk, add some comments explaining why that's ok?
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.
Maybe it would also simplify things if you pulled out
rust/src/librustc_mir/interpret/intrinsics/type_name.rs
Lines 216 to 219 in 0f0ad45
let path = AbsolutePathPrinter { tcx, path: String::new() }.print_type(ty).unwrap().path; | |
let len = path.len(); | |
let alloc = Allocation::from_byte_aligned_bytes(path.into_bytes(), ()); | |
let alloc = tcx.intern_const_alloc(alloc); |
pub(super)
visibility) and called that here? That way we'd not have the ConstValue::Slice
wrapper and could simplify the code shown above to
let alloc_id = self.tcx.alloc_map.lock().create_memory_alloc(data);
let name_val = Immediate::new_slice(Scalar::Ptr(ptr.into()), data.len(), self);
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 reddit post you linked to shows a typecast example
Yeah. As I believe I stated in there though, I don't think those are particularly realistic use cases for what someone who is using compile-time intrinsics at all is likely to want to do with this kind of function.
I was quite literally unaware the "breaking parametricity" thing was a concern that anyone had at all before being told about it by a few people in that thread, as the general concept of doing something like that hadn't ever crossed my mind.
With regards to the changes, I'll work on them now (I didn't notice you had replied until right after I sent off the minor commit I just made, haha.)
Thanks for the review!
Also make it more clear we're talking about the `librustc_mir`-specific function called `type_name()` in the bug message.
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 |
(Ignore the last commit, basically.) |
I hope this works like I think it does!
I feel like the function name is good, so just knocking it down a line seems like the right way to go.
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 |
Within `tag_static_base_pointer`, that is. The separate `id_ptr` variable *does* seem to be necessary in general, however. Trying to do `name_id.into()` *directly* inside of `Scalar::Ptr()` in this context fails to compile, as `rustc` wants this super-specific specialization of `Pointer` which it apparently doesn't get from that.
just one more nit then this lgtm. Can you also squash the commits? |
I don't think there is a direct connection to referential transparency here. There is a possibility to violate parametricity for types, but that's nothing new for Rust and specialization will let us do that, too. The type cast example is a typical parametricity violation. There are so many problems with making parametricity violations UB that I don't think that is a realistic option. As a starter, it is entirely uncheckable even dynamically. |
Uh... what?^^ Why did you close this? |
Sorry about that. Kinda got myself into a mess with the commit history. I'm admittedly not super experienced with Git, and was trying to see if there was a reasonable way to "squash" the commit history from the web interface as I don't have access to my local repo with actual Git tools where I am right now. It seems there is not! (At least that I could find.) See here for the reopened issue. |
Looks like the build finished successfully over there. Was there anything else you needed on my end? |
…oli-obk Add "type_name" support in emulate_intrinsic() I did some dumb Git things and deleted my original fork repo semi-accidentally (but probably for the best as I'd messed up the history.) This is the same issue as rust-lang#61399, which was obviously auto-closed, to be clear.
Add "type_name" support in emulate_intrinsic() I did some dumb Git things and deleted my original fork repo semi-accidentally (but probably for the best as I'd messed up the history.) This is the same issue as #61399, which was obviously auto-closed, to be clear.
This allows it to be used in "const fn" contexts.
(It was suggested by /u/YatoRust on Reddit that I open a PR for this, so I am.)