1565001 - Remove uneval usage from assertDeepEq in jstests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tom S [:evilpie]]

Jstests are also run as non-chrome code in the browser, so we need to remove all uses of toSource and uneval. The most frequent failure here is assertDeepEq, which uses uneval to check if two symbols are roughly equally constructed.

It seems like with standard ECMAScript functionality it's impossible to make assertDeepEq(Symbol("abc"), Symbol("abc")) work, but not assertDeepEq(Symbol.toStringTag, Symbol("Symbol.toStringTag")).

Comment 1

[André Bargull [:anba]]

The following function to compare symbols should work:

const symbolDeepEqual = (function(global) {
  // Cache primordial functionality.
  const undefined = void 0;
  const ArrayPrototypeIncludes = global.Array.prototype.includes;
  const Symbol = global.Symbol;
  const SymbolKeyFor = Symbol.keyFor;
  const SymbolPrototypeDescription =
    global.Object.getOwnPropertyDescriptor(Symbol.prototype, "description").get;
  const ReflectApply = global.Reflect.apply;

  // List of well-known symbols, generated from:
  //   Object.getOwnPropertyNames(Symbol)
  //         .filter(name => typeof Symbol[name] === "symbol")
  //         .sort()
  //         .map(name => Symbol[name])
  const wellKnownSymbols = [
    Symbol.asyncIterator, Symbol.hasInstance, Symbol.isConcatSpreadable, Symbol.iterator,
    Symbol.match, Symbol.matchAll, Symbol.replace, Symbol.search, Symbol.species, Symbol.split,
    Symbol.toPrimitive, Symbol.toStringTag, Symbol.unscopables
  ];

  return function symbolDeepEqual(a, b) {
    if (typeof a !== "symbol" || typeof b !== "symbol") {
      return false;
    }

    // Fast path for same symbols.
    if (a === b) {
      return true;
    }

    // 1. Symbol descriptions must match.
    // 2. Either both symbols are in the registry or none is.
    // 3. Neither symbol must be a well-known symbol, because those are already handled
    //    through the fast path.
    return ReflectApply(SymbolPrototypeDescription, a, []) === ReflectApply(SymbolPrototypeDescription, b, []) &&
           SymbolKeyFor(a) === SymbolKeyFor(b) &&
           !ReflectApply(ArrayPrototypeIncludes, wellKnownSymbols, [a]) &&
           !ReflectApply(ArrayPrototypeIncludes, wellKnownSymbols, [b]);
  }
})(this);

A complete uneval replacement in JavaScript is almost possible, cf. https://github.com/anba/es6draft/blob/master/src/main/scripts/source.js, only certain things like adding parentheses around function expressions, but not function declarations, is impossible in pure JavaScript:

js> function f() {}     
js> uneval(f)
"function f() {}"
js> uneval(function f() {})
"(function f() {})"

Comment 2

[Tom S [:evilpie]]

Attachment: isSimilarSymbol

Oh wow, I actually worked on this yesterday night and came up with something quite similar \o/

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 1: Move remaining ReadableStream test from js/src/tests/whatwg to js/src/tests/non262/ReadableStream. r=evilpie!

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 2: Remove `uneval` calls in test debug calls. r=evilpie!

Remove calls to uneval when only used to help debugging the test code, e.g.
in print(uneval(foo)).

Depends on D56916

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 3: Remove `uneval` calls when the argument is expected to be convertible to a string. r=evilpie!

extensions/clone-complex-object.js

• pa[i][0] is propname, which is earlier asserted to be a string.

Error/regress-465377.js

• tmp is expected to be a String value, but also add an explicit String call to handle symbols.
• Drive-by fix: Move tmp definition before first use.

Symbol/enumeration.js

• x is required to be a string by the spec, add explicit String call in case this ever changes.

object/getOwnPropertySymbols-proxy.js

• key is required to be a string by the spec, add explicit String call in case this ever changes.

object/toPrimitive.js

• trapName is required to be a string by the spec, add explicit String call in case this ever changes.

regress/regress-407024.js

• Test function f should either return 1 or undefined, both are convertible to a string.

reflect-parse/Match.js

• uneval for only used to quote the string value, so replace by manually quoting key.

extensions/shell.js

• pa[i][0] is earlier asserted to be a string.

Depends on D56917

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 4: Add skip-if annotation to `uneval` tests. r=evilpie!

These tests can be removed when uneval itself is removed.

Depends on D56918

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 5: Remove a Rhino test using `uneval`. r=evilpie!

Depends on D56919

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 6: Replace various `uneval` calls. r=evilpie!

extensions/regress-311792-01.js

• Replace uneval with join to trigger array traversal and stringification per the original test case.

extensions/regress-363040-01.js
extensions/regress-363040-02.js

• a and frac are Array objects which can stringified implicitly in these print calls.
• The print calls itself weren't removed, because the both "test" files are actually
  only example code and don't contain any proper assertions.

extensions/regress-476414-01.js
extensions/regress-476414-02.js

• uneval only referenced in jsfunfuzz clean-up code.

extensions/regress-465453.js

• Replace uneval on array with calling Array.prototype.toString.

extensions/regress-465337.js
extensions/regress-465276.js

• Replace uneval with assertEqArray to ensure the array contains the expected values.

extensions/regress-407501.js

• Replace uneval on array with calling Array.prototype.toString.

extensions/regress-363988.js

• Replace uneval with JSON.stringify to build JS source string.

regress/regress-477234.js

• Remove references to uneval and toSource.
• Deleting and re-adding these properties may have been necessary to trigger
  the bug in TraceMonkey, but it isn't worth the trouble to validate this now.

extensions/clone-errors.js
extensions/shell.js
reflect-parse/Match.js
reflect-parse/PatternAsserts.js
regress/regress-418641.js

• Use JSON.stringify instead of uneval to generate a source-like representation for error messages.

extensions/regress-226078.js
statements/for-in-with-assignments.js

• Call dis instead of uneval to disassemble a function.

extensions/regress-476653.js

• Remove a reference to uneval, which may have only be necessary to trigger
  a bug in TraceMonkey.

Depends on D56920

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 7: Split tests using `uneval`. r=evilpie!

Move the uneval tests into new test files.

Depends on D56921

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 8: Replace `uneval` usage in `assertDeepEq`. r=evilpie!

• Use String to generate the string representation of primitive values, including symbols.
• Use JSON.stringify to generate the string representation of object values.
• Add a new helper to distinguish "similar-symbols".

Depends on D56922

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 9: Replace `uneval` in jit-tests/tests/ion. r=evilpie!

ion/bug1057598.js
ion/dce-with-rinstructions.js
ion/recover-arrays.js
ion/recover-cow-arrays.js
ion/recover-lambdas.js
ion/recover-objects.js
ion/rinstructions-no-sse4.js
ion/test-scalar-replacement-float32.js

• uceFault is a function expression, so the manually adding parentheses around
  the toString representation gives the same string as calling uneval.
• Same goes for the other calls to uneval in these files.

ion/bug1071879.js

• uneval is called on number values in a print call, replace it with String.

ion/bug1314438.js
ion/bug1538083.js

• Replace uneval with Object.is to test for negative zero.

ion/bug743099.js

• uneval isn't called, replace it with another native function

ion/bug913749.js

• Replace uneval with JSON.stringify to compute the string representation of an array.
• Replace uneval with String to convert the .message property (which is most likely already a string)

Depends on D56923

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 10: Replace `uneval` in jit-tests/tests/basic. r=evilpie!

basic/array-copyWithin.js
basic/bug951346.js
basic/shell-principals.js
basic/testNEWINIT.js

• Use JSON.stringify to get the source-like representation of an object.

basic/bug1001090-3.js
basic/bug1001090-4.js
basic/bug1078871.js
basic/testDenseArrayProp.js
basic/testGCWhileRecording.js

• Use JSON.stringify here, too. (The exact result isn't relevant for theses tests.)

basic/bug699166.js
basic/bug744285.js

• Replace uneval() with String(), because it doesn't matter if "(void 0)" or "" is used.

basic/bug913445.js

• Mark uneval-only test accordingly.

basic/parseIntTests.js

• Replace uneval on number values with String, plus special-casing -0.

basic/truthies.js

• Add a simple replacement function for uneval here.

Depends on D56924

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 11: Replace `uneval` in jit-tests/tests/auto-regress. r=evilpie!

auto-regress/bug486139.js
auto-regress/bug522624.js
auto-regress/bug562028.js
auto-regress/bug584423.js

• Mark uneval-only tests accordingly.

auto-regress/bug580694.js
auto-regress/bug736609.js

• Replace uneval() with String(), because the result isn't observed anyway.

auto-regress/bug605011.js
auto-regress/bug745452.js

• Use JSON.stringify to traverse the global object instead of uneval.

Depends on D56925

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 12: Replace `uneval` in jit-tests/tests/debug. r=evilpie!

debug/Environment-variables.js

• Use String instead of uneval for the string representation of a number value.

debug/*.js

• Replace uneval with JSON.stringify.
• The exact result isn't relevant, because it's only used in print calls.

Depends on D56926

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 13: Replace `uneval` in remaining jit-tests. r=evilpie!

asm.js/testBug1057248.js
collections/Map-iterator-remove-1.js
gc/bug-1075546.js
heap-analysis/findPath.js
profiler/AutoEntryMonitor-01.js
proxy/testDirectProxySetReceiverLookup.js
saved-stacks/principals-01.js
saved-stacks/principals-02.js

• Use JSON.stringify here, too. (The exact result isn't relevant for theses tests.)

baseline/bug845331.js

• Use Function.p.toString instead of uneval to get the source reprensentation.
• toString is implicitly called through String to keep the test structure the same,
  in case that's important for this test.

fields/bug1540798.js

• Directly call Function.p.toString instead of implicitly through uneval(global).
• This ensures the test semantics are kept intact.

for-of/decompiler.js

• Use JSON.stringify to get a quoted string.

gc/bug-1259490.js
heap-analysis/byteSize-of-scripts.js
saved-stacks/async-max-frame-count.js

• Use String to get the string representation of a number value.

gc/bug-832103.js

• Use String here, too. (The exact result isn't relevant for theses tests.)

gc/bug-886630.js

• Remove jsfunfuzz clean-up code referencing uneval.

saved-stacks/1438121-async-function.js
saved-stacks/1438121-generator.js

• Use JSON.stringify to get a source-like representation of an object.
• And also use JSON.stringify to quote a string value.

Depends on D56927

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 14: Replace `uneval` in jit-test lib files. r=evilpie!

lib/*.js

• Replace uneval with JSON.stringify, because the result is only used for
  print output or error messages.

etc/generate-nosuchproperty-tests.js

• Use normal Function.p.toString representation instead of uneval.

Depends on D56928

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 15: Remove obsolete tests for `Script` function. r=evilpie!

The Script constructor has been removed in bug 543057.

Depends on D56930

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 16: Remove e4x test case. r=evilpie!

This test was added for an e4x bug and originally contained <e4x>{x}</e4x>;
after print(x.length);. When e4x was removed, <e4x>{x}</e4x>; was deleted
from the test file instead of directly deleting the whole file.

Depends on D56931

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 17: Move tests which are no longer our extensions into the main test file. r=evilpie!

And additionally remove tests for deleted extensions like Object.prototype.watch.

Depends on D56932

Comment 20

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 18: Add skip-if annotation to `toSource`-only tests. r=evilpie!

These tests can be removed when toSource itself is removed.

Depends on D56933

Comment 21

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 19: Make tests for `toSource` conditional where necessary. r=evilpie!

Allow these toSource specific tests to run even when toSource is not defined.

Depends on D56934

Comment 22

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 20: Replace remaining `toSource` calls in jstests. r=evilpie!

RegExp/regress-yarr-regexp.js

• Compare arrays using assertEqArray instead of using Array.prototype.toSource.

TypedObject/scalar_types.js
TypedObject/size_and_alignment.js

• Use the new describeType helper (see changes in TypedObject/shell.js) to
  get a descriptive name for a TypedObject type.

TypedObject/shell.js

• Add describeType helper to mimic the output of toSource for TypedObject types.
• And use JSON.stringify to stringify TypedObject instances.

class/superPropBasicCalls.js

• Use toString instead of toSource for class property lookup tests.
• Interestingly the test class is already named toStringTest.

expressions/regress-96526-delelem.js

• Call formatArray to get the expected output, because the formatArray helper
  generates a slightly different ouput ("[3, , ]") than Array.prototype.toSource.

extensions/clone-v1-typed-array.js

• Use toString instead of toSource for the print output of an Error object.

extensions/dataview.js

• Use JSON.stringify to generate a source-like representation of an array of number values.

extensions/regress-314874.js

• Directly use expect and actual for the comparison call.

extensions/regress-459606.js

• Directly use the toFixed result for the comparison call.

extensions/regress-476414-01.js
extensions/regress-476414-02.js

• Remove jsfunfuzz clean-up code.

jit/regress-452498-01.js

• Use JSON.stringify to generate a source-like representation of an array of number values.
• Additionally fixup the ouput to match the expected string.

reflect-parse/Match.js

• Use JSON.stringify to get a string-representation for error messages.

Depends on D56935

Comment 23

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 21: Use assertDeepEq to compare array objects. r=evilpie!

assertDeepEq allows to compare arrays with nested objects, which isn't possible with arraysEqual.

Depends on D56936

Comment 24

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 22: Replace `toSource` with `toString` on function objects. r=evilpie!

• Calling toSource is equivalent to calling toString on function declarations.
• For function expressions, the result of toString must be enclosed in parentheses
  to get the equivalent result when compared to toSource.

Depends on D56937

Comment 25

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 23: Make `toSource` tests conditional. r=evilpie!

These changes handle the following three cases:

1. Regression tests for toSource functions.

• Added skip-if annotation.

2. Contains toSource (a few) conformance tests, but also additional conformance tests.

• Added if-statements to skip the toSource bits when toSource is not present.

3. Contains many toSource conformance tests.

• Split the file into two separate files (see asm.js/testBug1147144.js and asm.js/testSource.js).

Depends on D56939

Comment 26

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 24: Replace additional `toSource` tests. r=evilpie!

auto-regress/bug713209.js

• Replace toSource with toString on string value for print output.

baseline/bug1063878.js

• Replace Math.toSource with String, which is also a non-scripted function.

basic/bug1141154.js
debug/bug1353356.js

• Replace Number.prototype.toSource with toString when called on an Array object.
• Both calls ensure a TypeError is thrown for this test.

basic/bug728609.js

• Replace indirect toSource calls with explicit calls.
• (The test relied on calling o.first.toSource being called when o.toSource is called.)

gc/bug-1108007.js

• Replace toSource call on a string value with the direct result.
• Based on the stack in the bug report, calling toSource is not necessarily required for this test.

gc/bug-1439284.js

• Replace Boolean.prototype.toSource with toString when called on a String object.
• Both calls ensure a TypeError is thrown for this test.

ion/bug1348777.js

• Replace toSource with toString when testing polymorphic calls.

ion/bug847412.js
ion/bug913749.js
xdr/tagged-template-literals-2.js

• Replace toSource on the (global) object with JSON.stringify to emulate object traversal.

ion/inlining/TypedObject-ObjectIsTypeDescr-*.js

• Replace toSource on TypedObjects with a call to equivalent.
• This should ensure the tests are still valid, because equivalent is also calling ObjectIsTypeDescr,
  which is the function under test here.

ion/scalar-replacement-bug1138693.js

• Also replace toSource with equivalent in this test, because similar to toSource, equivalent
  is also performing a load from a reserved slot.

Depends on D56940

Comment 27

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 26: Remove generator comprehension test. r=evilpie!

Remove the test file because generator comprehensions have been removed a while ago.

Comment 28

[André Bargull [:anba]]

@evilpie: You mentioned on IRC that you were interested in trying again to remove uneval and since I've already had these patches laying around, I thought I could save you the trouble of redoing these changes again.

Comment 29

[André Bargull [:anba]]

Attachment: Bug 1565001 - Part 25: Use assertDeepEq to compare array objects on mandelbrot test. r=evilpie!

The changes in "lib/mandelbrot-results.js" are too large for a normal diff, so this
change was split from part 21.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9694344d9e20
Part 1: Move remaining ReadableStream test from js/src/tests/whatwg to js/src/tests/non262/ReadableStream. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/861dc1bd9471
Part 2: Remove `uneval` calls in test debug calls. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/f4d9184defcf
Part 3: Remove `uneval` calls when the argument is expected to be convertible to a string. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/76e1d3524ade
Part 4: Add skip-if annotation to `uneval` tests. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/b8a987e0ca33
Part 5: Remove a Rhino test using `uneval`. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/10b1dbc9b217
Part 6: Replace various `uneval` calls. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/f4361789fb67
Part 7: Split tests using `uneval`. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/34f1b35bd87a
Part 8: Replace `uneval` usage in `assertDeepEq`. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/923158e63358
Part 9: Replace `uneval` in jit-tests/tests/ion. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/4c184d3ba9d6
Part 10: Replace `uneval` in jit-tests/tests/basic. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/ee19f22e8f2a
Part 11: Replace `uneval` in jit-tests/tests/auto-regress. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/a2601cc45eb1
Part 12: Replace `uneval` in jit-tests/tests/debug. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/1a0ca03f6491
Part 13: Replace `uneval` in remaining jit-tests. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/76085e7c6e81
Part 14: Replace `uneval` in jit-test lib files. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/b026488acb2e
Part 15: Remove obsolete tests for `Script` function. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/9b54bce9a504
Part 16: Remove e4x test case. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/d3b05632992c
Part 17: Move tests which are no longer our extensions into the main test file. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/c75989dc9b71
Part 18: Add skip-if annotation to `toSource`-only tests. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/82706c3dffc6
Part 19: Make tests for `toSource` conditional where necessary. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/4aff3495ccd4
Part 20: Replace remaining `toSource` calls in jstests. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/796b0e6c15c2
Part 21: Use assertDeepEq to compare array objects. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/478531f112c8
Part 22: Replace `toSource` with `toString` on function objects. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/90aa16c48a36
Part 23: Make `toSource` tests conditional. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/303b0a9c0b1b
Part 24: Replace additional `toSource` tests. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/49f149652d65
Part 25: Use assertDeepEq to compare array objects on mandelbrot test. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/1036d6391be2
Part 26: Remove generator comprehension test. r=evilpie

Comment 31

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9694344d9e20
https://hg.mozilla.org/mozilla-central/rev/861dc1bd9471
https://hg.mozilla.org/mozilla-central/rev/f4d9184defcf
https://hg.mozilla.org/mozilla-central/rev/76e1d3524ade
https://hg.mozilla.org/mozilla-central/rev/b8a987e0ca33
https://hg.mozilla.org/mozilla-central/rev/10b1dbc9b217
https://hg.mozilla.org/mozilla-central/rev/f4361789fb67
https://hg.mozilla.org/mozilla-central/rev/34f1b35bd87a
https://hg.mozilla.org/mozilla-central/rev/923158e63358
https://hg.mozilla.org/mozilla-central/rev/4c184d3ba9d6
https://hg.mozilla.org/mozilla-central/rev/ee19f22e8f2a
https://hg.mozilla.org/mozilla-central/rev/a2601cc45eb1
https://hg.mozilla.org/mozilla-central/rev/1a0ca03f6491
https://hg.mozilla.org/mozilla-central/rev/76085e7c6e81
https://hg.mozilla.org/mozilla-central/rev/b026488acb2e
https://hg.mozilla.org/mozilla-central/rev/9b54bce9a504
https://hg.mozilla.org/mozilla-central/rev/d3b05632992c
https://hg.mozilla.org/mozilla-central/rev/c75989dc9b71
https://hg.mozilla.org/mozilla-central/rev/82706c3dffc6
https://hg.mozilla.org/mozilla-central/rev/4aff3495ccd4
https://hg.mozilla.org/mozilla-central/rev/796b0e6c15c2
https://hg.mozilla.org/mozilla-central/rev/478531f112c8
https://hg.mozilla.org/mozilla-central/rev/90aa16c48a36
https://hg.mozilla.org/mozilla-central/rev/303b0a9c0b1b
https://hg.mozilla.org/mozilla-central/rev/49f149652d65
https://hg.mozilla.org/mozilla-central/rev/1036d6391be2