Hejsil / zig-clap

Command line argument parsing library
MIT License
939 stars 67 forks source link

clap.Param(clap.Help) can fail if we exceed EvalBranchQuota #42

Closed nfisher1226 closed 3 years ago

nfisher1226 commented 3 years ago
/usr/local/lib/zig/std/mem.zig:819:5: error: evaluation exceeded 1000 backwards branches
    while (i < slice.len) : (i += 1) {
    ^
/usr/local/lib/zig/std/mem.zig:804:28: note: called from here
    return indexOfScalarPos(T, slice, 0, value);
                           ^
/usr/local/lib/zig/std/mem.zig:790:41: note: called from here
    while (begin < end and indexOfScalar(T, values_to_strip, slice[begin]) != null) : (begin += 1) {}
                                        ^
/home/nathan/src/zcore/lib/zig-clap/clap.zig:121:49: note: called from here
    return Param(Help){ .id = .{ .msg = mem.trim(u8, line, " \t") } };
                                                ^
/home/nathan/src/zcore/lib/zig-clap/clap.zig:101:29: note: called from here
    var res = parseParamRest(it.rest());
                            ^
./src/main.zig:14:18: note: called from here
  clap.parseParam("-q, --quiet            Never display a header.") catch unreachable,

I've managed to work around this by enclosing all of the comptime in a named block and setting the quota within.

const params = comptime par: {
  @setEvalBranchQuota(1200);
  break :par [_]clap.Param(clap.Help){
   ... <more code> ...
  };
};

This works but is less than ergonomic and requires finding a workaround by the use of the library. Not sure if there's a way to fix it within zig-clap, but at the least I would suggest showing an example of the issue and workaround in the documentation.

Hejsil commented 3 years ago

an example of the issue and workaround in the documentation.

Yea, maybe a good idea. It might also be possible to just set the eval quota inside parseParam to something high (the quota is only there to prevent infinite loops, which we have none of. I'll look into it later :+1:

nfisher1226 commented 3 years ago

It might also be possible to just set the eval quota inside parseParam to something high

I thought of that. Only problem I see is you're then locking in that value, so if somebody tried to go over it that's a failure point. It also makes the function comptime only, but that's probably fine.

Hejsil commented 3 years ago

Seems we can indeed set the eval branch quota inside parseParam. The problem is that you probably can't override it from outside anymore, so we have to choose a very good default (std.math.maxInt(u32) is an option 🤔). Also, this does not make the function comptime only.

I guess since this library is not 1.0.0 we can play around, so I might just push a change that sets the eval quota to something large. It can be removed later in a breaking change if that turns out to be a bad idea.

nfisher1226 commented 3 years ago

Would it be possible to make it an optional parameter to the function? Then if it's passed in a null, it uses the sensible default.