phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Assert if EnumerationIO instances shadow each other #99

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

Related to work from https://github.com/phetsims/chipper/issues/1106.

zepumph commented 2 years ago

@samreid, I made this patch, and ran fuzzing on all phet-io sims. I'm a bit nervous about this, but perhaps it would be best to know about this eagerly.

I found one error in Density/Buoyancy and one in quad, and will want to investigate. Do you have any thoughts about this assertion?

Index: js/types/EnumerationIO.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/types/EnumerationIO.ts b/js/types/EnumerationIO.ts
--- a/js/types/EnumerationIO.ts (revision bfaf86072beb6efca0316da986c7fbc4b486a6f9)
+++ b/js/types/EnumerationIO.ts (date 1642699160138)
@@ -14,11 +14,17 @@
 // Cache each parameterized IOType so that it is only created once.
 const cache = new Map<IEnumeration<any>, IOType>();

+let typeNameSet: Set<string> | null = null;
+
 const joinKeys = ( keys: string[] ) => keys.join( '|' );

 const EnumerationIO = <T extends EnumerationValue>( enumerationContainer: EnumerationContainer<T> ): IOType => {
   const enumeration = enumerationContainer.enumeration;

+  if ( assert ) {
+    typeNameSet = typeNameSet || new Set<string>();
+  }
+
   // This caching implementation should be kept in sync with the other parametric IO Type caching implementations.
   if ( !cache.has( enumeration ) ) {

@@ -28,7 +34,11 @@
     const keys = enumeration.keys;
     const values = enumeration.values;

-    cache.set( enumeration, new IOType( `EnumerationIO(${joinKeys( keys )})`, {
+    const ioTypeName = `EnumerationIO(${joinKeys( keys )})`;
+
+    assert && assert( !typeNameSet!.has( ioTypeName ), `Typename has already been registered in the cache: ${ioTypeName}` );
+
+    cache.set( enumeration, new IOType( ioTypeName, {
       validValues: values,
       documentation: `Possible values: ${keys.join( ', ' )}.${additionalDocs}`,
       toStateObject: ( t: T ) => enumeration.getKey( t ),
@@ -41,6 +51,8 @@
         isValidValue: ( key: string ) => keys.includes( key )
       } )
     } ) );
+
+    typeNameSet!.add( ioTypeName );
   }

   return cache.get( enumeration )!;
samreid commented 2 years ago

Testing with this patch (which is O(N^2) but N is small):

```diff Index: js/types/EnumerationIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/EnumerationIO.ts b/js/types/EnumerationIO.ts --- a/js/types/EnumerationIO.ts (revision df318810426b957b7246594e626e2f2f6902cc59) +++ b/js/types/EnumerationIO.ts (date 1662177205914) @@ -28,7 +28,14 @@ const keys = enumeration.keys; const values = enumeration.values; - cache.set( enumeration, new IOType( `EnumerationIO(${joinKeys( keys )})`, { + const ioTypeName = `EnumerationIO(${joinKeys( keys )})`; + + assert && assert( + !Array.from( cache.values() ).find( ioType => ioType.typeName === ioTypeName ), + 'There was already another IO Type with the same name: ' + ioTypeName + ); + + cache.set( enumeration, new IOType( ioTypeName, { validValues: values, documentation: `Possible values: ${keys.join( ', ' )}.${additionalDocs}`, toStateObject: ( t: T ) => enumeration.getKey( t ), ```

I see the following problems in aqua (same 3 repos as reported above):

buoyancy
Uncaught Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(STYROFOAM|WOOD|ICE|BRICK|ALUMINUM|CUSTOM)
Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(STYROFOAM|WOOD|ICE|BRICK|ALUMINUM|CUSTOM)
    at window.assertions.assertFunction (http://localhost/main/assert/js/assert.js:28:13)
    at assert (EnumerationIO.ts:33:14)
    at EnumerationIO (MaterialMassVolumeControlNode.ts:114:23)
    at  (BlockControlNode.ts:20:4)
    at  (PrimarySecondaryControlsNode.ts:40:33)
    at  (BuoyancyExploreScreenView.ts:145:20)
    at  (ExploreScreen.ts:25:15)
    at createView (Screen.ts:301:22)
    at initializeView (Sim.ts:862:15)
    at  (Sim.ts:870:10)

density
Uncaught Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(STYROFOAM|WOOD|ICE|BRICK|ALUMINUM|CUSTOM)
Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(STYROFOAM|WOOD|ICE|BRICK|ALUMINUM|CUSTOM)
    at window.assertions.assertFunction (http://localhost/main/assert/js/assert.js:28:13)
    at assert (EnumerationIO.ts:33:14)
    at EnumerationIO (MaterialMassVolumeControlNode.ts:114:23)
    at  (BlockControlNode.ts:20:4)
    at  (PrimarySecondaryControlsNode.ts:40:33)
    at  (DensityIntroScreenView.ts:38:20)
    at  (IntroScreen.ts:30:15)
    at createView (Screen.ts:301:22)
    at initializeView (Sim.ts:862:15)
    at  (Sim.ts:870:10)

quadrilateral
Uncaught Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(ONE|TWO|THREE|FOUR)
Error: Assertion failed: There was already another IO Type with the same name: EnumerationIO(ONE|TWO|THREE|FOUR)
    at window.assertions.assertFunction (http://localhost/main/assert/js/assert.js:28:13)
    at assert (EnumerationIO.ts:33:14)
    at EnumerationIO (EnumerationProperty.ts:35:23)
    at  (QuadrilateralSoundOptionsModel.ts:143:36)
    at  (QuadrilateralPreferencesModel.ts:24:38)
    at  (QuadrilateralPreferencesModel.ts:21:0)
    at  (quadrilateral-main.ts:25:25)
samreid commented 2 years ago

I applied workarounds for the sim-specific cases and committed the new assertion. @zepumph want to review? Please close if all is well.

zepumph commented 2 years ago

Excellent! Thanks