Simplify Property type validation

[zepumph]

While talking to @samreid about #182, we discussed ways of trying to (a) have the best validation code possible for Emitter and Property as well as (b) have them both share as much api/code as is reasonable.

We came up with a strategy that would allow us to remove isValidValue and validValues, and instead inline everything into valueType

The goal would be to support all types of valueType in the same parameter. Here are the pieces we want to support:

• 'number' -> {string} that you can call typeof value to get the valueType
• Constructor -> {function} that you can call value instanceof X to get the valueType
• predicate functions -> {function} that returns a boolean given the value of if the value is valid.

Property.valueType currently supports the first two, but to support the difference between the second two, @samreid and I have done some brainstorming.

• a constructor has a name property. For example:

// requirejs mode
phet.scenery.Node.name --> 'Node'

// build mode
phet.scenery.Node.name --> 'e'

Anonymous functions don't have a name, so by checking on a name, we could cover many cases, for example:

new Property( { valueType: v => v ===0 || v === null})

If there was a case where you wanted a predicate to have a name, then we could make a convention that that function then needs a reserved property that we can check on:

var namedPredicate = v => { v >=0 || v === null };
namedPredicate.isPredicate = true // or something like that
new Property( { valueType: namedPredicate })

The validation would be pretty simple, and not that different than the current form of AXON/assertValueType:

  function assertValueType( value, valueType, allowUndefined ) {

. . .
   // ******************************************************************************************
    else if( typeof valueType === 'function' && (!valueType.name || valutType.isPredicate){
       assert( valueType( value) ); // because this is a predicate function
     }
   // ******************************************************************************************
. . .
  }

On top of this (sorry for the long issue), having this reserved predicate marker could be really nice for factoring some of these out.

We could have a Validators class where we keep some simple validators like
Validators.greaterThanZero
Validators.numberOrNull
etc. . .

Each of the above static functions would have the isPredicate prop to support the validation as a named function.

We will need to have something like this for each emitter arg as well, so it could be nice to factor it out into a central place

Marking for dev meeting.

[samreid]

PhET-iO uses validValues to enumerate values for radio buttons, so I'm not sure that would be replaced. Or maybe we don't need that feature any more?

Also, if we combine all of this checking into one place, we may need a better name than valueType (since it would support predicates in general, not just types).

[pixelzoom]

Also, if we combine all of this checking into one place, we may need a better name than valueType (since it would support predicates in general, not just types).

+1. Look to lodash for a better name than valueType.

[samreid]

Look to lodash for a better name than valueType.

It seems lodash uses the term predicate for cases like this.

[jonathanolson]

It would be nice to have nice enumeration support here.

What is the recommended way to port validValues?

[zepumph]

What is the recommended way to port validValues?

Depending on how formal we want to get, I like one of the following two ways:

• declare valid values as a function:

predicate: ( value )=>{
   return value === value1 || value === value2 . . .;
}

• Or we create a static Validators type with functions to do this for you:

predicate: Validators.predicateForValidValues( [value1, value2])

Maybe the second is a bit clunky, not sure which one I prefer.

[pixelzoom]

Validation is not always an equality comparison. E.g.:

predicate: value => ( typeof value === 'number' ) && ( value >= 0 )

predicate: value => value instanceof Quadratic || value === null

So validation needs to support general boolean expressions. I don't think the Validators approach suggested above is practical for that, and I prefer a function declaration.

[samreid]

With our new Enumeration library, we could use:

predicate: MyEnumeration.includes.bind(MyEnumeration)

[pixelzoom]

If we flatten Property validation into one predicate option, here are the things that we need to support. Some of these are currently supported by Property's validation options, some of these we've had to workaround by writing isValidValue functions.

• value => {{boolean expression}}
• instanceof one or more classes
• typeof one or more JS types
• Enumeration
• null
• combinations of some of the above?

[zepumph]

This was discussed in the meeting today. A large focus was trying to see how the phet-io needs would fit into a general case.

There seemed to be a consensus that moving to a pattern that only accepted functions as predicates seemed ok. This goes against what @pixelzoom recommended above, and instead looked at converting all of the above list into functions to be passed in as the predicate option (whereas before they were "converted" into predicate functions in Property.

Then the Validator could server to factor out much of this logic:

• instanceof --> Validator.instanceof( Node )
• typeof --> Validator.typeof ( 'number') or Validator.number() (and one function for each typeof call possible.
• Enumeration --> Validator.validValues( 'choiceA', 'choiceB' )
• null --> Validator.null() and perhaps also Validator.nullOr( Validator.number() )
• combinations of the above. --> Not sure how this would work!

While the predicate option would accept an arbitrary anonymous function, for PhET-iO support it would be important to use the Validator options so that they could send along metadata to the wrapper frame about what the function is doing.

A main con of this to me is the verbosity. Instead of having the option to just pass valueType: 'number' and be done with it, the predicate option would be more verbose. This is less important in Property than it is in Emitter, which can have arbitrary numbers of args, and something like:
valueTypes: ['number', 'string', Node]
turns into
valueTypes: [Validator.number(), Validator.string(), Validator.instanceof( Node )]

That doesn't look very appealing to me, especially because for simple cases like above (where we aren't testing complex opperations like "vale is null or less -4") we likely can get at the PhET-iO metadata from other places.

We decided to talk about it more next week.

[pixelzoom]

@zepumph said:

... This goes against what @pixelzoom recommended above ...

To clarify, my recommendation was based on the requirements that @zepumph established in the first comment of this issue:

The goal would be to support all types of valueType in the same parameter. ...

If you want to change the requirements and convert everything to predicate functions, I might have a different recommendation.

[pixelzoom]

See also #195, proper handling of valueType: Array.

[zepumph]

Marking as high priority so that we get to it next dev meeting. A fair bit of work for emitter and property is blocked by this issue.

[zepumph]

Again this was discussed again today.

Most of the conversation was taken up in just understanding the current limitations, as well as the variety of potential solutions that we could go with.

Right now we are basically trying to determine a general, and consistent way to conduct value validation. In Property we want to have a value validated in the same way as for each argument for Emitter, as well as testing values in TypeIOs.

At one point, there was a thought to make generic value validation only ever accept a function. So each arg in Emitter would need a corresponding function (predicate) that when given a potential argument value, the function returned whether that value was valid.

There was some consensus that it is really nice to be able to specify simple typeof strings and Type constructors as well as predicate functions. I.e basically what Property's valueType option supports right now.

At one point there was talk of having an option in Emitter to support the arg value validations, and then when instrumenting this Emitter, to convert those over to where the phetioType was created. @ariel-phet felt strongly that we shouldn't have to touch this code once to add phet validation, and again to convert it to phet-io, since it is basically just copying functionality over from one option to another place.

So for next week. I'm going to implement a solution that involves just using phetioType for validation to see how it goes. This means that even uninstrumented Emitters would use phetioType to validate the emitter args.

Note that @pixelzoom expressed concern in how we are conflating the phetioType with the accepted value validation, which seemed weird. We weren't able to finish that piece of the discussion before the dev meeting ended.

[samreid]

Pushed to next week.

[zepumph]

I'm sorry I didn't get to this again. I will work on it this afternoon and we will talk next week.

[zepumph]

Right! That is what we were questioning. I think right now I am leaning toward getting rid of the complexity (for example in Property's option extend call) because this performance hit is only realized with assertions enabled.

On the other hand. We know that @jonathanolson has implemented some tests/checks under assertSlow for this exact reason. I don't think we are talking about performance problems on that level, but anything being called upwards of 30,000 times a second like Property.set during fuzzing I think deserves some discussion.

[pixelzoom]

From Property.js -

      // By default, check the options once in the constructor, not on each subsequent value validation, to improve
      // performance in requirejs mode
      validateOptionsOnValidateValue: false

The default is not to validate?... We might as well not validate anything if this is the default.

[zepumph]

I thought this too! But notice that property is validating options in the constructor still. Then @samreid showed me the possibilities for generality with usages in wave interference. After looking there, how do you feel about the option?

[pixelzoom]

I thought this too! But notice that property is validating options in the constructor still.

Property's constructor is validating initialization only. That's better than nothing, but close to nothing, and certainly not an appropriate default when assertions are enabled.

Then @samreid showed me the possibilities for generality with usages in wave interference. After looking there, how do you feel about the option?

What am I looking at in Wave Interference? I see that Scene is using Validator for some validation that's not related to Property, and that's nice. But what justification does that provide for not validating Property after initialization?

[samreid]

The implementation of Property.set is:

      set: function( value ) {
        assert && Validator.validate( value, this.validatorOptions );
        if ( !this.equalsValue( value ) ) {
          this.setValueAndNotifyListeners( value );
        }
        return this;
      },

If assertions are enabled, the Property value is validated every time the value changes.

The thing that Property only validates once in the constructor are the validation options, such as {validValues: [1,2,3]]}. From the option documentation in Validator:

// By default, always check the provided options.  Can be turned off for cases where the same options are used
// repeatedly, such as in Property
validateOptionsOnValidateValue: true

Since the validation options do not change once a Property is constructed, they do not need to be validated more than once (certainly not 10,000 times per second).

[pixelzoom]

OK... validateOptionsOnValidateValue is confusing. Next question is, if validation options are indeed immutable, why is validateOptionsOnValidateValue needed at all? When would validation of validation options need to be done more than once?

[pixelzoom]

Couldn't Validation.pluckOptions validate what it "plucks"?

Also recommend to rename pluckOptions to pickOptions. Options are picked or omitted, not plucked.

[samreid]

OK... validateOptionsOnValidateValue is confusing.

I was worried it would be confusing. I considered omitting it altogether but wasn't ready to incur an up to 1% ?ea performance penalty without discussing it with the team, so I have instead attempted to document it thoroughly and I'm glad we are discussing it.

Next question is, if validation options are indeed immutable, why is validateOptionsOnValidateValue needed at all?

There are two types of use cases of Validator.validate(). Type A (exemplified in Property and will also be seen in Emitter) is called repeatedly with the same options but different values. Type B is called only once, like this example in Wave Interference's Scene.js:

// @public (read-only) {number}
this.numberOfSources = Validator.validate( config.numberOfSources, { validValues: [ 1, 2 ] } );

In both cases, it seems appropriate to validate the validation options themselves, such as making sure the user didn't pass a non-array like {validValues: 'hello'} . However, for Type A, we can optimize by only validating the values on the first pass and never again. We could alternatively solve this with a separate method on Validator's interface. Or we could think of a better name than validateOptionsOnValidateValue? Or... maybe Validator could "stamp" approved options so this would be handled automatically and not appear in the API? But that seems kind of magical and may lead to worse confusion?

Couldn't Validation.pluckOptions validate what it "plucks"?

Sure, but pluckOptions is never called for Type B so that doesn't solve the root of the problem.

Also recommend to rename pluckOptions to pickOptions. Options are picked or omitted, not plucked.

Thanks, I'll rename that.

[samreid]

Or... maybe Validator could "stamp" approved options so this would be handled automatically and not appear in the API?

Here's a patch that accomplishes that. Once a set of options has passed validation, it is marked as approved. Options marked as approved aren't checked again. However, this has the side effect that it mutates options (and hence kind of appears in the API anyways). Or if options are mutated and reused it could have an incorrect approval, etc. etc.

Index: js/Property.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Property.js	(revision 0df6e306ce59eac4851dd207a01095668801936f)
+++ js/Property.js	(date 1545882767000)
@@ -44,15 +44,12 @@
       // Use this to detect or prevent update cycles. Update cycles may be due to floating point error,
       // faulty logic, etc. This may be of particular interest for PhET-iO instrumentation, where such
       // cycles may pollute the data stream. See https://github.com/phetsims/axon/issues/179
-      reentrant: false
+        reentrant: false
+      },
 
       // See Validator.DEFAULT_OPTIONS for validation options.
-    }, Validator.DEFAULT_OPTIONS, {
-
-      // By default, check the options once in the constructor, not on each subsequent value validation, to improve
-      // performance in requirejs mode
-      validateOptionsOnValidateValue: false
-    }, options );
+      Validator.DEFAULT_OPTIONS,
+      options );
 
     // @private
     this.validatorOptions = Validator.pickOptions( options );
Index: js/Validator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Validator.js	(revision 0df6e306ce59eac4851dd207a01095668801936f)
+++ js/Validator.js	(date 1545882656000)
@@ -36,11 +36,7 @@
     // {function|null} function that validates the value. Single argument is the value, returns boolean. Unused if null.
     // Example:
     // isValidValue: function( value ) { return Util.isInteger( value ) && value >= 0; }
-    isValidValue: null,
-
-    // By default, always check the provided options.  Can be turned off for cases where the same options are used
-    // repeatedly, such as in Property
-    validateOptionsOnValidateValue: true
+    isValidValue: null
   };
 
   const DEFAULT_OPTIONS_KEYS = _.keys( DEFAULT_OPTIONS );
@@ -58,8 +54,7 @@
 
       if ( assert ) {
 
-        // validate options, but allow opting out for improved performance for repeat cases like Property
-        options.validateOptionsOnValidateValue && Validator.validateOptions( options );
+        Validator.validateOptions( options );
 
         // TODO: now that we have Validator, do we still want to overload valueType?  Or split it up into:
         // TODO: {valueTypeOf: 'boolean'} vs {valueInstanceOf: 'Vector2'}
@@ -91,7 +86,7 @@
      * @public
      */
     validateOptions: options => {
-      if ( assert ) {
+      if ( assert || !options.validatorApproved ) {
         const valueType = options.valueType;
         assert(
           typeof valueType === 'function' ||
@@ -122,6 +117,7 @@
           const remainingOptions = _.omit( options, 'validValues' );
           options.validValues.forEach( v => Validator.validate( v, remainingOptions ) );
         }
+        options.validatorApproved = true;
       }
       else {
         return true;

[samreid]

This issue overlaps somewhat with phetsims/wave-interference#275 (comment)

[samreid]

From today's dev meeting:

Conversation A: using Validator in Property, and in standalone and in Emitter
CM: Seems OK.
SR: MK and I will leverage that in Emitter.

CM: can NumberProperty use validator for

    // @private {function|null} value validation that is specific to NumberProperty, null if assertions are disabled
this.assertNumberPropertyValidateValue = assert && function( value ) {
  if ( options.numberType === 'Integer' ) {
    assert( value % 1 === 0, 'numberType was Integer but value was ' + value );
  }
  options.range && assert( value >= options.range.min && value <= options.range.max,
    'value is out of range, value=' + value + ', range=[' + options.range.min + ',' + options.range.max + ']' );
};

[zepumph]

Previous list of "next step"

Code review. @pixelzoom would be a natural fit, in part because this overlaps with phetsims/wave-interference#275. @zepumph would be good to review the overall usage in Property and how it can be used in Emitter, and double checking that the change in phetioInherit is good.

This change in phetioInherit looks good. Right now we aren't using isInstance at all, but may with changes in #204.

Note the TODO: now that we have Validator, do we still want to overload valueType? Or split it up into: {typeOf: 'boolean'} vs {instanceOf: 'Vector2'} (not sure about the best key names here).

I feel like it is pretty safe to differentiate these 2 based on checking type X === 'string'. If it isn't then test instanceof.

Leverage this code for Emitter

Out for review as part of #182

Can this approach replace our typical options pattern, so that everything about an option is consolidated? (We would need an alternative to _.extend).

see #202

Next steps for this issue:

• discuss "pickOptions" pattern, and if we should keep it.
• Implement Validator logic in NumberProperty

@samreid can you think of anything else to do here that won't be covered by follow-up issues?

[samreid]

I feel like it is pretty safe to differentiate these 2 based on checking type X === 'string'. If it isn't then test instanceof.

Sounds reasonable, I'll remove the corresponding TODO from the code.

Implement Validator logic in NumberProperty

I moved that to #205

discuss "pickOptions" pattern, and if we should keep it.

I'll create a new issue for that too.

@samreid can you think of anything else to do here that won't be covered by follow-up issues?

I'll create a focused issue regarding the questions about validateOptionsOnValidateValue. @zepumph can this issue be closed?

[pixelzoom]

@zepumph said:

I feel like it is pretty safe to differentiate these 2 based on checking type X === 'string'. If it isn't then test instanceof.

@samreid said:

Sounds reasonable, I'll remove the corresponding TODO from the code.

If I understand correctly, the proposal is to make valueType support both instanceof and typeof checks. For example: valueType: Vector2 and valueType: 'string'. If that's the case... Reminder that Array needs to be handled specially, see #195.

[samreid]

If I understand correctly, the proposal is to make valueType support both instanceof and typeof checks.

Not just a proposal, that is how Property validation has been since it was added in February 2018 in 866c63d

I agree those are unsuitable for Array, we would probably need to use isValidValue or build custom support for it.

[zepumph]

validateOptionsOnValidateValue

very good thanks!

Good to note that #195 will be worked on more, but this feels like things have wrapped up well.

I'm going to close, and new issues can be created from here. Thanks all!