Keep assert and doAssert in system

[metagn]

Abstract

This is an RFC for this PR https://github.com/nim-lang/Nim/pull/20354. This PR can be used as a reference implementation for this RFC.

Add the assert and doAssert symbols, moved out from the system module in https://github.com/nim-lang/Nim/pull/19599, back to the system module.

Motivation

assert and doAssert are extremely commonly used, from tests to examples. In other languages it is occasionally part of the syntax. It would be a massive inconvenience to import a module for it every time. Not to mention that AssertionDefect is still part of the system module.

Their implementations are also reasonably tiny. They are not likely to affect compile time by any noticeable margin.

Description

The PR made for this RFC at https://github.com/nim-lang/Nim/pull/20354 showcases the implementation for it that I am thinking of.

Code Examples

No response

Backwards Compatibility

Only modules that use raiseAssert, failedAssertImpl, onFailedAssert, doAssertRaises will need to import std/assertions after this is implemented. Meaning the amount of required std/assertions imports will go down significantly. There is no backwards incompatibility.

[Araq]

The problem with assertions is that they got their own switch --assertions:off whereas alternative assertion implementations use something like -d:useSysAssert. In my ideal world we would use custom assertion implementations more often so that I can decide more easily per library which ones to keep in a production build.

I also don't like the current design of assert producing an exception, it should simply call quit.

[metagn]

It should be easy for assert to require compileOption("assertions") and not defined(nimDisableSystemAssert). Then --assertions can also apply to custom implementations.

I think the way Nim's assert is designed is in line with how popular languages do it (throwing a Defect), therefore they are predictable, except that they are possible to disable. So I don't think it's bad from an optics perspective.

I don't think alternative implementations should also use the symbols assert and doAssert. Because then you would have to check whether or not every module that uses them imported std/assertions (which would be a very large portion) or a custom assertions implementation. If you use a template wrapper that decides which assert to use on a compile option, then it's easier to find that template definition than it is to find each import.

[mratsim]

The problem with assertions is that they got their own switch --assertions:off whereas alternative assertion implementations use something like -d:useSysAssert. In my ideal world we would use custom assertion implementations more often so that I can decide more easily per library which ones to keep in a production build.

I also don't like the current design of assert producing an exception, it should simply call quit.

This is what I do in Taskpools and Weave:

https://github.com/status-im/nim-taskpools/blob/17e8479a/taskpools/instrumentation/contracts.nim

in particular, in a multithreading context, you need to had more context, like threadID to begin investigating bugs.

And in other contexts like embedded, you shouldn't allocate, so no exceptions. And assertions are defects anyway.

[Araq]

And in other contexts like embedded, you shouldn't allocate, so no exceptions.

It's easy enough not to allocate with exceptions after an initial setup:

let ve = ValueError(msg: "value error")

proc p() =
  ...
  raise ve # look, no allocation!

[metagn]

Maybe we could make assert call an assertImpl symbol in scope if found, and use the default assert implementation otherwise? That way custom assert implementations can override assertImpl. Any problem with this would also be a problem with different versions of assert IMO.

[metagn]

Since the PR got closed and this didn't, even if it was a mistake, I'll ask for clarification on the reason why.

If the problem is that we can't have custom assert implementations ever, I have a local branch ready that implements the assertImpl idea that I can make a PR for.

If the problem is that we can't use the name assert for custom assert implementations, then I don't get why. It should have the same signature every time, allowing the possibility of different signatures makes it more confusing.

I don't know what other problem there could be.

[Araq]

system.nim should be minimal. It should only contain what is necessary and cannot be implemented somewhere else. One criterion that is reasonably objective is whether the feature needs a .magic annotation or not and assertions don't.

[metagn]

Without comments and whitespace the assertion implementation in https://github.com/nim-lang/Nim/pull/20354 is 25 lines. That is nothing compared to the rest of system. Not to mention that as seen in the PR other parts of system already import std/assertions to use assert so it would have to be compiled regardless.

In most other languages assert is implemented as a magic/language feature. I don't know if that counts towards anything but it might mean people expect it to be there out of the gate, or that we're spoiled by how powerful the language is. Although technically assert uses instantiationInfo and compileOption which are magics, so maybe it's no different. The same applies to Slice/.. and BackwardsIndex for the most part.

A related situation is fields which is implemented as a magic for other parts of system to use (== for objects and repr on ARC). Technically this doesn't need to be a magic, it would be better as a macro, but it would require importing macros in system which is not viable. Maybe we could move object == and ARC repr out of system, but this is not always guaranteed to be possible.

I'm a bit tunnel visioned on the ergonomics so I can't comment further. I think other people's opinions are also important.

[Araq]

It doesn't matter if it's 25 lines or not. There is assert and then doAssert and then doAssertRaises and some mechanism to override defaults and error messages etc.

[metagn]

Not to nitpick, there is no doAssertRaises in the PR. Only assert and doAssert are moved back. Meaning the entire implementation system uses is 25 lines, vs importing std/assertions which has the extra stuff.

[metagn]

Alright whatever