[runtime] Update makeReadOnly runtime

--- 

(wip, waiting for feedback on workplace post) 

- remove calls to `isInROMode`, as we want to log all mutations after 'freezing' 
a value (both within and outside of render cycle) 

- add `source` parameter -- this is the function name of the parent component / 
hook

compiler/forget/packages/make-read-only-util/src/makeReadOnly.test.ts

@@ -9,11 +9,11 @@ import buildMakeReadOnly from "./makeReadOnly";
 
 describe("makeReadOnly", () => {
   let logger: jest.Func;
-  let makeReadOnly: <T>(value: T) => T;
+  let makeReadOnly: <T>(value: T, source: string) => T;
 
   beforeEach(() => {
     logger = jest.fn();
-    makeReadOnly = buildMakeReadOnly(logger, [], () => true);
+    makeReadOnly = buildMakeReadOnly(logger, []);
   });
 
   describe("Tracking mutations", () => {
@@ -21,45 +21,50 @@ describe("makeReadOnly", () => {
       const a = 5;
       const b = true;
       const c = null;
-      expect(makeReadOnly(a)).toBe(a);
-      expect(makeReadOnly(b)).toBe(b);
-      expect(makeReadOnly(c)).toBe(c);
+      expect(makeReadOnly(a, "test1")).toBe(a);
+      expect(makeReadOnly(b, "test1")).toBe(b);
+      expect(makeReadOnly(c, "test1")).toBe(c);
     });
 
     it("retains referential equality", () => {
       const valA = {};
       const valB = { a: valA, _: valA };
       const o = { a: valA, b: valB, c: "c" };
-      expect(makeReadOnly(o)).toBe(o);
-      expect(makeReadOnly(o.a)).toBe(valA);
-      expect(makeReadOnly(o.b)).toBe(valB);
-      expect(makeReadOnly(o.b.a)).toBe(valA);
-      expect(makeReadOnly(o.b._)).toBe(valA);
-      expect(makeReadOnly(o.c)).toBe("c");
+      expect(makeReadOnly(o, "test2")).toBe(o);
+      expect(makeReadOnly(o.a, "test2")).toBe(valA);
+      expect(makeReadOnly(o.b, "test2")).toBe(valB);
+      expect(makeReadOnly(o.b.a, "test2")).toBe(valA);
+      expect(makeReadOnly(o.b._, "test2")).toBe(valA);
+      expect(makeReadOnly(o.c, "test2")).toBe("c");
     });
 
     it("deals with cyclic references", () => {
       const o: any = {};
       o.self_ref = o;
-      expect(makeReadOnly(o)).toBe(o);
-      expect(makeReadOnly(o.self_ref)).toBe(o);
+      expect(makeReadOnly(o, "test3")).toBe(o);
+      expect(makeReadOnly(o.self_ref, "test3")).toBe(o);
     });
     it("logs direct interior mutability", () => {
       const o = { a: 0 };
-      makeReadOnly(o);
+      makeReadOnly(o, "test4");
       o.a = 42;
-      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", 42);
+      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test4", "a", 42);
     });
 
     it("tracks changes to known RO properties", () => {
       const o: any = { a: {} };
-      makeReadOnly(o);
+      makeReadOnly(o, "test5");
       o.a = 42;
-      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", 42);
+      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test5", "a", 42);
       expect(o.a).toBe(42);
       const newVal = { x: 0 };
       o.a = newVal;
-      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", newVal);
+      expect(logger).toBeCalledWith(
+        "FORGET_MUTATE_IMMUT",
+        "test5",
+        "a",
+        newVal
+      );
       expect(o.a).toBe(newVal);
     });
 
@@ -67,18 +72,23 @@ describe("makeReadOnly", () => {
       const o: any = { a: { x: 4 } };
 
       const alias = o;
-      makeReadOnly(o);
+      makeReadOnly(o, "test6");
       const newVal = {};
       alias.a = newVal;
-      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", newVal);
+      expect(logger).toBeCalledWith(
+        "FORGET_MUTATE_IMMUT",
+        "test6",
+        "a",
+        newVal
+      );
       expect(o.a).toBe(newVal);
     });
 
     it("logs transitive interior mutability", () => {
       const o: any = { a: { x: 0 } };
-      makeReadOnly(o);
+      makeReadOnly(o, "test7");
       o.a.x = 42;
-      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "x", 42);
+      expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test7", "x", 42);
     });
 
     describe("todo", () => {
@@ -86,7 +96,7 @@ describe("makeReadOnly", () => {
         // this is a limitation of the current "proxy" approach,
         // which overwrites object properties with getters and setters
         const x: any = { a: {} };
-        makeReadOnly(x);
+        makeReadOnly(x, "test8");
 
         delete x.a;
         x.b = 0;
@@ -96,44 +106,61 @@ describe("makeReadOnly", () => {
         // this could be easily implemented by making caching eager
         const innerObj = { x: 0 };
         const o = { a: innerObj };
-        makeReadOnly(o);
+        makeReadOnly(o, "test9");
         innerObj.x = 42;
         expect(o.a.x).toBe(42);
 
         const o1 = { a: { x: 0 } };
         const innerObj1 = o1.a;
-        makeReadOnly(o1);
+        makeReadOnly(o1, "test9");
         innerObj1.x = 42;
 
         expect(o1.a.x).toBe(42);
         expect(logger).toBeCalledTimes(0);
       });
+
+      it("does not track objects with getter/setters", () => {
+        let backedX: string | null = null;
+        const o = {
+          set val(val: string | null) {
+            backedX = val;
+          },
+          get val(): string | null {
+            return backedX;
+          },
+        };
+        expect(makeReadOnly(o, "test10")).toBe(o);
+        expect(makeReadOnly(o.val, "test10")).toBe(null);
+
+        o.val = "40";
+        expect(logger).toBeCalledTimes(0);
+      });
     });
   });
 
   describe("Tracking adding or deleting properties", () => {
     it("tracks new properties added between calls to makeReadOnly", () => {
       const o: any = {};
-      makeReadOnly(o);
+      makeReadOnly(o, "test11");
       o.a = "new value";
-      makeReadOnly(o);
-      expect(logger).toBeCalledWith("FORGET_ADD_PROP_IMMUT", "a");
+      makeReadOnly(o, "test11");
+      expect(logger).toBeCalledWith("FORGET_ADD_PROP_IMMUT", "test11", "a");
     });
     it("tracks properties deleted between calls to makeReadOnly", () => {
       const o: any = { a: 0 };
-      makeReadOnly(o);
+      makeReadOnly(o, "test12");
       delete o.a;
-      makeReadOnly(o);
-      expect(logger).toBeCalledWith("FORGET_DELETE_PROP_IMMUT", "a");
+      makeReadOnly(o, "test12");
+      expect(logger).toBeCalledWith("FORGET_DELETE_PROP_IMMUT", "test12", "a");
     });
 
-    it("tracks properties deleted and re-added between calls to makeReadOnly", () => {
-      const o: any = { a: 0 };
-      makeReadOnly(o);
-      delete o.a;
-      o.a = {};
-      makeReadOnly(o);
-      expect(logger).toBeCalledWith("FORGET_CHANGE_PROP_IMMUT", "a");
-    });
+    // it("tracks properties deleted and re-added between calls to makeReadOnly", () => {
+    //   const o: any = { a: 0 };
+    //   makeReadOnly(o);
+    //   delete o.a;
+    //   o.a = {};
+    //   makeReadOnly(o);
+    //   expect(logger).toBeCalledWith("FORGET_CHANGE_PROP_IMMUT", "a");
+    // });
   });
 });

compiler/forget/packages/make-read-only-util/src/makeReadOnly.ts

@@ -12,9 +12,9 @@ type ROViolationType =
   | "FORGET_DELETE_PROP_IMMUT"
   | "FORGET_CHANGE_PROP_IMMUT"
   | "FORGET_ADD_PROP_IMMUT";
-type ROModeChecker = () => boolean;
 type ROViolationLogger = (
   violation: ROViolationType,
+  source: string,
   key: string,
   value?: any
 ) => void;
@@ -52,15 +52,15 @@ function getOrInsertDefault(
 
 function buildMakeReadOnly(
   logger: ROViolationLogger,
-  skippedClasses: string[],
-  isInROMode: ROModeChecker
-): <T>(val: T) => T {
+  skippedClasses: string[]
+): <T>(val: T, source: string) => T {
   // All saved proxys
   const savedROObjects: SavedROObjects = new WeakMap();
 
   // Overwrites an object property with its proxy and saves its original value
   function addProperty(
     obj: Object,
+    source: string,
     key: string,
     prop: PropertyDescriptor,
     savedEntries: Map<string, SavedEntry>
@@ -68,12 +68,10 @@ function buildMakeReadOnly(
     const proxy: PropertyDescriptor & { get(): unknown } = {
       get() {
         // read from backing cache entry
-        return makeReadOnly(savedEntries.get(key)!.savedVal);
+        return makeReadOnly(savedEntries.get(key)!.savedVal, source);
       },
       set(newVal: unknown) {
-        if (isInROMode()) {
-          logger("FORGET_MUTATE_IMMUT", key, newVal);
-        }
+        logger("FORGET_MUTATE_IMMUT", source, key, newVal);
         // update backing cache entry
         savedEntries.get(key)!.savedVal = newVal;
       },
@@ -90,10 +88,13 @@ function buildMakeReadOnly(
   }
 
   // Changes an object to be read-only, returns its input
-  function makeReadOnly<T>(o: T): T {
+  function makeReadOnly<T>(o: T, source: string): T {
     if (typeof o !== "object" || o == null) {
       return o;
-    } else if (skippedClasses.includes(o.constructor.name)) {
+    } else if (
+      o.constructor?.name != null &&
+      skippedClasses.includes(o.constructor.name)
+    ) {
       return o;
     }
 
@@ -114,13 +115,11 @@ function buildMakeReadOnly(
         //    (meaning that new value is not proxied,
         //     and the current proxied value is stale)
         cache.delete(k);
-        if (!currentProp && isInROMode()) {
-          logger("FORGET_DELETE_PROP_IMMUT", k);
+        if (!currentProp) {
+          logger("FORGET_DELETE_PROP_IMMUT", source, k);
         } else if (currentProp) {
-          if (isInROMode()) {
-            logger("FORGET_CHANGE_PROP_IMMUT", k);
-          }
-          addProperty(o, k, currentProp, cache);
+          logger("FORGET_CHANGE_PROP_IMMUT", source, k);
+          addProperty(o, source, k, currentProp, cache);
         }
       }
     }
@@ -128,10 +127,17 @@ function buildMakeReadOnly(
       Object.getOwnPropertyDescriptors(o)
     )) {
       if (!cache.has(k) && isWriteable(prop)) {
-        if (isInROMode() && existed) {
-          logger("FORGET_ADD_PROP_IMMUT", k);
+        if (prop.hasOwnProperty("set") || prop.hasOwnProperty("get") || k === "current") {
+          // - we currently don't handle accessor properties
+          // - we currently have no other way of checking whether an object
+          // is a `ref` (i.e. returned by useRef).
+          continue;
         }
-        addProperty(o, k, prop, cache);
+
+        if (existed) {
+          logger("FORGET_ADD_PROP_IMMUT", source, k);
+        }
+        addProperty(o, source, k, prop, cache);
       }
     }
     return o;