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

result assignment should stay optional, except for var/lent result #241

Closed
timotheecour opened this issue Jul 12, 2020 · 4 comments
Closed

Comments

@timotheecour
Copy link
Member

timotheecour commented Jul 12, 2020

goal: discuss this topic here and not in unrelated issues like nim-lang/Nim#14777 (comment)

Assignments like ' result = "" ' will become mandatory, we really want definite assignment analysis like C#, so don't remove them.

proposal

  • P1: keep result assignment optional
  • P2: make var result assignment mandatory in all code paths and make sure no read to a var result memory can happen before var result is initialized; currently this is not the case, see example 4 in bugs with var result  Nim#13975 which accepts invalid code where result is not initialized in a trivial case
  • P3: tuple with var elements, eg (var int, int) currently work with iterators (in RT, not VM), but not with procs; when they do, the proposal should apply to the components that are var, so that only var components should be initalized before use (and initialization stays optional for other components)
  • P4: for lent result, the rules are the same as for var result

example

when true:
  var g0=0
  proc main(): var int =
    echo result # bug: should give CT error
    result.inc # bug: should give CT error
    result = 1 # ok, gives CT error
    result = g0 # ok
  main().inc

  proc main2(): (var int, int) =
    # currently not supported at all see https://github.com/nim-lang/Nim/issues/13975
    echo result[1] # should be ok
    echo result[0] # should be CT error
    result[0] = g0 # should be ok

rationale for P2,P3,P4

should not be controversial, it's just correctness (otherwise you get SIGSEGV at runtime); ditto with lent result

rationale for P1

  • a ton of code uses and relies on implicit result assignment eg:
proc fn1(a: int): string = result.add $a
proc fn2(a: int): string = discard
proc fn3(a: int): string = when defined(foo): "bar"
proc fn4(a: int): Foo = result.x = a
  • implicit assignment is guaranteeed by codegen and could be optimized out when safe to do so (refs cgen creates redundant nimZeroMem calls Nim#14602), instead of forcing every function to intialize its result which seems pointless, since codegen can do it.

The analysis is simple, and behaves as if result = default(typeof(result)) was the first statement.

  • I don't see any performance advantage to forcing users to initialize the result

links

@arnetheduck
Copy link

arnetheduck commented Jul 13, 2020

...or simply deprecate and eventually remove result and let users type var result = ...; return result which is explicit and doesn't introduce an implicit variable and a third return style that has few good use cases but costs confusion, compiler maintenance + extra compiler logic to make sure that usage of result is somewhat sane, macro difficulties etc.

@andreaferretti
Copy link

The only advantage I see in result return value optimization. It seems to me (but I don't know the internals of the compiler) that if all code paths only assign to result.somefield, it should be easy to optimize this and just create result on the previous stack frame, thereby avoiding a copy when returning.

It may be the case that this is difficult to do in general, or that it is easy to do with other ways of returning. But the picture I have in my mind is that result is a preallocated slot which lives before the current stack frame.

If result does not help with RVO, I agree with @arnetheduck that its complexities dwarf its advantages.

@Araq
Copy link
Member

Araq commented Jul 14, 2020

Definite assignment analysis (DAA) has little to do with result! And the motivation for it is like this:

var x: int # please check for me every branch assigns a value to 'x'
case selector
of caseA:
  if cond:
    x  = 3
  else:
    # more complex logic here
    # oops I forgot to assign to 'x' 
of caseB:
  x = 4
of caseC:
  x = 8

Nim's case statement checks for exhaustiveness, DAA is a nice extension.

See https://docs.oracle.com/javase/specs/jls/se6/html/defAssign.html for the rules we could simply take over from Java. It works.

Before Nim got default(T) certain patterns were hard to express without Nim's current implicit initialization but now that we have default(T) we could make the next steps, if your code depends on result = default(T) as the first statement of your proc, so write that down explicitly.

@Araq
Copy link
Member

Araq commented Jul 24, 2020

Closing because it misrepresents my proposal which is #49

@Araq Araq closed this as completed Jul 24, 2020
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

No branches or pull requests

4 participants