VM and js backend require result for lent or var annotations to work

[mratsim]

Most of the standard library is working at compile-time and there are checks to ensure that.

Unfortunately from #14417 (comment), lent is not supported in the VM.

For example the JSON test will fail with

Error: limited VM support for 'addr'

This would prevent using lent to optimize arrays, sequences, options, "result" types, object variants. All of which have a typical no-copy use-case.

The easy fallback would be to make lent a no-op in the VM.

[cooldome]

Please provide a self contained test so I can have a look

[mratsim]

You can revert this commit: 0964462

i.e. in the option module change

proc get*[T](self: Option[T]): T {.inline.} =

to

proc get*[T](self: Option[T]): lent T {.inline.} =

And the test suite will crash in many places like Json.

[timotheecour]

still, a minimal example would've been nice;
which of these since you've already ran this?

lib/pure/json.nim
lib/pure/parsejson.nim
tests/stdlib/mjsonexternproc.nim
tests/stdlib/tjson_unmarshall.nim
tests/stdlib/tjsonexternproc.nim
tests/stdlib/tjsonmacro.nim
tests/stdlib/tjsonmacro_reject.nim
tests/stdlib/tjsontestsuite.nim

[mratsim]

I'll cook up one this evening it was tjsonmacro.nim

[mratsim]

Here is an example

type MyOption[T] = object
  case has: bool
  of true:
    value: T
  of false:
    discard

func some[T](val: T): MyOption[T] =
  result = MyOption[T](has: true, value: val)

func get[T](opt: MyOption[T]): lent T =
  assert opt.has
  opt.value

const x = some(10)
echo x.get()

static:
  echo x.get()

As a potential impact, I tried to use options in my RayTracer, but they were 5x slower (not just a couple dozen percents like the iterator issue in #14421) than passing mutable parameter + bool

[Araq]

Well nobody claimed Option[T] is a lightweight abstraction.

[mratsim]

Well nobody claimed Option[T] is a lightweight abstraction.

I was misled by Rust code :/

[cooldome]

Issue turned out completely different. Can use stmtListExpr to assign lent T/var T or you get addr of temporary variable. Just changed get to use stmtListExpr to assign to result

[timotheecour]

indeed: reduced case:

when true: # D20200525T015650
  type Foo = object
    x: float

  proc fn(a: var Foo): var float =
    ## discard <- turn this into a comment (or a `discard`) and error disappears
    # result = a.x # this would work
    a.x #  Error: limited VM support for 'addr'

  template main =
    var a = Foo()
    discard fn(a)
  main() # works
  static: main()  # fails

[mratsim]

So using a return statement or result work.

type MyOption[T] = object
  case has: bool
  of true:
    value: T
  of false:
    discard

func some[T](val: T): MyOption[T] =
  result = MyOption[T](has: true, value: val)

func get[T](opt: MyOption[T]): lent T =
  assert opt.has
  return opt.value

const x = some(10)
echo x.get()

static:
  echo x.get()

Can we change the implicit return expression (not sure how to call that) to be the same as implicit result or explicit return statement?

At the very least the error message should be changed to "Maybe try an explicit return or the implicit result variable".

Furthermore this is a pattern that we favor in nim-beacon-chain. We are already fighting stack issues and genericResets and if return expression introduced non-optimized away temporaries, they might contribute to that.

[timotheecour]

At the very least the error message should be changed to "Maybe try an explicit return or the implicit result variable".

that's fine as an improvement, but obviously this issue should be kept open until this bug is fixed

[arnetheduck]

Maybe try an explicit return or the implicit result variable

er, why should that be a recommendation? ie they're supposed to be equivalent, so it seems counterproductive to implement a feature that suggests a workaround instead of focusing on the actual bug?

[timotheecour]

we should reopen this issue (refs #14442 (comment))

[mratsim]

Maybe try an explicit return or the implicit result variable

er, why should that be a recommendation? ie they're supposed to be equivalent, so it seems counterproductive to implement a feature that suggests a workaround instead of focusing on the actual bug?

If it's possible to fix this in a timely manner sure, if not we need to tell users how to workaround that.

[cooldome]

IMO, this issue is not VM specific. All backends introduce a temporary variable in this case. IMO, solution is to introduce lowering in transf phase:
Transform:

x = block:
  stmt1
  stmt2
  expr

Into

block:
  stmt1
  stmt2
  x = expr

Similar lowering for if, block, case, stmtList expressions.
injectdestuctors already does this lowering. It needs to be moved from injectdestructors to transf.

[timotheecour]

solution is to introduce lowering in transf phase:

@cooldome do you think this could fix #9230 as well?

see also:

Nim/compiler/lambdalifting.nim

Line 917 in 9502e39

if not (body.kind == nkForStmt and body[^2].kind in nkCallKinds):

proc liftForLoop*(g: ModuleGraph; body: PNode; owner: PSym): PNode =
if not (body.kind == nkForStmt and body[^2].kind in nkCallKinds):

[timotheecour]

IMO, this issue is not VM specific

indeed, just got hit by this in #14875

nim c -r main # ok
nim js -r main # bug

proc byLent2[T](a: T): lent T =
  runnableExamples: discard
  a

proc byLent3[T](a: T): lent T =
  runnableExamples: discard
  result = a

block:
  var a = 10
  # let x = byLent3(a) # works
  let x = byLent2(a) # Error: internal error: genAddr: nkStmtListExpr