nim-lang / RFCs

A repository for your Nim proposals.
136 stars 23 forks source link

Allow return in expressions again #251

Closed Clyybber closed 3 years ago

Clyybber commented 4 years ago

This is a feature that was introduced in https://github.com/nim-lang/Nim/pull/13520 and removed again in https://github.com/nim-lang/Nim/pull/15281. You can argue that for consistency with the fact that raise/break/continue are allowed too return should be allowed too, but there are also practical valid usecases of that feature, like allowing one to implement a custom exception mechanism, using something like this

template customRaise =
  setException()
  return

which could then be used in the same manner like a builtin raise statement would. This ceases to work after https://github.com/nim-lang/Nim/pull/15281

disruptek commented 4 years ago

And what about break in expressions?

Araq commented 4 years ago

-1 as I don't know anybody who desires to implement his own exception system and such a desire seems misguided and based on superstitions to me.

disruptek commented 3 years ago

I would like this RFC to remain open until we're certain we wouldn't want to exploit such a feature in CPS. :grin:

ghost commented 3 years ago

FYI, @zedeus uses this in Nitter to reduce some code size - https://github.com/zedeus/nitter/blob/master/src/parserutils.nim#L15 which is used in let definitions like https://github.com/zedeus/nitter/blob/master/src/parserutils.nim#L150

SolitudeSF commented 3 years ago

@zedeus uses this in Nitter to reduce some code size

he doesnt need to, tho

stefantalpalaru commented 3 years ago

Returns in expressions (and templates) are used frequently in Nimbus projects.

For example in a try block: https://github.com/status-im/nimbus-eth2/blob/d98be078c317d21eaec4e8bb33f626c6a54287f1/beacon_chain/keystore_management.nim#L155-L160

Another return in a try block, inside a template: https://github.com/status-im/nimbus-eth2/blob/d98be078c317d21eaec4e8bb33f626c6a54287f1/beacon_chain/keystore_management.nim#L629-L634

haxscramper commented 3 years ago

While custom exception system is probably not the most common use-case for return statement, this change introduces inconsistency with other {.noreturn.} statements - all expressions (including custom ones and break) are allowed to be used in if and case expressions. For people that do use case as expression this might be an annoying surprise.

All of this code works for 1.2.0 - 1.2.6, but with 1.2.8 onwards return no longer works.

proc impl1: int = (if false: 12 else: quit 1)
proc impl2: int = (if false: 12 else: return 0)
proc impl3: int = (if false: 12 else: raiseAssert("12"))

proc customNoreturn() {.noreturn.} = discard

proc impl4: int = (if false: 12 else: customNoreturn())
proc impl5: seq[int] = 
  for i in [1,2,3]:
    result.add:
      if true:
        12
      else:
        break

proc impl6: seq[int] = 
  for i in [1,2,3]:
    result.add:
      if true:
        12
      else:
        return

While above example is pretty synthetic, my real-world use-case was

```nim proc visitCursor( cursor: CXCursor, parent: CNamespace, conf: WrapConfig): tuple[ decls: seq[CDecl], recurse: bool] = if not conf.ignoreCursor(cursor, conf): result.decls.add case cursor.cxKind: # Case expression allows to write single add instead of multiple ones of ckNamespace: visitNamespace(cursor, parent, conf) of ckClassDecl, ckClassTemplate, ckClassTemplatePartialSpecialization, ckStructDecl: @[ visitClass(cursor, parent, conf) ] of ckFunctionDecl, ckFunctionTemplate: @[ visitFunction(cursor, parent, conf) ] of ckTypedefDecl: @[ visitAlias(cursor, parent, conf) ] of ckEnumDecl: @[ visitEnum(cursor, parent, conf) ] else: result.recurse = true return # I need `noreturn` statement here, but the only available that does nothing is either return # or adding custom proc solely for this purpose ```
Araq commented 3 years ago

Real-world or not this is highly convoluted code. When you read result.decls.add ... few readers assume that it might not add anyhing at all to result and instead return immediately / leave the loop / whatever.

Araq commented 3 years ago

I gave up my resistance, too much code depends on it. Both 1.4.2 and 1.2.8 will support this construct again...