fix protect/unprotect rchk issues

[alexcb]

rjson has two functions with loops that build up elements and protects them as they are created.

upon an error, a call to UNPROTECT( objs ); is made where objs is an int which is incremented for each PROTECT; however the rchk PROTECT/UNPROTECT linter does not support this form (however R does, and rjson predates these rchk checks).

             +rcheck | Function parseArray
             +rcheck |   [UP] protect stack is too deep, unprotecting all variables, results will be incomplete
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:508
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:514
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:535
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:585
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:600
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:604
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:614

             +rcheck | Function parseList
             +rcheck |   [UP] protect stack is too deep, unprotecting all variables, results will be incomplete
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:637
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:642
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:650
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:667
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:672
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:680
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:691
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:707
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:727
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:741
             +rcheck |   [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/hWyBjPqk/rjson/src/parser.c:755
             +rcheck | Analyzed 166 functions, traversed 6001242 states.

I then also tried calling UNPROTECT in a loop ( see 9d05c9b ) but that introduced other issues.

A CRAN update is blocked until this is resolved.

[brodieG]

I had a similar problem with the UP checks. Conditional UNPROTECT/PROTECT in loops do not work well for rchk. However, in this case (at least looking at parseArray), it seems you don't need to be incrementing the protection counter at all, as you'll only add one object to the PROTECT stack.

What I would do that I think will fix the problem is do one PROTECT_WITH_INDEX outside of the loop on a dummy SEXP (e.g. R_NilValue), REPROTECT inside the loop, and instead of UNPROTECT inside the loop, set a failure flag, break out of the loop. You can then just run the existing code if the failure flag is not set (and skip it if not), and UNPROTECT once at the very end since the failure will not affect how many things you have on the protect stack (either the dummy object, or whatever you put on there in the loop).

[alexcb]

I made some changes in https://github.com/alexcb/rjson/tree/simplify-protection-logic which gets the warnings down to a single imbalance:

#output from running: earthly -i +rcheck
...
             +rcheck | Function parseArray
             +rcheck |   [PB] has possible protection stack imbalance /rchk/packages/build/Wc7jXdEM/rjson/src/parser.c:611
             +rcheck | Analyzed 166 functions, traversed 1445 states.

I'm scratching my head on what's causing this one.

[brodieG]

My 2c of advice is to forsake the seeming convenience of the early return. It makes keeping track of the logic harder, and it makes it easier to introduce bugs later even if the code is correct when first written (e.g. you have one return that needs to UNPROTECT(2) vs the others; if you add more code later you need to make sure that remains right everywhere).

I reviewed the code and at first I thought it was balanced, although it is complicated enough I wouldn't have bet my life on it. Then, I searched for each return statement in the function, and noticed that this return is unbalanced. There is the original PROTECT left that needs to be UNPROTECT(1), AFAICT (untested, so I could be wrong).

I don't trust myself to be able to rigorously maintain multiple exit points from my functions, so I very rarely use multiple returns, even though I am often tempted to do so.

Even if the code does turn out to be balanced (and I'm wrong), it still worth writing simpler code that rchk can understand so that when it does issue a true positive it can be fixed (rchk just saved my bacon on the submission I'm preparing).

[alexcb]

Great eyes @brodieG, that was a bug I introduced with this refactor. It's been fixed in bb313d7

I agree it's complicated code to look over -- I wrote it over a decade ago, and find myself scratching my head at times.

so I very rarely use multiple returns

perhaps having an error label at the end of the function with a goto error could make life easier; i've used that pattern in several other C projects. Perhaps down the road this might be useful, but I think keeping it as-is for the time being is less risky.

Thank you very much for helping me get this cleaned up to the state where I can try a new update-submission to CRAN.