Property should support validator from phetioType

[zepumph]

Related to phetsims/dot#88 because there we are talking about how to support phetioType and validator keys in Property. Similar to #204 because we did similar work in Emitter to omit duplication of supplying validators and phetioType when not necessary.

I created this issue because I saw work being done in phetsims/dot#88 like
335ef9f which worried me. Would it be possible to have that valueType come from the phetioType so that it isn't duplicated?

[pixelzoom]

Would it be possible to have that valueType come from the phetioType so that it isn't duplicated?

Certainly desirable not to have both. I don't have a feel for how much work is involved.

[samreid]

@zepumph can you make a proposal for how we should deal with this in files like BooleanProperty?

[pixelzoom]

I'm going to transfer this issue from dot to axon, because it's about Property, not specific to dot.

[zepumph]

@zepumph can you make a proposal for how we should deal with this in files like BooleanProperty?

It may be simpler to start with thinking just about Property and not sub types. In Emitter we developed a strategy for having EmitterIO define the validators for each argument, and then in Emitter it was simple to say that either the client provided validators options OR phetioType but not both, and we get the validators from the EmitterIO type in the latter case.

Here is just one possible solution. I would propose a similar logic but perhaps not as strict. I think it would be best to say that if you provide any validator keys, those take precident and the phetioType is ignored, but IF you provide a phetioType and NO validator keys to Property, then it will get the validator from the phetioType. The logic in PropertyIO will be extremely simple, because it just needs to export the value typeIO's validator.

Usages looks like this:

new Property( {
  valueType: 'number'
  phetioType: 'PropertyIO( NumberIO )'
});

This would not error, but could be simplified to:

new Property( {
  phetioType: 'PropertyIO( NumberIO )'
});

Above, the validator comes from NumberIO.validator

More cases create more choices for us to decide direction:

new Property( {
  valueType: 'number',
  validValues: [23,3,4],
  phetioType: 'PropertyIO( NumberIO )'
});

I think that the simplest logic in the code would be to say that we ignore the validator from the phetioType here. So there would be no simplification available.

---

Another, more complicated but more paralleled to Emitter approach would be to only be able to specify the validator through the phetioType if the phetioType is provided. We could have many more assertions here, and would be more strict. In general there would be less options to Property too (because more validator keys would move to the PropertyIO type definition).

• Not allowed to pass in any validator keys if you provide a phetioType. Note that there is no default phetioType in Property like there is in Emitter (not sure how that matters just yet).
• If you provide the phetioType, then you get the validator from the value typeIO.
• If you need more validation than the default value typeIO can provide (like isValidValue/validValues), then you must provide the ENTIRE validator as an option to PropertyIO when creating the type, similar to our rules for defining EmitterIO types. So specifying a validator may look like:

new Property( {
  phetioType: PropertyIO( StringIO, { 
    valueType: 'string', 
    validValues: ['up', 'not down'] 
} );

---

As it pertains to Property sub types, BooleanProperty would only define the phetioType, and not the valueType (because that would come from the phetioType). NumberProperty would look differently I think depending on the strategy above. Just a reminder that I think at this time I prefer the first option for its flexibility.

[pixelzoom]

Feedback on previous comment...

First, I think it would be preferable to synthesize phetioType from valueType, not the other way around. The valueType syntax is much more natural. And validators have uses that don't involve PhET-iO

Re these 2 examples, which I'll give names here:

// Example 1
new Property( {
  valueType: 'number'
  phetioType: 'PropertyIO( NumberIO )'
});

// Example 2
new Property( {
  valueType: 'number',
  validValues: [23,3,4],
  phetioType: 'PropertyIO( NumberIO )'
} );

About Example 2, you said:

I think that the simplest logic in the code would be to say that we ignore the validator from the phetioType here. So there would be no simplification available.

I don't understand why no simplification would be available, or why it would be more complicated (or any different) to implement than Example 1. phetioType and valueType are 2 ways of specifying the same thing. And validValues is a completed independent check that happens after the type has been validated. So it should be possible to simplify regardless of what other validation options are provided.

Re:

Another, more complicated but more paralleled to Emitter approach would be to only be able to specify the validator through the phetioType if the phetioType is provided. ....

I don't see the advantage to this. It has the same duplication of type information that phetioType and valueType currently have. And it seems even more complicated to grok and implement.

[pixelzoom]

Above, I said:

First, I think it would be preferable to synthesize phetioType from valueType, not the other way around.

Over in #228, we came to the consensus that we should support valueTypes: [ Type1, Type2, ... ] and allow null as one of the type values. I still think that's a good design. But...

It makes synthesizing phetioType trick. PhET-iO doesn't support Properties that have dynamic types - which I still think is a mistake, but that ship has sailed. So something like:

valueTypes: [ VoltMeter, OhmMeter, MeasuringTape, null ]

... is not expressible as a phetioType. We would be limited to valueTypes: [ Type ] or valueTypes: [ Type, null ], which would convert to phetioType: TypeIO and phetioType: NullableIO( TypeIO ) respectively.

[pixelzoom]

First, I think it would be preferable to synthesize phetioType from valueType, not the other way around.

Here's an untested function to illustrate what I had in mind. See especially the "NEW" comment.

  function valueTypesToPhetioType( valueTypes ) {

    assert && assert( Array.isArray( valueType ), 'valueTypes must be an Array: ' + valueTypes );
    assert && assert( valueTypes.length === 1 || ( valueTypes.length === 2 && valueTypes.indexOf( null ) !== -1 ),
      'valueTypes must be either [ Type ] or [ Type, null ]' );

    const typeName = _.first( valueTypes, value => value !== null );
    const nullable = ( valueTypes.indexOf( null ) !== -1 );

    if ( typeName === 'boolean' ) {
      return nullable ? NullableIO( BooleanIO ) : BooleanIO;
    }
    else if ( typeName === 'number' ) {
      return nullable ? NullableIO( NumberIO ) : NumberIO;
    }
    else if ( typeName === 'string' ) {
      return nullable ? NullableIO( StringIO ) : StringIO;
    }
    else {
      assert && assert( typeName.hasOwnProperty( 'prototype' ), `not a type or class: ' + typeName );

      // NEW: Every type that supports PhET-iO must have a phetioType field that identifies its associated IO type.
      assert && assert( typeName.hasOwnProperty( 'phetioType' ), 'missing phetioType for ' + typeName );
      return nullable ? NullableIO( typeName.phetioType ) : typeName.phetioType;
    }
  }

[zepumph]

I would be willing to experiment with the above if we agree it is the right direction, but I feel like it is backwards. A phetioType already has a validator built into it. It is easy to map from phetioType to validator, and it is easy to assert that if a phetioType is specified, then we don't need the valueType specified through Property options. This is what is done in Emitter as well. Note that the "NEW" comment above illustrates a pattern that could work, but would involve quite a bit of refactoring, because each type would now need a phetioType assigned to it, and with so many parametric phetioTypes around, it seems fragile and like we may be boxing ourselves into a corner.

[zepumph]

Assigning @pixelzoom and @samreid for comment

[pixelzoom]

Discussed with @zepumph on Zoom, and I'm convinced that we can't map from valueType to phetioType. That's unfortunate since valueType is (or should be) already present in code that is being instrumented, and converting valueType to phetioType during instrumentation is more work, and an opportunity to introduce errors.

It's also currently the case that you can't remove valueType because phetioType, because phetioType validation doesn't occur in brand=phet (this is currently the case for Property). That's unfortunate because we're going to end up with both valueType and phetioType type specification in code. Recommended to correct this deficiency asap for all cases.

[samreid]

Is it your recommendation to run phetioType-based validation even in brand=phet, or some other strategy?

[zepumph]

After a conversation with @pixelzoom, we came up with my favorite recommendation for a path forward for Property so far:

• Property support either valueType OR phetioType, but not both
• Property gets the valueType from the phetioType.validator if phetioType is supplied.
• Property still supports other validator options like validValues and isValidValue on its own; PropertyIO does not.
• Property validates on two validators appropriately, when phetioType is specified. One from the phetioType.validator (which can include more than just phetioType), and the validator keys passed in from options (asserting that valueType isn't specified).

Here phetioType validation would occur even in phet brand. This helps us remove the duplication of needing valueType in phet brand, and phetioType for phet-io brand.

It is also good to note that @pixelzoom and I discussed that it is still a bummer to need to refactor valueType code into the phetioType upon instrumentation, but we didn't see a way around it, so we are moving forward with the phetioType centric approach.

[zepumph]

It's also currently the case that you can't remove valueType because phetioType, because phetioType validation doesn't occur in brand=phet (this is currently the case for Property). That's unfortunate because we're going to end up with both valueType and phetioType type specification in code. Recommended to correct this deficiency asap for all cases.

Bumping priority.

[zepumph]

Yes indeed we are looking for discussion. Our conclusion is that valueType is redundant the majority of the time when phetioType is specified, and it doesn't need to be included, but if it is, then validate will just validate on both and there won't be any problems or assertions.

What do you think about that?

[zepumph]

Also note that as we speak, support is being added to ValidatorDef to support phetioType as a validator key, see #258

[pixelzoom]

What does "then validate will just validate on both" mean? What is "both"?

[zepumph]

The direct answer to your question is phetioType and valueType. The meaningful answer to your question is that in #258 we added phetioType to ValdatorDef. So a validator can have any combination of the following keys:

valueType
phetioType
isValidValue
validValues

If phetioType is passed in to the validator, then ValidatorDef will look at the phetioType.validator and validate on that too. Please see #258 If you have any more questions about that new feature.

For this issue I'm insterested in discussing #235 (comment) and the pieces that I now feel have changed because of the example in #235 (comment).

Here is a new bullet list of items that I think should be the understanding of how Property behaves.

• Each Property that is passed a phetioType has its value validated on the phetioType's element type's validator.
• The above occurs independent of the other ValidatorDef options that can be passed to Property (valueType, isValidValue, validValues).
• In general it is redundant to specify valueType on a phet-io instrumented Property because the validator from the phetioType will already be checked. In these cases, I propose that it is conventional and cleaner to remove valueType, so that there is less junk passed in Property options. There is no assertion mandating that the valueType is removed.
• phetioType based validation is still done in phet brand
• It is ok for the only validation done to a Property to be via passing phetioType.

NOTE: all of these are now supported. Property gained support for phetioType in #258 along with all other core Types that are validating. Specifically in b55a3bf#diff-a24406f5f2879d5846b0ae23318df8d3R65. I don't think there is anything else to do with this issue so long as we agree on the above bullets.

Assigning for confirmation or discussion.

[samreid]

Everything in the preceding comment sounds great to me, thanks for your efforts on this.

[pixelzoom]

#235 (comment) offers this hypothetical example as the (sole?) reason for keeping both valueType and phetioType:

// We want a more general public phet-io api, but more specific phet core validation
new SubType1( {
	phetioType: SuperTypeIO,
	valueType: SubType1
})

Please convince me. Why would you want a "more general public phet-io api"? Can you provide concrete cases where we would want phetioType and valueType to be semantically different like this?

That said... I'm disappointed that we will apparently continue to have 2 ways of specifying validation, with 2 entirely different syntaxes. I look forward to seeing guidelines for when to use valueType vs phetioType, and when to use both.

[samreid]

For instance, SubType may expose implementation details that are irrelevant to the PhET-iO client. Let's say I have WebGLParticleNode which is essential for the simulation to have good performance, so it is nice to be able to test valueType: WebGLParticleNode, but the detail of WebGL is irrelevant to the PhET-iO client, so they would receive it as type ParticleNodeIO or perhaps only NodeIO depending on the level of detail that is relevant.

Can you provide concrete cases where we would want phetioType and valueType to be semantically different like this?

I believe generally they will not be semantically different, which is why @zepumph said:

In general it is redundant to specify valueType on a phet-io instrumented Property because the validator from the phetioType will already be checked. In these cases, I propose that it is conventional and cleaner to remove valueType, so that there is less junk passed in Property options. There is no assertion mandating that the valueType is removed.

Going to the next part:

That said... I'm disappointed that we will apparently continue to have 2 ways of specifying validation, with 2 entirely different syntaxes.

To clarify, the primary function of a phetioType is to specify how a PhET-iO object can be saved/restored and interoperated with. Since we realized it can also do basic valueType checking, that has been added too as an extra cautionary step. If you want to check that a number is even, then your validator would need isValidValue: n => n%2===0 and that cannot and should not be covered by the IO type. If you have a trivial case like "the type should be a MyObject", then you don't need to specify that via valueType: MyObject if you specify phetioType: MyObjectIO because the IO type will already have valueType: MyObject.

with 2 entirely different syntaxes.

Yes, the syntaxes for valueType and phetioType are different because they are solving fundamentally different problems.

I look forward to seeing guidelines for when to use valueType vs phetioType, and when to use both.

I'd say a simple but (mostly?) accurate rule of thumb would be that the valueType should be omitted if and only if the phetioType.validator is equally or more specific.

the (sole?) reason for keeping both valueType and phetioType

Other reasons to keep valueType and phetioType:

• valueType can be used for types that do not have a corresponding IO type
• valueType can be used in non-PhET-iO simulations.

If we are worried about too many redundant valueTypes after phetioTypes have been added, we could output a warning or assertion error for that?

[pixelzoom]

In general it is redundant to specify valueType on a phet-io instrumented Property because the validator from the phetioType will already be checked

I'd say a simple but (mostly?) accurate rule of thumb would be that the valueType should be omitted if and only if the phetioType.validator is equally or more specific.

My understanding is that IO types are stubs when running with brand !== phet-io. If I replace valueType with phetioType, how do you propose to make phetioType.validator work for all brands?

[samreid]

IO types used to be implemented in the phet-io repo and buried behind ifphetio which replaces things with stubs. However, we decided our code would be more maintainable to move the IO types adjacent to their "core" types. When we made that change, we also changed to loading them as standard modules rather than with the ifphetio plugin. I recall a conversation this week where I also said the IO types were only loaded in PhET-iO brand and was swiftly corrected by @zepumph--my apologies. @zepumph did report above that:

phetioType based validation is still done in phet brand

I tested this by changing QuadraticIO.validator to read:

validator: {
  isValidValue: v => {
    console.log( 'hello' );
    return true;
  }
},

and when running Graphing Quadratics in "phet" brand, I indeed saw several "hello"s. This confirms that IO types are loading and validating in "phet" brand.

[pixelzoom]

Great, thanks for confirming. That makes me more comfortable with replacing valueType with phetioType. Still not excited to do that throughout sims that use valueType, but if that's what we have to do...

[zepumph]

Can you provide concrete cases where we would want phetioType and valueType to be semantically different like this?

In case more is needed. . . I think it is likely that in the future we will have a single TextIO, hiding all of our different "implementation detail" text types behind a single api. In which case the phetioType would be TextIO, but the valueType could be RichText.

Still not excited to do that throughout sims that use valueType, but if that's what we have to do...

I understand that! The choice is between keeping both, and removing valueType when it is redundant with phetioType.validator. It does seem like removing the redundancy may be best long term, but perhaps an argument could be made for keeping it? (not sure).

@pixelzoom do you feel ready to close this issue?

[pixelzoom]

I said:

I look forward to seeing guidelines for when to use valueType vs phetioType, and when to use both.

@samreid said:

I'd say a simple but (mostly?) accurate rule of thumb would be that the valueType should be omitted if and only if the phetioType.validator is equally or more specific.

I would like to see something related to this in the "How to Instrument" document before closing.

[zepumph]

Please review.

[pixelzoom]

Doc looks good, fixed a typo. Closing.