add --lean compiler flag; improve error message when strutils.% or addf throws by showing all useful context

[timotheecour]

• move tests/modules/tstrutils_insert_sep.nim to where it belongs, tests/stdlib/tstrutils.nim
• test on c js vm
• strutils.% now shows a more helpful error msg:

import std/strutils
let a = "a $key2 b" % ["key", "val"]

before PR:
Error: unhandled exception: invalid format string [ValueError]

after PR:
Error: unhandled exception: invalid char '$' at index: '2' for input: 'a $key2 b' [ValueError]

I found it quite useful while working on #14278; without this, you got cryptic errors when format string was wrong (eg doc/advopt.txt)
after this you get all useful context which should help you spot the error faster.
The error formatting is not set in stone so shouldn't be relied on in hardcoded tests.

future work

This can be moved to lib/std/private/miscdollars.nim to be usable in other modules that need to report parsing error with useful context:

• json
• perhaps Add rst parse option for continuing after parse error #14280 (comment)
• (EDIT) merge the pre-existing badly named tests/stdlib/tstrutil.nim with tests/stdlib/tstrutils.nim

links

• up to 20x faster jsonutils deserialization #18183 (comment)

[kaushalmodi]

If you are planning to commit this, I will close #14486.

[timotheecour]

If you are planning to commit this

ya I am planning to, will get to it

[Araq]

IMHO "invalid format string: " & formatstr as the error message would be sweet spot between usefulness and code size. Keep in mind that Nim's strategic target is embedded devices and people enjoy using the full Nim language (plus all of its stdlib) for it. As a alternative, add a specialization for --opt:size (when compileOption("opt", "size")).

[timotheecour]

sweet spot between usefulness and code size. Keep in mind that Nim's strategic target is embedded devices

raiseParseError is noinline, the difference in cgen'd code is only 27LOC (or even 16LOC if I don't allow optional msg [1]) out of 340 LOC just for addf:

nim r --stacktrace:off --excessivestacktrace:off main

import strutils
proc main()=
  echo "abc1 $foo1 " % ["goo", "bar0"]
  echo "abc $foo2 " % ["foo", "bar"]
main()

before PR:
addf is 325 LOC, invalidFormatString is 14 LOC
after PR:
addf is 325 LOC, raiseParseError is 41 LOC

nim --eval:'echo (41-14)/(325 + 14)'
0.07964601769911504

an overall 27 LOC increase (8% increase in addf) is IMO insignificant, especially given fact that any real program will dwarf that size in cgen, and raiseParseError can as mentioned be used in other contexts, for any parsing function; the code will then be reused and won't add to the code size.

There are plenty of things that will have much bigger impact on reducing code size that should be tried first, but if somehow that 8% increase is a problem, I can introduce and document a nimOptimizeSize to avoid the extra size (that flag could be used in similar situations), but I don't think we should conflate --opt:size with this, --opt:size shouldn't affect semantics, even for error messages.

[1] but msg is useful if we reuse raiseParseError in other parsing procs

[Araq]

I am not talking about "LOC" (lines of code), I'm talking about the resulting binary sizes. And while it's hardly significant for this "one little thing", these little things do add up. And on an embedded chip, I'm not willing to pay the price for debug conveniences in the final build, eventually I stop debugging my code and then I'm left with the bloat. So arguably, this should all be done for debug mode only.

I can introduce and document a nimOptimizeSize to avoid the extra size (that flag could be used in similar situations), but I don't think we should conflate --opt:size with this, --opt:size shouldn't affect semantics, even for error messages.

We already have debug vs release vs danger, adding more doesn't seem wise.

[timotheecour]

• for reference here's binary sizes, using nim r --eval:'import os; echo getFileSize("/tmp/main")'
  (du has coarser resolution, even with BLOCKSIZE=1 it hardcodes as 512)

nim c -o:/tmp/main --stacktrace:off --listfullpaths:off --excessivestacktrace:off main.nim

before PR: 80824
after PR: 85920
after PR, wo msg: 85872

same with -d:danger
before PR: 60160
after PR: 69352
after PR, wo msg: 69304

I'm not willing to pay the price for debug conveniences in the final build

fine but then let's do it properly and without conflating features (which always bites you back), via a proper dedicated flag --lean which can be combined with other flags:

• --lean: libraries (eg stdlib) can query for this and produce leaner error messasges, which does affect observable semantics even in normal conditions (raising CatchableErrors is "exceptional" but "normal")
• -d:danger -d:release --opt:size --opt:speed # affect optimization + checks/asserts; does not affect semantics in normal conditions, only affects semantics for defects
• --panics # ditto, this only affects semantics for defects

It's easy enough for a user on scarce resource to combine however they wish, eg nim r --lean -d:danger --panics:on --opt:size main

• this nicely solves fix #14905: new recommended flag for performance/benchmarking: -d:nimDisableAssertMsgs #14907 too: --lean can be used instead of -d:nimDisableAssertMsgs, with similar spirit

• --lean can be used in many other places, and a user using -d:danger will never have to worry about whether their exceptions's messages get changed just because of -d:danger, since this would be controlled by a separate flag.

• stdlib can be even more aggressive about shortening messages when --lean is passed since that's the intent; it could even be reduced to an error code, but that can be discussed separately

[Araq]

fine but then let's do it properly and without conflating features (which always bites you back), via a proper dedicated flag --lean which can be combined with other flags

When I want to optimize for size, it means I care about the size. Why shouldn't the stdlib adapt to my wish? How is --opt:size --lean simpler to use than just --opt:size? Yes, it's not only an "optimization" in the strictest sense of the word. At least make lean imply --opt:size --stackTrace:off then

[timotheecour]

PTAL, added --lean accordingly.

Why shouldn't the stdlib adapt to my wish?

because users would not expect --opt:size to affect semantics, including of messages in exceptions. Having a dedicated flag that is explicitly documented as enabling leaner messages (hence different semantics) enables even more aggressive size optimizations. So it's a win-win, at the price of 1 extra flag to learn about, but likely just for users targetting applications such as embedded.

At least make lean imply --opt:size --stackTrace:off then

done but we may need to revisit this decision soon; i'm not sure it's a good idea, because --lean can also be useful for other things, eg benchmarking or release/danger builds where user is fine about the aggressive size optimizations involving semantic changes (to exception messages etc), and --opt:size is mutually exclusive with --opt:speed

(I also made it imply --listfullpaths:off; at least these flags should be overrideable as usual, eg --lean --listfullpaths:on)

Keeping flags orthogonal is always the simplest/less buggy / most flexible; documentation is always there to help picking which flags for size/speed/debug.

[timotheecour]

ping @xflywind @Araq (i had to rebase to avoid bitrot conflicts)

[Varriount]

Is this really so important that it needs an entirely new compiler flag? How often are library writers actually going to check for this flag? And if it affects semantics, why would users use it? Affecting semantics means libraries depending on "regular" semantics may break, sometimes in unexpected ways.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.