redundant code in EnumerationIO?

[pixelzoom]

For https://github.com/phetsims/tasks/issues/1017.

Possible redundant code in EnumerationIO:

    EnumerationIOImpl.validator = ObjectIO.validator; // TODO: is this redundant?

[samreid]

@zepumph can you please take a look at this?

[zepumph]

I ended up just updating the validator to be much more specific. @samreid will you please review. I found that this was working in Projectile Motion (which has a few instrumented EnumerationProperty instances. I was a bit worried for a second because of how the cache works, and since IO types must have unique typenames with no overlap, but I think that since it is just based on the parameter (enumeration), then we are alright.

[pixelzoom]

Slack dev-public channel:

Chris Malley 4:26 PM I've done a pull-all.sh, build-a-molecule and masses-and-springs are both failing to start in requirejs mode with "Uncaught Error: Assertion failed: value is not a member of Enumeration [object Object]". Ring a bell with anyone?

Michael Kauzmann:house_with_garden: 4:53 PM In https://github.com/phetsims/phet-core/issues/79 I changed the validator to be more specific for Enumerations and it is currently out for review. The mistake I made was that it effects non phet-io brand too. I'll handle it!

[zepumph]

@samreid, when I changed the validator here, most cases only improved, with more specific validation. One issue that was exposed from https://github.com/phetsims/masses-and-springs/issues/354 is that if there are two instances of an Enumeration that map to the same type name, then validation will fail.

This is because of how we use a cache for EnumerationIO where the cache key is solely based on the keys of the Enumeration. First off note that this failure largely exposed a malpractice, as in general we don't want duplicated Enumeration instances with the same keys. Second I think that to guard against this in the future we should remove the cache in EnumerationIO. It is largely useless, and I don't feel like it is safe to leave it in there AND keep the enumeration valueType validator. Even though it is a bit more graceful, the error we got was very obfuscated and generally unhelpful. We could also fail loudly and assert out if there is ever more than one Enumeration that maps to the same typeName. The more I talk the more I feel like we should likely only support a single Enumeration instance per type name. What do you think?

[samreid]

@chrisklus and I discussed this problem and wanted to ask:

What if we change the EnumerationIO cache to be a new Map where the keys are Enumeration instances? That way we can support multiple Enumeration instances with the same keys, and still have caching work as expected. We feel it is too constraining to only support one Enumeration per key name set. For instance, a sim could define ON|OFF for a light switch, then perhaps one day in common code, we would define ON|OFF for another feature. We should be able to support that, and still have caching work.

If you agree this is a good pattern for caching in EnumerationIO, would it work to use this as a caching pattern in other types as well? For instance, could PropertyIO use new Map with parameterType key instead of a string map with parameterType.typeName keys?

[pixelzoom]

That way we can support multiple Enumeration instances with the same keys, and still have caching work as expected.

I didn't realize that caching currently had this limitation. That definitely needs to be changed, especially in light on rich enumerations.

[zepumph]

Yes, this should certainly be solved for EnumerationIO. The reason that it is buggy is because it is the only IOtype with parameter(s) that aren't IO types. Thus I think making the EnumerationIO cache a Map sounds correct. Because this is the only type like this (search regex: function \w+IO\() I think that we don't need to change any other types. @samreid does that sound reasonable to you?

[pixelzoom]

It sounds like keys must currently be unique across Enumerations, and I hit this in Normal Modes (see https://github.com/phetsims/normal-modes/issues/34). If that's indeed the case, then that defeats the whole point of Enumeration, and this should be addressed with high priority.

[zepumph]

In the above commit I added support for this with EnumerationIO and when it is used in PropertyIO. I did this by using Map, and was careful to see what Map methods were supported by IE.

It was easy and obvious to add this to EnumerationIO, but it took a bit to understand that the problem is compounded via parametric types using EnumerationIO as a parameter type. To support PropertyIO I added a new field on ObjectIO called cacheKey which can be used instead of the typeName. I only added this to support this with PropertyIO for now because I think it should be code reviewed before propagating this pattern to all usages of const cache = {}. I also want to add that EnumerationIO is the only type we have who's parameter is not a phetioType. This is part of the added complexity, and in general I think it will be rare to run into this with other types. Still we will need to fix this for all parametric types before they support having EnumerationIO as a parameter type.

I also added a test that exposes this problem. Basically the last line of code will fail without my new changes because the second EnumerationProperty is getting its IO type from the cache with the value of the first IO type.

@samreid let me know if you want to discuss this further. Please review.

[samreid]

I think I understand the proposed pattern, but I'm having trouble visualizing how this will be applied throughout a type "tree". For instance, let's say I have PropertyIO(FunctionIO(EnumerationIO)). Or FunctionIO(EnumerationIO,EnumerationIO). How will these be cached? Maybe we could decide that "rare" cases don't need to be cached at all?

Also, I think this is your vision, but a note to myself for as we continue discussion. The code like this in PropertyIO:

const cacheKey = parameterType.cacheKey || parameterType.typeName;

Seems like it will ultimately become

const cacheKey = parameterType.cacheKey;

where some of the cacheKey values are just the typeName (for types that don't recursively involve EnumerationIO). But I'm a bit fuzzy on this and would like to discuss further.

I also added one // REVIEW comment I noticed during this, but I encourage you to move it to a new issue at your discretion.

[zepumph]

where some of the cacheKey values are just the typeName (for types that don't recursively involve EnumerationIO). But I'm a bit fuzzy on this and would like to discuss further.

This isn't quite the same as what I have in my mind currently. Can't this line just become:

const cacheKey = parameterType;

Then we wouldn't even need to have a public cacheKey. I thought that was the value in making things a map in the first place. I'm not sure what I was thinking adding cacheKey to be used by Property. Let's talk more!

[samreid]

I'll need further discussion on the top paragraph in https://github.com/phetsims/phet-core/issues/79#issuecomment-599070818 or collaboration before I can proceed on this issue.

[zepumph]

Today the phet-io team found a bug. We don't need to support cacheKey, and instead should just use the parameter type as the key itself. PropertyIO should just use its parameterType as the cache key. Then we should delete cacheKey from EnumerationIO.

[zepumph]

• [ ] Once this issue is complete, we can continue with https://github.com/phetsims/tandem/issues/169.

[zepumph]

@samreid this issue is quite long, and ever changing, allow me to summarize and give you specifics on what to review:

Issue Focus This issue has turned into discussion about using Map for caching. It is a bit beyond the original scope, and as a result here we should just discuss the changes thus far made to PropertyIO and EnuemrationIO and then move over to https://github.com/phetsims/tandem/issues/169 when we are ready to apply the pattern to all parametric types.

The summary of changes for EnumerationIO and PropertyIO are converting the cache from an object literal with string keys to a Map with parameter IO Types as keys. This is very simply done, and can be confirmed to work in axon tests, especially in EnumerationPropertyTests entitled EnumerationIO validation. This is a good test because in that we use the phetioType as validation, and create two Enumeration instances with the same value names. Here we test and make sure that it does not just validate based on the name of the enumeration value, but the actual Enumeration instance itself.

Please review that strategy.

I think I understand the proposed pattern, but I'm having trouble visualizing how this will be applied throughout a type "tree". For instance, let's say I have PropertyIO(FunctionIO(EnumerationIO)). Or FunctionIO(EnumerationIO,EnumerationIO). How will these be cached? Maybe we could decide that "rare" cases don't need to be cached at all?

Currently this is buggy because we haven't applied the pattern everywhere. Here is what I think to be another way of stating the same proof of this thought as does the aforementioned EnumerationProperty test (from the console of a standalone phet-io sim):

phet.axon.PropertyIO( phet.tandem.BooleanIO ) === phet.axon.PropertyIO( phet.tandem.BooleanIO )
->true
const Birds1 = phet.phetCore.Enumeration.byKeys( [ 'ROBIN', 'JAY', 'WREN' ] );
const Birds2 = phet.phetCore.Enumeration.byKeys( [ 'ROBIN', 'JAY', 'WREN' ] );
->undefined

phet.axon.PropertyIO( phet.phetCore.EnumerationIO( Birds1 ) ) === phet.axon.PropertyIO( phet.phetCore.EnumerationIO( Birds1 ) )
->true
phet.axon.PropertyIO( phet.phetCore.EnumerationIO( Birds1 ) ) === phet.axon.PropertyIO( phet.phetCore.EnumerationIO( Birds2 ) )
->false

Thus using actual IO Types as cache keys for Map will solve the "chain" of parametric IO Types you mentioned above.

@samreid is there anything else here?

[samreid]

This looks great, thanks! I'll unblock https://github.com/phetsims/tandem/issues/169 and close this issue.