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
Allow using fn pointers in const fn with unleash miri #64635
Allow using fn pointers in const fn with unleash miri #64635
Changes from 1 commit
9cf9030
d434496
9d4053f
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.
Since the feature is inherently broken, maybe just do the unleash part and no feature gate?
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.
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 other errors do not follow this style. cc @oli-obk
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 other errors are wrong then.
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.
Agreed, but I'd rather not touch them before #64470 is merged
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 think that PR also enables function pointer calling behind unleash
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 this is the issue that we chatted about. Note that
bar
can be called at run-time, but it will always fail with a constant evaluation error when called in a const context because it tries to call a non-const fn.I'm not sure how to generate an error in this case. Right now it generates a constant-evaluation error when used in const-context, but it would be nice to generate a better error message.
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.
XD welcome to RFC territory. This needs an RFC, because we need surface language changes in order to differentiate between callable and not callable function pointers.
This is what I meant by "we can do it behind a feature gate for experimentation, but we'll never be able to stabilize it.". For all intents and purposes, this is const unsound just like casting pointers to usize or comparing pointers. The pointer ops have been made unsafe for this reason, so maybe we make this unsafe, too? Idk. Seems all fishy without an RFC but as an experimental unstable feature seems fine
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 I've updated the OP with a description of the semantics that this PR implements. I'm not sure why this would be
const
unsound. Theconst fn
taking pointers should be parametric over the pointerconst
ness, and we should be able to reject that at type checking time without new syntax AFAICT.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.
Sure, that would be one way to do it, it would mean you can't pass any function pointers, because there are no const function pointers. Not compiling is the current semantics and is totally sound. This is all discussed in my RFC and needs its own RFC. I'm 🤧 right now and my brain is mush. I'd prefer not to write out an explanation right now.
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 I see what you mean now.
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 am not sure if we need language syntax for this initially, e.g., we could just add a type that users cannot write, add coercions to it from
const fn
s, and then do the const parametricity part. But that's 99% of the work, and at that point not allowing people to write down the type feels like unnecessary.Either way, the intent of this PR was never to go through all that trouble.
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 dead_code warnings are off-topic and shouldn't be here.
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 and Z should not type-check.
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 is the same issue mentioned in the OP and here and should be fixed by fixing that as well.