gcsafe warning with dispatchMulti

[pb-cdunn]

I have a clean build with dispatch but a gcsafe warning with dispatchMulti.

import "cligen"

proc absolutize: int =
  echo "absolutely!"
  return 0

echo "good"

Clean:

when isMainModule:
  cligen.dispatch(absolutize, short={}, help={})

Warning:

when isMainModule:
  cligen.dispatchMulti([absolutize])

[pb-cdunn]

dataset.nim(15, 23) template/generic instantiation of `dispatchMulti` from here
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(604, 13) Warning: not GC-safe: 'parser(cmdline)' [GcUnsafe]
        parser()
              ^
dataset.nim(15, 23) template/generic instantiation of `dispatchMulti` from here
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(570, 12) Warning: not GC-safe: 'multi(cmdLineParamDispatch, dflUsage
, "  ")' [GcUnsafe]
    result = quote do:
             ^
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(889, 9) Hint: global variable declared here [GlobalVar]
      let ps = cast[seq[string]](commandLineParams())
          ^
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(720, 7) Hint: global variable declared here [GlobalVar]
    let sugg = suggestions(cmd, subCmds, subCmds)
        ^
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(720, 7) Hint: global variable declared here [GlobalVar]
    let sugg = suggestions(cmd, subCmds, subCmds)
        ^
/home/UNIXHOME/cdunn/.nimble/pkgs/cligen-0.9.19/cligen.nim(893, 55) Warning: not GC-safe: 'multiSubs(@[ps347037[1], "--help"], "", "${prelude}" &
  var pairs390280: seq[seq[string]]
  for i390281 in 0 ..< len(multiSubCmds):
    add(pairs390280, @[multiSubCmds[i390281], replace(multiSubDocs[i390281], "\n",
        " ")])
  """
  $1 {CMD}  [sub-command options & parameters]

where {CMD} is one of:

$2
$1 {-h|--help} or with no args at all prints this message.
$1 --help-syntax gives general cligen syntax help.
Run "$1 {help CMD|CMD --help}" to see help for just CMD.
Run "$1 help" to get *comprehensive* help.$3""" %
      ["dataset", addPrefix("  ", alignTable(pairs390280, 2, 2, 16, "", @[0, 1])), if (
    0 < len(cligenVersion)): "\nTop-level --version also available"
  else:
    ""
  ], "", "", false)' [GcUnsafe]
        if ps[1] in `subCmdsId`: cligenQuit(`SubsDispId`(@[ ps[1], "--help" ]))

[pb-cdunn]

I'm on Nim/devel, as of 1-2 weeks ago. I just ran nimble install cligen, so I think I'm up-to-date there.

[pb-cdunn]

--verbosity:2 of course.

[c-blake]

Hmm. Well, I can reproduce the warning chatter. It seems to run fine. So, could be false positives. E.g., I might do something that assumes dispatch/dispatchMulti which call cligenQuit are the end of life for the program which the GC safe analyzer might not understand. I do not usually run with --verbosity:2. So, I have little experience with how reliable these warnings are. Do you have much such experience?

[c-blake]

Also, I'm not against moves to not generate warnings. I don't see how that proc parser(..)=..; parser() is GCSafe for dispatch but not dispatchMulti. We can make a copy of its single argument args if we really need to, though. The other GCSafe warnings are all similar about passing around cmdLine-style arguments. Do you know of this causing you an actual issue at run-time or are you just trying to get a cleaner compile?

[pb-cdunn]

I try to eliminate all warnings. The gcsafeness has something to do with a handful of funcs in cligen I think.

There are also global-variable warnings, which are from the latest nim. It's possible that the globals are the reason for the gcsafe things.

Maybe you could mark your methods as gcsafe, with a caveat that it's a lie. Or maybe just mark your globals as threadvars, since this is always top-level anyway.

[c-blake]

Well, I'm happy to take a PR to eliminate whatever warnings are easy to deal with. Some of those globals like ps can be stuffed inside a block: pretty easily and locally. There might be a couple I cannot, though. You can do those changes, too, if you want. Some like subCmdsId and subDocsId may need to be at the same top-level as imports have to happen at. For those the threadvar solution might be all we could do.

Also, there may be a real issue with mergeParams in dispatchMulti that might even be related to the warning. It works for one level, but flattens out for 2 or more. E.g. if you put an echo cmdNames in cligen/mergeCfgEnv.nim:mergeNames and then run nim c -run test/MultiMulti.nim apple demo you see it replaces "apple" with "demo" instead of having all three levels. The one guy who was asking for tri-level dispatch seems to have gone quiet, though. Even with that bug fixed you'd need your own config file parser anyway since stdlib's parsecfg only handles two levels. So, fixing that has not been a priority for me.

[c-blake]

Just a minor note on this - I may be wrong about stuffing varibles in a block making them "non-global" from the perspective of that globals warning analyzer based on some experiments I did for that other issue you filed. I think it may only be warning if things are not in a function scope/without a call frame.

A block scope is a proper sub-scope, though. So, if we put things in a block and they still show up as global warnings then I vote we file a Nim issue (if there isn't one already) about the warning being too aggressive or being poorly phrased. False positives in warning systems make people turn them off and/or tune them out. So, either the warning should be fixed to not fire if inside a guarding block or it should read differently cueing the user that it's about a dynamic property like "without a Nim call frame" rather than a static property like "global Nim lexical scope". Well, they're both kinda "static", but hopefully you get my meaning. { We can still try to annotate them to silence the warning for you -- happy to accept a PR doing that whenever you get around to it. }

[pb-cdunn]

Hmm. I don't think a block scope would or should make something non-global, as it does not introduce a closure. I guess the issue is about multi-threading, rather than namespacing.

[c-blake]

Right. So, "global storage" more than "global visibility" (what I was calling "scope"). I still think that warning language should be more specific that it's about storage since there is an ambiguity.

[c-blake]

(Of course, being part of a "GC analysis" may make some say it's already obvious...)

[c-blake]

I was trying to get a more quiet compile, but if I annotate proc parser as {.gcsafe.} then even without --verbosity:2 we still get a warning -- just a different one -- because that call accesses the global cligenVersion (yes, assumed to be set up once in the main thread and then never touched again, but I'd be pretty surprised if people mucked about with that variable post-setup). Similar happens for proc multiId.

I also get errors if I try to annotate cligenVersion as a {.threadvar.}.

I really am sympathetic to your situation, but unless we have ideas that actually work to get a warning-less compile, I am going to close this issue. Happy to accept a PR if you can come up with a way to get a clean compile.

[pb-cdunn]

What happens if I put the cligen stuff inside main(), instead of in the top-level isMainModule block?

[c-blake]

As currently written dispatchGen/dispatch has to be at the top-level to do import which is a Nim-constraint I have no control over. I probably don't want to mess up the for-dummies simple operation just to appease warnings that may not indicate any real problem.

It would probably be easy to factor the actual imports into a separate macro, cligenImport, say and a dispatchGenNoImport/dispatchNoImport pair. dispatch/dispatchGen could then just do cligenImport; dispatchNoImport, etc. Someone in your situation wanting to put everything into a proc could then do the cligenImport at the top level, and then do a proc main(): auto = dispatchNoImport(...). It would be necessary to also call that main() afterward, though.

Further, even doing all that and imposing that burden on warning-avoiders, I'm not at all certain the cligenVersion global would not still trigger warnings. My guess is that if you used cligenVersion in a regular dispatch setting (like test/Version.nim) that you would also see the warning.

I mean, ultimately it probably is GC-unsafe in some technical sense, but also just unusual practice to ever do more than set the version string just once in a program's lifetime. That's unusual enough to just not support, which is why some annotation would be an acceptable solution, but I couldn't find one. If you can find one, I am happy to use it.

[pb-cdunn]

Why does the macro need to "import"? Why not simply export the needed symbols in cligen.nim?

[c-blake]

Yeah...I suppose I could export what's needed. I still think the warning would not go away, though, as long as the protocol is to communicate via a global. I think there should be a way to mark such things safe to block this warning. When/if there is, I'm happy to use it.

[c-blake]

Ok. I now have cligen.nim do all the needed imports and then export. So, it is possible to call dispatch or dispatchMulti from within a sub-scope. See test/SubScope.nim. I like that as it's a bit cleaner overall. Good call.

I also fixed the mergeNames problem which had nothing to do with any GC bug that I can see. AFAICT there is no safety problem and this is a false positive warning.

I was also able to silence the warning about cligenVersion with a {.push hint[GlobalVar]: off.} / {.pop.} pair around its declaration. I don't personally see any problem using a global to communicate a static value, especially since it's namespace-prefixed with "cligen".

As I mentioned, if I annotate, e.g. parser() as gcsafe it just transforms from a [GcUnsafe] warning to [GcUnsafe2].

Maybe there is some similar hint push we can do in a strategic place (or maybe around the entire dispatch() call? Happy to accept any PR along those lines. {.push hint[GcUnsafe]: off.} gave me an Error: invalid pragma compile error.

We could probably have cligenVersion passed as a macro parameter instead of using a global, but it's been that way a long time and that would break at least one other user's code. It also looks inconvenient in a dispatchMulti setting. Really, the usual convention is that a whole program has just one version, not each separate subcommand. So, "set once" is easy with a global and tediously repetitive to do with every subcommand. A lot of programs just have a global var for the version string. So, it feels natural, too, and I'd prefer to figure out some other way to silence these warnings. I'll re-open this issue since it's not really entirely "done".

The new critbits usage for unambiguous prefix matching also started generating some [ProveField] messages. So, we may need some other targeted push pragmas.