Mutability and returning existing Decimals from methods

[mcpower]

tl;dr: Some methods can return this, an argument or one of the static Decimal.dConst constants. If users mutate those, they might see unintended consequences. WAI or bug? If it's a bug, dealing with it in a performant way is complicated.

---

This is a contrived function, but bear with me.

function f(a: Decimal, b: Decimal): Decimal {
  // Calculates 11a + 11b in a weird way.
  const sum = a.add(b);
  sum.exponent++;
  // Sum is now 10(a+b) = 10a + 10b.
  return sum.add(a).add(b);
}

This function works as expected... until b is 0. Then the first .add will return a as-is, setting sum to be a. The exponent increment multiplies a by 10, and the final return is 20a. This also mutates the value of a which is probably unexpected to anyone calling this function. The same applies if a is 0, as the first .add will return b in that case.

A similar thing happens with any function that returns this, an argument, or one of the static Decimal.dConst constants. If users mutate those, they might see unintended consequences - especially the constants, as they are assumed to be immutable and widely used in internal break_eternity functions.

Is this working as intended, or is this a bug in break_eternity?

If this is a bug in break_eternity, then we'll need to create copies (and therefore new allocations) of all return values if they could be one of the aforementioned "should not be mutated" values...

...but copies would presumably slow down code - even code which doesn't mutate Decimals...
...so a user-controlled switch could be added to say "I swear I will never mutate a Decimal"...

...but then internal break_eternity code like d_lambertw, which doesn't mutate decimals, will be slowed down if that is false...
...unless internal code has an override for the above switch which always sets it to false while inside of an internal break_eternity function...
...which will cause problems if the function throws an error, as the user's original choice wouldn't be restored...
...but that could be caught using a try/except...
...but try/except would presumably slow down code.

I haven't benchmarked the exact slowdown of (creating copies of Decimals all the time) vs. (giving users the option to not create copies) vs (always setting that to "don't copy" inside internal functions, with a try/except statement to restore the user's choice). Each additional performance mitigation introduces more and more complicated code, which is harder to maintain.

Perhaps it's worth it to enforce immutability by deprecating the .fromX methods and properties, instead of making copies selectively?

[Patashu]

Yeah, it's gross. Some of my thinking:

For both this and break_infinity.js, what I would love is if javascript implemented structs ala C#, and I could just pass these around as structs. It would be a ton more efficient too since they wouldn't go on the heap and have to get GC'd later, and there'd be no mutability related problems since you couldn't accidentally have the same object be in two places (since it's not an object).

But since we don't live in a language where structs exist, I've never been sure what the best option would be. I think from a performance perspective it'd be optimal to make all functions mutate in place and it's on the end user to explicitly clone the Decimal when they want a new copy, but break_infinity.js was designed not being allowed to do that (since it was a drop-in replacement for Decimal.js), and break_eternity.js just kind of inherits that way of thinking. Plus, it'd be a lot more prone to error (the moment you accidentally don't copy before mutating but think you are, you get Inscrutable Bugs). You could make _copy and _nocopy versions of (most? all? if there's two parameters which one gets mutated?) functions and I guess that'd work, but it gets pretty ugly looking.

I dunno what the best solution is - there basically isn't one because javascript isn't a clever enough language, you have to pick which end of the performance<->ease of use tradeoff you want, and I see the benefits both in 'default to mutability to go fast' and 'default to immutability to prevent surprising bugs'.

As for the constants, maybe using Object.freeze on them isn't too slow? No clue what that does under the hood. I do agree that THAT would be a surprising and janky enough bug that it should be impossible. At least when it comes to doing things like directly setting fields it carries an air of 'this isn't a precise mathematical operation so whether or not this does something meaningful is now on you', but accidentally modifying a library constant is gross.

I don't think that we should totally prevent the user from mutating fields directly if they've thought about it and it's what they need to do to make their program run faster. A lot of an incremental game's overhead ends up being futzing with its number library, especially creating and disposing new Decimal instances every tick, and we don't want to get in the way of that.

[mcpower]

You could make _copy and _nocopy versions of (most? all? if there's two parameters which one gets mutated?) functions and I guess that'd work, but it gets pretty ugly looking.

I think it could work:

• all methods are "copy" unless stated otherwise (i.e. you need to opt into the dangerous no-copy optimisations) so we don't break existing users
• nocopy methods are never static (i.e. this is always defined)
• this always gets mutated in the nocopy methods, not any arguments
• for operations where the order of operations matters, like sub_nocopy and div_nocopy, have rsub_nocopy and rdiv_nocopy which is the same thing, but with the operation reversed
  • some operations like log(base: DecimalSource): Decimal don't make sense in a "reversed" order
  • there are no operations with three or more Decimal operands
• for methods which have both a copy and a nocopy method, implement the copy method using the nocopy method like:

  class Decimal {
    // ...
    add(value: DecimalSource): Decimal {
      return this.copy().add_nocopy(value);
    }
  
    add_nocopy(value: DecimalSource): this {
      value = D(value);
      // ...
    }

• add a .copy helper method to help users of the nocopy methods, like internal break_eternity.js methods, to make things a bit more readable - add would look like new Decimal(this).add_nocopy(value) without it

(warning: bikeshedding) My only gripe is the naming - "nocopy" is ugly. Here's an excerpt from an optimised d_lambertw, which mixes copy methods (three calls) and nocopy methods:

// wewz should not be used after this line (it's the same as wn)
wn = wewz.div_nocopy(w.add(1).sub_nocopy(w.add(2).mul_nocopy(wewz).div_nocopy(w.mul(2).add_nocopy(2)))).rsub_nocopy(w);

This will be even uglier when the "avoid fromNumber calls by passing in Decimal.dOne instead of 1" optimisation is merged in (which is currently unsafe due to possibly returning arguments from methods).

In Ruby, there's a nice convention for "methods which mutate this", which is adding an exclamation mark at the end of the method name. That doesn't work in JavaScript, unfortunately. Looking at the ECMAScript spec, we could use a trailing $ or _ instead. However, a trailing $ might confuse people who are familiar with frameworks where that is the naming convention for observables, and a trailing _ might confuse people who are familiar with style guides where that is the naming convention for private methods. Maybe one of the below?

• addNC
• addMutate
• addM

---

As for the constants, maybe using Object.freeze on them isn't too slow?

The call to Object.freeze itself is, IIRC, a bit slow, but if we're calling that on ~10 constants when the library is loaded I don't think it's a big deal. Doing a quick benchmark, frozen objects seem to be 3%-8% slower than using non-frozen objects when doing operations like tetrate and lambertw (on layer 1 or 2).

I think if we do end up doing the "nocopy" thing above, we'll never return a constant to the user, so there's probably no need to use Object.freeze.

[MathCookie17]

It can be argued that this issue is now resolved. I did not make the "nocopy" versions, but I did ensure that Decimal methods no longer return the constants directly, so except in the cases where mutation is outright expected (fromDecimal, for example), Decimal methods are always non-mutating. Since I didn't make the nocopy versions, though, I'll leave it up to mcpower whether this issue should be considered closed or not.