Skip to content
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

Better support for FnPtr ... #854

Merged
merged 26 commits into from
Oct 22, 2024
Merged

Better support for FnPtr ... #854

merged 26 commits into from
Oct 22, 2024

Conversation

ranjitjhala
Copy link
Contributor

fixes #807

@ranjitjhala
Copy link
Contributor Author

@enjhnsn2 -- I made some progress here with the fnptr stuff #807 -- can you see if it helps in the vtock or perhaps we need to do more/better?

@enjhnsn2
Copy link
Collaborator

@ranjitjhala -- Awesome, thanks. This pr fixes two of the issues related to FnPtr, and makes the rest into #829 errors. don't think we need any more FnPtr support from what I can see.

Copy link
Member

@nilehmann nilehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this is unsound if you coerce a function definition to a function pointer. We can merge it but please create an issue so we can track the unsoundness

}
mir::CallKind::FnPtr { operand, .. } => {
let ty = self.check_operand(infcx, env, terminator_span, operand)?;
if let Some(BaseTy::FnPtr(fn_sig)) = ty.as_bty_skipping_existentials() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try not to skip existentials if we can avoid it

Suggested change
if let Some(BaseTy::FnPtr(fn_sig)) = ty.as_bty_skipping_existentials() {
if let TyKind::Indexed(BaseTy::FnPtr(fn_sig), _) = ty.kind() {

If ty is not directly a TyKind::Indexed we should unpack it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that if its already an Indexed then unpack will not do anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if it's already Indexed no need to do unpack (it would be a noop)

crates/flux-refineck/src/checker.rs Show resolved Hide resolved
@ranjitjhala ranjitjhala merged commit e722365 into main Oct 22, 2024
7 checks passed
@ranjitjhala ranjitjhala deleted the fnptr branch October 22, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better support for FnPtr
3 participants