The return value of KTransformation.toKPattern() is not guaranteed to be immutable

[Wyverex42]

Steps to reproduce the issue

• Call KPuzzle.algToTransformation(new Alg()).toKPattern()
• Change some data in the resulting KPattern, e.g. swap some edges
• Call KPuzzle.defaultPattern()
• Observe that the default pattern has changed

Observed behaviour

I've written a pseudo-scramble algorithm that scrambles just a part of the cube based on a mask (while maintaining parity) and sometimes the whole cube was messed up after a few scrambles. I found that there is an early out in applyTransformationDataToKPatternData that returns the pattern data for a given orbit as-is if the transformation is the identity transform.

I assume this was done as an optimization.

Now the question is what the expectations around KPatterns are. Is each pattern supposed to be a separate instance? The fact that the library seems to return new KPatterns in various places when it could return an existing one, seems to suggest to me that they are meant to be instanced. If that's the case, then this optimization breaks that assumption.

I don't know if anyone else has tried to change KPatternData directly, so I wouldn't be surprised if that use case is unsupported. However, my assumption is that this behavior wasn't intended?

🖼 Screenshots

No response

Expected behaviour

KPatterns are separate instances

Environment

node v21.6.0

Additional info

No response

[lgarron]

You've hit on a very salient API point. The core issue here is that immutable objects in JS are not super straightforward, and have performance issues (or at least used to). But immutability gets us some very useful properties. The API I'd like to implement is actually pretty close to Rust semantics:

• Immutable by default.
• Mutable when there is a single owner.
• Clone an immutable object if you want a mutable copy.

The problem is that JavaScript mutability is scoped by the object, not its ownership. So I've put off the issue. I suspect the best approach is to:

• Move the data into private fields.
• Provide read-only accessors for the data if needed.
• Provide a version of the class that allows modifications, and either:
  • Provide a function to freeze the object.
  • Let people keep it mutable and hope they don't accidentally rely on immutability properties.

One way to avoid the latter would be to have some sort of mutation class, similar to a builder but specifically for mutating objects. We already have this kind of operation for Move, but it would have to be pretty flexible for KPattern and KTransformation. Other options might include:

• Accessors with copy-on-write semantics.
• A special handle to modify an object, which you can only (optionally) get at construction time.

I don't really have a great answer, but I would recommend the following for now:

• Treat KPattern and KTransformation as immutable.
• Use a function like this when you need a mutable version:

function mutationSafeClone(kpattern: KPattern): KPattern {
  return new KPattern(kpattern.kpuzzle, structuredClone(kpattern.patternData));
}

This is in fact what we do inside the codebase when we need a mutable copy.

This all needs a much better answer, but given that you're the first person to run into this issue in 2 years I'm hoping structuredClone(…) works for you until we have a long-term approach.

[Wyverex42]

Yeah, I'm using the cloning approach as a workaround and it's good enough. I just have to keep this in mind whenever I want to make changes to a pattern. But those cases should be quite rare.

I mainly wanted to raise visibility on this issue in case it wasn't known. I agree with your assessment and it sounds like a lot of work for a rather small benefit currently.