Finalizers take function as first argument (closes #16307) #24605

oxinabox · 2017-11-14T15:09:43Z

This closes #16307
Which is a good idea, according to #19150 (comment)

If you do not think this is a good idea, because you think #19150 is wrong, I think that #19150 is the correct place for that debate (rather than this PR).

If finalizer really is going to be deprecated outright, in favor of close (something I'm not really keen on).
Then I guess this PR can be closed. (as can #16307 ?)

This is a breaking change, and the deprecation don't quiet cover all the cases.
(Though I doubt many of the uncovered cases exist in the wild).
Also some of the cases finaliser(::Ptr{Void}, ::Ptr{Void}) will fail silently or segfault (though idk if that is legal code in the first place), until updated to swap their argument order.
There are no ambiguities in the code.

It needs NEWS.jl, but I'll write that later.

(
Past versions of this PR:
#8775,
#12578
)

yuyichao · 2017-11-14T16:00:04Z

If we are really going to make this non-sense change, we must document in the docstring of finalizer that the user must not use an anonymous function as finalizer.

StefanKarpinski · 2017-11-14T16:16:53Z

Please pay no attention to @yuyichao – he has been against this change for no explicable reason from the beginning. However, he is the only one who is so vehemently against fixing this lone exception to the "function arguments go first" rule. @yuyichao, please feel free to make a PR adding that to the documentation for finalizer. Thank you for doing this, @oxinabox!

ararslan · 2017-11-14T19:47:19Z

base/deprecated.jl

+@deprecate finalizer(o, f::Function) finalizer(f, o)
+# This misses other callables but they are very rare in the wild
+
+@deprecate finalizer(o::T, f::Ptr{Void}) where {T} finalizer(f, o)


T where T is Any, which is the same as applying no type annotation to the argument, so this could be simplified to match the deprecation above it.

ararslan · 2017-11-15T18:31:07Z

NEWS.md

+    finalized as its second (rather than the reverse). For the majorities of uses cases this 
+	will trigger deprecation warnings. However, it will not if the 'function' is not a 
+	subtype of `Function`, nor will it for the ambiguous cases where both arguments are 
+	`Function`s or `Ptr{Void}`s ([#24605]).


The indentation here is all funky. All should be aligned beneath the first ` before "finalizer" or it won't render properly.

doh,
my editor has failed to detect that this is a space indented file, and so the tab key has inserted tabs.
I need to sort that out. (both in that file, and in my vim plugin configurations)

Sacha0 · 2017-11-21T01:39:19Z

NEWS.md

+  * `Base.find_in_path` is now `Base.find_package` or `Base.find_source_file` ([#24320]).
+
+  * `finalizer` now takes functions or pointers as its first argument, and the object being 
+    finalized as its second (rather than the reverse). For the majorities of uses cases this 


"For the majority of use cases, this change will..."

Sacha0 · 2017-11-21T01:42:16Z

NEWS.md

+
+  * `finalizer` now takes functions or pointers as its first argument, and the object being 
+    finalized as its second (rather than the reverse). For the majorities of uses cases this 
+    will trigger deprecation warnings. However, it will not if the 'function' is not a 


"However, this change will not trigger deprecation warnings where: (1) the callable argument is not a subtype of Function; or (2) both arguments are Functions or Ptr{Void}s ..."

Sacha0

lgtm! Thanks @oxinabox! :)

@ararslan @StefanKarpinski, is this pull request in shape to merge by you?

ararslan · 2017-11-21T02:19:43Z

NEWS.md

-  * `Base.find_in_path` is now `Base.find_package` or `Base.find_source_file` ([#24320])
+  * `Base.find_in_path` is now `Base.find_package` or `Base.find_source_file` ([#24320]).
+
+  * `finalizer` now takes functions or pointers as its first argument, and the object being 


All of these lines have trailing whitespace, which is causing the CI failures. (It looks like we forgot to set up make check-whitespace on Circle CI.)

oxinabox · 2017-11-21T02:40:29Z

Fixed the whitespace.
Made new phrasing inspired by @Sacha0's feedback.

Sacha0 · 2017-11-21T02:49:16Z

NEWS.md

-    will trigger deprecation warnings. However, it will not if the 'function' is not a 
-    subtype of `Function`, nor will it for the ambiguous cases where both arguments are 
+  * `finalizer` now takes functions or pointers as its first argument, and the object being
+    finalized as its second (rather than the reverse). For the majorities of use cases


"majorities" -> "majority"?

Sacha0 · 2017-11-21T02:49:49Z

NEWS.md

-    subtype of `Function`, nor will it for the ambiguous cases where both arguments are 
+  * `finalizer` now takes functions or pointers as its first argument, and the object being
+    finalized as its second (rather than the reverse). For the majorities of use cases
+    deprecation warnings will be triggered. However, deprecation warning will not trigger where:


"warning" -> "warnings"?

Sacha0 · 2017-11-21T02:50:12Z

NEWS.md

+  * `finalizer` now takes functions or pointers as its first argument, and the object being
+    finalized as its second (rather than the reverse). For the majorities of use cases
+    deprecation warnings will be triggered. However, deprecation warning will not trigger where:
+    (1) the callable argument is not a subtype of `Function`; or (2) when both arguments are


"when" redundant with the "where" preceding the colon?

ararslan

Should be squashed on merge. Thanks for doing this!

Sacha0

Thanks @oxinabox! :)

rfourquet · 2017-11-21T16:34:14Z

we must document in the docstring of finalizer that the user must not use an anonymous function as finalizer.

Can someone explain this to me (with an example of what must not be done), as I absolutly don't understand what is the impact of the argument order on what can be allowed as arguments.

StefanKarpinski · 2017-11-21T21:37:08Z

It's totally unrelated, it's just a (imo bogus) argument against fixing the argument order.

ararslan · 2017-11-21T21:54:33Z

I'm still confused though. Regardless of argument order, Yichao mentioned that finalizers shouldn't use anonymous functions, and yet this change shows that at least half of all uses of finalizer in Base are using anonymous functions. I'm not trying to argue or advocate one way or another, but argument order completely aside, @yuyichao could you explain what the issue is with anonymous functions?

StefanKarpinski · 2017-11-21T21:58:03Z

Right, so the current argument order is clearly in no way preventing anyone – including us – from passing anonymous functions to finalizer. Hence my statement that these are unrelated.

oxinabox · 2017-11-22T01:27:38Z

... this change shows that at least half of all uses of finalizer in Base are using anonymous functions.

Not only are half the uses in base using anon functions, there are at least a couple that are using closures over local variables that are not directly stored in the objects fields.
Which would require more serious rewriting.

StefanKarpinski · 2017-11-22T10:00:22Z

It would be good to have an explanation of why passing anonymous functions to finalizer is bad. If it's considered bad enough, should we fix it in Base? Apparently it's not that bad since we're doing it all over the place.

rfourquet · 2017-11-22T10:06:51Z

from #16307 (comment) :

Base itself currently uses anonymous function as second arg of finalizer in a few places (e.g. in "regex.jl")

To which @yuyichao already answered:

So those should be fixed.

StefanKarpinski · 2017-11-22T11:24:45Z

But why? Are they "bad" in the sense of:

They are broken and will cause programs to fail.
They are will cause the runtime to be slower.
@yuyichao does not personally approve of them.

If (1) is the case then yes, we should definitely fix all of them and probably make finalizer fail when given an anonymous function or closure as the cleanup function. If (2) is the case then we may want to make an effort to change them, but maybe not depending on how bad the impact is. If (3) is the case then without further explanation, I don't see any reason to change anything here.

oxinabox · 2017-11-22T11:35:14Z

I think it is:
4. Finalizers normally represent the action required to free some external resource, and users want a way to perform them determanistically.

For example: malmaud/TensorFlow.jl#342 (comment)
was required because users of TensorFlow.jl wanted a way to guarantee that their GPU RAM was no-longer being consumed by tensorflow.

And things like sess=nothing; gc()
are not good enough, as they are not very determanistic https://stackoverflow.com/a/44503038/179081.

I'm not sure why finalize(sess) isn't good enough, except perhaps the notion that it unclearly leaves sess in an invalid state.

rfourquet · 2017-11-22T11:40:35Z

A bit more digging into #16307 to try to understand Yichao's points, which seem to confirm point 4.:

The usecase (do block) should not be used and must not be encouraged as finalizer. Doing that is usually a sign that the library provide no safe way to explicitly free external resources.

And:

it's a bad idea to not close resources explicitly. We can have more deterministic destruction syntax but that won't be a finalizer.

So I understand it's a matter of style (3.) rather than technical incorrectness (1. or even 2.).

martinholters · 2017-11-22T12:54:59Z

But how does replacing an anonymous function (or do block) with a named function help with 4?

oxinabox · 2017-11-22T13:45:21Z

But how does replacing an anonymous function (or do block) with a named function help with 4?

It doesn't. It neither helps nor hinders.
Which is why it was done regardless.

I would suggest that the problem only exists for resources that are considered external.
And its not a clearly line as to what is an external resource and what is not an external resource.
Is some state that lives in a Ptr to a C object any more external, than say some state that lives in a closure?
Or some state that is say the parent string of a Substring. (Something that has caused me to "leak" 4GB memory to keep hold of a 10 character word)

StefanKarpinski · 2017-11-22T14:26:37Z

This leaves us with this conclusion:

There is nothing actually wrong with using anonymous functions or closures for finalizers.
It may be an API code smell in cases when the finalizer frees an external resource, since the user may want to explicitly finalize the resource, in which case a close function should exist and that close function is precisely the function you'd pass to finalizer.

Making it slightly syntactically less convenient to pass an anonymous function for this reason is an extremely dubious reason to violate the conventional argument order. It is clearly not effective as a preventative measure since we are already passing anonymous functions to finalizer all over the place in Base. It is also unwarranted since there are situations where the user may want to pass an anonymous function to finalizer.

musm · 2017-11-22T19:31:53Z

I don't understand the issues related to the argument order. However, I think this change is a bit strange and I prefer the previous ordering, since I am used to thinking about attaching a finalizer function to an object and therefore the function I am attaching should come second in argument order to the item I am attaching it to.

To me it is pretty strange to have the object declared in the second argument based on this experience.

An argument to keep the existing order is that there are default resource cleaning functions and that declaring finalize(obj) automatically attaches those function to the obj without specifically naming the function, e.g. a close function on the object (i.e. finalize(obj) means attach finalize(obj, close)). Thus changing the ordering for the two argument method is non-intuitive, from this perspective.

1 argument version finalize(obj) (finalize the object obj)
2 argument version finalize(foo, obj) (does this mean finalize foo with obj ?) (to me this is less clear than the previous way of calling the function) .

However the do syntax functionality is nice with the current ordering (if it is indeed valid to pass anon functions as finalizers, which it seems is discouraged in certain circumstances)

edited for clarity.

StefanKarpinski · 2017-11-22T22:52:16Z

All of those arguments apply to every function that takes a function and applies it to an object – which is way beyond the scope of this discussion.

…24605)" This reverts commit 52d81b0.

yuyichao · 2017-12-13T05:17:59Z

Note that every reply in this comment are repeating my comments in
#16307.
This is also why I don't feel like replying to them instead of simply
preparing for taking actions, i.e. work on PR that will affect this
that was going to be post-1.0.
The suggestion in #16307 (comment)
of suggesting the change to style recommendation also won't work
since discussions in the past on this topic has made it very clear
that the same set of abstract argument will simply be repeated and
it just doesn't make sense to issue a recommendation based on a feature without a more concrete plan.
(OTOH, it makes sense to hold back action so that the change can be done later without breakage.)

As a summary, the original order makes it possible to change the
recommendation without breakage. The feature of namespaced method
lookup would have been added without breakage in base.
Certain functions may need to be updated to follow
the new recommendation but finalizer is the only Core base function
that need to be changed and is likely the only one that's
ambiguous. With this change, the non-breaking change to the style
recommendation is not possible anymore which make it important
to get the simple form of the feature in before 1.0.

as I absolutly don't understand what is the impact of the argument order on what can be allowed as arguments.

Note that I was following the rule in the top post and I'm only commenting on the effect of the PR.
My comment in this thread did not mention if this should be done (it shouldn't but that has been explained in other threads) but this PR (as proved by the arguments used in #16307) will very much encourage the use of anonymous functions as finalizers.
Therefore documents should be changed to avoid that.
(Do note that the method lookup PR will still make the do syntax possible.)

Right, so the current argument order is clearly in no way preventing anyone – including us – from passing anonymous functions to finalizer. Hence my statement that these are unrelated.

Correct, the previous comment in this thread is not at all about
why not to do this.

Apparently it's not that bad since we're doing it all over the place.

For things that are not actually used (created) much, sure.
Using all over the place in base is never an argument for
how good/bad it is.

Finalizers normally represent the action required to free some external resource, and users want a way to perform them determanistically.

Correct.

So I understand it's a matter of style (3.) rather than technical incorrectness (1. or even 2.).

Well, it's not incorrectness in that the code won't crash if you have infinite memory. It's technical in that it'll make it impossible to write correct/robust code. It's not just style since it makes a difference what you can and can't do as the user.

All of those arguments apply to every function that takes a function and applies it to an object – which is way beyond the scope of this discussion.

No. It only applies to functions that has clearly an object to operate on that happens to take a function as argument as well. (i.e. map is not one of them but finalizer is).
And then yes, for those functions it is a broader issue and as mentioned before,
such functions are not currently special but will likely be special in the future.
What make finalizer special among those functions is that it's the only one that can't be changed without breakage.

…24605)" This reverts commit 52d81b0.

…16307) (JuliaLang#24605)" This reverts commit 52d81b0.

oxinabox added 2 commits November 14, 2017 23:00

=make finalizers take function as first argument

b0bf9c2

=update all files so finalizers take functions as their first argument

747444b

ararslan reviewed Nov 14, 2017

View reviewed changes

ararslan added the needs news A NEWS entry is required for this change label Nov 14, 2017

oxinabox added 2 commits November 15, 2017 08:17

=remove pointless type constraint

1534c38

=add news for JuliaLang#24605

687e86a

ararslan reviewed Nov 15, 2017

View reviewed changes

=spaces not tabs

1c3b17a

ararslan removed the needs news A NEWS entry is required for this change label Nov 17, 2017

Sacha0 reviewed Nov 21, 2017

View reviewed changes

Sacha0 approved these changes Nov 21, 2017

View reviewed changes

ararslan approved these changes Nov 21, 2017

View reviewed changes

ararslan requested changes Nov 21, 2017

View reviewed changes

=fix whitespacing and phrasing

03ff4fb

Sacha0 reviewed Nov 21, 2017

View reviewed changes

oxinabox added 2 commits November 21, 2017 11:04

=writing news

f94d5c9

=writing news

9516d83

ararslan approved these changes Nov 21, 2017

View reviewed changes

Sacha0 approved these changes Nov 21, 2017

View reviewed changes

StefanKarpinski approved these changes Nov 21, 2017

View reviewed changes

Sacha0 merged commit 52d81b0 into JuliaLang:master Nov 21, 2017

Sacha0 mentioned this pull request Nov 21, 2017

22018: make finalizer(f, x) return x rather than nothing #24679

Merged

oxinabox deleted the ox/finalizerfunctionfirst branch November 22, 2017 13:45

yuyichao added a commit that referenced this pull request Dec 13, 2017

Revert "Finalizers take function as first argument (closes #16307) (#…

f7c6095

…24605)" This reverts commit 52d81b0.

yuyichao added a commit that referenced this pull request Dec 15, 2017

Revert "Finalizers take function as first argument (closes #16307) (#…

f602163

…24605)" This reverts commit 52d81b0.

yuyichao added a commit that referenced this pull request Dec 21, 2017

Revert "Finalizers take function as first argument (closes #16307) (#…

ff2c4ae

…24605)" This reverts commit 52d81b0.

jgoldfar pushed a commit to jgoldfar/julia that referenced this pull request Jan 9, 2018

Revert "Finalizers take function as first argument (closes JuliaLang#…

f9b36f3

…16307) (JuliaLang#24605)" This reverts commit 52d81b0.

ChrisRackauckas mentioned this pull request Jun 1, 2018

The upgrade to Julia 1.0 SciML/DifferentialEquations.jl#271

Closed

oxinabox mentioned this pull request Aug 24, 2018

Work in progress in migrating the TensorFlow.jl into the Julia 1.0.0 malmaud/TensorFlow.jl#419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalizers take function as first argument (closes #16307) #24605

Finalizers take function as first argument (closes #16307) #24605

oxinabox commented Nov 14, 2017 •

edited

Loading

yuyichao commented Nov 14, 2017

StefanKarpinski commented Nov 14, 2017 •

edited

Loading

ararslan Nov 14, 2017

ararslan Nov 15, 2017

oxinabox Nov 16, 2017

Sacha0 Nov 21, 2017

Sacha0 Nov 21, 2017

Sacha0 left a comment

ararslan Nov 21, 2017

oxinabox commented Nov 21, 2017

Sacha0 Nov 21, 2017

Sacha0 Nov 21, 2017

Sacha0 Nov 21, 2017

ararslan left a comment

Sacha0 left a comment

rfourquet commented Nov 21, 2017

StefanKarpinski commented Nov 21, 2017

ararslan commented Nov 21, 2017

StefanKarpinski commented Nov 21, 2017 •

edited

Loading

oxinabox commented Nov 22, 2017

StefanKarpinski commented Nov 22, 2017

rfourquet commented Nov 22, 2017

StefanKarpinski commented Nov 22, 2017 •

edited

Loading

oxinabox commented Nov 22, 2017

rfourquet commented Nov 22, 2017

martinholters commented Nov 22, 2017

oxinabox commented Nov 22, 2017 •

edited

Loading

StefanKarpinski commented Nov 22, 2017

musm commented Nov 22, 2017 •

edited

Loading

StefanKarpinski commented Nov 22, 2017

yuyichao commented Dec 13, 2017

Finalizers take function as first argument (closes #16307) #24605

Finalizers take function as first argument (closes #16307) #24605

Conversation

oxinabox commented Nov 14, 2017 • edited Loading

yuyichao commented Nov 14, 2017

StefanKarpinski commented Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan left a comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

rfourquet commented Nov 21, 2017

StefanKarpinski commented Nov 21, 2017

ararslan commented Nov 21, 2017

StefanKarpinski commented Nov 21, 2017 • edited Loading

oxinabox commented Nov 22, 2017

StefanKarpinski commented Nov 22, 2017

rfourquet commented Nov 22, 2017

StefanKarpinski commented Nov 22, 2017 • edited Loading

oxinabox commented Nov 22, 2017

rfourquet commented Nov 22, 2017

martinholters commented Nov 22, 2017

oxinabox commented Nov 22, 2017 • edited Loading

StefanKarpinski commented Nov 22, 2017

musm commented Nov 22, 2017 • edited Loading

StefanKarpinski commented Nov 22, 2017

yuyichao commented Dec 13, 2017

oxinabox commented Nov 14, 2017 •

edited

Loading

StefanKarpinski commented Nov 14, 2017 •

edited

Loading

StefanKarpinski commented Nov 21, 2017 •

edited

Loading

StefanKarpinski commented Nov 22, 2017 •

edited

Loading

oxinabox commented Nov 22, 2017 •

edited

Loading

musm commented Nov 22, 2017 •

edited

Loading