Avoid duplication in DerivedProperty boilerplate and make sure all dependencies registered

[samreid]

While reviewing phetsims/center-and-variability#433, I saw that each property in a DerivedProperty is listed 3x times:

    this.isKeyboardSelectArrowVisibleProperty = new DerivedProperty( [ this.focusedSoccerBallProperty,
        this.isSoccerBallKeyboardGrabbedProperty, this.isKeyboardFocusedProperty, this.hasKeyboardSelectedDifferentBallProperty ],
      ( focusedSoccerBall, isSoccerBallGrabbed, isKeyboardFocused, hasKeyboardSelectedDifferentBall ) => {
        return focusedSoccerBall !== null && !isSoccerBallGrabbed && isKeyboardFocused && !hasKeyboardSelectedDifferentBall;
      } );

There is also the opportunity to forget to register certain properties in the listeners. I have seen code like this (someOtherProperty) not registered as a dependency:

    this.isKeyboardSelectArrowVisibleProperty = new DerivedProperty( [ this.focusedSoccerBallProperty,
        this.isSoccerBallKeyboardGrabbedProperty, this.isKeyboardFocusedProperty, this.hasKeyboardSelectedDifferentBallProperty ],
      ( focusedSoccerBall, isSoccerBallGrabbed, isKeyboardFocused, hasKeyboardSelectedDifferentBall ) => {
        return  someOtherProperty.value && focusedSoccerBall !== null && !isSoccerBallGrabbed && isKeyboardFocused && !hasKeyboardSelectedDifferentBall ;
      } );

[samreid]

We could use a pattern like this, where the list of dependencies is inferred:

    this.isKeyboardSelectArrowVisibleProperty = derive(
      () => this.focusedSoccerBallProperty.value !== null &&
            !this.isSoccerBallKeyboardGrabbedProperty.value &&
            this.isKeyboardFocusedProperty.value &&
            !this.hasKeyboardSelectedDifferentBallProperty.value
    );

The derived function would track all of the Property instances that are visited during the closure evaluation.

So something like this:

  public static derive<T>( closure: () => T ) {
    // doubly nested, etc.
    const tracker: Property<unknown>[] = [];
    closure();
    // done tracking

    // TODO: No need to evaluate closure again.
    return DerivedProperty.deriveAny( tracker, closure );
  }

Some caveats discussed with @matthew-blackman

    // MB: With DerivedProperty, you explicitly list your dependencies. With this, it is more implicit.
    // See React, where the method: useEffect() or any react hooks (maybe useState). That also has the auto-update
    // thing so it is more implicit. That can lead to headaches.
    // SR: What if one dependencies triggers a change in another? Then you would get 2x callbacks?
    // Maybe a way to visualize the dependencies would be helpful and make sure they are what you expect.

[samreid]

In discussion with @zepumph:

• You could write the dependencies, then use the closure. But that doesn't seem great.
• Would this work with deferred properties? Probably...
• Would this need to be a push/pop for tracking the relevant Properties? Maybe just assert that it doesn't nest.

Michael also describes that the 2 problems of: slight duplication and maybe forgetting a dependency are not too bad. This proposal suffers from a magical/unexpected symptom where the behavior is tricky rather than straightforward.

Also, we could not get rid of DerivedProperty, so then there would be 2 ways of doing the same thing.

[samreid]

I was interested in how this could clean up call sites, however this implementation may be unworkable due to short circuiting in the derivations.

For instance, I converted:

    this.hasDraggedCardProperty = new DerivedProperty( [ this.totalDragDistanceProperty, this.hasKeyboardMovedCardProperty ], ( totalDragDistance, hasKeyboardMovedCard ) => {
      return totalDragDistance > 15 || hasKeyboardMovedCard;
    } );

to

this.hasDraggedCardProperty = new DerivedPropertyC( () => this.totalDragDistanceProperty.value > 15 || this.hasKeyboardMovedCardProperty.value );

But since the totalDragDistanceProperty evaluated to true it didn't even get a chance to visit the hasKeyboardMovedCardProperty, so it isn't aware it is a dependency. Same problem in this predicate:

    this.isKeyboardSelectArrowVisibleProperty = new DerivedProperty( [ this.focusedCardProperty, this.isCardGrabbedProperty,
        this.hasKeyboardSelectedDifferentCardProperty, this.isKeyboardFocusedProperty ],
      ( focusedCard, isCardGrabbed, hasSelectedDifferentCard, hasKeyboardFocus ) => {
        return focusedCard !== null && !isCardGrabbed && !hasSelectedDifferentCard && hasKeyboardFocus;
      } );

Subject: [PATCH] The selection arrow is shown over the same ball as the mouse drag indicator ball, see https://github.com/phetsims/center-and-variability/issues/515
---
Index: axon/js/TinyOverrideProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyOverrideProperty.ts b/axon/js/TinyOverrideProperty.ts
--- a/axon/js/TinyOverrideProperty.ts	(revision e87e8b6ce0bc0132cced505878c63a25118c2a17)
+++ b/axon/js/TinyOverrideProperty.ts	(date 1693021982490)
@@ -8,7 +8,7 @@
  */
 
 import axon from './axon.js';
-import TinyProperty from './TinyProperty.js';
+import TinyProperty, { trap } from './TinyProperty.js';
 import TReadOnlyProperty from './TReadOnlyProperty.js';
 
 export default class TinyOverrideProperty<T> extends TinyProperty<T> {
@@ -80,6 +80,9 @@
   }
 
   public override get(): T {
+    if ( trap.length > 0 ) {
+      trap[ trap.length - 1 ].add( this );
+    }
     // The main logic for TinyOverrideProperty
     return this.isOverridden ? this._value : this._targetProperty.value;
   }
Index: axon/js/DerivedPropertyC.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DerivedPropertyC.ts b/axon/js/DerivedPropertyC.ts
new file mode 100644
--- /dev/null	(date 1693021864976)
+++ b/axon/js/DerivedPropertyC.ts	(date 1693021864976)
@@ -0,0 +1,34 @@
+// Copyright 2013-2023, University of Colorado Boulder
+
+/**
+ * A DerivedPropertyC is computed based on other Properties. This implementation inherits from Property to (a) simplify
+ * implementation and (b) ensure it remains consistent. Dependent properties are inferred.
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+import axon from './axon.js';
+import { trap } from './TinyProperty.js';
+import DerivedProperty, { DerivedPropertyOptions } from './DerivedProperty.js';
+import TReadOnlyProperty from './TReadOnlyProperty.js';
+
+export default class DerivedPropertyC<T> extends DerivedProperty<T, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown> {
+
+  /**
+   * @param derivation - function that derives this Property's value, expects args in the same order as dependencies
+   * @param [providedOptions] - see Property
+   */
+  public constructor( derivation: () => T, providedOptions?: DerivedPropertyOptions<T> ) {
+
+    trap.push( new Set<TReadOnlyProperty<unknown>>() );
+    const initialValue = derivation();
+    console.log( initialValue );
+    const collector = trap.pop()!;
+    const from = Array.from( collector );
+    console.log( 'dependencies: ' + from.length );
+    // @ts-expect-error
+    super( from, derivation, providedOptions );
+  }
+}
+
+axon.register( 'DerivedPropertyC', DerivedPropertyC );
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts	(revision e87e8b6ce0bc0132cced505878c63a25118c2a17)
+++ b/axon/js/ReadOnlyProperty.ts	(date 1693022029254)
@@ -16,7 +16,7 @@
 import VoidIO from '../../tandem/js/types/VoidIO.js';
 import propertyStateHandlerSingleton from './propertyStateHandlerSingleton.js';
 import PropertyStatePhase from './PropertyStatePhase.js';
-import TinyProperty from './TinyProperty.js';
+import TinyProperty, { trap } from './TinyProperty.js';
 import units from './units.js';
 import validate from './validate.js';
 import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
@@ -222,7 +222,13 @@
    * or internal code that must be fast.
    */
   public get(): T {
-    return this.tinyProperty.get();
+    trap.length > 0 && trap[ trap.length - 1 ].add( this );
+    const value = this.tinyProperty.get();
+
+    // Remove the tinyProperty from the list, if it added itself. We will listen through the main Property.
+    trap.length > 0 && trap[ trap.length - 1 ].delete( this.tinyProperty );
+
+    return value;
   }
 
   /**
Index: axon/js/TinyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyProperty.ts b/axon/js/TinyProperty.ts
--- a/axon/js/TinyProperty.ts	(revision e87e8b6ce0bc0132cced505878c63a25118c2a17)
+++ b/axon/js/TinyProperty.ts	(date 1693022250139)
@@ -21,6 +21,19 @@
 export type TinyPropertyEmitterParameters<T> = [ T, T | null, TReadOnlyProperty<T> ];
 export type TinyPropertyOnBeforeNotify<T> = ( ...args: TinyPropertyEmitterParameters<T> ) => void;
 
+export const trap: Array<Set<TReadOnlyProperty<unknown>>> = [];
+
+window.trap = trap;
+
+export const debugTrap = ( text?: string ): void => {
+  if ( trap.length === 0 ) {
+    console.log( text, 'no trap' );
+  }
+  else {
+    console.log( text, 'depth = ', trap.length, 'last level = ', trap[ trap.length - 1 ].size );
+  }
+};
+
 export default class TinyProperty<T> extends TinyEmitter<TinyPropertyEmitterParameters<T>> implements TProperty<T> {
 
   public _value: T; // Store the internal value -- NOT for general use (but used in Scenery for performance)
@@ -42,6 +55,9 @@
    * or internal code that must be fast.
    */
   public get(): T {
+    if ( trap.length > 0 ) {
+      trap[ trap.length - 1 ].add( this );
+    }
     return this._value;
   }
 
Index: center-and-variability/js/median/model/InteractiveCardContainerModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/center-and-variability/js/median/model/InteractiveCardContainerModel.ts b/center-and-variability/js/median/model/InteractiveCardContainerModel.ts
--- a/center-and-variability/js/median/model/InteractiveCardContainerModel.ts	(revision bcadd2ee0c7c3819044331c814803ea064343854)
+++ b/center-and-variability/js/median/model/InteractiveCardContainerModel.ts	(date 1693022220136)
@@ -31,6 +31,8 @@
 import Tandem from '../../../../tandem/js/Tandem.js';
 import dotRandom from '../../../../dot/js/dotRandom.js';
 import CAVQueryParameters from '../../common/CAVQueryParameters.js';
+import DerivedPropertyC from '../../../../axon/js/DerivedPropertyC.js';
+import { debugTrap, trap } from '../../../../axon/js/TinyProperty.js';
 
 const cardMovementSounds = [
   cardMovement1_mp3,
@@ -73,7 +75,7 @@
   public readonly isKeyboardDragArrowVisibleProperty: TReadOnlyProperty<boolean>;
   public readonly isKeyboardSelectArrowVisibleProperty: TReadOnlyProperty<boolean>;
 
-  // Properties that track if a certain action have ever been performed vai keyboard input.
+  // Properties that track if a certain action have ever been performed via keyboard input.
   public readonly hasKeyboardMovedCardProperty = new BooleanProperty( false );
   public readonly hasKeyboardGrabbedCardProperty = new BooleanProperty( false );
   public readonly hasKeyboardSelectedDifferentCardProperty = new BooleanProperty( false );
@@ -81,12 +83,17 @@
   // Property that is triggered via focus and blur events in the InteractiveCardNodeContainer
   public readonly isKeyboardFocusedProperty = new BooleanProperty( false );
 
+  public get focusedCard(): CardModel | null { return this.focusedCardProperty.value; }
+
+  public get hasKeyboardMovedCard(): boolean { return this.hasKeyboardMovedCardProperty.value; }
+
   public constructor( medianModel: MedianModel, providedOptions: InteractiveCardContainerModelOptions ) {
     super( medianModel, providedOptions );
 
     // Accumulated card drag distance, for purposes of hiding the drag indicator node
     this.totalDragDistanceProperty = new NumberProperty( 0 );
 
+    // this.hasDraggedCardProperty = new DerivedPropertyC( () => this.totalDragDistanceProperty.value > 15 || this.hasKeyboardMovedCardProperty.value );
     this.hasDraggedCardProperty = new DerivedProperty( [ this.totalDragDistanceProperty, this.hasKeyboardMovedCardProperty ], ( totalDragDistance, hasKeyboardMovedCard ) => {
       return totalDragDistance > 15 || hasKeyboardMovedCard;
     } );
@@ -98,11 +105,29 @@
       phetioDocumentation: 'This is for PhET-iO internal use only.'
     } );
 
-    this.isKeyboardDragArrowVisibleProperty = new DerivedProperty( [ this.focusedCardProperty, this.hasKeyboardMovedCardProperty, this.hasKeyboardGrabbedCardProperty,
-        this.isCardGrabbedProperty, this.isKeyboardFocusedProperty ],
-      ( focusedCard, hasKeyboardMovedCard, hasGrabbedCard, isCardGrabbed, hasKeyboardFocus ) => {
-        return focusedCard !== null && !hasKeyboardMovedCard && hasGrabbedCard && isCardGrabbed && hasKeyboardFocus;
-      } );
+    this.isKeyboardDragArrowVisibleProperty = new DerivedPropertyC( () => {
+      // const theTrap = trap[ trap.length - 1 ];
+      // const allTrap = trap;
+      // debugger;
+
+      debugTrap( 'a' );
+      // this.focusedCard;
+      debugTrap( 'b' );
+      // this.hasKeyboardMovedCard;
+      debugTrap( 'c' );
+      // this.hasKeyboardGrabbedCardProperty.value;
+      debugTrap( 'd' );
+      // this.isCardGrabbedProperty.value
+      debugTrap( 'e' );
+      // this.isKeyboardFocusedProperty.value;
+      debugTrap( 'f' );
+
+      debugger;
+      const result = this.focusedCard !== null && !this.hasKeyboardMovedCard &&
+                     this.hasKeyboardGrabbedCardProperty.value && this.isCardGrabbedProperty.value && this.isKeyboardFocusedProperty.value;
+      debugTrap( 'g' );
+      return result;
+    } );
 
     this.isKeyboardSelectArrowVisibleProperty = new DerivedProperty( [ this.focusedCardProperty, this.isCardGrabbedProperty,
         this.hasKeyboardSelectedDifferentCardProperty, this.isKeyboardFocusedProperty ],

[samreid]

Based on the short circuiting, this seems unworkable. I wonder if we want to explore lint rules an alternative:

I wonder if a lint rule constraining the parameter names to match the Property names would be helpful? For instance, in the above example: the property is hasKeyboardSelectedDifferentCardProperty but the parameter hasSelectedDifferentCard so it would be renamed to hasKeyboardSelectedDifferentCard. But writing this rule to support the arbitrary case could be very difficult.

Or a lint rule that tries to avoid Property access on a non-dependency? If you are listening to propertyA and propertyB, but the derivation checks propertyC.value, then that may be buggy. But writing this rule to support the arbitrary case would be very difficult.

@zepumph any other thoughts, or should we just close?

[zepumph]

Has chatgtp been any help on those two rules? They seem potentially helpful and possible given the control we have over knowing when a DerivedProperty is being created within many cases.

[zepumph]

I like those rules and feel like it may be helpful to spend a bit of time on them. What do you think?

[samreid]

The bug identified above in phetsims/center-and-variability#519 was caused by querying a Property.value during a callback, but it was via another method call so could not be caught by a lint rule. Likewise a lint rule for parameter names could be of limited value in cases like new DerivedProperty( [this.someFunctionThatGivesAProperty()] ).

We could maybe add a runtime assertion that when executing a DerivedProperty or Multilink you are not allowed to query a Property.value that isn't in the listener list, but I don't know if that could be problematic in recursion (one DerivedProperty triggers another). So I'm not sure what's best here. I was hoping someone could think of a way out of the short circuiting problem above, because other than that it is pretty nice. So let's double check on that, and if we cannot see any way forward, let's close this issue and consider lint rules elsewhere (if at all).

[zepumph]

I like a lint rule that catches some but not all cases when creating a multilink or derivedProperty and we have the list of Properties in the first array, assert that no variable in the derivation ends with Property that isn't in the list of dependencies. My guess is that we will catch an large number of cases that are currently potentially buggy.

was hoping someone could think of a way out of the short circuiting problem above, because other than that it is pretty nice.

I'm not sure of way around this personally.

[samreid]

This patch checks at runtime for Property access outside of the dependencies.

Subject: [PATCH] Update documentation, see https://github.com/phetsims/phet-io-wrappers/issues/559
---
Index: bending-light/js/common/view/BendingLightScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/common/view/BendingLightScreenView.ts b/bending-light/js/common/view/BendingLightScreenView.ts
--- a/bending-light/js/common/view/BendingLightScreenView.ts	(revision bf019363ad52c03fa0bad57174c5c0281fde780e)
+++ b/bending-light/js/common/view/BendingLightScreenView.ts	(date 1699451028163)
@@ -203,13 +203,13 @@
     if ( typeof bendingLightModel.rotationArrowAngleOffset === 'number' ) {
       // Shows the direction in which laser can be rotated
       // for laser left rotation
-      const leftRotationDragHandle = new RotationDragHandle( this.modelViewTransform, bendingLightModel.laser,
+      const leftRotationDragHandle = new RotationDragHandle( bendingLightModel.laserViewProperty, this.modelViewTransform, bendingLightModel.laser,
         Math.PI / 23, showRotationDragHandlesProperty, clockwiseArrowNotAtMax, laserImageWidth * 0.58,
         bendingLightModel.rotationArrowAngleOffset );
       this.addChild( leftRotationDragHandle );
 
       // for laser right rotation
-      const rightRotationDragHandle = new RotationDragHandle( this.modelViewTransform, bendingLightModel.laser,
+      const rightRotationDragHandle = new RotationDragHandle( bendingLightModel.laserViewProperty, this.modelViewTransform, bendingLightModel.laser,
         -Math.PI / 23,
         showRotationDragHandlesProperty, ccwArrowNotAtMax, laserImageWidth * 0.58,
         bendingLightModel.rotationArrowAngleOffset
Index: joist/js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/Sim.ts b/joist/js/Sim.ts
--- a/joist/js/Sim.ts	(revision 083404df0d3dff7b4425fd64b84cdd6c8d7039cf)
+++ b/joist/js/Sim.ts	(date 1699456037732)
@@ -71,7 +71,7 @@
 import ArrayIO from '../../tandem/js/types/ArrayIO.js';
 import { Locale } from './i18n/localeProperty.js';
 import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js';
-import DerivedStringProperty from '../../axon/js/DerivedStringProperty.js';
+import StringIO from '../../tandem/js/types/StringIO.js';
 
 // constants
 const PROGRESS_BAR_WIDTH = 273;
@@ -524,15 +524,19 @@
       }
     } );
 
-    this.displayedSimNameProperty = new DerivedStringProperty( [
+    this.displayedSimNameProperty = DerivedProperty.deriveAny( [
       this.availableScreensProperty,
       this.simNameProperty,
       this.selectedScreenProperty,
       JoistStrings.simTitleWithScreenNamePatternStringProperty,
+      ...this.screens.map( screen => screen.nameProperty )
 
       // We just need notifications on any of these changing, return args as a unique value to make sure listeners fire.
-      DerivedProperty.deriveAny( this.simScreens.map( screen => screen.nameProperty ), ( ...args ) => [ ...args ] )
-    ], ( availableScreens, simName, selectedScreen, titleWithScreenPattern ) => {
+    ], () => {
+      const availableScreens = this.availableScreensProperty.value;
+      const simName = this.simNameProperty.value;
+      const selectedScreen = this.selectedScreenProperty.value;
+      const titleWithScreenPattern = JoistStrings.simTitleWithScreenNamePatternStringProperty.value;
       const screenName = selectedScreen.nameProperty.value;
 
       const isMultiScreenSimDisplayingSingleScreen = availableScreens.length === 1 && allSimScreens.length > 1;
@@ -556,7 +560,9 @@
     }, {
       tandem: Tandem.GENERAL_MODEL.createTandem( 'displayedSimNameProperty' ),
       tandemNameSuffix: 'NameProperty',
-      phetioDocumentation: 'Customize this string by editing its dependencies.'
+      phetioDocumentation: 'Customize this string by editing its dependencies.',
+      phetioFeatured: true,
+      phetioValueType: StringIO
     } );
 
     // Local variable is settable...
Index: joist/js/Screen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/Screen.ts b/joist/js/Screen.ts
--- a/joist/js/Screen.ts	(revision 083404df0d3dff7b4425fd64b84cdd6c8d7039cf)
+++ b/joist/js/Screen.ts	(date 1699413792525)
@@ -207,7 +207,7 @@
     this.createKeyboardHelpNode = options.createKeyboardHelpNode;
 
     // may be null for single-screen simulations
-    this.pdomDisplayNameProperty = new DerivedProperty( [ this.nameProperty ], name => {
+    this.pdomDisplayNameProperty = new DerivedProperty( [ this.nameProperty, screenNamePatternStringProperty ], name => {
       return name === null ? '' : StringUtils.fillIn( screenNamePatternStringProperty, {
         name: name
       } );
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts	(revision fcbfac464e3c714de5e60b1b6e876d89d97b9f3c)
+++ b/axon/js/ReadOnlyProperty.ts	(date 1699456341390)
@@ -77,6 +77,8 @@
   phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
 };
 
+export const derivationStack: IntentionalAny = [];
+
 /**
  * Base class for Property, DerivedProperty, DynamicProperty.  Set methods are protected/not part of the public
  * interface.  Initial value and resetting is not defined here.
@@ -224,6 +226,14 @@
    * or internal code that must be fast.
    */
   public get(): T {
+    if ( assert && derivationStack && derivationStack.length > 0 ) {
+      const currentDependencies = derivationStack[ derivationStack.length - 1 ];
+      if ( !currentDependencies.includes( this ) ) {
+        assert && assert( false, 'accessed value outside of dependency tracking' );
+        // console.log( 'trouble in ', derivationStack, new Error().stack );
+        // debugger;
+      }
+    }
     return this.tinyProperty.get();
   }
 
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 fcbfac464e3c714de5e60b1b6e876d89d97b9f3c)
+++ b/axon/js/DerivedProperty.ts	(date 1699456294909)
@@ -19,7 +19,7 @@
 import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
 import optionize from '../../phet-core/js/optionize.js';
 import { Dependencies, RP1, RP10, RP11, RP12, RP13, RP14, RP15, RP2, RP3, RP4, RP5, RP6, RP7, RP8, RP9 } from './Multilink.js';
-import ReadOnlyProperty from './ReadOnlyProperty.js';
+import ReadOnlyProperty, { derivationStack } from './ReadOnlyProperty.js';
 import PhetioObject from '../../tandem/js/PhetioObject.js';
 
 const DERIVED_PROPERTY_IO_PREFIX = 'DerivedPropertyIO';
@@ -37,8 +37,14 @@
  */
 function getDerivedValue<T, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>( derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15> ): T {
 
+  assert && derivationStack.push( dependencies );
+
   // @ts-expect-error
-  return derivation( ...dependencies.map( property => property.get() ) );
+  const result = derivation( ...dependencies.map( property => property.get() ) );
+
+  assert && derivationStack.pop();
+
+  return result;
 }
 
 // Convenience type for a Derived property that has a known return type but unknown dependency types.
Index: bending-light/js/common/view/RotationDragHandle.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/common/view/RotationDragHandle.ts b/bending-light/js/common/view/RotationDragHandle.ts
--- a/bending-light/js/common/view/RotationDragHandle.ts	(revision bf019363ad52c03fa0bad57174c5c0281fde780e)
+++ b/bending-light/js/common/view/RotationDragHandle.ts	(date 1699455887178)
@@ -16,10 +16,12 @@
 import { Node, Path } from '../../../../scenery/js/imports.js';
 import bendingLight from '../../bendingLight.js';
 import Laser from '../model/Laser.js';
+import LaserViewEnum from '../model/LaserViewEnum.js';
 
 class RotationDragHandle extends Node {
 
   /**
+   * @param laserViewProperty
    * @param modelViewTransform - Transform between model and view coordinate frames
    * @param laser - model of laser
    * @param deltaAngle - deltaAngle in radians
@@ -30,7 +32,7 @@
    * @param rotationArrowAngleOffset - for unknown reasons the rotation arrows are off by PI/4 on the
    *                                            intro/more-tools screen, so account for that here.
    */
-  public constructor( modelViewTransform: ModelViewTransform2, laser: Laser, deltaAngle: number, showDragHandlesProperty: Property<boolean>, notAtMax: ( n: number ) => boolean,
+  public constructor( laserViewProperty: Property<LaserViewEnum>, modelViewTransform: ModelViewTransform2, laser: Laser, deltaAngle: number, showDragHandlesProperty: Property<boolean>, notAtMax: ( n: number ) => boolean,
                       laserImageWidth: number, rotationArrowAngleOffset: number ) {
 
     super();
@@ -39,7 +41,8 @@
     const notAtMaximumProperty = new DerivedProperty( [
         laser.emissionPointProperty,
         laser.pivotProperty,
-        showDragHandlesProperty
+        showDragHandlesProperty,
+        laserViewProperty
       ],
       ( emissionPoint, pivot, showDragHandles ) => notAtMax( laser.getAngle() ) && showDragHandles
     );

About half of our sims fail this assertion, here is local aqua fuzzing:

Here is the list of failing sims.

http://localhost/aqua/fuzz-lightyear/?loadTimeout=30000&testTask=true&ea&audio=disabled&testDuration=10000&brand=phet&fuzz&testSims=area-model-algebra,area-model-decimals,area-model-introduction,area-model-multiplication,balancing-act,beers-law-lab,blast,build-a-fraction,build-a-molecule,capacitor-lab-basics,center-and-variability,circuit-construction-kit-ac,circuit-construction-kit-ac-virtual-lab,collision-lab,diffusion,energy-skate-park,energy-skate-park-basics,forces-and-motion-basics,fourier-making-waves,fractions-equality,fractions-intro,fractions-mixed-numbers,gas-properties,gases-intro,geometric-optics,geometric-optics-basics,graphing-quadratics,greenhouse-effect,hookes-law,keplers-laws,masses-and-springs,masses-and-springs-basics,my-solar-system,natural-selection,number-compare,number-line-distance,number-play,pendulum-lab,ph-scale,ph-scale-basics,ratio-and-proportion,sound-waves,unit-rates,vegas,wave-interference,wave-on-a-string,waves-intro

By the way, the patch fixes 2 related common code dependency issues and one sim specific one (bending light).

[pixelzoom]

Instead of overhauling/changing DerivedProperty (and Multilink?) would a lint rule address the problem?

[samreid]

@pixelzoom and I discussed the patch above. We also discussed that a lint rule is not sufficient to cover many cases. @pixelzoom would like to take a closer look at his sims, to see if these are potential bugs and if this is a good use of time.

• If this still seems reasonable after looking at DerivedProperty, we will check Multilink next.
• Also, if a link callback queries other Property values, maybe it should be a multilink.

We will start with investigating DerivedProperty, thanks!

[zepumph]

• Also, if a link callback queries other Property values, maybe it should be a multilink.

I think we need to be careful about making generalizations like this if we are codifying behavior. There could be many spots where the event should just be a single item, but the value requires 20 entities.

[pixelzoom]

Sims that I'm going to take a look at:

• beers-law-lab

• 2 missing dependencies for 1 DerivedProperty, see Missing dependencies for DerivedProperties beers-law-lab#333

• fourier-making-waves

• 8 missing dependencies for 4 DerivedProperties, see Missing dependencies for DerivedProperties fourier-making-waves#239

• gas-properties, gases-intro, diffusion

• 1 missing dependency for 1 DerivedProperty, see Missing dependency for DerivedProperty gas-properties#211
• Missing dependencies for NumberDisplay numberFormatter, see NumberDisplay is missing dependencies related to numberFormatter. scenery-phet#824

• geometric-optics

• 10 missing dependencies for 7 DerivedProperties, see Missing dependencies for DerivedProperties geometric-optics#486

• graphing-quadratics

• No problems were reported.

• hookes-law

• 1 missing dependency for 1 DerivedProperty, see Missing dependency for DerivedProperty hookes-law#84

• keplers-laws

• Missing dependencies for NumberDisplay numberFormatter, see NumberDisplay is missing dependencies related to numberFormatter. scenery-phet#824

• my-solar-system

• No problems were reported.

• natural-selection

• No problems were reported.

• ph-scale

• No problems were reported.

• units-rates

• No problems were reported.

---

My process was:

• Applying the patch in Avoid duplication in DerivedProperty boilerplate and make sure all dependencies registered #441 (comment)
• Make this change to ReadOnlyProperty get:

-     if ( !currentDependencies.includes( this ) ) {
+     if ( !currentDependencies.includes( this ) && !phet.skip ) {
        assert && assert( false, 'accessed value outside of dependency tracking' );

• Run each of the above sims from phetmarks.
• When I encountered a problem, silence that problem by surrounding it with phet.skip, like this:

    this.rightProperty = new DerivedProperty( [ this.equilibriumXProperty, this.displacementProperty ],
      ( equilibriumX, displacement ) => {
+       phet.skip = true;
        const left = this.leftProperty.value;
+       phet.skip = false;

• Report my findings for each sim in the checklist above, and in GitHub issues as appropriate.

[samreid]

After my local changes, I tested a few sims and wrappers. The precommit hooks say we are OK:

node chipper/js/scripts/precommit-hook-multi.js
detected changed repos: area-model-common, axon, beers-law-lab, center-and-variability, chipper, collision-lab, density-buoyancy-common, energy-skate-park, expression-exchange, faradays-electromagnetic-lab, fractions-common, gas-properties, gravity-and-orbits, joist, keplers-laws, masses-and-springs, number-compare, number-line-distance, number-play, ph-scale, phetmarks, scenery-phet, sun, unit-rates
area-model-common: Success
axon: Success
beers-law-lab: Success
center-and-variability: Success
chipper: Success
collision-lab: Success
density-buoyancy-common: Success
energy-skate-park: Success
expression-exchange: Success
faradays-electromagnetic-lab: Success
fractions-common: Success
gas-properties: Success
gravity-and-orbits: Success
joist: Success
keplers-laws: Success
masses-and-springs: Success
number-compare: Success
number-line-distance: Success
number-play: Success
ph-scale: Success
phetmarks: Success
scenery-phet: Success
sun: Success
unit-rates: Success
Done in 295116ms

I'll spot check the delta one more time before pushing.

[samreid]

OK I removed the strictAxonDependencies feature. Closing.

[zepumph]

Thank you for the quick turnaround here! I appreciate it.