Avoid duplication in DerivedProperty boilerplate and make sure all dependencies registered

[samreid]

While reviewing https://github.com/phetsims/center-and-variability/issues/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;
      } );

```diff 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 extends TinyProperty { @@ -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 extends DerivedProperty { + + /** + * @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 ) { + + trap.push( new Set>() ); + 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 | null, TReadOnlyProperty ]; export type TinyPropertyOnBeforeNotify = ( ...args: TinyPropertyEmitterParameters ) => void; +export const trap: Array>> = []; + +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 extends TinyEmitter> implements TProperty { 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; public readonly isKeyboardSelectArrowVisibleProperty: TReadOnlyProperty; - // 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 https://github.com/phetsims/center-and-variability/issues/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.

```diff 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>; }; +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( derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies ): 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, notAtMax: ( n: number ) => boolean, + public constructor( laserViewProperty: Property, modelViewTransform: ModelViewTransform2, laser: Laser, deltaAngle: number, showDragHandlesProperty: Property, 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:

• [x] beers-law-lab

• 2 missing dependencies for 1 DerivedProperty, see https://github.com/phetsims/beers-law-lab/issues/333

• [x] fourier-making-waves

• 8 missing dependencies for 4 DerivedProperties, see https://github.com/phetsims/fourier-making-waves/issues/239

• [x] gas-properties, gases-intro, diffusion

• 1 missing dependency for 1 DerivedProperty, see https://github.com/phetsims/gas-properties/issues/211

• Missing dependencies for NumberDisplay numberFormatter, see https://github.com/phetsims/scenery-phet/issues/824

• [x] geometric-optics

• 10 missing dependencies for 7 DerivedProperties, see https://github.com/phetsims/geometric-optics/issues/486

• [x] graphing-quadratics

• No problems were reported.

• [x] hookes-law

• 1 missing dependency for 1 DerivedProperty, see https://github.com/phetsims/hookes-law/issues/84

• [x] keplers-laws

• Missing dependencies for NumberDisplay numberFormatter, see https://github.com/phetsims/scenery-phet/issues/824

• [x] my-solar-system

• No problems were reported.

• [x] natural-selection

• No problems were reported.

• [x] ph-scale

• No problems were reported.

• [x] units-rates

• No problems were reported.

---

My process was:

• Applying the patch in https://github.com/phetsims/axon/issues/441#issuecomment-1802186505
• 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.

[pixelzoom]

I completed investigation of the sims listed in https://github.com/phetsims/axon/issues/441#issuecomment-1802852296. I was surprised that 5 of the sims exhibited no problems -- @samreid thoughts?

For the sims that did exhibit problems, the problems seemed worrisome and worth addressing, and I created sim-specific issues. There was 1 common-code problem, see https://github.com/phetsims/scenery-phet/issues/824.

@samreid let's discuss where to go from here.

[samreid]

From discussion with @pixelzoom:

• [x] @samreid will add an option to DerivedProperty so that certain cases can opt out of this check.
• [x] @samreid will visit all failing cases and opt out of the check
• [ ] @samreid will run an initial investigation to see how many problems trigger with multilink, @pixelzoom will take a look at his sims (like he did above for DerivedProperty)
• [ ] @samreid will run an initial investigation to see how many problems trigger with link
• [ ] We will bring this to developer meeting on Thursday to discuss. @pixelzoom will not be present at that one, but his opinion is represented in this issue.

[samreid]

I also saw that a DerivedProperty in ph-scale accesses itself in its own derivation:

    this.colorProperty = new DerivedProperty(
      [ this.soluteProperty, this.soluteVolumeProperty, this.waterVolumeProperty ],
      ( solute, soluteVolume, waterVolume ) => {
        if ( this.ignoreVolumeUpdate ) {
          return this.colorProperty.value;
        }

[pixelzoom]

I also saw that a DerivedProperty in ph-scale accesses itself in its own derivation:

I don't think that's a problem. There's no way that it can return a stale or incorrect value. Unless I'm missing something...

[pixelzoom]

Two questions about the above commits, where @samreid added accessNonDependencies: true to many sims

(1) Should those usages have a TODO that points to a GitHub issue? (...either this issue or a sim-specific issue.) I realize that we can search for accessNonDependencies: true. But how will we tell the difference between uses that have reviewed and kept, versus those that no one has looked at?

(2) I had previously addressed beers-law-lab problems in https://github.com/phetsims/beers-law-lab/issues/333. So I was surprised to see new problems in https://github.com/phetsims/beers-law-lab/commit/308dae021080663fa6e437cdb526e2c7f6b0fe9d. @samreid Can you clarify? Did you change the patch used to identify missing dependencies?

[samreid]

Should those usages have a TODO that points to a GitHub issue?

That would be good to discuss. We haven't decided if we want to chip away and eliminate most/all of these. There are 68 occurrences of accessNonDependencies: true across 40 directories.

But how will we tell the difference between uses that have reviewed and kept, versus those that no one has looked at?

As far as I can tell, none have been looked at yet or reviewed and kept. I agree we will need a clear way of annotating which is which if we ever decide "this is permanent" for one.

I was surprised to see new problems

I did not change the test harness, but I did fuzz quite a bit longer.

[pixelzoom]

My opinion is that putting workarounds, or (worse) code that is intended to temporarily silence errors, into production code without a TODO is a bad practice. Suite yourself for the sims that you're responsibile for. But in the above commits, I added TODOs for accessNonDependencies: true in my sims and common code.

There are now 35 occurrences of accessNonDependencies: true with no comment or TODO that links back to this issue.

[samreid]

Based on my initial investigation on Multilink, it seems we should address it in the same way.

[pixelzoom]

But Multilink does not have a way to provide options. The constructor signature is:

  public constructor( dependencies: RP1<T1>, callback: ( ...params: [ T1 ] ) => void, lazy?: boolean ) ;

Perhaps we should change the API to something like:

type SelfOptions = {
   lazy?: boolean; //TODO document
   accessNonDependencies?: boolean; //TODO document
};

type MulitlinkOptions = SelfOptions;

...
  public constructor( dependencies: RP1<T1>, callback: ( ...params: [ T1 ] ) => void, providedOptions?:  MulitlinkOptions ) ;

[samreid]

I have added the options in my working copy. Should we use the same option name accessNonDependencies or a different one to distinguish it from DerivedProperty?

[pixelzoom]

Same option name, accessNonDependencies.

[pixelzoom]

For https://github.com/phetsims/unit-rates/issues/223:

    this.quantityProperty = new DerivedProperty(
      [ this.numberOfBagsProperty, this.numberOfItemsProperty ],
      ( numberOfBags, numberOfItems ) => {
        if ( this.quantityUpdateEnabled ) {
          return ( numberOfBags * options.quantityPerBag ) + numberOfItems;
        }
        else {
          return this.quantityProperty.value;
        }
      },

@samreid It looks like this is similar to the case that you reported in https://github.com/phetsims/axon/issues/441#issuecomment-1810181500 for ph-scale, where a DerivedProperty has itself as a dependency. As I mentioned in https://github.com/phetsims/axon/issues/441#issuecomment-1810338145, I don't think that's a problem, so I don't think that (in cases like this) a DerivedProperty needs to include itself as a dependency.

Thoughts?

[samreid]

Here is my patch so far investigating Multilink:

```diff Subject: [PATCH] Add accessNonDependencies: true, see https://github.com/phetsims/axon/issues/441 --- Index: area-model-common/js/proportional/model/ProportionalArea.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/proportional/model/ProportionalArea.js b/area-model-common/js/proportional/model/ProportionalArea.js --- a/area-model-common/js/proportional/model/ProportionalArea.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/proportional/model/ProportionalArea.js (date 1699977051298) @@ -156,6 +156,8 @@ primaryPartition.coordinateRangeProperty.value = new Range( 0, size ); secondaryPartition.coordinateRangeProperty.value = null; } + }, { + accessNonDependencies: true } ); // Remove splits that are at or past the current boundary. Index: area-model-common/js/common/model/Area.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/common/model/Area.js b/area-model-common/js/common/model/Area.js --- a/area-model-common/js/common/model/Area.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/common/model/Area.js (date 1699976980603) @@ -102,6 +102,8 @@ else { partitionedArea.areaProperty.value = horizontalSize.times( verticalSize ); } + }, { + accessNonDependencies: true } ); return partitionedArea; 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 a01ba883ab3da140148e44d538d515eebcdea6fd) +++ b/joist/js/Screen.ts (date 1699975977959) @@ -344,6 +344,8 @@ // if there is a screenSummaryNode, then set its intro string now this._view!.setScreenSummaryIntroAndTitle( simName, pdomDisplayName, titleString, numberOfScreens > 1 ); + }, { + accessNonDependencies: true } ); assert && this._view.pdomAudit(); Index: area-model-common/js/game/view/GameAreaScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/game/view/GameAreaScreenView.js b/area-model-common/js/game/view/GameAreaScreenView.js --- a/area-model-common/js/game/view/GameAreaScreenView.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/game/view/GameAreaScreenView.js (date 1699977353545) @@ -295,6 +295,8 @@ else { totalContainer.children = [ totalNode ]; } + }, { + accessNonDependencies: true } ); const productContent = this.createPanel( totalAreaOfModelString, panelAlignGroup, totalContainer ); Index: beers-law-lab/js/concentration/view/ConcentrationMeterNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/beers-law-lab/js/concentration/view/ConcentrationMeterNode.ts b/beers-law-lab/js/concentration/view/ConcentrationMeterNode.ts --- a/beers-law-lab/js/concentration/view/ConcentrationMeterNode.ts (revision c5e76689ec414207baecc77922346de5c9fce68b) +++ b/beers-law-lab/js/concentration/view/ConcentrationMeterNode.ts (date 1699978613099) @@ -115,7 +115,9 @@ stockSolutionNode.boundsProperty, solventFluidNode.boundsProperty, drainFluidNode.boundsProperty - ], () => updateValue() ); + ], () => updateValue(), { + accessNonDependencies: true + } ); this.addLinkedElement( concentrationMeter, { tandemName: 'concentrationMeter' Index: states-of-matter/js/atomic-interactions/view/InteractivePotentialGraph.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/states-of-matter/js/atomic-interactions/view/InteractivePotentialGraph.js b/states-of-matter/js/atomic-interactions/view/InteractivePotentialGraph.js --- a/states-of-matter/js/atomic-interactions/view/InteractivePotentialGraph.js (revision 44d1b3973bc29ce9b58d62ffee54af85c9aa1fc9) +++ b/states-of-matter/js/atomic-interactions/view/InteractivePotentialGraph.js (date 1699977817954) @@ -253,6 +253,8 @@ this.setLjPotentialParameters( dualAtomModel.getSigma(), dualAtomModel.getEpsilon() ); this.updateInteractivityState(); this.drawPotentialCurve(); + }, { + accessNonDependencies: true } ); Index: joist/js/KeyboardHelpDialog.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/KeyboardHelpDialog.ts b/joist/js/KeyboardHelpDialog.ts --- a/joist/js/KeyboardHelpDialog.ts (revision a01ba883ab3da140148e44d538d515eebcdea6fd) +++ b/joist/js/KeyboardHelpDialog.ts (date 1699975977958) @@ -102,6 +102,8 @@ assert && assert( currentContentNode, 'a displayed KeyboardHelpButton for a screen should have content' ); content.children = [ currentContentNode ]; } + }, { + accessNonDependencies: true } ); // (a11y) Make sure that the title passed to the Dialog has an accessible name. Index: joist/js/preferences/PreferencesPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/PreferencesPanel.ts b/joist/js/preferences/PreferencesPanel.ts --- a/joist/js/preferences/PreferencesPanel.ts (revision a01ba883ab3da140148e44d538d515eebcdea6fd) +++ b/joist/js/preferences/PreferencesPanel.ts (date 1699976139792) @@ -43,6 +43,8 @@ // PhET-iO. Multilink.multilink( [ selectedTabProperty, tabVisibleProperty ], ( selectedTab, tabVisible ) => { this.visible = selectedTab === preferencesType && tabVisible; + }, { + accessNonDependencies: true } ); } } Index: sun/js/buttons/RectangularButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RectangularButton.ts b/sun/js/buttons/RectangularButton.ts --- a/sun/js/buttons/RectangularButton.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/buttons/RectangularButton.ts (date 1699975986583) @@ -202,6 +202,8 @@ } isFirstlayout = false; + }, { + accessNonDependencies: true } ); } } Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASEView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASEView.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASEView.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASEView.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASEView.js (date 1699978475040) @@ -182,6 +182,8 @@ else if ( greenDragged ) { this.greenBalloonLayerNode.moveToFront(); } + }, { + accessNonDependencies: true } ); Index: states-of-matter/js/atomic-interactions/model/DualAtomModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/states-of-matter/js/atomic-interactions/model/DualAtomModel.js b/states-of-matter/js/atomic-interactions/model/DualAtomModel.js --- a/states-of-matter/js/atomic-interactions/model/DualAtomModel.js (revision 44d1b3973bc29ce9b58d62ffee54af85c9aa1fc9) +++ b/states-of-matter/js/atomic-interactions/model/DualAtomModel.js (date 1699977790276) @@ -154,6 +154,8 @@ this.setAdjustableAtomSigma( atomDiameter ); } this.updateForces(); + }, { + accessNonDependencies: true } ); Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonVelocitySoundGenerator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonVelocitySoundGenerator.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonVelocitySoundGenerator.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonVelocitySoundGenerator.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonVelocitySoundGenerator.js (date 1699978099073) @@ -96,6 +96,8 @@ this.stop(); this.setOutputLevel( 0 ); } + }, { + accessNonDependencies: true } ); Index: sun/js/ComboBoxButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/ComboBoxButton.ts b/sun/js/ComboBoxButton.ts --- a/sun/js/ComboBoxButton.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/ComboBoxButton.ts (date 1699977957805) @@ -201,6 +201,8 @@ separatorLine.mutateLayoutOptions( { rightMargin: rightMargin } ); + }, { + accessNonDependencies: true } ); // Margins are different in the item and button areas. And we want the vertical separator to extend Index: sun/js/buttons/ButtonModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/ButtonModel.ts b/sun/js/buttons/ButtonModel.ts --- a/sun/js/buttons/ButtonModel.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/buttons/ButtonModel.ts (date 1699975986582) @@ -203,6 +203,8 @@ // PressListeners created by this ButtonModel look pressed. this.looksPressedMultilink = Multilink.multilinkAny( looksPressedProperties, ( ...args: boolean[] ) => { this.looksPressedProperty.value = _.reduce( args, ( sum: boolean, newValue: boolean ) => sum || newValue, false ); + }, { + accessNonDependencies: true } ); const looksOverProperties = this.listeners.map( listener => listener.looksOverProperty ); @@ -212,6 +214,8 @@ // because its implementation relies on arguments. this.looksOverMultilink = Multilink.multilinkAny( looksOverProperties, ( ...args: boolean[] ) => { this.looksOverProperty.value = _.reduce( args, ( sum: boolean, newValue: boolean ) => sum || newValue, false ); + }, { + accessNonDependencies: true } ); return pressListener; Index: area-model-common/js/game/model/AreaChallenge.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/game/model/AreaChallenge.js b/area-model-common/js/game/model/AreaChallenge.js --- a/area-model-common/js/game/model/AreaChallenge.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/game/model/AreaChallenge.js (date 1699977324706) @@ -100,6 +100,8 @@ ], ( horizontal, vertical ) => { // horizontal or vertical could be null (resulting in null) entry.valueProperty.value = horizontal && vertical && horizontal.times( vertical ); + }, { + accessNonDependencies: true } ); } return entry; @@ -186,6 +188,8 @@ const terms = _.map( nonErrorProperties, 'value' ).filter( term => term !== null ); const lostATerm = terms.length !== nonErrorProperties.length; this.totalProperties.get( orientation ).value = ( terms.length && !lostATerm ) ? new Polynomial( terms ) : null; + }, { + accessNonDependencies: true } ); } } ); Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonRubbingSoundGenerator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonRubbingSoundGenerator.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonRubbingSoundGenerator.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonRubbingSoundGenerator.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BalloonRubbingSoundGenerator.js (date 1699978178273) @@ -102,6 +102,8 @@ // The smoothed velocity has dropped to zero, turn off sound production. this.stop(); } + }, { + accessNonDependencies: true } ); } Index: sun/js/buttons/ButtonNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/ButtonNode.ts b/sun/js/buttons/ButtonNode.ts --- a/sun/js/buttons/ButtonNode.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/buttons/ButtonNode.ts (date 1699975986582) @@ -252,6 +252,8 @@ [ buttonBackground.boundsProperty, this.layoutSizeProperty ], ( backgroundBounds, size ) => { alignBox!.alignBounds = Bounds2.point( backgroundBounds.center ).dilatedXY( size.width / 2, size.height / 2 ); + }, { + accessNonDependencies: true } ); this.addChild( alignBox ); Index: beers-law-lab/js/concentration/model/Shaker.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/beers-law-lab/js/concentration/model/Shaker.ts b/beers-law-lab/js/concentration/model/Shaker.ts --- a/beers-law-lab/js/concentration/model/Shaker.ts (revision c5e76689ec414207baecc77922346de5c9fce68b) +++ b/beers-law-lab/js/concentration/model/Shaker.ts (date 1699978775256) @@ -89,6 +89,8 @@ if ( isEmpty || !visible ) { this.dispensingRateProperty.value = 0; } + }, { + accessNonDependencies: true } ); // If the position changes while restoring PhET-iO state, then set previousPosition to position to prevent the Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASESummaryNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASESummaryNode.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASESummaryNode.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASESummaryNode.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/BASESummaryNode.js (date 1699978234717) @@ -107,6 +107,8 @@ balloonChargeNode.innerContent = this.getBalloonChargeDescription(); sweaterWallChargeNode.innerContent = this.getSweaterAndWallChargeDescription(); } + }, { + accessNonDependencies: true } ); const inducedChargeProperties = [ this.yellowBalloon.positionProperty, this.greenBalloon.positionProperty, this.greenBalloon.isVisibleProperty, model.showChargesProperty, model.wall.isVisibleProperty ]; @@ -120,6 +122,8 @@ if ( showInducingItem ) { inducedChargeNode.innerContent = this.getInducedChargeDescription(); } + }, { + accessNonDependencies: true } ); // If all of the simulation objects are at their initial state, include the position summary phrase that lets the @@ -136,6 +140,8 @@ model.wall.isVisibleProperty.initialValue === wallVisible; objectPositionsNode.pdomVisible = initialValues; + }, { + accessNonDependencies: true } ); } Index: area-model-common/js/common/view/PartialProductLabelNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/common/view/PartialProductLabelNode.js b/area-model-common/js/common/view/PartialProductLabelNode.js --- a/area-model-common/js/common/view/PartialProductLabelNode.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/common/view/PartialProductLabelNode.js (date 1699977595814) @@ -117,8 +117,8 @@ // Product else if ( choice === PartialProductsChoice.PRODUCTS ) { productRichText.string = ( horizontalSize === null || verticalSize === null ) - ? '?' - : horizontalSize.times( verticalSize ).toRichString( false ); + ? '?' + : horizontalSize.times( verticalSize ).toRichString( false ); children = [ productRichText ]; } @@ -158,6 +158,8 @@ box.center = Vector2.ZERO; background.rectBounds = box.bounds.dilatedXY( 4, 2 ); } + }, { + accessNonDependencies: true } ); } } Index: balloons-and-static-electricity/js/balloons-and-static-electricity/view/SweaterNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/SweaterNode.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/SweaterNode.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/view/SweaterNode.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/view/SweaterNode.js (date 1699978071324) @@ -113,6 +113,8 @@ updateChargesVisibilityOnSweater( charge ); this.setDescriptionContent( sweaterDescriber.getSweaterDescription( showCharges ) ); + }, { + accessNonDependencies: true } ); // When setting the state using phet-io, we must update the charge visibility, otherwise they can get out of sync Index: area-model-common/js/common/view/AreaDisplayNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/common/view/AreaDisplayNode.js b/area-model-common/js/common/view/AreaDisplayNode.js --- a/area-model-common/js/common/view/AreaDisplayNode.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/common/view/AreaDisplayNode.js (date 1699977206470) @@ -107,6 +107,8 @@ else { throw new Error( 'unexpected number of partitions for a11y' ); } + }, { + accessNonDependencies: true } ); return partitionLabel; } ); @@ -169,6 +171,8 @@ else { throw new Error( 'unknown situation for a11y partial products' ); } + }, { + accessNonDependencies: true } ); } ); this.pdomParagraphNode.addChild( accessiblePartialProductNode ); Index: sun/js/Carousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/Carousel.ts b/sun/js/Carousel.ts --- a/sun/js/Carousel.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/Carousel.ts (date 1699976429569) @@ -396,6 +396,8 @@ // animation disabled, move immediate to new page scrollingNodeContainer[ orientation.coordinate ] = targetValue; } + }, { + accessNonDependencies: true } ); // Don't stay on a page that doesn't exist Index: joist/js/preferences/PreferencesTab.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/PreferencesTab.ts b/joist/js/preferences/PreferencesTab.ts --- a/joist/js/preferences/PreferencesTab.ts (revision a01ba883ab3da140148e44d538d515eebcdea6fd) +++ b/joist/js/preferences/PreferencesTab.ts (date 1699975977959) @@ -131,6 +131,8 @@ this.focusable = selectedTab === value; underlineNode.visible = selectedTab === value; + }, { + accessNonDependencies: true } ); } } Index: area-model-common/js/proportional/view/ProportionalAreaDisplayNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/proportional/view/ProportionalAreaDisplayNode.js b/area-model-common/js/proportional/view/ProportionalAreaDisplayNode.js --- a/area-model-common/js/proportional/view/ProportionalAreaDisplayNode.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/proportional/view/ProportionalAreaDisplayNode.js (date 1699977281111) @@ -104,11 +104,15 @@ [ areaDisplay.activeTotalProperties.horizontal, this.modelViewTransformProperty ], ( totalWidth, modelViewTransform ) => { activeAreaBackground.rectWidth = modelViewTransform.modelToViewX( totalWidth ); + }, { + accessNonDependencies: true } ); Multilink.multilink( [ areaDisplay.activeTotalProperties.vertical, this.modelViewTransformProperty ], ( totalHeight, modelViewTransform ) => { activeAreaBackground.rectHeight = modelViewTransform.modelToViewY( totalHeight ); + }, { + accessNonDependencies: true } ); this.areaLayer.addChild( activeAreaBackground ); Index: area-model-common/js/proportional/view/ProportionalDragHandle.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/area-model-common/js/proportional/view/ProportionalDragHandle.js b/area-model-common/js/proportional/view/ProportionalDragHandle.js --- a/area-model-common/js/proportional/view/ProportionalDragHandle.js (revision 3bae3eb74c63b3e841a2dfb7e151ed8da4faa63a) +++ b/area-model-common/js/proportional/view/ProportionalDragHandle.js (date 1699977627242) @@ -172,6 +172,8 @@ } ); circle.addInputListener( keyboardListener ); + }, { + accessNonDependencies: true } ); // Apply offsets while dragging for a smoother experience. Index: balloons-and-static-electricity/js/balloons-and-static-electricity/model/BASEModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity/model/BASEModel.js b/balloons-and-static-electricity/js/balloons-and-static-electricity/model/BASEModel.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity/model/BASEModel.js (revision c2c093dac51cb30d035e9dfc86325c309c4d846e) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity/model/BASEModel.js (date 1699978041047) @@ -102,6 +102,8 @@ // update whether the balloon is currently inducing charge in the wall Multilink.multilink( [ this.wall.isVisibleProperty, balloon.positionProperty ], ( wallVisible, position ) => { balloon.inducingChargeProperty.set( balloon.inducingCharge( wallVisible ) ); + }, { + accessNonDependencies: true } ); } ); Index: sun/js/NumberPicker.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/NumberPicker.ts b/sun/js/NumberPicker.ts --- a/sun/js/NumberPicker.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/NumberPicker.ts (date 1699977138764) @@ -469,11 +469,15 @@ // Update colors for increment components. No dispose is needed since dependencies are locally owned. Multilink.multilink( [ incrementButtonStateProperty, incrementEnabledProperty ], ( state, enabled ) => { updateColors( state, enabled, incrementBackgroundNode, this.incrementArrow, backgroundColors, arrowColors ); + }, { + accessNonDependencies: true } ); // Update colors for decrement components. No dispose is needed since dependencies are locally owned. Multilink.multilink( [ decrementButtonStateProperty, decrementEnabledProperty ], ( state, enabled ) => { updateColors( state, enabled, decrementBackgroundNode, this.decrementArrow, backgroundColors, arrowColors ); + }, { + accessNonDependencies: true } ); // Dilate based on consistent technique which brings into account transform of this node. @@ -594,6 +598,8 @@ !isOver && !isPressed ? 'up' : 'out' ); + }, { + accessNonDependencies: true } ); } Index: sun/js/Dialog.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/Dialog.ts b/sun/js/Dialog.ts --- a/sun/js/Dialog.ts (revision 6144ca1e04ff0a187f90f41ec888f4bc711df059) +++ b/sun/js/Dialog.ts (date 1699976100657) @@ -378,6 +378,8 @@ if ( bounds && screenBounds && scale ) { options.layoutStrategy( this, bounds, screenBounds, scale ); } + }, { + accessNonDependencies: true } ); // Setter after the super call Index: axon/js/Multilink.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/Multilink.ts b/axon/js/Multilink.ts --- a/axon/js/Multilink.ts (revision 96122725e6c20ec990f4407ea9bcc495b19f7b48) +++ b/axon/js/Multilink.ts (date 1699975962652) @@ -18,6 +18,14 @@ import axon from './axon.js'; import TReadOnlyProperty from './TReadOnlyProperty.js'; +import optionize from '../../phet-core/js/optionize.js'; +import { derivationStack } from './ReadOnlyProperty.js'; + +type SelfOptions = { + lazy?: boolean; + accessNonDependencies?: boolean; +}; +type MultilinkOptions = SelfOptions; // Shorthand to make the type definitions more legible type ROP = TReadOnlyProperty; @@ -73,25 +81,30 @@ /** * @param dependencies * @param callback function that expects args in the same order as dependencies - * @param [lazy] Optional parameter that can be set to true if this should be a lazy multilink (no immediate callback) + * @param providedOptions */ - public constructor( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, lazy?: boolean ) ; - public constructor( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, lazy?: boolean ); - public constructor( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, lazy?: boolean ) { + public constructor( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, providedOptions?: MultilinkOptions ) ; + public constructor( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, providedOptions?: MultilinkOptions ); + public constructor( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, providedOptions?: MultilinkOptions ) { + + const options = optionize()( { + lazy: false, + accessNonDependencies: false + }, providedOptions ); this.dependencies = dependencies; @@ -107,8 +120,15 @@ // don't call listener if this Multilink has been disposed, see https://github.com/phetsims/axon/issues/192 if ( !this.isDisposed ) { - const values = dependencies.map( dependency => dependency.get() ) as [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ]; - callback( ...values ); + assert && !options.accessNonDependencies && derivationStack.push( dependencies ); + try { + const values = dependencies.map( dependency => dependency.get() ) as [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ]; + callback( ...values ); + return; + } + finally { + assert && !options.accessNonDependencies && derivationStack.pop(); + } } }; this.dependencyListeners.set( dependency, listener ); @@ -122,10 +142,17 @@ } ); // Send initial call back but only if we are non-lazy - if ( !lazy ) { + if ( !options.lazy ) { - const values = dependencies.map( dependency => dependency.get() ) as [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ]; - callback( ...values ); + assert && !options.accessNonDependencies && derivationStack.push( dependencies ); + try { + const values = dependencies.map( dependency => dependency.get() ) as [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ]; + callback( ...values ); + return; + } + finally { + assert && !options.accessNonDependencies && derivationStack.pop(); + } } this.isDisposed = false; @@ -165,32 +192,32 @@ * @param dependencies * @param callback function that takes values from the properties and returns nothing */ - static multilink( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static multilink( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void ): Multilink { // eslint-disable-line @typescript-eslint/explicit-member-accessibility - return new Multilink( dependencies, callback, false /* lazy */ ); + static multilink( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static multilink( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, options?: MultilinkOptions ): Multilink { // eslint-disable-line @typescript-eslint/explicit-member-accessibility + return new Multilink( dependencies, callback, options ); } /** * Create a Multilink from a dynamic or unknown number of dependencies. */ - public static multilinkAny( dependencies: Readonly[]>, callback: () => void ): UnknownMultilink { + public static multilinkAny( dependencies: Readonly[]>, callback: () => void, options?: MultilinkOptions ): UnknownMultilink { // @ts-expect-error - return new Multilink( dependencies, callback ); + return new Multilink( dependencies, callback, options ); } /** @@ -198,23 +225,25 @@ * @param dependencies * @param callback function that takes values from the properties and returns nothing */ - static lazyMultilink( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility - static lazyMultilink( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void ): Multilink { // eslint-disable-line @typescript-eslint/explicit-member-accessibility - return new Multilink( dependencies, callback, true /* lazy */ ); + static lazyMultilink( dependencies: RP1, callback: ( ...params: [ T1 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP2, callback: ( ...params: [ T1, T2 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP3, callback: ( ...params: [ T1, T2, T3 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP4, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP5, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP6, callback: ( ...params: [ T1, T2, T3, T4, T5, T6 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP7, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP8, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP9, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP10, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP11, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP12, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP13, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP14, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: RP15, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, options?: MultilinkOptions ): Multilink; // eslint-disable-line @typescript-eslint/explicit-member-accessibility + static lazyMultilink( dependencies: Dependencies, callback: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => void, options?: MultilinkOptions ): Multilink { // eslint-disable-line @typescript-eslint/explicit-member-accessibility + options = options || {}; + options.lazy = true; + return new Multilink( dependencies, callback, options ); } /** ```

I also observed that cases like this are triggering. Maybe the check is hitting set as well as get? Can we restrict it to just visit the get occurrences?

    // Set the dispensing rate to zero when the shaker becomes empty or invisible.
    Multilink.multilink( [ this.isEmptyProperty, this.visibleProperty ], ( isEmpty, visible ) => {
      if ( isEmpty || !visible ) {
        this.dispensingRateProperty.value = 0;
      }
    }, {
      accessNonDependencies: true
    } );

[pixelzoom]

... Can we restrict it to just visit the get occurrences?

Yes. There is no dependency on a Property's value unless get is called.

[samreid]

I wonder if https://github.com/phetsims/axon/issues/441#issuecomment-1810598219 is just triggering gets elsewhere. The guard is only in ReadOnlyProperty.get

[samreid]

Anyways, it feels there are 2x-5x more issues with Multilink than we saw with DerivedProperty. I'm at a good point to check in with @pixelzoom synchronously.

[jonathanolson]

I have some concerns that these checks might lead to brittle code. Ran into the above thing broken, however I'm able to break things with completely-unrelated changes now.

Say I need to refactor Color, and it needs to read a Property during the execution of withAlpha. Bam, we've broken the DerivedProperty in RectangularButton that uses withAlpha. (immediate fails on any sims)

Even more tricky, say, I add a Property access to Color.colorUtilsBrighter... BAM now I've broken a DerivedProperty in gravity-force-lab. Or accessing a Property in BunnyCounts' constructor will error out natural-selection.

Essentially, now you have to provide a flag to a DerivedProperty if ANY part if its implementation could possibly in the future access a Property?

But also... does this mean for instance if we ever have something in StringUtils.format/fillIn that accesses a Property, we need to tag thousands of DerivedProperties with this flag? Or are we saying "hey, for all sorts of common behaviors, we're no longer ALLOWED to use Properties in their implementation"?

I'm worried people are going to be breaking this left-and-right.

[samreid]

Based on https://github.com/phetsims/greenhouse-effect/issues/370 and https://github.com/phetsims/axon/issues/441#issuecomment-1811150610 I've commented out the assertion for now. Let's check in at or before next developer meeting to discuss.

[samreid]

Demo patch, may be stale in a few weeks:

```diff Subject: [PATCH] Update API due to gravity change and ignore initial value changes, see https://github.com/phetsims/phet-core/issues/132 --- Index: my-solar-system/js/common/view/InteractiveNumberDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/my-solar-system/js/common/view/InteractiveNumberDisplay.ts b/my-solar-system/js/common/view/InteractiveNumberDisplay.ts --- a/my-solar-system/js/common/view/InteractiveNumberDisplay.ts (revision ace4470b735cb3bf015b8edc26d9622b2cefc370) +++ b/my-solar-system/js/common/view/InteractiveNumberDisplay.ts (date 1700160330360) @@ -61,6 +61,8 @@ tandem: Tandem.OPT_OUT } ); + const shouldBeGrayProperty = new BooleanProperty( false ); + const options = optionize()( { // SelfOptions @@ -77,7 +79,11 @@ backgroundFill: new DerivedProperty( [ userIsControllingProperty, isEditingProperty, hoverListener.looksOverProperty, bodyColorProperty ], ( userControlled, isEditing, looksOver, bodyColor ) => { - return userControlled || isEditing || looksOver ? bodyColor.colorUtilsBrighter( 0.7 ) : Color.WHITE; + return userControlled || isEditing || looksOver ? bodyColor.colorUtilsBrighter( 0.7 ) : + Color.WHITE; + + // return userControlled || isEditing || looksOver ? bodyColor.colorUtilsBrighter( 0.7 ) : + // shouldBeGrayProperty.value ? Color.GRAY : Color.WHITE; } ), backgroundStroke: Color.BLACK, 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 0a4fea8bd25044bbdfe23736ce029032a918166d) +++ b/axon/js/ReadOnlyProperty.ts (date 1700159937574) @@ -233,7 +233,7 @@ if ( !currentDependencies.includes( this ) ) { // TODO: Re-enable assertion, see https://github.com/phetsims/axon/issues/441 - // assert && assert( false, 'accessed value outside of dependency tracking' ); + assert && assert( false, 'accessed value outside of dependency tracking' ); } } return this.tinyProperty.get(); Index: keplers-laws/js/common/view/KeplersLawsScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/keplers-laws/js/common/view/KeplersLawsScreenView.ts b/keplers-laws/js/common/view/KeplersLawsScreenView.ts --- a/keplers-laws/js/common/view/KeplersLawsScreenView.ts (revision b44f1a18abd88ce751cf8183ced00297a68a512b) +++ b/keplers-laws/js/common/view/KeplersLawsScreenView.ts (date 1700159249850) @@ -392,6 +392,9 @@ } ); this.topLayer.addChild( stopwatchNode ); + setInterval( () => { + KeplersLawsStrings.units.yearsStringProperty.value += '.'; + }, 1000 ); // Slider that controls the bodies mass this.interfaceLayer.addChild( lawsPanelsBox ); ```

[pixelzoom]

Following up on https://github.com/phetsims/axon/issues/441#issuecomment-1810338145 and https://github.com/phetsims/axon/issues/441#issuecomment-1810592906... @samreid Can we please remove the requirement for self-referential dependencies? Besides the fact that it's impossible to add a DerviedProperty as a dependency of itself ("used before being assigned" error), it's not even an actual problem. And I have GitHub issues on hold that I'd like to close.

[pixelzoom]

After addressing missing depedencies in several of my sims, CT is reporting numerous problems -- see the issues linked immediately above. I'm regretting making these changes, because this is going to be significant work to investigate.

[samreid]

In this patch, I experimented with allowing the DerivedProperty to access its own value in its derivation. It is working but I don't want to commit without discussion due to some complex parts.

```diff Subject: [PATCH] Fix tandem for FieldPanel, see https://github.com/phetsims/projectile-data-lab/issues/7 --- 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 a15e2ee2627699f0b595555e762826ada9ad67e7) +++ b/axon/js/ReadOnlyProperty.ts (date 1700425220427) @@ -233,7 +233,7 @@ if ( !currentDependencies.includes( this ) ) { // TODO: Re-enable assertion, see https://github.com/phetsims/axon/issues/441 - // assert && assert( false, 'accessed value outside of dependency tracking' ); + assert && assert( false, 'accessed value outside of dependency tracking' ); } } return this.tinyProperty.get(); Index: ph-scale/js/common/model/Solution.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/ph-scale/js/common/model/Solution.ts b/ph-scale/js/common/model/Solution.ts --- a/ph-scale/js/common/model/Solution.ts (revision c5f72807ffca872b301b3ec616b22003acc265cf) +++ b/ph-scale/js/common/model/Solution.ts (date 1700425719920) @@ -112,9 +112,11 @@ } ); this.pHProperty = new DerivedProperty( - [ this.soluteProperty, this.soluteVolumeProperty, this.waterVolumeProperty ], - ( solute, soluteVolume, waterVolume ) => { - if ( this.ignoreVolumeUpdate ) { + [ this.soluteProperty, this.soluteVolumeProperty], + ( solute, soluteVolume) => { + + const waterVolume = this.waterVolumeProperty.value; + if ( this.hasOwnProperty( 'pHProperty' ) ) { return this.pHProperty.value; } else { @@ -125,9 +127,11 @@ phetioFeatured: true, phetioValueType: NullableIO( NumberIO ), phetioDocumentation: 'pH of the solution', - phetioHighFrequency: true, - accessNonDependencies: true //TODO https://github.com/phetsims/ph-scale/issues/290 dependency on itself + phetioHighFrequency: true + // accessNonDependencies: true //TODO https://github.com/phetsims/ph-scale/issues/290 dependency on itself } ); + + this.waterVolumeProperty.value++; this.colorProperty = new DerivedProperty( [ this.soluteProperty, this.soluteVolumeProperty, this.waterVolumeProperty ], 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 a15e2ee2627699f0b595555e762826ada9ad67e7) +++ b/axon/js/DerivedProperty.ts (date 1700425670378) @@ -41,9 +41,12 @@ /** * Compute the derived value given a derivation and an array of dependencies */ -function getDerivedValue( accessNonDependencies: boolean, derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies ): T { +function getDerivedValue( + accessNonDependencies: boolean, + derivedProperty: ReadOnlyProperty | null, + derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies ): T { - assert && !accessNonDependencies && derivationStack.push( dependencies ); + assert && !accessNonDependencies && derivationStack.push( [ derivedProperty, ...dependencies ] ); try { @@ -102,7 +105,7 @@ assert && assert( dependencies.every( _.identity ), 'dependencies should all be truthy' ); assert && assert( dependencies.length === _.uniq( dependencies ).length, 'duplicate dependencies' ); - const initialValue = getDerivedValue( options.accessNonDependencies, derivation, dependencies ); + const initialValue = getDerivedValue( options.accessNonDependencies, null, derivation, dependencies ); // We must pass supertype tandem to parent class so addInstance is called only once in the subclassiest constructor. super( initialValue, options ); @@ -171,7 +174,7 @@ this.hasDeferredValue = true; } else { - super.set( getDerivedValue( this.accessNonDependencies, this.derivation, this.definedDependencies ) ); + super.set( getDerivedValue( this.accessNonDependencies, this, this.derivation, this.definedDependencies ) ); } } @@ -204,7 +207,7 @@ */ public override setDeferred( isDeferred: boolean ): ( () => void ) | null { if ( this.isDeferred && !isDeferred ) { - this.deferredValue = getDerivedValue( this.accessNonDependencies, this.derivation, this.definedDependencies ); + this.deferredValue = getDerivedValue( this.accessNonDependencies, this, this.derivation, this.definedDependencies ); } return super.setDeferred( isDeferred ); } ```

First, I could not find a suitable type for the derivedProperty in getDerivedValue because I could not get UnknownDerivedProperty to work.

Second, we must compute the initial value of the DerivedProperty before calling super() so it knows the initial value. However, we cannot access this before the super call. So I added a null alternative for that call. But it also means that DerivedProperty instances should not self-access during their first derivation.

[marlitas]

Dev Meeting 12/7/23

• JB: This seems like a good idea overall, but I have concerns from working in Description. I am consciously not using description strings as a dependency.
• JO: I would like this to be a tool and not an assertion failure. I am concerned about writing common code expressions that could break when running simulations.
• SR: We could add a shut-off valve in DerivedProperty where you are consciously turning off the requirement.
• JO: I can't write common code that accesses a bunch of different Properties. This would require hundreds of shut-off valves. I'm worried about what might happen, but I also ran into this failure already.
• CM: We did not see a systemic problem. I am also concerned about description strings, is this going to create a maintenance headache?
• JB: With a NumberProperty example, maybe you don't care if the range changes you only care about the number.
• SR: For now can we just bury it behind a QueryParameter? And there is still an option to opt out.
• MS: That sounds good. Also add to CRC?
• CM: This seems important for dynamic locale. Would catch a lot of potential errors.
• JO: What if I add something to common code that breaks all of this?
• MK: We'll address it then.

Consensus: Turn it into a Query Parameter. SR & CM will finalize other details.

[pixelzoom]

@samreid let's pick a time to meet and figure out next steps.

Also a reminder that before we can put this behind a query parameter (and/or package.json config), we need to figure out how to omit self-referrential dependencies, see https://github.com/phetsims/axon/issues/441#issuecomment-1817537544.

[pixelzoom]

Notes from meeting with @samreid:

• Add a query parameter, to be used for opting in, as a development tool: &strictAxonDependencies=true|false (default false). We chose this name for it's potential to be applicable to Multilink, etc. in the future. And we don't want to create confusion with other types of "dependencies" like dev/repo dependencies.

• Add a package.json entry for opting in for a repo: phet.simFeatures.strictAxonDependencies: true|false (default false). Query parameter takes precedence over package.json.

• Add strictAxonDependencies to phetmarks, with UI: o true o false o Simulation Default

• Rename DerivedPropertyOptions.accessNonDependencies: false to strictAxonDependencies: true (and invert logic).

This can be done in the next iteration, followed by a developer PSA.

@samreid will take the lead on the above bullets, @pixelzoom will review.

[pixelzoom]

@samreid and I moved forward on the issue of self-referrential dependency. Below is a refined version on the patch that @samreid created above in https://github.com/phetsims/axon/issues/441#issuecomment-1817969157.

We decided that this complicates DerivedProperty unnecessarily. And self-reference should be something that is rarely done, possibly even to be avoided. In cases where it is necessary, the developer should opt out of with strictAxonDependencies: false.

patch

```diff Subject: [PATCH] doc fix --- Index: js/DerivedProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/DerivedProperty.ts b/js/DerivedProperty.ts --- a/js/DerivedProperty.ts (revision 15c7181b222ae6c85b13abfde7775364c010e1e4) +++ b/js/DerivedProperty.ts (date 1702925402671) @@ -41,9 +41,19 @@ /** * Compute the derived value given a derivation and an array of dependencies */ -function getDerivedValue( accessNonDependencies: boolean, derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies ): T { +function getDerivedValue( + accessNonDependencies: boolean, + derivedProperty: ReadOnlyProperty | null, // null when computing initialValue during construction + derivation: ( ...params: [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ] ) => T, dependencies: Dependencies ): T { - assert && !accessNonDependencies && derivationStack.push( dependencies ); + if ( assert && !accessNonDependencies ) { + const array = [ ...dependencies ]; + + // Exclude this DerivedProperty from its list of required dependencies. + // See https://github.com/phetsims/axon/issues/441 + derivedProperty && array.push( derivedProperty ); + derivationStack.push( array ); + } try { @@ -102,7 +112,7 @@ assert && assert( dependencies.every( _.identity ), 'dependencies should all be truthy' ); assert && assert( dependencies.length === _.uniq( dependencies ).length, 'duplicate dependencies' ); - const initialValue = getDerivedValue( options.accessNonDependencies, derivation, dependencies ); + const initialValue = getDerivedValue( options.accessNonDependencies, null, derivation, dependencies ); // We must pass supertype tandem to parent class so addInstance is called only once in the subclassiest constructor. super( initialValue, options ); @@ -171,7 +181,7 @@ this.hasDeferredValue = true; } else { - super.set( getDerivedValue( this.accessNonDependencies, this.derivation, this.definedDependencies ) ); + super.set( getDerivedValue( this.accessNonDependencies, this, this.derivation, this.definedDependencies ) ); } } @@ -204,7 +214,7 @@ */ public override setDeferred( isDeferred: boolean ): ( () => void ) | null { if ( this.isDeferred && !isDeferred ) { - this.deferredValue = getDerivedValue( this.accessNonDependencies, this.derivation, this.definedDependencies ); + this.deferredValue = getDerivedValue( this.accessNonDependencies, this, this.derivation, this.definedDependencies ); } return super.setDeferred( isDeferred ); } ```

[samreid]

OK, I addressed the bullet points in https://github.com/phetsims/axon/issues/441#issuecomment-1861261509 and ran the following tests:

1. Add this code to projectile-data-lab main:

  const aProperty = new Property( 1 );
  const bProperty = new Property( 2 );
  const dProperty = new Property( 'x' );
  const cProperty = new DerivedProperty( [ aProperty, bProperty ], ( a, b ) => {
    console.log( dProperty.value );
    return a + b;
  } );
  console.log( cProperty.value );

Test these scenarios:

query parameter	package.json	expected	actual
true	true	assertion error	assertion error
true	false	assertion error	assertion error
true	not specified	assertion error	assertion error
false	true	no error	no error
false	false	no error	no error
false	not specified	no error	no error
not specified	true	assertion error	assertion error
not specified	false	no error	no error
not specified	not specified	no error	no error

Additionally test with one of the assertion error rows above, and this option, to make sure it doesn't fali.

  const aProperty = new Property( 1 );
  const bProperty = new Property( 2 );
  const dProperty = new Property( 'x' );
  const cProperty = new DerivedProperty( [ aProperty, bProperty ], ( a, b ) => {
    console.log( dProperty.value );
    return a + b;
  }, {
    strictAxonDependencies: false
  } );
  console.log( cProperty.value );

Everything seems to be working correctly here, and this additionally caught 2x regressions in Projectile Data Lab that would not have i18n correctly. So I think we are ready for a commit and review.

Further sim testing

I additionally tried turning off the strictAxonDependencies: false in ConcentrationSolution, and specifying the ?strictAxonDependencies=true but was unable to get that error to trigger by saturating a solution in Beer's Law Lab. @pixelzoom should double check this context and make sure there is no trouble.

I wanted to double-check the opt-out cases in ph-scale, but it was catching an earlier problem in ComboBoxButton.toString which was calling a Property.toString. Likely this was introduced in https://github.com/phetsims/sun/commit/071966ef2adeecc77d75340a86c3b17f51f52d33. So we will likely need to re-review cases as we enable this test for specific sims.

That being said, the tests above seem like it is a reasonable point for check in with @pixelzoom before we go further. @pixelzoom can you please review and advise next steps?

[samreid]

@pixelzoom and I discussed that we aren't certain whether this should be opt-in or opt-out. It is currently set for opt-in, with only 1 sim (projectile-data-lab) opting in.

[pixelzoom]

In https://github.com/phetsims/axon/issues/441#issuecomment-1873399696, @samreid said:

... Everything seems to be working correctly here, and this additionally caught 2x regressions in Projectile Data Lab that would not have i18n correctly. So I think we are ready for a commit and review.

Your testing looks thorough.

I additionally tried turning off the strictAxonDependencies: false in ConcentrationSolution, and specifying the ?strictAxonDependencies=true but was unable to get that error to trigger by saturating a solution in Beer's Law Lab. @pixelzoom should double check this context and make sure there is no trouble.

I'll investigate.

I wanted to double-check the opt-out cases in ph-scale, but it was catching an earlier problem in ComboBoxButton.toString which was calling a Property.toString. Likely this was introduced in https://github.com/phetsims/sun/commit/071966ef2adeecc77d75340a86c3b17f51f52d33. So we will likely need to re-review cases as we enable this test for specific sims.

I investigated. This was because ph-scale.Solute.toString was using a StringProperty, and being called in an assertion message in ComboBoxButton. I changed the implementation fo Solute.toString, and the problem is resolved.

That being said, the tests above seem like it is a reasonable point for check in with @pixelzoom before we go further. @pixelzoom can you please review and advise next steps?

Which commits should I review?

[pixelzoom]

In https://github.com/phetsims/axon/issues/441#issuecomment-1881584454, @samreid said:

@pixelzoom and I discussed that we aren't certain whether this should be opt-in or opt-out. It is currently set for opt-in, with only 1 sim (projectile-data-lab) opting in.

The default should definitely be strictAxonDependencies: true.

I tested locally to see how many sims fail with strictAxonDependencies=true. Here's my phetmarks URL: http://localhost:8080/aqua/fuzz-lightyear/?loadTimeout=30000&testTask=true&ea&audio=disabled&testDuration=10000&brand=phet&fuzz&strictAxonDependencies=true

The failing repos are:

• arithmetic - needs to use DerivedStringProperty instead of PatternStringProperty
• gas-properties, gases-intro, diffusion - because of https://github.com/phetsims/scenery-phet/issues/781
• number-line-operations - needs to use DerivedStringProperty instead of PatternStringProperty
• wave-interference, waves-intro - because of https://github.com/phetsims/scenery-phet/issues/781

Note that 5 sims are still failing due to problems with StopwatchNode and NumberDisplay, https://github.com/phetsims/scenery-phet/issues/781.

Since there are only a handful of sims failing, I recommend:

• Set the default so that each repo is strict, and can opt out as necessary.
• Create GitHub issues for the failing sims.
• Add specific uses of strictAxonDependencies: false (with a TODO referencing GitHub issue) for the failing cases. If that's not possible, opt out in package.json. Note in the sim issues.
• Resolve https://github.com/phetsims/scenery-phet/issues/781.

@samreid How do you want to proceed?

[pixelzoom]

@samreid said:

I additionally tried turning off the strictAxonDependencies: false in ConcentrationSolution, and specifying the ?strictAxonDependencies=true but was unable to get that error to trigger by saturating a solution in Beer's Law Lab. @pixelzoom should double check this context and make sure there is no trouble.

I had the same result, with fuzzing. For this case, I was opting out because of a missing self-referential dependency. Did you commit something that omits self-referential dependencies?

UPDATE from discussion with @samreid and @pixelzoom SR: We saw precipitateMolesProperty has a self reference, but has to opt out of strict check due to that self reference.

[samreid]

Would be good to review:

https://github.com/phetsims/axon/commit/e411b7b41630d7c9f1eca473f1e0cf22b930cd38

[pixelzoom]

@samreid and I worked on this for ~1 hour today, related commits above.

Remaining work, which I'll do:

• [x] aqua test
• [x] commit change to initialize-globals.ts
• [x] create sim-specific GitHub issues for https://github.com/phetsims/arithmetic/commit/3b8b9e4c3f324a58b1f97b452e9e75bff99e5115 , https://github.com/phetsims/number-line-operations/commit/f2f4fb0369f1998de9b9451adfdf607a2a8d0c50
• [x] PSA to Slack#developer
• [x] review https://github.com/phetsims/axon/commit/e411b7b41630d7c9f1eca473f1e0cf22b930cd38
• [x] close this issue

[pixelzoom]

Reviewing https://github.com/phetsims/axon/commit/e411b7b41630d7c9f1eca473f1e0cf22b930cd38 resulted in 1 documentation fix in https://github.com/phetsims/axon/commit/089c4b10fbc059ead6657692779b57a6d8883a7f.

All work completed, closing.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[pixelzoom]

There are TODOs in common code, see below. I'll handle these, either by addressing them, or by creating specific GitHub issues.

• [x] scenery-phet DragBoundsProperty.js (1)
• [x] scenery-phet MeasuringTapeNode.ts (1)
• [x] scenery-phet NumberAccumulator.ts (1)
• [ ] sun ComboBox (2)
• [x] vegas LevelCompletedNode.ts (1)

[zepumph]

• [x] The scenery-phet demo also looks pretty broken on CT. I'm unsure how bad though (just won't run).

pixelzoom edit: Fixed in https://github.com/phetsims/scenery-phet/commit/1fb6dc83a7f9dcda7e941be041a9ffa46ddbd0e0 and https://github.com/phetsims/scenery-phet/commit/0e57c5738fd486c45b3c49d329a4863ee80ec893