Add Validation.valueComparisonStrategy

[zepumph]

For a while there has a been a discrepancy between Property deep equality and Validation's version. Currently we use reference equality to see if validValues match:

https://github.com/phetsims/axon/blob/40e574c241fce41a37dec0539b93a0c70fcd35cc/js/Validation.ts#L290

This is not acceptable, and I'm glad we got to a point where we want to change! From a conversation over in https://github.com/phetsims/studio/issues/291 with @samreid, we should add a more general strategy that helps us get this coverage in our central validation code.

While we are at it, we can loop into the conversation ReadOnlyProperty.useDeepEquality. This can also use our more useful, general algorithm.

In terms of the size of this change, it is not huge. I have Validator.valueCompareStrategy working well in my working copy, and we will just want to expand that to Property next.

[zepumph]

Ok. I got to a commit point on the feature, and tested and noted that this fixed the original case. I also wrote unit tests to assist in development.

Next steps:

• [x] A couple TODOs
• [x] Promote Property.useDeepEquality to use this (could be done as a partial commit, leaving TinyProperty's implementation alone
• [x] Promote TinyProperty.useDeepEquality to this whole feature set.
• [x] Convert all useDeepEquality usages to equalsFunction valueComparisonStrategy
• [ ] What sun components, like AquaRadioButton, should be updated to respect valueComparisonStrategy instead of hard coding triple equals?

[zepumph]

ReadOnlyProperty.useDeepEquality is no more. We now use valueComparisonStratetgy but only care if we support equalsFunction. This, in my opinion, is only half way, but the inner API can improve and expand more gradually, at a lower priority.

I will conclude my work for this week by notifying the google group to use valueComparisonStrategy instead of useDeepEquality.

[zepumph]

I posted to the google group. Next steps here would be to factor out the valueComparisonStrategy logic from Validation to be used there and in TinyProperty. Also convert TinyProperty.useDeepEquality to valueComparisonStrategy as well. I'm going to co-assign @samreid in case he can get to this sooner than I could, which would be march.

[samreid]

@pixelzoom identified a major problem in https://github.com/phetsims/studio/issues/291#issuecomment-1412924563.

The problem is that RadioButtonInteractionStateProperty has this logic:

  public constructor( buttonModel: ButtonModel, property: TProperty<T>, value: T ) {
    super(
      [ buttonModel.focusedProperty, buttonModel.overProperty, buttonModel.looksOverProperty, buttonModel.looksPressedProperty, property ],
      ( focused, over, looksOver, looksPressed, propertyValue ) => {
        const isSelected = ( propertyValue === value );

Note that the radio button checks whether it matches a Property value via ===. This is incompatible with how Property and Validation want to be able to do different equality tests. But search the project for occurrences of .value === and you'll see logic in AquaRadioButton, ComboBoxButton, ToggleNode, RectangularToggleButton, etc.

I am very skeptical that we can efficiently and accurately convert every occurrence of === to use a different equality test. And even if we do, it is going to complicate many usage sites. Also, what do we do if we have two objects that require a different equality test (other than ===) but there is no associated Property?

Property has had support for .equals via isDeepEquals but until now the only equality check used in Sun components has been ===.

So @zepumph and I will need to discuss how to proceed for this issue.

[zepumph]

As noted in https://github.com/phetsims/studio/issues/291#issuecomment-1414042025, I do not believe that that problem should bleed into this issue, though we could add support for valueComparisonStrategy across our common code, it would most likely not be worth it at all. We have not had a need for it to this point. And the studio issue outlines how to proceed with a PhET-iO solution.

[samreid]

I reviewed what's been committed above, and I agree valueComparisonStrategy: 'equalsFunction' is much better than useDeepEquals, which was inaccurately named to begin with. It feels inconsistent to use valueComparisonStrategy in Property and === elsewhere. I agree we have not had a need for it until this point, but that is by chance--radio buttons and other components often point to references. We saw a case in https://github.com/phetsims/studio/issues/291#issuecomment-1414042025 where if the radio buttons were using the same equality test as the Property, there would not have been a problem. We have since found a better solution. So maybe at a minimum making sure it's well documented and clear to the team that even though Property supports valueComparisonStrategy, many libraries like sun do not.

But if we want to respect the value comparison strategy in Property at usage sites, it would be like this:

Subject: [PATCH] Address TODO, see https://github.com/phetsims/chipper/issues/1360
---
Index: js/AquaRadioButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/AquaRadioButton.ts b/js/AquaRadioButton.ts
--- a/js/AquaRadioButton.ts (revision cb5784eaf0e86d0039aa83e722dd92a24fbe818a)
+++ b/js/AquaRadioButton.ts (date 1675573870956)
@@ -141,7 +141,7 @@

     // sync control with model
     const syncWithModel = ( newValue: T ) => {
-      selectedNode.visible = ( newValue === value );
+      selectedNode.visible = property.areValuesEqual( newValue, value );
       deselectedNode.visible = !selectedNode.visible;
     };
     property.link( syncWithModel );

Also, for the review, it seems unfortunate that TinyEmitter still uses useDeepEquality instead of the new API.

[samreid]

@jessegreenberg and I reviewed this issue and agreed there is more to do. However, we agreed it is not a blocking problem, in part, because it is not a new problem. ReadOnlyProperty has had useDeepEquality for a long time, which was never respected in sun components or elsewhere. So we feel completing this issue is not necessary for the current sprint, and we can schedule it for the future.

[zepumph]

When next we run into this issue, we should update TinyProperty also:

https://github.com/phetsims/axon/blob/b2379daed7f53435547eda49697cd8cfc5a706d4/js/TinyProperty.ts#L107-L122

[zepumph]

https://github.com/phetsims/energy-forms-and-changes/issues/423

[jonathanolson]

Ran into wanting this exact feature when working on hotkeys, but TinyProperty isn't hooked up with this yet!

[zepumph]

Alright. I updated the whole Property stack to support valueComparisonStrategy. It went incredibly smoothly and easy. There are just two items left to discuss:

• TinyProperty's implementation had this guard that I hate: https://github.com/phetsims/axon/blob/5e7e28e4e42c1aaf8fb66c423fb87d82f7bb114d/js/TinyProperty.ts#L118 aObject && bObject && aObject.constructor === bObject.constructor. It seems really hard coded, and doesn't support things like comparing against Vector2.ZERO since it has another constructor (ImmutableVector2).
• @samreid had some really great points up in https://github.com/phetsims/axon/issues/428#issuecomment-1412957431 and https://github.com/phetsims/axon/issues/428#issuecomment-1416925037. Basically I think we should take a hard look at our common code usages of ===. The easiest way to see some issues is to search for property.value ===, which I see 31 usages of in sun. We should be careful though, for example, in ABSwitch, it seems like the actual reference provided for a value is quite important.

[zepumph]

After discussing the first item with @jonathanolson, we decided on the changes I committed above. I'd love it if @samreid could review this. I'd also be curious about how he'd like to handle the second bullet. Separate issue? As we go? Eagerly look for places to change?

Let me know if you'd like to sync discuss.

[samreid]

Regarding this part:

        // NOTE: If you hit this, and you think it is a bad assertion because of subtyping or something, then let's
        // talk about removing this. Likely this should stick around (thinks JO and MK), but we can definitely discuss.
        assert && assert( aComparable.equals( bComparable ) === bComparable.equals( aComparable ),
          'incompatible equality checks' );

Would it fail in cases like this?

class Vector2 {
  x: number;
  y: number;

  equals( other: Vector2 ) {
    return other.x === this.x && other.y === this.y;
  }
}

class Vector3 extends Vector2 {
  z: number;

  equals( other: Vector3 ) {
    return super.equals( other ) && other.z === this.z;
  }
}

Or is the idea that this would be a rare case in our code and we should be alerted about it?

The easiest way to see some issues is to search for property.value ===, which I see 31 usages of in sun. We should be careful though, for example, in ABSwitch, it seems like the actual reference provided for a value is quite important.

I can't think of a lint rule or search technique that would catch 100% of cases here, but maybe we can just raise awareness about this dimension and rely on runtime behavior/qa to tell us when something isn't behaving as expected?

Also happy to sync discuss, let me know what you recommend.

[zepumph]

Would it fail in cases like this?

Yes. It would fail if they give conflicting results about if they equal each other. In discussion with @jonathanolson, it seemed nice to narrow the scope of this feature, as I believe he was worried this would seem like a code smell or that current code relying on equalsFunction would be buggy without it. I don't fully remember. Given the pretty simple example you have above, and the potential need for heterogeneous single-Property value equality, should we just get rid of that eagerly? @jonathanolson can you post your thoughts on that assertion here in this issue? UPDATE: After more thought and discussion, @samreid and I both recommend removing that assertion.

I can't think of a lint rule or search technique that would catch 100% of cases here, but maybe we can just raise awareness about this dimension and rely on runtime behavior/qa to tell us when something isn't behaving as expected?

@samreid and I spoke more about this in person, and we feel like pretty much everywhere that equality is being done assumes that the Property is using reference equality. In the cases we have seen, it would be buggy for them to not be using reference equality. So to start, we'd like to add an assertion to all these cases that the Property must be using reference equality. From there, we can see if any usages fail this, and how it is looking from there. If we want, we can add non-reference support via areValuesEqual() on a case by case basis. Searching for property.value !== and property.value === is useful.

[jonathanolson]

The assertion was explicitly designed to catch cases where the equality was asymmetric. If a.equals( b ) is not the same as b.equals( a ), things are kind of "buggy based on the order in which the Property is set", which seems like a bad precedent.

I guess I'm a bit confused, maybe it would be good to connect. I recall using non-reference-equality a lot in Properties.

If you are using Vector2 | Vector3 as a type for a Property, it WOULD be buggy to just use instance.equality. Instead, the user should provide the equality function. Removing the assertion would allow this buggy case.

[zepumph]

Ok. That sounds reasonable to me. I'll just add doc to that assertion pointing to this comment, and recommending a custom valueComparisonStrategy. Thanks.

[zepumph]

I got to a commit point today with adding reference equality assertions where triple equals is used. I'm surprised that nothing showed up on local fuzzing, but I'll check in the morning. If anyone starting getting an assertion like: "ToggleButtonModel depends on "===" equality for value comparison"

From https://github.com/phetsims/sun/blob/e7e6216051e1b6d1c32bfcd127798fecc65a1d06/js/buttons/ToggleButtonModel.ts#L39-L40. We know it is from this.

There are still a few TODOs that I want to discuss, including one about TypeScript covariance that just do not understand. Perhaps @samreid and I could get to the bottom of this in 30 minutes or so.

[samreid]

I was able to change to ValueComparisonStrategy<T> by turning other unknown into IntentionalAny like so:

```diff Subject: [PATCH] Simplify IntervalTool and edge management, see https://github.com/phetsims/projectile-data-lab/issues/274 --- Index: scenery/js/listeners/DragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/listeners/DragListener.ts b/scenery/js/listeners/DragListener.ts --- a/scenery/js/listeners/DragListener.ts (revision df9ab1245ac2fd0a5f2e1832bf54e1f5c750ba5e) +++ b/scenery/js/listeners/DragListener.ts (date 1712022129265) @@ -109,7 +109,7 @@ transform?: Transform3 | TReadOnlyProperty | null; // If provided, the model position will be constrained to be inside these bounds. - dragBoundsProperty?: TReadOnlyProperty | null; + dragBoundsProperty?: TReadOnlyProperty | TReadOnlyProperty | null; // If true, unattached touches that move across our node will trigger a press(). This helps sometimes // for small draggable objects. Index: scenery-phet/js/NumberDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts --- a/scenery-phet/js/NumberDisplay.ts (revision 342e360ad920fa7c6b0f4c81f6fc0a3effa70c0f) +++ b/scenery-phet/js/NumberDisplay.ts (date 1712022431362) @@ -56,7 +56,7 @@ // If your numberFormatter depends on other Properties, you must specify them so that the text will update when those // dependencies change. You can test for missing dependencies with ?strictAxonDependencies=true - numberFormatterDependencies?: TReadOnlyProperty[]; + numberFormatterDependencies?: TReadOnlyProperty[]; useRichText?: boolean; Index: scenery/js/listeners/KeyboardDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/listeners/KeyboardDragListener.ts b/scenery/js/listeners/KeyboardDragListener.ts --- a/scenery/js/listeners/KeyboardDragListener.ts (revision df9ab1245ac2fd0a5f2e1832bf54e1f5c750ba5e) +++ b/scenery/js/listeners/KeyboardDragListener.ts (date 1712022162664) @@ -135,7 +135,7 @@ transform?: Transform3 | TReadOnlyProperty | null; // If provided, the model position will be constrained to be inside these bounds, in model coordinates - dragBoundsProperty?: TReadOnlyProperty | null; + dragBoundsProperty?: TReadOnlyProperty | null|TReadOnlyProperty; // If provided, it will allow custom mapping // from the desired position (i.e. where the pointer is) to the actual possible position (i.e. where the dragged Index: axon/js/TReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TReadOnlyProperty.ts b/axon/js/TReadOnlyProperty.ts --- a/axon/js/TReadOnlyProperty.ts (revision 98016006dc741b9146f474d21aa0e386667938d1) +++ b/axon/js/TReadOnlyProperty.ts (date 1712021871373) @@ -32,7 +32,7 @@ // TODO: why doesn't "T" work here? https://github.com/phetsims/axon/issues/428 // TODO: is it ok to add this to the interface? https://github.com/phetsims/axon/issues/428 - valueComparisonStrategy: ValueComparisonStrategy; + valueComparisonStrategy: ValueComparisonStrategy; isDisposed?: boolean; toString(): string; Index: axon/js/DerivedProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DerivedProperty.ts b/axon/js/DerivedProperty.ts --- a/axon/js/DerivedProperty.ts (revision 98016006dc741b9146f474d21aa0e386667938d1) +++ b/axon/js/DerivedProperty.ts (date 1712022348829) @@ -218,11 +218,11 @@ * Creates a derived boolean Property whose value is true iff firstProperty's value is equal to secondProperty's * value. */ - public static valueEquals( firstProperty: TReadOnlyProperty, secondProperty: TReadOnlyProperty, options?: DerivedPropertyOptions ): TReadOnlyProperty { + public static valueEquals( firstProperty: TReadOnlyProperty, secondProperty: TReadOnlyProperty, options?: DerivedPropertyOptions ): TReadOnlyProperty { return new DerivedProperty( [ firstProperty, secondProperty ], ( u: unknown, v: unknown ) => u === v, options ); } - public static valueEqualsConstant( firstProperty: TReadOnlyProperty, value: unknown, options?: DerivedPropertyOptions ): TReadOnlyProperty { + public static valueEqualsConstant( firstProperty: TReadOnlyProperty, value: unknown, options?: DerivedPropertyOptions ): TReadOnlyProperty { return new DerivedProperty( [ firstProperty ], ( u: unknown ) => u === value, options ); } @@ -272,7 +272,7 @@ /** * Create a DerivedProperty from any number of dependencies. This is parallel to Multilink.multilinkAny */ - public static deriveAny( dependencies: Array>, derivation: () => T, providedOptions?: DerivedPropertyOptions ): UnknownDerivedProperty { + public static deriveAny( dependencies: Array>, derivation: () => T, providedOptions?: DerivedPropertyOptions ): UnknownDerivedProperty { return new DerivedProperty( // @ts-expect-error we have to provide a mapping between an arbitrary length array and our max overload of 15 types. dependencies, ```

This patch still has 300+ type errors but I think this strategy could work if we continue the strategy.

[zepumph]

@samreid and I have learned a great thing about types and interfaces today.

In https://stackoverflow.com/a/55992840/3408502 and https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-6.html#strict-function-types we learned that types declaring a method function behave differently than those declaring a property function. This caused much confusion for us for a couple hours, but we definitely enjoyed learning this nuance, and confirming that our current type system is working well, but with some quirks.

We were able to get around this for valueComparisonStrategy, without making systemic changes.

[zepumph]

Alright. All is pushed up and TODOs have been handled. While writing up the assertions for reference equality, I couldn't see any the I really wanted to add support for other strategies, so I believe that we are all done and good here. I also didn't find any assertions fired on CT last night. @samreid I'm ready to close, do you have anything else here?

[samreid]

The commits look good to me, there are no more TODOs, CT is clear, and I confirmed the valueComparisonStrategy in AncestorNodesProperty is being called. So I think we are good to close, nice work @zepumph and @jonathanolson