651793 - JSON reviver checking not consistent with self nor ECMA

Status: RESOLVED INVALID

(Tamarin Graveyard :: Library, enhancement, P4)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

From Bug 640711, comment 70:

  By design, JSON.stringify() differs from ECMA-262 in throwing an exception for an invalid replacer argument.  In contrast, JSON.parse() silently ignores a non-null non-callable reviver parameter until a call is attempted, whereupon we presumably throw a non-JSON-specific exception.  Would it be appropriate to check the parameter earlier?  ECMA-262 doesn't even attempt the walk unless the reviver is callable.  Should we follow suit?  Either way, we are not consistently following ECMA-262 or consistently doing aggressive validation.

We should be able to check the replacer the same way as the reviver.  And we already allocated an entry in the ErrorConstants table for it anyway.  :)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Hopefully addressed by patch C' on Bug 640711 (see Bug 640711, comment 73.)

I recently saw code that led me to think that functions let you inspect their arity via their length property.  That's a fun bit of evil that I chose not to employ here, since I'm not sure how reliable it is (e.g. what do var args functions do?  I don't want to take the time to find out right now).

Comment 2

[Lars T Hansen]

The arity check is not reliable, specifically, class objects can be used as functions but explicitly provide a value for length, and that value can be misleading.

However, a class object cannot be passed to a typed argument expecting a Function (this is surprising to me, but true).  So you probably can depend on the arity if you know that the value you're looking at is typed 'Function'.

That said, at most I would check that the reviver is a Function, which you should be able to do in the signature of JSON.parse.  Since your class synopsis on the spec page already has that annotation, I'm not sure you need to do more.  Checking arity is really polishing a turd, what we want is a robust notion of function types.  We can evolve the JSON API when we add such a notion to the language.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

I ended up landing JSON without patch C' on Bug 640711 for exactly the reason Lars describes in comment 2: the signature of JSON.parse already says that its input has to be a Function; the type-checker and runtime linker conspire to ensure this.  (See Bug 640711, comment 75.)  We even removed the error message from the string constants table at the behest of the localization team.

So I will close this as not a bug.