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

Definite assignment analysis and out parameters #378

Closed
Araq opened this issue May 20, 2021 · 7 comments
Closed

Definite assignment analysis and out parameters #378

Araq opened this issue May 20, 2021 · 7 comments

Comments

@Araq
Copy link
Member

Araq commented May 20, 2021

Proposal to add out parameters to Nim and to enable Nim's "definite assignment analysis"
(https://docs.oracle.com/javase/specs/jls/se6/html/defAssign.html) for all types.

Current situation

This code compiles without warnings:

proc p(): seq[int] =
  result.add 4

Variables (including result) are initialized to their default value automatically. This
design has its merits but it is also considered to be error prone:

proc p(cond: bool): ref T =
  if cond:
    result = (ref T)()
  # implied: else: result = nil

It's hard to know if the "implied" statement was meant and I think it's the most frequent
source of nil bugs. Nim's original design lacked system.default(T) so initializing variables
to default(T) implicitly made some sense. Another benefit was this solution does not
require an analysis over the control flow graph to ensure that no uninitialized values
can be accessed.

But for some types like var T or ptr T not nil there is no default value -- variables
of such a type need to be initialized explicitly.

Proposal: Require explicit initialization

This RFC proposes to remove the special casing of variables that lack a default value
-- all variables need to be initialized explicitly before they can be used. This is what
C# and Java (see https://docs.oracle.com/javase/specs/jls/se6/html/defAssign.html) do
for decades and it works very well.

Proposal: Add out parameters to Nim

This proposal was implemented and found to not work too well with today's Nim. The
reason is that code like:

proc open(f: var File, filename: string): bool

var f: File
if open(f, "t.txt"):
  ...

is very common. The var File here is an output parameter, not a read-write
parameter. This is why I propose to add out T parameters to Nim. While output
parameters are usually bad style, APIs from Windows and POSIX use output parameters
heavily.

When the implementation of a routine with output parameters is analysed, the compiler
checks that every path before the (implicit or explicit) return does set every output
parameter:

proc p(x: out int; y: out string; cond: bool) =
  x = 4
  if cond:
    y = "abc"
  # error: not every path initializes 'y'

Out parameters and exception handling

This analysis takes exceptions into account which can produce annoying results:

proc p(x: out int; y: out string; cond: bool) =
  x = canRaise(45)
  y = "abc" # <-- error: not every path initializes 'y'

However, I think this is acceptable for two reasons:

  • Output parameters should be used rarely. It's better style to return a tuple of
    values instead.
  • It's easy enough to assign to output parameters in the beginning of the proc body
    via outParam = default(typeof(outParam)).

Out parameters and inheritance

It is not valid to pass an lvalue of a supertype to an out T parameter:

type
  Superclass = object of RootObj
    a: int
  Subclass = object of Superclass
    s: string

proc init(x: out Superclass) =
  x = Superclass(a: 8)

var v: Subclass
init v
use v.s # the 's' field was never initialized but the compiler does not understand this!

However, in the future this could be allowed and give us a better way to write object
constructors that take inheritance into account.

Code breakage / Transition period

Code like the following var x: T; use x is not allowed anymore. As a long transition
period the compiler will warn about the "usage before definition" and translate it as
if var x = default(T); use x was written.

Future extensions

Object fields that are declared as field: T must be explicitly initialized in object
constructors, to get a default value for an object field the syntax field = default(T)
should be used.

Alternatives

  1. Keep the language as it is, it's fine.
  2. Enable definite assignment analysis for all types but without adding out parameters.
@Clyybber
Copy link

Output parameters should be used rarely. It's better style to return a tuple of values instead.

Then why introduce such complexity for a feature that would only be used rarely anyways?

IMO code like

proc p(): seq[int] =
  result.add 4

should keep working without warnings.
I think we could come up with a more nuanced analysis that warns for:

proc p(b: bool): seq[int] =
  if b:
    result = @[4]
  else:
    echo "hmmm, did I forget to assign result here?"

but not for the previous snippet.

@juancarlospaco
Copy link
Contributor

Can it produce a warning for var T and no need for out T ?.
Even if I have to add a {.used.} or similar to my var T to explicitly silent the warning about it.
ptr should be work of strictNotNil as best as it can.

I think that for 2.0 we need to reduce the amount of experimental switches, not duplicate it.
We have 2 "not nil" thingies etc, a natural selection must happen.

@haxscramper
Copy link

haxscramper commented May 23, 2021

I believe making default value assignment mandatory would break quite a lot of code, especially templates/macros and introduce unnecessary verbosity in the language. When I write var matching: seq[int] or maxItem: MyTypeThatIKnowIsFineToDefault I don't think it is necessary to require explicit initialization. At the same time, idea of rewriting var t: T as var t: T = default(T) is exactly what was proposed in #252, with only difference for default-value-creator signature: init=(var T) vs default(typedesc[T]). It is already possible to override default so

translate it as if var x = default(T); use x was written.

Seems like a good idea, but forcing initialization for var is not. Warnings - maybe, but not hard errors.

type
  CustomType = object
    field: string

proc default(T: typedesc[CustomType]): T =
  # Same is `init=` in my origina proposal
  CustomType(field: "sfasdf")

# `var c: CustomType` is translated into
var c = default(CustomType)
echo c

@xigoi
Copy link

xigoi commented Apr 30, 2022

Whether or not this is accepted, I think a function that doesn't mention result and doesn't directly return a value (that is, it always returns the default value) should be an error. Forgetting to return is very common and currently the compiler will just silently do the wrong thing.

@Clonkk
Copy link

Clonkk commented May 3, 2022

I agree with @haxscramper here :this seems very much like #252

Whether you call it =init or =default having a an initialization hook is necessary; initialization based on zeroMem has been the cause of many headache when handling C++ type.

@Araq
Copy link
Member Author

Araq commented Oct 6, 2022

This has been implemented is available under .experimental: "strictDefs"

@Araq Araq closed this as completed Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants