Deprecate the 'defer' statement

Araq commented 4 years ago

As the title says, Nim version 2 should get rid of defer. (It is part of version 1.) For these reasons:

It's yet another language feature that we have to maintain and it keeps adding costs as it's buggy. See https://github.com/nim-lang/Nim/issues/4567

Also, defer does not open a scope but try does, see https://github.com/nim-lang/RFCs/issues/218#issuecomment-622240409 It's unclear whether this is better or worse than introducing a new scope.

There is considerable overlap with a try finally statement and Nim's destructors feature.
It doesn't model the idea it tries to model very well. The basic idea here is something like "let's pair do/undo operations" but do(); defer: undo() doesn't do that. Instead undo() is called at the end of the scope regardless of whether do() really did run successfully. We can do this better in today's Nim with a macro like so:


macro transaction(body: untyped): untyped =
  # translate the (do ~> undo) pairs into if + try finally statements
  ... # implementation is an exercise for the reader

proc test {.transaction.} =
  var f: File
  open(f, "test.txt") ~> close(f)

ghost commented 4 years ago

If you check the use of defer via GitHub (excluding the compiler forks) - https://github.com/search?q=language%3Anim+defer&type=Code , then you can see that a lot of these are either just to show how defer works or to close/free objects (which is not needed since we have destructors/try finally)

stefantalpalaru commented 4 years ago

Sorry but, once you add a public keyword/API function, you're bound to maintain to it.

People don't like building their castles on quicksand, so any programming language, framework or library you offer them as a solid foundation for their own software needs to be just that: a solid, backwards-compatible foundation.

Araq commented 4 years ago

@stefantalpalaru IMHO it's acceptable to call for help in an open source project.

stefantalpalaru commented 4 years ago

it's acceptable to call for help in an open source project

Yes, of course, but is it feasible to expect someone removing language keywords or stdlib procedures/functions/macros to fix all the projects using them?

How about commercial projects you don't find with a GitHub search? Should they just stick to an old version?

Araq commented 4 years ago

Well we still got semver in place so this is stuff for Nim version 2.

solo989 commented 4 years ago

Are defer and try finally equivalent?

defer:
  result = x
return

try:
  return
finally:
  result = x

Defer could internally be turned into some kind of transformation to turn the first into the second. Would that be easier to maintain? Or is that what you are already doing?

For that matter if you supported macros that were automatically passed everything below it in the current scope it wouldn't even need to be a keyword. So instead of

deferMacro:
  result = x
do:
  return

you would have

deferMacro:
  result = x
return

though I suppose that would be a different feature to maintain. However you already do something similar with how some pragmas can be applied

{.push pragma.}
{.pop.}

Maybe it could be


{.push defer.}:
  result = x
return
{.pop.}

timotheecour commented 4 years ago

I disagree with this RFC but not for the reasons brought by https://github.com/nim-lang/RFCs/issues/236#issuecomment-646225219

if a feature is bad/broken beyond repair/replaceable by something better, then indeed it should be deprecated from the language otherwise you end up with C++; and we have good ways to deal with deprecations. However this isn't applicable here.

regarding point 1

lots/most of nim features have bugs; these should be fixed, but are not an indicator the feature is bad or should be deprecated, unless it's beyond repair, which isn't the case here.

regarding point 3

I don't understand point 3.

The basic idea here is something like "let's pair do/undo operations" but do(); defer: undo() doesn't do that. Instead undo() is called at the end of the scope regardless of whether do() really did run successfully

@Araq in fact, IIUC that's the same misconception as was written in this thread about defer, see https://forum.nim-lang.org/t/4022#25088

this works as intended:

type Foo = object
  x: int

proc initFoo(x: int): ptr Foo =
  if x >= 10: raise newException(ValueError, "bad x")
  result = create(Foo)
  result.x = x

proc releaseFoo(a: ptr Foo) =
  echo ("releaseFoo", a.x)
  dealloc(a)

proc main()=
  let a = initFoo(12) # if this raises...
  defer: releaseFoo(a) # ... `releaseFoo` correctly won't be called
main()

regarding point 2

There is considerable overlap with a try finally statement and Nim's destructors feature.

anyone interested in forming a solid opinion on this should read the whole thread here https://forum.nim-lang.org/t/4022 which discusses defer vs alternatives.
I particular, scope(exit) is a well-loved feature in D that's extensively used even though D has RAII (with some caveats but unrelated for this discusssion)

see the motivation for this feature in D: https://dlang.org/articles/exception-safe.html

The scope(exit) statement [...] places the unwinding code where it aesthetically belongs, next to the creation of the state that needs unwinding. It's far less code to write than either the RAII or try-finally solutions, and doesn't require the creation of dummy structs.

The RAII solution would be to try and capture the false state of verbose as a resource, an abstraction that doesn't make much sense. The try-finally solution requires arbitrarily large separation between the conceptually linked set and reset code, besides requiring the addition of an irrelevant scope.

Clyybber commented 4 years ago

@solo989 defer: ... is currently transformed into try: ... finally: ...

solo989 commented 4 years ago

It sounds like it's not an exact transformation as otherwise a new scope would be introduced. If it was something more akin to a macro transformation that is passed two code blocks it might be easier to maintain.

Clyybber commented 4 years ago

A new scope is introduced. But you don't notice because it lasts as long as the scope the defer is being used in.

solo989 commented 4 years ago

@Araq undo should be put first in your example

proc test {.transaction.} =
  var f: File
  close(f) ~> 
    open(f, "test.txt")

as undo is probably shorter and its keeps the undo logic closer to the actual declaration of the file variable.

@timotheecour if you wanted not to run close if open fails you would do this instead

proc test {.transaction.} =
var f:File
open(f, "test.txt")
close(f) ~>
  discard

Araq commented 4 years ago

lots/most of nim features have bugs; these should be fixed, but are not an indicator the feature is bad or should be deprecated, unless it's beyond repair, which isn't the case here.

It's not just bugs we have to fix, it's also we have to ensure it keeps working for a decade. There is always a cost/benefit tradeoff and IMO defer clearly falls under "not worth its costs". Yes, this applies to other features, yes, we cannot apply this principle consistently either. But combined with the other arguments against defer I think it's a reasonable way forward.

krux02 commented 4 years ago

How would you implement the scoped functions that I declared in my opengl wrapper here https://github.com/krux02/opengl-sandbox/blob/master/fancygl/scoped.nim#L14 without defer?

OpenGL functions always set a value, so they change the state constantly. The scoped functions ensure that at the end of the scope the value is reset to the original state. In other words the value is just changed locally for that function. This is a pretty common use case for example GUI libraries that need to set an OpenGL state to render something, but they also need to reset that state to not break the main application.

Thinking of it. Transaction could work.

But apart from it. I don't see why transaction is in any form simpler to implement than to maintain defer.

Araq commented 4 years ago

But apart from it. I don't see why transaction is in any form simpler to implement than to maintain defer.

You cannot implement today's defer as a macro because you cannot go to parent's node in Nim AST. This made the implementation tricky in the compiler too. It's also a NodeKind so every macro that rewrites try finally must rewrite defer as a special case. My proposed transaction macro side-steps these problems.

zah commented 4 years ago

There is one non-obvious way defer and destructors are not the same. No language has invented yet a notion of an "async destructor". When you have a resource with an async close operation, this is still the most convenient way to manage it:

proc asyncOperation {.async.} =
  var res = createAsyncResource()
  defer: await close(res)
  ...

Perhaps we can invent async destructors somehow, but that's quite unexplored territory.

Araq commented 4 years ago

@zah A try finally or ~> does cover this use case.

timotheecour commented 4 years ago

@araq in the spirit of keeping this RFC honest and up to date, I suggest editing the top-post.

It's yet another language feature that we have to maintain and it keeps adding costs as it's buggy. See nim-lang/Nim#13899 Now, before you downvote me into hell, please consider to contribute bugfixes about the remaining defer bugs if you really want to keep the feature in the language.

=> i've fixed it in https://github.com/nim-lang/Nim/pull/14723, and i'm not aware of other defer bugs. in fact this was an async bug caused by a defer gotcha, not by a defer bug.

It doesn't model the idea it tries to model very well. The basic idea here is something like "let's pair do/undo operations" but do(); defer: undo() doesn't do that. Instead undo() is called at the end of the scope regardless of whether do() really did run successfully.

as explained in https://github.com/nim-lang/RFCs/issues/236#issuecomment-646247553 and https://forum.nim-lang.org/t/4022#25088 this is incorrect.

proposal: `deferScoped`

I've proposed deferScoped here https://github.com/nim-lang/Nim/pull/14723#issuecomment-646503823 which is a trivial template sugar which may address this RFC, but I'm curious where it would fall short. It avoids the above mentioned gotcha, but may not be a suitable replacement everywhere. @zah Examples/counter-examples welcome.

# maybe in sugar.nim, but I'd prefer a "thin" module, because modules are dirt-cheap (https://github.com/nim-lang/Nim/pull/14614)
template deferScoped(cleanup, body) =
  try: body
  finally: cleanup

with benefits:

simpler than transaction
simpler than defer since it's a library code (just 1 template)
like defer, brings the "cleanup" close to the code that needs cleaning up
avoids gotchas like the one that caused nim-lang/Nim#13899 since there is no scope escaping

and drawback:

introduces a scope
flip side of "avoids gotchas" is it might be actually a desirable property to extend to end of scope where defer is called

example

let a = open("foo.db")
deferScoped: close(a) # immediately follows code that needs cleanup
do:
  # long rest of code
  use(a)

which syntactically improves a bit over try/finally because the cleanup code immediately follows the code that needs cleaning up

Araq commented 4 years ago

as explained in #236 (comment) and https://forum.nim-lang.org/t/4022#25088 this is incorrect.

No, it's not incorrect because I don't only talk about scoping. The question is what does it mean that open fails? It's not always done by raising an exception. Ideally there would be hidden flags introduced so that close is really only called when open returned true.

in the spirit of keeping this RFC honest and up to date, I suggest editing the top-post.

I did and mentioned a different defer bug. ;-)

sinkingsugar commented 4 years ago

Btw a nearly as useful defer is trivial to implement in user code... at least in C++, just by using constructors destructors and RAII (not sure on the reliability state of them with nim), so that it would be called in reverse order too (try/finally won't do that iirc). Shouldn't this be doable fairly easily in nim too? (I know it depends on destructors but hey they should working by now)

see my code for example here: https://github.com/sinkingsugar/chainblocks/blob/056aca9400c3a9a7c0e5b89bb03458cb31509279/include/utility.hpp#L281

P.S. I know for sure that we in nim-libp2p are in huge need for defer and it is a shame it's not working properly, but we are (sadly) still bound by the GC and maybe the destructor approach I suggested would not work in that case...

krux02 commented 4 years ago

Btw a nearly as useful defer is trivial to implement in user code... at least in C++, just by using constructors destructors and RAII (not sure on the reliability state of them with nim), so that it would be called in reverse order too (try/finally won't do that iirc).

No it is not, because c++ destructors are block scope based, not function scope based.

You cannot implement today's defer as a macro because you cannot go to parent's node in Nim AST. This made the implementation tricky in the compiler too. It's also a NodeKind so every macro that rewrites try finally must rewrite defer as a special case. My proposed transaction macro side-steps these problems.

@Araq what, if you would require the transaction macro or some equivalent to allow a defer to be placed, and then resolve the defer in that macro?

Going back to by scoped setters from the OpenGL wrapper. I don't think they would still be possible with the transaction macro. Maybe with some haks that will prevent ~> to be resolved until a certain point in time.

sinkingsugar commented 4 years ago

Btw a nearly as useful defer is trivial to implement in user code... at least in C++, just by using constructors destructors and RAII (not sure on the reliability state of them with nim), so that it would be called in reverse order too (try/finally won't do that iirc).

No it is not, because c++ destructors are block scope based, not function scope based.

Wait.. this is new for me.. are you saying that nim defer is "function" scope based (like go basically), in a language with destructors?

If that is the case I agree that it should be removed as it's another semi-useful and not intuitive feature.

P.S. It actually makes sense since destructors came after... but yeah in a language with scoped destruction it is logical that it should be block scope based...

treeform commented 4 years ago

I don't use defer that much. I don't have a strong opinion.

I do find ~> really strange though. I would rather type defer or try..finally.

kaushalmodi commented 4 years ago

My opinion is same as @treeform 's:

I haven't used defer. So I don't have a strong opinion.
I do find ~> really strange though. Keywords are better than cryptic series of chars.. Keywords are easier to search in docs, etc.

Araq commented 4 years ago

This RFC does not propose ~>! It was only an example of how things could be handled better!

filcuc commented 4 years ago

I simply love defer and D scope(exit) concept and i totally dislike try finally blocks 1) try...finally blocks cause visual nesting. defer no 2) Error handling in destructors is basically impossible since you don't have a return value thus destructors are not an alternative to try..finally or defer

timotheecour commented 4 years ago

indeed, destructors is not a replacement for try..finally or defer.

I often use this pattern for debugging:

proc fn(a: T): T2 =
  when defined(mydebug): (defer: myDebug(a, result))
  restOfCode # typically multiline

with try...finally, this would cause a large diff (restOfCode needs to be indented) instead of a 1 line diff

furthermore it would make it awkard or impossible to wrap inside a try/finally conditionally on when defined(mydebug)

Araq commented 4 years ago

Error handling in destructors is basically impossible

Er, error handling can be done via exceptions, return values are not required.

with try...finally, this would cause a large diff

So, should we have backwards if blocks too then that don't require indentation? Because hey, indentation is bad for "git diff". Come on that's one of worst design criteria I can imagine. We might as well get back to linear assembler-like code then, no more problems with "git diff".

filcuc commented 4 years ago

Er, error handling can be done via exceptions, return values are not required.

I'll try to explain better. How to you think one could handle exception propagation from destructors when not using ARC. What about ORC? or full GC? Is nim v2 planning to kill using a full GC? I think is unfeasible to handle exceptions thrown from destructors when destruction is deferred (ORC? GC?). If we agree on that we end in what is done in C++ no exceptions from destructors. This in turn implies that for example closing a file in its destructors should be used only when the user is not interested in really knowing if the file has been really closed or for those that are lazy. This in turn implies that destructors are not a replacement for try..catch or defer for full error handling.

Come on that's one of worst design criteria I can imagine.

The idea is that

try:
  try:
    try:
    finally:
  finally:
finally:

is not as good to my eye than

defer: ...
defer: ...
defer: ...

Araq commented 4 years ago

The idea is that...

I know, that's also why many prefer return heavy code. I don't. I think it's completely misguided. If nesting is not "good to the eye" the move from gotos to structured programming wasn't a good move. If you have deeply nested code then you have complex logic. Maybe the complexity is required, maybe not, but faking that it's not really there, because you used the unstructured return/defer is worse.

Araq commented 4 years ago

How to you think one could handle exception propagation from destructors when not using ARC.

Well I didn't propose to remove try finally, did I?

What about ORC? or full GC?

ORC is a full GC btw.

Is nim v2 planning to kill using a full GC?

No. It's also off-topic.

filcuc commented 4 years ago

filcuc: How to you think one could handle exception propagation from destructors when not using ARC. Araq: Well I didn't propose to remove try finally, did I?

My question was an objection to your previous objection

filcuc: Error handling in destructors is basically impossible Araq: Er, error handling can be done via exceptions, return values are not required.

If a destructors is called asynchronously, like a GC usually does, handling exception propagation from destructors is basically unfeasible.

Well I didn't propose to remove try finally, did I?

No but you did throw destructors in this RFC as an argument for removing defer. I would say that defer should be only judged/compared against try...finally (so for explicit error handling).

I know, that's also why many prefer return heavy code. I don't. I think it's completely misguided. If nesting is not "good to the eye" the move from gotos to structured programming wasn't a good move. If you have deeply nested code then you have complex logic. Maybe the complexity is required, maybe not, but faking that it's not really there, because you used the unstructured return/defer is worse.

This opens a lot of subjective arguments....and btw aren't map and reduce removing for loop nesting? Aren't Nim macros a way to fake/hide complexity (some packages are black magic due to macros)?

In the end if the real reason for removing defer is the lack of development bandwitdth then there's no need to argue or write RFCs.

Araq commented 4 years ago

This opens a lot of subjective arguments....and btw aren't map and reduce removing for loop nesting? Aren't Nim macros a way to fake/hide complexity (some packages are black magic due to macros)?

It's not subjective at all and "map" and "reduce" are not hidden gotos. Macros cannot easily introduce unstructured control flow when there are no unstructured primitives in the language. Destructors are indeed a substitute for the cases where there is no error path. We have the bandwidth to write and implement RFCs, esp if they are about simplifying Nim.

filcuc commented 4 years ago

It's not subjective at all and "map" and "reduce" are not hidden gotos.

I see defer a hidden goto as much as raising/catching an exception

filcuc commented 4 years ago

With this i'm out: TBH i don't even see too much difference in a defer vs the implicit call to a destructor at scope exit...with the difference that defer is explicit

D-Nice commented 4 years ago

I have actually used defer, since there were some mentions/claims that it's only used in example code. I have production apps running using it, and seems there are others that use it.

https://github.com/nim-lang/RFCs/issues/236#issuecomment-649226367 makes a good point, if people really are abusing defers to nest like this, it's more of a danger in hiding complexity, potentially unkowingly even. I like having it, but can easily live without it, especially if it's such a burden to maintain.

c-blake commented 4 years ago

I like defer and use it, though not always..and perhaps not coincidentally I like "early return" as a way to check many various pre-conditions before the main logic of a proc.

I see the analogy with goto vs. structured, but I think the complaints about goto were about the induced non-linear/complex control flow, not "all roads lead to proc exit" or even all gotos lead to a single target which was the rationale for keeping it in C. There are ways to use early return that are closer in spirit to boolean short circuiting (do this or this or this or this or exit). So, I would counter https://github.com/nim-lang/RFCs/issues/236#issuecomment-649226367 with "If early returns are a bad idea then so is boolean short circuiting". :-) Which maybe @Araq thinks is also terrible. :-)

Humans have sufficient variety in reasoning experience that "easy to reason about" is subjective/experience related. One "psychological study" style response with a larger sample size than "just me" might be the Linux kernel which is a gagillion lines of code maintained by thousands of people highly sensitive to error handling stuff. They prefer early return style and that was in the style guide since the earliest versions. I think it's common in kernel/systems development in general, but I mostly know Linux.

Anyway, neither early return nor boolean short circuits are on the chopping block (and hopefully never will be). I'd be fine with the deferScoped/do: fixup of https://github.com/nim-lang/RFCs/issues/236#issuecomment-646855314 if defer is hard to maintain, though. I think the main point is some non-OO way/lightweight syntax to keep related logic lexically close.

Araq commented 4 years ago

Well there is no science behind Linux's coding style so that's unconvincing. More to the point though:


if cond: return
...
# what do I **know** at this point? That ``not cond`` holds. How is this visible in the code? It's not.
moreCode()
...

Now compare it to:


if not cond: 
  ...
  # what do I **know** at this point? That ``not cond`` holds. How is this visible in the code? It's immediately obvious
  # due to the **structure** of the code.
  moreCode()
  ...

Nothing subjective about it, sorry. ;-)

(It's of course also reflected in the algorithms you have to use to analyse the code, one needs a control flow graph, the other is fine with a simple tree traversal.)

c-blake commented 4 years ago

The only difference between your two cases is that the second has a redundant visual cue - both after the if and indented. The work for a compiler may vary, but both cases absolutely require human readers to "know what comes before".

So, your argument only persuades if one thinks this particular redundancy always helps a human/the abbreviation always hurts. Some redundancy is good, but too much makes people get lost. The right amount of brevity / verbosity is subjective and usually context dependent (and I struggle with that writing prose). I love that Nim is terse in general, though.

The way this particular indent redundancy scales is not great. Consider adding in cond2 and cond3 from previously unnoticed assumptions...add in a new check & return. Find another, add another. No need to alter/re-indent the main body of moreCode() or worry about some elif/else potential way below. A set of many pre-conditions/assumptions are like a big list to humans not big trees whatever the impl details. The conceptual & code structure match better.

Beyond that, if you want to echo a failed condition-specific diagnostic? if cond1: message1; return then if cond2: message2; return then if cond3: message3; return co-locates condi & messagei, not shunting the message way down the page/proc to some else: clause that you have to visually line-up..and maybe not change when you tweak the condition!

Another example is return out of a for loop for some scan for when a condition is found true. With return you simply exit at the if. That seems simpler (again for a person) than a set-a-flag; break; post-loop result=flag triplet...If it's a nested loop where without return you would need a block and a break to the outer as well as the flag to decide what result should be. Ugh. Yeah..That may be easier to translate in a compiler, but it's hard to believe many would find that cleaner code to read as people.

None of this is true for all code, but I think return really can simplify for a human reader. Lest my own verbosity undermine communication, we can just agree to disagree, and I'll shut up about this side issue unless someday you try to take away return. Then I'll make more noise. :-)

dom96 commented 4 years ago

defer is mainly useful for cleaning up resources (FDs etc.), but I assume that we will soon have a working destructor implementation that will take care of that, making defer useless, right?

Araq commented 4 years ago

Well I have nothing to add. What kind of algorithms that you have to use in order to reason about the code effectively is the most objective criterion I can come up with. The fact that "return" is much more convenient for nested search loops is a strong indicator that our loop structures are misdesigned.

disruptek commented 4 years ago

I think we should deprecate defer but add explicit scopes, even if they are not exposed to the macro system. They help code reason about structure and could even let the sadists reimplement defer.

I would just add that @Araq convinced me to switch from continue to dominating if and I like the change. It composes better with Nim's result and other structure, and I do find the code easier to grasp at a glance.

Both return and continue are usually code smell to me, now, and the idiom is pretty painless:

block found:
  for i in x:
    if i == 3:
      result = "3"
      break found
  result = "nah"

zah commented 4 years ago

The whole premise of this RFC is that defer is difficult to maintain. Knowing the compiler internals, I find this to be a significant exaggeration. I can name dozens of features in Nim that are much harder to maintain. If the thesis here is that nothing that's strictly optional deserves maintenance, a critical read of the manual would put a lot of features in that category.

disruptek commented 4 years ago

I agree, but let's start with removing defer and move on from there, right?

zah commented 4 years ago

Well, the subtext of my message was that these nice syntax sugar features are not all that bad. They make Nim what it is - a highly elegant and expressive language that is a pleasure to write. I'll name just one feature from this "strictly optional" category that I'm sure you wouldn't enjoy losing - tuple unpacking. Everything that can be written with it, can also be written without it. Shall we remove it?

disruptek commented 4 years ago

If it makes the compiler harder to maintain, then I think it's fair to weigh that maintenance burden against the ergonomics of the feature. Ideally, it can be maintained outside of the compiler. I know you are busy with Status and probably don't want to also be responsible for maintaining tuple unpacking, let alone defer, am I right?

Araq commented 4 years ago

Closing this for the time being. It seems wiser to focus on more pressing problems. I'm sorry for bringing it up.

treeform commented 4 years ago

Maybe related to it being buggy: https://github.com/nim-lang/Nim/issues/15071 ?

nim-lang / RFCs