Should Tandem.supplied be public only to PhetioObject?

[zepumph]

I thought of this while reviewing https://github.com/phetsims/joist/issues/616. It seems like we should prefer PhetioObject.isPhetioInstrumented() instead of Tandem.prototype.supplied. I only see 6 usages outside of Tandem and PhetioObject.

@samreid will you review this untested patch and see what you think? All the other usages occurred before the super call, yet this patch for Property seems right, and I would prefer to have isPhetioInstrumented used when possible. Should we spend time limiting tandem.supplied usages further? Is this even an issue? Should we at least apply this patch?

Index: js/Property.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Property.js b/js/Property.js
--- a/js/Property.js    (revision 00d698b1b1a2949255a186c988efb89b582d02b0)
+++ b/js/Property.js    (date 1614643943917)
@@ -97,7 +97,7 @@
       assert && assert( !!options.phetioType.parameterTypes[ 0 ],
         'phetioType parameter type must be specified (only one). Tandem.phetioID: ' + this.tandem.phetioID );
     }
-    assert && assert( !options.tandem.supplied ||
+    assert && assert( !this.isPhetioInstrumented() ||
                       options.tandem.name.endsWith( 'Property' ) ||
                       options.tandem.name === 'property',
       `Property tandem.name must end with Property: ${options.tandem.phetioID}` );

[samreid]

That looks like a good improvement. It also seems we should guard it with VALIDATION, like so:

    assert && Tandem.VALIDATION && assert( !this.isPhetioInstrumented() ||
                                           options.tandem.name.endsWith( 'Property' ) ||
                                           options.tandem.name === 'property',
      `Property tandem.name must end with Property: ${options.tandem.phetioID}` );

Sound OK?

[zepumph]

We don't guard in other cases. Should we?

https://github.com/phetsims/sun/blob/260e29027c31d1193a830a22417e337e476e2b8b/js/buttons/RectangularRadioButton.js#L82-L83

I understand why you want to, but I also feel like it further separates the code path for phet and phet-io.

[samreid]

I was going with the "incremental instrumentation" philosophy where that style of validation wouldn't be checked until necessary. But maybe this is a middle ground where "if the tandem was provided, it should be correct". I don't have a strong preference, what's your recommendation?

[zepumph]

I was going with the "incremental instrumentation" philosophy where that style of validation wouldn't be checked until necessary.

I actually think I have heard the opposite opinion from instrumenters like @pixelzoom, where it is sometimes MORE confusing to only get the assertion in PhET-iO brand, since it splits the code path. Perhaps you may be developing in PhET brand for some time, and then run it once in PhET-iO and find that you reach an assertion error that has nothing to do with your changes because of commits earlier that weren't tested in PhET-iO brand. I'm not sure if that is the exact case, but I can see both sides.

I guess we should just be consistent. @pixelzoom would you prefer assertions about tandem names to be hidden behind "Tandem.VALIDATION" (which is only true when running in phet-io brand).

[pixelzoom]

In general, it's preferrable to verify as many things as possible in all brands. The more brand-specific code paths that you introduce, the more likely that something is going to break "now" and not be noticed until "later".

[samreid]

Excellent point, thanks! I removed the validation guard for this case. There are 32 occurrences of Tandem.VALIDATION, should we open a new issue to check if any of them should be removed?

[zepumph]

It sorta relates, since many check on isPhetioInstrumented(). In general we guard on VALIDATION when we don't want to expose that part of the PhetioObject api in PhET brand. Mostly for phetioType. I think we want to be wary of over doing this though. Turning validation off is a primary way to remove "gotchas" for incremental development.

Looking through all usages, here are cases we could potentially get rid of

• https://github.com/phetsims/axon/blob/9af0a092673437b0fdac369ee5f6b483893d7d8b/js/PropertyStateHandler.js#L93. I would recommend leaving it in even though isPhetioInstrumented can be checked in phet brand.
• https://github.com/phetsims/axon/blob/0cbcf1504e662bc56a97ad22332cba17847b7d18/js/NumberProperty.js#L118. Forcing rangeProperty instrumentation to match the numberProperty's feels like a gotcha, but technically it could be removed.
• [x] This should be removed because the Property one was removed: https://github.com/phetsims/joist/blob/92c5bebfc4e23fbb459be46845e80ddf2d51005c/js/Screen.js#L128
• [x] This should be removed because the Property one was removed: https://github.com/phetsims/joist/blob/92c5bebfc4e23fbb459be46845e80ddf2d51005c/js/ScreenView.js#L74
• [ ] ~Likely should be removed: https://github.com/phetsims/sun/blob/8ce10cb4e4776f437ed12485c666fb051ac7048f/js/Slider.js#L315~

Honestly I'm ambivalent. @samreid do you have a preference on any of the above?

[samreid]

The cross-property ones should keep a VALIDATION guard, to enable incremental instrumentation. The ones that are about a single instance should have the guard removed, and I'll commit that momentarily. https://github.com/phetsims/axon/blob/9af0a092673437b0fdac369ee5f6b483893d7d8b/js/PropertyStateHandler.js#L93 seems general enough that I'd want to leave it as it is.

[samreid]

Back to @zepumph to proceed with the review.

[zepumph]

The cross-property ones should keep a VALIDATION guard, to enable incremental instrumentation. The ones that are about a single instance should have the guard removed,

Well said!

Closing