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

Constructing a ref of a requiresInit type causes destructions before initialization #19139

Closed
alaviss opened this issue Nov 13, 2021 · 3 comments

Comments

@alaviss
Copy link
Collaborator

alaviss commented Nov 13, 2021

An another case of #16607

Example

type
  O {.requiresInit.} = object
    initialized: bool

proc `=destroy`(o: var O) =
  doAssert o.initialized, "O was destroyed before initialization!"

proc initO(): O =
  O(initialized: true)

proc main() =
  let r = new O
  r[] = initO()

main()

Current Output

test.nim(15)        test
test.nim(9)         main
test.nim(6)         =destroy
lib/system/assertions.nim(38) failedAssertImpl
lib/system/assertions.nim(28) raiseAssert
lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: test.nim(6, 12) `o.initialized` O was destroyed before initialization! [AssertionDefect]

Expected Output

no output

Additional Information

  • Found during development of nim-sys.
$ nim -v
Nim Compiler Version 1.7.1 [Linux: amd64]
Compiled at 2021-11-08
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 83a9c3ba31d180cd5e31026d8b7603bf7adea18c
active boot switches: -d:release -d:nimUseLinenoise
@alaviss
Copy link
Collaborator Author

alaviss commented Nov 13, 2021

The issue is that it fails with {.requiresInit.}.

While the destructors spec allows =destroy to be called on zero-initialized values, the requiresInit spec forces a strict requirement on a value to be initialized at construction. By this reasoning, it means that a value is not considered valid until it has been constructed and that no hooks can be executed before that.

In Nim, "construction" is the first value to be written into the variable (of which =sink is skipped), and the compiler already has the analysis needed for this (as required for implementation of requiresInit). This appears to me to be a case of the analysis not accounting for this specific case (it happened before in #16607).

@derekdai
Copy link
Contributor

You are right, since compiler knows r refer to an uninitialized O, it should consider r[] = initO() is for initialize it.

alaviss added a commit to alaviss/nim-sys that referenced this issue Nov 13, 2021
@Araq
Copy link
Member

Araq commented Nov 21, 2021

There is no bug here, the spec does not mention special cases for requiresInit.

@Araq Araq closed this as completed Nov 21, 2021
alaviss added a commit to alaviss/nim-sys that referenced this issue Jun 30, 2022
Since nim-lang/Nim#19139 was closed as a
"won't fix", the "workaround" has to become permanent.

If we are to store a boolean next to the FD, it would double the
structure size, which is undesirable.

Instead, storage is now done via a lossless map so that zero-ed values
are invalid by default.

In this commit, Handle[T] API is changed to match other parts of the
project. In short:

* `get` -> `fd`
* `take` -> `takeFd`
alaviss added a commit to alaviss/nim-sys that referenced this issue Jun 30, 2022
Since nim-lang/Nim#19139 was closed as a
"won't fix", the "workaround" has to become permanent.

If we are to store a boolean next to the FD, it would double the
structure size, which is undesirable.

Instead, storage is now done via a lossless map so that zero-ed values
are invalid by default.

In this commit, Handle[T] API is changed to match other parts of the
project. In short:

* `get` -> `fd`
* `take` -> `takeFd`
alaviss added a commit to alaviss/nim-sys that referenced this issue Jun 30, 2022
handles: change storage strategy and conform API

Since nim-lang/Nim#19139 was closed as a
"won't fix", the "workaround" has to become permanent.

If we are to store a boolean next to the FD, it would double the
structure size, which is undesirable.

Instead, storage is now done via a lossless map so that zero-ed values
are invalid by default.

In this commit, Handle[T] API is changed to match other parts of the
project. In short:

    get -> fd
    take -> takeFd
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

3 participants