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

{.requiresInit.} has several bugs #20253

Closed
barcharcraz opened this issue Aug 21, 2022 · 2 comments · Fixed by #21174
Closed

{.requiresInit.} has several bugs #20253

barcharcraz opened this issue Aug 21, 2022 · 2 comments · Fixed by #21174

Comments

@barcharcraz
Copy link
Contributor

What happened?

this compiles fine, even though it should probably error when compiling initMeow

type Meow {.requiresInit.} = object 
  init: bool

proc initMeow(): Meow =
  discard

proc main() =
  var theMeow = initMeow()
  echo theMeow.init
main()

on the other hand the following does not compile, even though the manual says requiresInit "does a control flow analysis to prove the variable has been initialized and does not rely on syntactic properties"

type Meow {.requiresInit.} = object 
  init: bool

proc main() =
  var theMeow: Meow
  theMeow.init = true
  echo theMeow.init
main()

Also new and setLen both bypass requiresInit

type Meow {.requiresInit.} = object 
  init: bool

proc main() =
  var theMeow = new Meow
  echo theMeow.init

this last bug is the same as #19139 but the issue isn't that it crashes and it shouldn't crash but that the example compiles at all, new should be considered to default initialize things, not initialize them fully.

setLen is even more nasty, it doesn't have a way to initialize things at all.

Nim Version

Nim Compiler Version 1.7.1 [Windows: amd64]
Compiled at 2022-08-20
Copyright (c) 2006-2022 by Andreas Rumpf

active boot switches: -d:release

Current Standard Output Logs

No response

Expected Standard Output Logs

No response

Possible Solution

given all these issues requiresInit is pretty useless, and doesn't manage to do what the various "required to initialize" constructs in other languages do. Personally I think it might be better to make requiresInit just require an explicit initializer and expand it to flow based checks later, given that the flow based checks don't really work now anyway.

Additionally the gaps in what's considered initialization should be fixed. Ironically for returning an uninitialized result the "can not prove result is initialized" warning fires off, but the requiresInit checks still pass.

Additional Information

No response

@Araq
Copy link
Member

Araq commented Aug 23, 2022

The only bug here is that

proc initMeow(): Meow =
  discard

does compile. Everything else is you confusing control flow and data flow. ;-)

@barcharcraz
Copy link
Contributor Author

barcharcraz commented Sep 3, 2022

The documentation for requiresInit reads as such:

The implicit initialization can also be prevented by the requiresInit type pragma. The compiler requires an explicit initialization for the object and all of its fields. However, it does a control flow analysis to prove the variable has been initialized and does not rely on syntactic properties

I would argue that new should not be considered to initialize its result (and perhaps should not zero the memory for requiresInit types). So either it should not compile (like the initMeow proc that fails to initialize anything) or it should only compile if the compiler proves the result is initialized before being used. If new/setLen are going to be considered to initialize their results (to zero) then the documentation should be updated to indicate that requiresInit is only for avoiding the performance cost of zero initializing stuff on the stack, and not for actually ensuring values of that type are initialized to something other than zero. If this is the case I would argue it's very odd to do it behind a pragma on the object instead of a (pushable) pragma on variables.

My first example that's "confusing data flow for control flow" is a simplified version of the example in the manual

type
  MyObject = object {.requiresInit.}

proc p() =
  # the following is valid:
  var x: MyObject
  if someCondition():
    x = a()
  else:
    x = a()
  # use x

And if we adapt that example as follows (to fill in the missing bits and make it compile)

type
  MyObject {.requiresInit.} = object 
    a: int

proc a(): MyObject =
  result = MyObject(a: 4)

proc someCondition(): bool = false
proc p() =
  # the following is valid:
  var x: MyObject
  if someCondition():
    x = a()
  else:
    x = a()

we get

/usercode/in.nim(11, 7) Error: The MyObject type doesn't have a default value. The following fields must be initialized: a.

I don't really see how my example could be legitimately invalid while the example in the manual is valid. Perhaps the documentation is wrong. The documentation goes on to immediately imply that the above example should not compile, so it's likely it's just wrong.

bung87 added a commit to bung87/Nim that referenced this issue Dec 26, 2022
@bung87 bung87 mentioned this issue Dec 26, 2022
bung87 added a commit to bung87/Nim that referenced this issue Dec 27, 2022
Araq added a commit that referenced this issue Jan 13, 2023
* fix #20253

* change NimbleStableCommit

* Update koch.nim

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
survivorm pushed a commit to survivorm/Nim that referenced this issue Feb 28, 2023
* fix nim-lang#20253

* change NimbleStableCommit

* Update koch.nim

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
capocasa pushed a commit to capocasa/Nim that referenced this issue Mar 31, 2023
* fix nim-lang#20253

* change NimbleStableCommit

* Update koch.nim

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
bung87 added a commit to bung87/Nim that referenced this issue Jul 29, 2023
* fix nim-lang#20253

* change NimbleStableCommit

* Update koch.nim

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
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

Successfully merging a pull request may close this issue.

2 participants