Tandem.REQUIRED is no longer checked

[samreid]

Discovered in #191

I tested on a clean working copy by adding an uninstumented Checkbox and running with ?phetioValidateTandems and was surprised to see that this was not caught. I wonder if I'm testing wrong or if this will need to be corrected.

UPDATE: @pixelzoom tested on his working copy and saw the same thing. Creating an instrumented Checkbox with ?phetioValidateTandems (the default) does not trigger any error.

[samreid]

This used to be checked at

tandem/js/Tandem.js

Line 135 in 87e72e2

assert && Tandem.VALIDATION && assert( !( this.required && !this.supplied ), 'Tandem was required but not supplied' );

but now I don't think objects with {tandem: Tandem.REQUIRED} are reaching that point. @zepumph can you please take a look?

[pixelzoom]

Is this a problem in the ph-scale 1.4 release branch? If so, is it something that should be patched, so that we don't make PhET-iO mistakes in 1.4 maintenance releases?

[zepumph]

Yes I think that this has been a bug since a commit on June 5th where this was added:

    // The presence of `tandem` indicates if this PhetioObject can be intiialized. If not yet initialized, perhaps
    // it will be initialized later on, as in Node.mutate().
    if ( !( config.tandem && config.tandem.supplied ) ) {
      assert && !config.tandem && assert( !specifiesNonTandemPhetioObjectKey( config ), 'only specify metadata when providing a Tandem' );

      if ( config.tandem ) {
        this.tandem = config.tandem;
      }
      return;
    }

In this commit: 85f4515

This performance improvement exits early if the tandem is not supplied. We could change this to be only for unrequired Tandems. Another option would be to move up some validation about Tandem.REQUIRED/OPTIONAL to this block.

There is a balance to maintain. The most performant thing to do would be to detect and exit as early as possible, which is where this block is now at the top of PhetioObject.initializePhetioObject. That said it feels like this validation really applies to Tandem.js, and may be best where it is now in Tandem.addPhetioObject.

Probably the way to go would be to move these assertions up, and continue to opt out of as much logic as possible for types that don't need to use it.

@samreid, please review and discuss.

Is this a problem in the ph-scale 1.4 release branch? If so, is it something that should be patched, so that we don't make PhET-iO mistakes in 1.4 maintenance releases?

I understand the potential desire to add this into the branch for completeness and maintainability. That said, ph-scale was instrumented before that was broken, and so most likely this isn't effecting that sim. I recommend to fix this on master, see if there are any issues with ph-scale, and then likely decide from there.

It could be an easy patch, with just an addition of a single assertion to support erroring for Tandem.REQUIRED that are not supplied.

[pixelzoom]

ph-scale was instrumented before that was broken, and so most likely this isn't effecting that sim.

The concern is with ongoing maintenance of the 1.4 branch. There's no telling how long that branch will be the production branch for ph-scale. And if that branch doesn't correctly detect Tandem.REQUIRED, then it would be possible to accidentally remove a required tandem in a maintenance release. So once the fix is identified, please investigate patching the ph-scale 1.4 branch.

[samreid]

@zepumph are you recommending a check like this? It appears to test OK. If we do this, what should we do with the code in Tandem.js that used to do this part?

Index: js/PhetioObject.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetioObject.js	(revision 6fddb61195a44ba31c825191f0fb2addd768d6df)
+++ js/PhetioObject.js	(date 1595258592175)
@@ -232,6 +232,10 @@
   initializePhetioObject: function( baseOptions, config ) {
     assert && assert( config, 'initializePhetioObject must be called with config' );
 
+    if ( config.tandem && config.tandem.required ) {
+      assert && assert( config.tandem.supplied, 'required tandems must be supplied' );
+    }
+
     // The presence of `tandem` indicates if this PhetioObject can be intiialized. If not yet initialized, perhaps
     // it will be initialized later on, as in Node.mutate().
     if ( !( config.tandem && config.tandem.supplied ) ) {

[zepumph]

@samreid, could we please pair on this on Friday. I think that would be the "work-around" sort of solution—quick-and-dirty. I think the best way to proceed is to look at how and why initializePhetioObject is being called. Note that quite a bit of logic is being called even when !Tandem.PHET_IO_ENABLED. I would like to take this chunk of code to the next level of cleanliness. I foresee the conversation touching on how PhetioObject and Tandem have a blurred division of responsibility.

[zepumph]

Adding blocking publication since it is holding up ph-scale

[pixelzoom]

Let me know when this is fixed, so that I can cherry-pick for phetsims/ph-scale#183.

[zepumph]

I implemented the above workaround. This is now ready for cherry-picking into RC branches. @samreid I still don't know how to remedy this with the validation that is currently in Tandem.js. Can we still discuss this code duplication soon?

[pixelzoom]

fba18d1 has been cherry-picked to ph-scale and ph-scale-basics 1.4 branches for phetsims/ph-scale#183.

[zepumph]

I marked this for collaboration to discuss Tandem assertions that don't run often now because of the PhetioObject early exit.

[zepumph]

@samreid and I think that having the Tandem.REQUIRED check in Tandem.addPhetioObject and at the beginning of PhetioObject.initializePhetiObject is OK. That is really the only assertion in Tandem.addPhetioObject, so there isn't much duplication. Closing