optionize<ProvidedOptions, SelfOptions> is buggy

[pixelzoom]

The 2-parameter form of optionize seems to have a serious problem. It's not detecting when defaults are provided for required options. For example, in gas-properties CollisionCounter.ts:

type SelfOptions = {
  position: Vector2;
  visible: boolean;
};

type CollisionCounterOptions = SelfOptions & PickRequired<PhetioObjectOptions, 'tandem'>;

export default class CollisionCounter {

public constructor( collisionDetector: CollisionDetector, providedOptions: CollisionCounterOptions ) {

    const options = optionize<CollisionCounterOptions, SelfOptions>()( {

      // SelfOptions
      position: Vector2.ZERO,
      visible: false
    }, providedOptions );

In this example, the fields in SelfOptions have accidentally been specified as required, and I've provided default values in the first arg to optionize. With 3-parameters to optionize, the default values provided would be flagged as an error. With 2-parameters to optionize, this error goes unreported.

I've been running into this case quite frequently when converting sims to TypeScript, so I'll start with high-priority.

[zepumph]

At first glance I see that this feature isn't inherently broken, as I can trigger an error with this patch in our wilder examples:

Index: js/wilder/model/WilderOptionsPatterns.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/wilder/model/WilderOptionsPatterns.ts b/js/wilder/model/WilderOptionsPatterns.ts
--- a/js/wilder/model/WilderOptionsPatterns.ts	(revision 4950bcb35d51fbafd86fa63b614955d295b23e2b)
+++ b/js/wilder/model/WilderOptionsPatterns.ts	(date 1669410560197)
@@ -159,7 +159,9 @@
   private treeType: TreeItemSelfOptions[ 'treeType' ];
 
   public constructor( providedOptions: TreeItemOptions ) {
-    const options = optionize<TreeItemOptions, TreeItemSelfOptions, ItemOptions>()( {}, providedOptions );
+    const options = optionize<TreeItemOptions, TreeItemSelfOptions, ItemOptions>()( {
+      treeType: 'pipe'
+    }, providedOptions );
     super( options );
     this.treeType = options.treeType;
   }

It looks like in your case, specifying the parent options Type solves things. That said, looking into optionize and the implementation of HalfOptions, parent options defaults to object, so Partial<object> is likely blowing away much of the good logic that is in the first line of HalfOptions. I could see a few options for a better default of parentOptions, but I'd most like to have something that doesn't leak elsewhere. I tried something like "all provided options that aren't in the self options", but that attempt to be clever and simplify the call sites actually was just more confusing and caused some weird type errors if a key is changed between SelfOptions and ProvidedOptions. So I landed with an object with no keys in it. I'm a bit surprised it passed the type check, and even more surprised that it helped me find a few spots in code that we were actually doing things wrong. @pixelzoom thanks for the bug report.

@samreid can you please review the commits.

@samreid what I am finding here is that this was a regression, and it could have been solved if there was an assert.throws style unit test for type checking. Is it possible to formulate code that is expected to have a type error in it, and test that it does? I am not aware of anything, but I think it could greatly assist in helping us move WilderOptionsPatterns in to some sort of unit testable, regression proof system.

Thanks all!

[samreid]

I like the idea of using Record<never,never>, but I am still able to trigger a case where type checking passes but it shouldn't in this patch:

Subject: [PATCH] Update docs
---
Index: js/common/model/CollisionCounter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CollisionCounter.ts b/js/common/model/CollisionCounter.ts
--- a/js/common/model/CollisionCounter.ts	(revision 9dfeb6817d0073efb0862c6420160598ba9cf9a4)
+++ b/js/common/model/CollisionCounter.ts	(date 1669730414030)
@@ -18,8 +18,8 @@
 import CollisionDetector from './CollisionDetector.js';
 
 type SelfOptions = {
-  position?: Vector2;
-  visible?: boolean;
+  position: Vector2;
+  visible: boolean;
 };
 
 type CollisionCounterOptions = SelfOptions & PickRequired<PhetioObjectOptions, 'tandem'>;

Note that making one of those options required catches the type error, but making both required fails to catch the type error.

I'll open an issue in chipper about adding a "this should fail type checking" harness.

[zepumph]

I was finally able to crack it! We are now getting type errors when you add required options in the defaults. This helps to catch duplications that should be handled in other ways. This was not trivial, and there are still multiple TODOs here where I require other people's help. But in general we have fixed one of the primary challenges with optionize. Every spot that I found a type error was buggy, and I was able to fix it up!

I just want to say how happy I am about the changes here.

There are 15 TODOs marked with this issue https://github.com/phetsims/chipper/issues/1360. I marked who I need help from for each:

• TODO SR (7) @samreid
• TODO CM (7) @pixelzoom
• TODO JO (1) @jonathanolson

As for the actual commit is phetsims/phet-core@d02125b. I think that I would like to discuss it with @samreid. Questions for us to discuss (sync or async):

• Please note optionize3 and optionize4 do not use the ProvidedOptions arg to OptionizeDefaults for their provided defaults. This was purposeful because I felt like it would get a bit messy when not using a direct object literal. Let's discuss more though!
• Is there ever a place where we would want an outside usage to provide the ProvidedOptions to OptionizeDefaults? I couldn't think of one.
• I want to tag Required options are optional in optionize phet-core#130, I don't have time to ramp up on it again right now, but I feel like the work there was similar to this issue.

Still running git hooks, but I will commit soon.

[zepumph]

Excellent!

RE: #1360 (comment)

@samreid I updated to use phet-common types, and I'm much happier.

From here, all that is left are the TODOs that I need @pixelzoom's help with.

[pixelzoom]

... From here, all that is left are the TODOs that I need @pixelzoom's help with.

In the above commits, I addressed all of the TODOs that had my initials "CM" on them.

There's one TODO remaining for this issue, in PatternStringProperty.ts:

    // @ts-expect-error TODO This should be fixed as part of https://github.com/phetsims/chipper/issues/1360
    const options = optionize<PatternStringPropertyOptions<Values>, OptionalSelfOptions<Values>, SuperOptions>()( {

I spent ~10 minutes on it and got nowhere. You might need to consult with @jonathanolson.

[jonathanolson]

For the PatternStringProperty case, optionize probably needs to handle more complicated objects.

Effectively SelfOptions is Stuff & ( Condition ? { maps?: Type } : { maps: Type } ), where it is either optional or required based on a condition. It doesn't seem like it's possible to provide that into optionize at the moment (it complains about keys missing that are unrelated), so OptionalSelfOptions is passed in instead currently. How can I pass that type of SelfOptions in to optionize?

[samreid]

I added a commit that changes the type parameters to the optionize call in PatternStringProperty. It passes type checking. I don't know of a good long-term fix. @jonathanolson does that seem reasonable to you?

[jonathanolson]

That seems reasonable, if a bit unfortunate (I understand why). Thanks!

[samreid]

@zepumph I don't see any more TODOs referencing this issue, can it be closed?