Skip to content

fix #57749, model ccall binding access in inference #58872

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeffBezanson
Copy link
Member

I've played around with ccall semantics a bunch (#57931) but concluded that a bandaid like this is far better for 1.12.

fixes #57749

@JeffBezanson JeffBezanson added this to the 1.12 milestone Jul 1, 2025
@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug backport 1.12 Change should be backported to release-1.12 labels Jul 1, 2025
@topolarity topolarity requested a review from Keno July 1, 2025 23:13
@JeffBezanson
Copy link
Member Author

JeffBezanson commented Jul 2, 2025

So we basically need a custom interpreter here to handle the non-standard IR in ccall names, which I have tried to do but am flailing a bit 😄 It doesn't really need to be "correct" and its value is not used. It just needs to be enough for inference to tell which bindings are accessed.

So far I only have special support for nests of calls, which handles many cases like A.B.C but I wonder if there are any other syntaxes used out there?

@KristofferC KristofferC mentioned this pull request Jul 8, 2025
60 tasks
@Keno Keno added the needs pkgeval Tests for all registered packages should be run with this change label Jul 10, 2025
@Keno
Copy link
Member

Keno commented Jul 10, 2025

One inline comment and I think we need to pkgeval this to see if there's any corner cases, but I think as a workaround, this seems reasonable.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests()

@Keno
Copy link
Member

Keno commented Jul 16, 2025

@KristofferC already pkgeval'd this in #58987

@JeffBezanson
Copy link
Member Author

This PR wasn't linked nor the tag removed. In any case that run was not too useful since there were assertion failures from the other PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ccall sometimes fails to statically evaluate arguments due to binding ages
2 participants