nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.58k stars 1.47k forks source link

Change branch of an object variant is allowed #18977

Closed derekdai closed 1 year ago

derekdai commented 3 years ago

In section Object variants of manual, the example says

# invalid: would change the active object branch:
n.kind = nkInt

But in fact compiler accept it, and =destroy is called to prevent resources leak.

Example

type
  E = enum
    a, b, c, d
  X = object
    v: int
  O = object
    case kind: E
    of a:
      a: int
    of {b, c}:
      b: float
    else:
      d: X

proc `=destroy`(x: var X) =
  echo "x destroyed"

var o = O(kind: d, d: X(v: 12345))
echo o.d

o.kind = a
echo o.a

Current Output

$ nim r --gc:arc test.nim
(v: 12345)
x destroyed
0

=destroy will be called event with refc.

Expected Output

Compile with error.

Possible Solution

Additional Information

If I change d: X to d: ref X, with arc or orc, output

$ nim r --gc:arc test.nim
(v: 12345)
x destroyed
139738855620680

With refc

(v: 12345)
test.nim(22) test
~/.choosenim/toolchains/nim-#devel/lib/system/assign.nim(294) FieldDiscriminantCheck
~/.choosenim/toolchains/nim-#devel/lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: assignment to discriminant changes object branch [FieldDefect]

Tested with 1.4.8 and devel

Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-10-07
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: f03872d99ec2d25de829415a75162f9d8297bb19
active boot switches: -d:release
ghost commented 3 years ago

This is expected - newruntime (arc and orc) do support changing active branch at runtime as long as you don't have a custom destructor attached to your object variant

Araq commented 3 years ago

I think this should continue to be disallowed, the workaround is o = O(kind: a, ...) (reassign the complete structure).

ringabout commented 2 years ago

I think this should continue to be disallowed

I agree. It should be covered by {.cast(uncheckedAssign).} instead.