search

c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
496 stars 23 forks source link

unblock nim https://github.com/nim-lang/Nim/pull/17807 typeof(voidExpr) #193

Closed timotheecour closed 3 years ago

timotheecour commented 3 years ago

/cc @c-blake that was the only package that broke in https://github.com/nim-lang/Nim/pull/17807; the code here is forward and backward compatible wrt https://github.com/nim-lang/Nim/pull/17807, and is regardless of https://github.com/nim-lang/Nim/pull/17807 an improvement as it avoids a call to compiles() which causes extra compilation for the underlying expression

the p() in compiles(type(p())) seems suspicious given that p is apparently called also via p(hlp, usage=use, prefix=pfx, skipHelp=skipHlp, noHdr=noUHdr), but that's a pre-existing issue unrelated to this PR

c-blake commented 3 years ago

The p() is called in the when (and in the try: discard p(hlp,...) line below) since the cligenHelp call sites only pass the generated dispatcher name as the first arg and we want to test only the return type, not the whole type signature.

The whole when/else is only needed because dispatchers inherit returns from the wrapped proc and because you cannot discard a void and because discard is required for non-void. I could instead add {.discardable.} to generated dispatchers and ditch that when/else entirely.

EDIT: or maybe your new typeOrVoid tests only the return value type anyway? In that case we could also drop the parens/call. Sorry...Early in the AM here.

timotheecour commented 3 years ago

EDIT: or maybe your new typeOrVoid tests only the return value type anyway? In that case we could also drop the parens/call. Sorry...Early in the AM here.

that would have different semantics from the code before this PR, because it'd then consider the function type (not the expression/statement resulting in evaluating p);

this PR should still be correct (ie preserving the semantics) but the p() still looks funky, unless p() has same type as p(hlp, usage=use, prefix=pfx, skipHelp=skipHlp, noHdr=noUHdr) (ie, all args are optional and no overloads of p())

there are other options too, eg a maybeDiscard[T](a: T) which discards if T is not void

timotheecour commented 3 years ago

@c-blake oh btw, can you please push a new tag? the reason being, CI still fails, see https://github.com/nim-lang/Nim/pull/17807/checks?check_run_id=2399022131 , and I'm suspecting it's because it picks latest tagged release which doesn't pickup HEAD of cligen for these packages that depend on cligen: nimterop, prologue (see https://github.com/nim-lang/Nim/pull/17807)

not 100% sure whether this will fix CI but worth trying

c-blake commented 3 years ago

Yeah, yeah. The PR works ok. I was just trying to explain/motivate the funkiness of the p() "call" (which does have your all-defaulted properties, though no-overload may always be a bit more of a stretch..I do think of my namespace prefixing as guarding against collisions/carving out/declaring others shouldn't). There are a few options that maybe look less weird.

I have some edits to your fix (when declared(typeOrVoid) basically, removing the https link and cleaning up the TODO bit) that I am working on. I want to test against a freshly compiled Nim and should finish in an hour or so. I'll stamp a new version tag after that. I was actually going to ask if I needed to do that for the CI.

c-blake commented 3 years ago

So, I stamped a new version number for you.

Anyway, this p() aspect seems more like a "looks weird" than "actually a problem", and "looks weird" can be pretty subjective. I could make it safeer by including all the parameters.

Making dispatchFoo {.discardable.} would also eliminate the last branch in cligenQuit and it would clearly work at least as far back as nim-0.19.2 which I still support (I guess some Linux distros are veeeery slow to update Nim versions, if they have them at all) for the core functionality (everything all the way back seems impractical and "if you want fancy-new-thing, upgrade your compiler" seems mild).

Your maybeDiscard might work back then, too, but generics and void stuff has probably evolved more than discardable or at least has a potentially larger interaction surface area. So, there's probably more a chance that between 0.19.2 and the present there will have been Nim versions where that failed whereas discardable is used pretty heavily in the stdlib.

Also I doubt anyone out there is likely to call a dispatchFoo manually/directly unless they maybe wrote their own dispatchMulti (or multi-dispatch-like thing) yet still using cligen's dispatchGen macro which also seems pretty unlikely.

You said you wrote your own private cligen-like thing. What did you do? I imagine one could also just factor out the help string construction from argument parsing and not have to call the dispatcher for the effect of dumping help. Or also just not do that in a multi-command.

Anyway, I'm not against cleaning that up, but it also seems not urgent.

timotheecour commented 3 years ago

So, I stamped a new version number for you.

thanks, that did the trick, CI became green (until i added a changelog with [skip ci], unrelated issue)

You said you wrote your own private cligen-like thing. What did you do?

just checked, i'm not handling this properly, but should (probably with maybeDiscard); i still need to cleanup internal dependencies before publishing it as a nimble package... it probably doesn't have all the features from cligen but it does have real simplifications that you could reuse in cligen too ; i just need to get to it

c-blake commented 3 years ago

Yeah...maybeDiscard may be the way to go for me here, too..maybe for cligenQuit as well. Adapting at the call site is safer than the def site of the wrapper on the off chance people may want to call dispatchFoo as an "FFI like" alternate calling convention. One place that might be useful is in some run-time testing with a bunch of synthetic CL arg lists..Not sure I recall correctly, but your vitanim might do that.

c-blake commented 3 years ago

(In general my same call site vs. def site observation may motivate having maybeDiscard in the stdlib somewhere.)

c-blake commented 3 years ago

Well, I went with discarder. It's actually kind of tricky to name because the operation is more "discard If Possible". I have no idea what would be the least confusing name...forcedDiscard, discardVoid, or voidDiscard, non-type-but-emphasized Discard, alwaysDiscard, etc. I do see why Araq would not want a mistaken discard of a void to compile, though, since regular code wants to catch such mistakes.

c-blake commented 3 years ago

Well, I just punched a new release for unrelated reasons with the new fix. Hopefully I don't break the Nim CI...