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

remove Core._apply Builtin #39115

Merged
merged 1 commit into from
Jan 7, 2021
Merged

remove Core._apply Builtin #39115

merged 1 commit into from
Jan 7, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 6, 2021

We can emulate this deprecated function, until we delete it in v2.
This in turn revealed some missing error checks, which we also add.

Fixes #39113

@vtjnash vtjnash added the needs pkgeval Tests for all registered packages should be run with this change label Jan 6, 2021
@vtjnash vtjnash requested a review from Keno January 6, 2021 03:37
We can emulate this deprecated function, until we delete it in v2.
This in turn revealed some missing error checks, which we also add.

Fixes #39113
@maleadt
Copy link
Member

maleadt commented Jan 6, 2021

@nanosoldier runtests(ALL, vs = ":master")

@vtjnash vtjnash removed the needs pkgeval Tests for all registered packages should be run with this change label Jan 6, 2021
@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@StefanKarpinski
Copy link
Member

This seems like it should be removable: it's not exported and it starts with _; how much more internal can a function be? It would be good to fix any packages that removing it breaks, but deleting is definitely not disallowed by semver.

@simeonschaub
Copy link
Member

Should this also be backported then?

@simeonschaub
Copy link
Member

This seems like it should be removable: it's not exported and it starts with _; how much more internal can a function be? It would be good to fix any packages that removing it breaks, but deleting is definitely not disallowed by semver.

While it might not be covered by semver, there are still a lot of packages using it: https://juliahub.com/ui/CodeSearch?q=_apply&u=all&t=all. I don't see any issues with just keeping it for 1.x, so I don't think there's reason enough to break a bunch of code by removing it.

@Keno
Copy link
Member

Keno commented Jan 6, 2021

Yeah, we can't remove it. This fallback might be ok though.

@StefanKarpinski
Copy link
Member

Why can't we remove it?

@Keno
Copy link
Member

Keno commented Jan 6, 2021

Because it would be extremely breaking for no reason. This used to be inserted by the frontend, so it has propagated far and wide. It's also inside things like serialized closures inside Flux model files.

@vtjnash
Copy link
Member Author

vtjnash commented Jan 6, 2021

Looks great. The only failure this causes is Zygote, where we fixed a test_broken. GTG.

@vtjnash vtjnash added the backport 1.6 Change should be backported to release-1.6 label Jan 6, 2021
@JeffBezanson JeffBezanson merged commit 59eb9f9 into master Jan 7, 2021
@JeffBezanson JeffBezanson deleted the jn/39113 branch January 7, 2021 16:57
@KristofferC
Copy link
Member

Doesn't backport cleanly and my attempts at fixing it still errors. Can push a rebased commit to #39160.

@KristofferC KristofferC mentioned this pull request Jan 19, 2021
60 tasks
@KristofferC KristofferC mentioned this pull request Feb 1, 2021
10 tasks
@KristofferC KristofferC mentioned this pull request Feb 11, 2021
52 tasks
vtjnash added a commit that referenced this pull request Feb 16, 2021
We can emulate this deprecated function, until we delete it in v2.
This in turn revealed some missing error checks, which we also add.

Fixes #39113

(cherry picked from commit 59eb9f9)
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Mar 14, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
We can emulate this deprecated function, until we delete it in v2.
This in turn revealed some missing error checks, which we also add.

Fixes JuliaLang#39113
@laborg laborg mentioned this pull request Feb 8, 2022
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
We can emulate this deprecated function, until we delete it in v2.
This in turn revealed some missing error checks, which we also add.

Fixes #39113

(cherry picked from commit 59eb9f9)
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.

Core._apply emits bad code
8 participants