Fix precision/rounding corruption when cos/sin/tan throw - #265
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
cos, sin and tan all raise Ctor.precision (and set Ctor.rounding to 1) before doing their trig computation, then lower it back to the original value once that computation returns. If anything in between throws, for example toLessThanHalfPi's internal pi lookup once the raised precision exceeds the hard-coded 1025-digit limit on the PI constant, the lowering step never runs and the constructor is left with the raised precision and rounding mode permanently, even though the caller only asked for a single trig call. getLn10 already resets Ctor.precision before throwing for the same reason in the log/ln code path, so this brings cos/sin/tan in line with that existing convention using a plain try/finally, which also covers the rounding mode that getLn10's approach does not. Added a regression case to each of the cos, sin and tan test modules that raises the precision high enough to trip the pi lookup's limit, and checks that Decimal.precision and Decimal.rounding come back unchanged afterwards. Confirmed they fail without the fix (precision comes back changed) and pass with it, and ran the full suite locally with no other regressions. Fixes MikeMcl#97 for these three methods; the same save/restore pattern shows up in a few other trig-adjacent methods (atan2 in particular) that this doesn't touch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cos,sinandtanall follow the same pattern: saveCtor.precision/Ctor.rounding, raiseCtor.precision(and pin rounding to 1) for the internal computation, do the computation, then put the two settings back before returning.The problem is that "put the two settings back" only runs if the computation in between doesn't throw. It can throw:
toLessThanHalfPi, which bothcosandsincall to reduce the argument, does a lookup of the internal Pi constant at whatever the currently-elevated precision is, and that lookup throwsPrecision limit exceededonce the requested precision goes past the 1025 digits Pi is hard-coded to. Once that happens, the constructor is left at the raised precision (and rounding mode 1) permanently, well after thecos()/sin()/tan()call has returned control to the caller via a caught exception.tanhas the same problem twice over, since it delegates tosininternally and also does its own precision/rounding save-restore around that call.getLn10already has to deal with this in theln/logcode path and resetsCtor.precisionright before it throws for the same reason, so there's precedent for treating this as worth fixing rather than living with it. I used a plaintry/finallyinstead of followinggetLn10's exact approach, mostly because it also putsCtor.roundingback, whichgetLn10's fix does not need to since only precision gets elevated in that code path.Applied the same change to
decimal.mjs, which has its own copy of these three functions.Testing
Added one case to each of
test/modules/cos.js,sin.jsandtan.jsthat pushes the precision past the point where the Pi lookup throws and checksDecimal.precision/Decimal.roundingafterwards. Confirmed each one fails on master (precision comes back changed) and passes with this branch, then ran the full suite (npm test) with everything green.This addresses the
cos/sin/tanpart of #97. A few other methods share the underlying "save/raise/restore" shape without atry/finally(atan2in particular, in itsx.s < 0branch), which I left alone here since #97 is really asking for a broader look at the pattern across the library rather than a single fix; happy to take a look at those too if that would help.