Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Organize intrinsics const evaluability checks #61835
Organize intrinsics const evaluability checks #61835
Changes from 4 commits
0c2a54d
49fdb57
9f6ef64
21a5963
da6cc10
9484246
34951a9
5354f7f
c41c07b
54127de
07d1419
ab96b18
9328ee0
63cc638
e62acd4
3bc84dd
7ba0a57
ddaf56d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would not have this as a separate function, but rather use it before the usage of
FnLikeNode
, so intrinsics' definitions aren't checked forconst fn
(as they syntactically cannot beconst fn
).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.
Shouldn't we just allow them to be that syntactically?
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.
Didn't you say you tried that once and it did not work?
Also, that would allow nightly users to declare intrinsics as
const
. There might be ways to cause unsoundness there. Wouldn't that be a problem?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.
It would be a lot of work
Well... if there's an implementation in the miri-engine, then they can do whatever unstable fun they want I guess?
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. And that might subvert properties that
const fn
otherwise have.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 only consider this a problem in libstd. If you are using nightly features you can already brick properties that we aren't sure about yet, so...
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.
One of these days we should change intrinsics so they don't abuse
extern "..." {...}
anymore and then we can haveconst fn transmute
or w/e. As for not allowingconst fn
on intrinsics that shouldn't be, I suppose that should go inrustc_typeck
's list of intrinsic signatures.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.
We could just move to making them lang items and having libcore implement them by recursing on themselves and then have the compiler overwrite them. Then they'd be regular functions (and we used this trick on one intrinsic already:
rust/src/libcore/ptr/mod.rs
Line 197 in 605ea9d
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 would make it harder to figure out that a function is an intrinsic. Also
real_drop_in_place
actually codegens to real functions, while intrinsics always inline their code at the caller site. Another difference between intrinsics and (lang item) functions is that intrinsics cant be turned into function pointers, while (lang item) functions can 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.
@oli-obk sigh we don't have to re-use lang items, we can have a different mechanism (but also attribute-based) for them. I've been meaning to do this for ages but @nikomatsakis wanted an RFC which I never got around to writing, oops.
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.
Could we change this to match on a
Symbol
instead? This version is needlessly slow.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.
Also, @_nnethercote has been trying to remove queries that are easy to compute to improve performance. I think this could just be a function.Oh, this isn't actually exported in
providers
. My bad.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 wrapping intrinsics seem to be missing from this list. The errors mean the check is working! Awesome! Please add anything to this list that is needed to get the tests passing
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.
rename
is_const_evaluatable
tois_const_intrinsic
and check it first thing inis_const_fn_raw
. If it'strue
, returntrue
. (as suggested above in /~https://github.com/rust-lang/rust/pull/61835/files#r294078191)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.
Isn't this
IsNotPromotable
? Shouldn't that check therustc_promotable
attribute instead of allowing allconst fn
?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.
Ah dang I might have mixed this up again with
IsNotImplicitlyPromotable
...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.
Note that this can't go wrong anymore, without triggering a mismatch with
promote_consts
' copy of the logic (although we should make sure this PR updates both properly), which would ICE.@ecstatic-morse and I will be removing the copy from
qualify_consts
in a week or two (maybe we just need to wait for beta to branch?), soIsNot(Implicitly) Promotable
is going away soon!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 just hope we get the intrinsic thing landed as part of this refactor, it's been 4 months or so.^^
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.
Oh yeah this PR can land at any time, and I think
promote_consts
even has the right code already, without another copy of the list of intrinsics.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.
Okay, but what about the new const checking pass? Does that still allow calling intrinsics?
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's a good question for @ecstatic-morse.
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 reserved a spot for these checks here. I think we just need to call the
is_const_intrinsic
from that spot? I dunno if this PR makes additional changes to the validation logic though.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 missed that this was baked into
in_const_fn
now (see below), I think this PR just needs to remove thatreturn
along with theFIXME
above it.