Merge pull request #93 from bloomberg/ac/disallow-symbol-keys

Can no longer create records with symbol keys
bloomberg · Jul 18, 2022 · bea8da6 · bea8da6
2 parents 8a334b1 + 12d42de
commit bea8da6
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 21 deletions.
diff --git a/packages/record-tuple-polyfill/src/record.js b/packages/record-tuple-polyfill/src/record.js
@@ -18,6 +18,7 @@ import { InternGraph } from "./interngraph";
 import {
     isObject,
     fakeRecordFromEntries,
+    validateKey,
     validateProperty,
     isRecord,
     markRecord,
@@ -46,14 +47,17 @@ function createRecordFromObject(value) {
     // sort all property names by the order specified by
     // the argument's OwnPropertyKeys internal slot
     // EnumerableOwnPropertyNames - 7.3.22
-    const properties = Object.entries(value)
+    const properties = Reflect.ownKeys(value)
+        .flatMap(k => {
+            const desc = Object.getOwnPropertyDescriptor(value, k);
+            if (!desc.enumerable) return [];
+            return [[validateKey(k), validateProperty(value[k])]];
+        })
         .sort(function([a], [b]) {
             if (a < b) return -1;
             else if (a > b) return 1;
             return 0;
-        })
-        .map(([name, value]) => [name, validateProperty(value)]);
-
+        });
     return RECORD_GRAPH.get(properties);
 }
 

diff --git a/packages/record-tuple-polyfill/src/record.test.js b/packages/record-tuple-polyfill/src/record.test.js
@@ -34,10 +34,49 @@ test("records cannot contain objects", () => {
 });
 
 test("records doesn't unbox boxed primitives", () => {
-    expect(() => Record({ a: Object(true) })).toThrow(Error); // TODO: TypeError
-    expect(() => Record({ a: Object(1) })).toThrow(Error); // TODO: TypeError
-    expect(() => Record({ a: Object("test") })).toThrow(Error); // TODO: TypeError
-    expect(() => Record({ a: Object(Symbol()) })).toThrow(Error); // TODO: TypeError
+    expect(() => Record({ a: Object(true) })).toThrow(TypeError);
+    expect(() => Record({ a: Object(1) })).toThrow(TypeError);
+    expect(() => Record({ a: Object("test") })).toThrow(TypeError);
+    expect(() => Record({ a: Object(Symbol()) })).toThrow(TypeError);
+});
+
+test("records can't have symbol keys", () => {
+    expect(() => Record({ [Symbol()]: true })).toThrow(TypeError);
+});
+
+test("Record from object only uses enumerable properties", () => {
+    const template = Object.create(null, {
+        [Symbol("non-enumerable-symbol-prop")]: {
+            enumerable: false,
+            value: true,
+        },
+        ["non-enumerable-string-prop"]: {
+            enumerable: false,
+            value: true,
+        },
+        ["non-enumerable-string-getter"]: {
+            enumerable: false,
+            get() {
+                return true;
+            },
+        },
+        ["enumerable-string-prop"]: {
+            enumerable: true,
+            value: true,
+        },
+        ["enumerable-string-getter"]: {
+            enumerable: true,
+            get() {
+                return true;
+            },
+        },
+    });
+    expect(Record(template)).toBe(
+        Record({
+            "enumerable-string-prop": true,
+            "enumerable-string-getter": true,
+        }),
+    );
 });
 
 test("records are correctly identified as records", () => {
@@ -124,12 +163,67 @@ test("Record.fromEntries", () => {
             ["b", {}],
             ["a", 1],
         ]),
-    ).toThrow();
+    ).toThrow(TypeError);
 
     let sym = Symbol();
-    expect(() => Record.fromEntries([[sym, 1]])).toThrow();
+    expect(() => Record.fromEntries([[sym, 1]])).toThrow(TypeError);
     expect(Record.fromEntries([["foo", sym]])).toBe(Record({ foo: sym }));
 });
+
+test("Record.fromEntries validate entries in order", () => {
+    let iteratorCount = 0;
+
+    function countIterable(values) {
+        return {
+            [Symbol.iterator]() {
+                const it = values[Symbol.iterator]();
+                return {
+                    next() {
+                        iteratorCount++;
+                        return it.next();
+                    },
+                };
+            },
+        };
+    }
+
+    // Invalid value: (isPrimitive(value) === false):
+    expect(() => {
+        Record.fromEntries(
+            countIterable([
+                ["valid-key-1", {}],
+                ["valid-key-2", "valid-value"],
+            ]),
+        );
+    }).toThrow(TypeError);
+    expect(iteratorCount).toBe(1);
+    iteratorCount = 0;
+
+    // Invalid symbol key: (typeof key === 'symbol')
+    expect(() => {
+        Record.fromEntries(
+            countIterable([
+                [Symbol("invalid-key"), "valid-value"],
+                ["valid-key-2", "valid-value"],
+            ]),
+        );
+    }).toThrow(TypeError);
+    expect(iteratorCount).toBe(1);
+    iteratorCount = 0;
+
+    // Invalid object key: (toString(key) throws)
+    expect(() => {
+        Record.fromEntries(
+            countIterable([
+                [Object(Symbol("invalid-key")), "valid-value"],
+                ["valid-key-2", "valid-value"],
+            ]),
+        );
+    }).toThrow(TypeError);
+    expect(iteratorCount).toBe(1);
+    iteratorCount = 0;
+});
+
 test("Records work with Object.values", () => {
     expect(Object.values(Record({ a: 1, b: 2 }))).toEqual([1, 2]);
     expect(Object.values(Record({ b: 1, a: 2 }))).toEqual([2, 1]);

diff --git a/packages/record-tuple-polyfill/src/utils.js b/packages/record-tuple-polyfill/src/utils.js
@@ -28,15 +28,11 @@ export function isIterableObject(v) {
 }
 
 export function fakeRecordFromEntries(iterable) {
-    return [...iterable].reduce((obj, [key, val]) => {
-        if (typeof key === "symbol") {
-            throw new TypeError(
-                "A Symbol cannot be used as a property key in a Record.",
-            );
-        }
-        obj[key] = val;
-        return obj;
-    }, {});
+    const retVal = Object.create(null);
+    for (const [key, value] of iterable) {
+        retVal[validateKey(key)] = validateProperty(value);
+    }
+    return retVal;
 }
 
 const RECORD_WEAKSET = new WeakSet();
@@ -57,13 +53,23 @@ export function markTuple(value) {
 function isRecordOrTuple(value) {
     return isRecord(value) || isTuple(value);
 }
+
+export function validateKey(key) {
+    if (typeof key === "symbol") {
+        throw new TypeError(
+            "A Symbol cannot be used as a property key in a Record.",
+        );
+    }
+    return String(key);
+}
+
 export function validateProperty(value) {
     if (isObject(value) && !isRecordOrTuple(value)) {
-        throw new Error(
+        throw new TypeError(
             "TypeError: cannot use an object as a value in a record",
         );
     } else if (isFunction(value)) {
-        throw new Error(
+        throw new TypeError(
             "TypeError: cannot use a function as a value in a record",
         );
     }