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

warn about destructors with setLenUninit in docs #24345

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Oct 22, 2024

The compiler normally prevents destructor calls when assigning to noinit locations (sink, variable init). But with setLenUninit the compiler can't track that the location is uninitialized, and so destructor calls are still generated. This can cause crashes since destructors may assume that the assigned location is well-formed. To deal with this, grow, add, setLen all use nodestroy to prevent any destructor calls from being generated. So the docs for setLenUninit now warn about this issue, and suggest using nodestroy.

@arnetheduck
Copy link
Contributor

a compiler warning/error would be a lot more useful :/

@metagn
Copy link
Collaborator Author

metagn commented Oct 22, 2024

I don't know if the compiler can easily track this, it's decided at runtime whether the memory is uninitialized or not. It's already unsafe to read from, it just might be hard to guess that it's unsafe to write to as well. The docs for nodestroy say it's temporary until a better solution, maybe whatever that is would make it more obvious that it needs to be used?

@arnetheduck
Copy link
Contributor

I don't know if the compiler can easily track this

Well, the way I see it, newSeqUninit should only be exposed for what in C++ is called a trivial type - it's similar to supportsCopyMem but more nuanced - there are several such traits that would be useful in general, not just for newseq but for writing optimal serialization libraries and the like.

@Araq
Copy link
Member

Araq commented Oct 22, 2024

I'm tired of this uninit bullshit. Make the compiler use the DFA and stop exposing these nukes to Nim users.

@arnetheduck
Copy link
Contributor

DFA

constraining uninit by traits is the way to only expose the non-nukes (and still allowing efficient serialization frameworks etc). dfa won't get you all the way regardless.

@Araq
Copy link
Member

Araq commented Oct 23, 2024

DFA is one thing and the other is better API design like newSeqWith(size, value) or maybe newSeqIt(4, it + 3) (which would produce @[3, 4, 5, 6]).

@arnetheduck
Copy link
Contributor

arnetheduck commented Oct 23, 2024

newSeqWith(size, value)

what do you use to implement newSeqWith? or a deque? or any other custom collection? the traits are needed generally, not just for newSeqUninit because in order to implement serializers and collections, you need to cross the type/raw-byte boundary - the std lib does this using magics in a few places which constrains the design space of the language as a whole because libraries can't do it as efficiently, turning the type system into a bottleneck.

These facilities are not for "the general nim users" - they're primitives needed to build other safe abstractions.

@c-blake
Copy link
Contributor

c-blake commented Oct 23, 2024

I think A) while there is a footgun docs should warn - so good PR & B) a proc newSeqUninit*[T: CopyMemable](...) (CopyMemable or similar name) to block the footgun at compile-time could be useful.

It hasn't yet been observed in this converation that the old newSeqUninitialized was overly constrained to [T: SomeNumber] (overly = blocking say a bool plain-old-data member types, etc.) which led me to add a crappy (but so constrained) newSeqNoInit in cligen/sysUt. So, I think [T: TheRightConstraint] may be a reasonable middle ground between "none of this BS at all" and "a totally unconstrained footgun".

Very parenthetically, it would be good for very "audit bait" things like this to be spelled one way, esp since the sub-substring "init" is also used all over the place, conventionally). I don't really care if it's {.noinit.} (back to 2011) or "uninit", but just one way, pretty please. (It's not Earth shattering or anything, but seems like needless, life-complicating variety.)

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 this pull request may close these issues.

4 participants