goog.array.map(undefined, handler): the worst from both worlds

[artemy tregubenko]

Hello,

I've just found that in modern browsers goog.array.map(undefined,
handler) unexpectedly calls handler once, passing window object as a
parameter. Well, of course I do not call it exactly this way, I call
it like `goog.array.map(foo.bar, baz)` and sometimes there's no `bar`
in `foo`, so `foo.bar` has value `undefined`. But instead of doing
nothing map calls `baz(window)`. Unexpected, isn't it?

More detailed example:

var foo = {bar: []}, baz = {};
var f = function(param){ console.log(param) };
goog.array.map(foo.bar, f); // does nothing
goog.array.map(baz.bar, f); // prints `[object Window]`

In modern browsers goog.array.map is assigned this value:

function(arr, f, opt_obj) {
return goog.array.ARRAY_PROTOTYPE_.map.call(arr, f, opt_obj);
}

This can be reduced to `Array.prototype.map.call(undefined, handler)`
which too calls handler once, passing window as a parameter. I suppose
this behavior is correct by EcmaScript Language Specification 5th
edition [1] page 136. The first step of algorithm is "Let O be the
result of calling ToObject passing the `this` value as the argument.",
and probably `this` defaults to window when undefined. Anyway Opera,
Chrome and Mozilla behave same way, so I doubt it's possible to change
that.

Common javascript scenario for using `map` is `array.map(handler)`.
It's quite obvious that `array` may be undefined at this point, so one
should put additional check in code: `if (array) array.map(handler)`.
However when using not object's method but global function like
`goog.array.map` you would expect it does internal checks of
parameters or at least throws exception. Closure does none of that,
executing completely unreasonable code and continuing as if nothing
happened.

As a result Closure currently takes the worst from both worlds: you
have to manually check argument for being defined AND you have to
prepend `goog.array` to method name.

I doubt there's a scenario where programmer uses array iterators and
relies on defaulting the undefined parameter to window object. If
Closure is for programmer convenience, it should add extra check to
`map` and other iterators to work around this edge case.

Current alternatives are:

* monkey-patching Closure
* wrapping all iterators in own functions performing the checks and
replacing `goog.array.*` in the code with these own functions
* the worst case: putting check in all places where you use array
iterator, hoping that you don't miss any of them and that you won't
forget this in the future.

All these alternatives are quite unreliable and obviously it is better
in every way to just put ` !arr ? [] : ` in Closure's array iterators.

[1] http://www.ecma-international.org/publications/standards/Ecma-262.htm

--
arty ( http://arty.name )

[Erik Arvidsson]

Calling goog.array.map(window, fun) is correct since window is an
ArrayLike object. However, calling it with undefined should generate a
type error if you compile your code.

We could add a runtime check for non compiled mode but he have tried
not to do this in the past since it just makes the code harder to
maintain.

--
erik

[artemy tregubenko]

Thank you for your reply, but it looks like I've failed to explain my
problem.

The following example is quite close to the real code I have. I do not
call goog.array.map(window, fun). I do not call
goog.array.map(undefined, fun). I have an object `foo`, containing
array as its property `bar`. My code iterates over this array:
goog.array.map(foo.bar, fun). Sometimes object doesn't have that
property `bar`, like object `baz` here. Since there's no array and
there're no elements of array, I expect code not to do anything.
However code calls `foo(window)`. This is the problem.

var foo = {bar: []}, baz = {};
var f = function(param){ console.log(param) };
goog.array.map(foo.bar, f); // does nothing
goog.array.map(baz.bar, f); // prints `[object Window]`

This problem appears at runtime and thus cannot be solved by
compiler. I propose to replace

return goog.array.ARRAY_PROTOTYPE_.map.call(arr, f, opt_obj);

with

return !arr ? [] : goog.array.ARRAY_PROTOTYPE_.map.call(arr, f,
opt_obj);

[Erik Arvidsson]

I'm sorry. I did understand what you wrote but I was not really replying to it.

Calling goog.array.map with undefined is wrong. I agree that it is confusing that it iterates over the window. Adding runtime checks for invalid usage is something we have policy against. It adds to the code size as well as to the runtime cost.

On May 16, 2010 3:26 AM, "artemy tregubenko" <m...@arty.name> wrote:

Thank you for your reply, but it looks like I've failed to explain my
problem.

The following example is quite close to the real code I have. I do not
call goog.array.map(window, fun). I do not call
goog.array.map(undefined, fun).  I have an object `foo`, containing
array as its property `bar`. My code iterates over this array:
goog.array.map(foo.bar, fun). Sometimes object doesn't have that
property `bar`, like object `baz` here. Since there's no array and
there're no elements of array, I expect code not to do anything.
However code calls `foo(window)`. This is the problem.

var foo = {bar: []}, baz = {};
var f = function(param){ console.log(param) };

goog.array.map(foo.ba...

This problem appears at runtime  and thus cannot be solved by
compiler. I propose to replace

return goog.array.ARRAY_PROTOTYPE_.map.call(arr, f, opt_obj);

with

   return !arr ? [] : goog.array.ARRAY_PROTOTYPE_.map.call(arr, f,
opt_obj);

On May 16, 11:09 am, Erik Arvidsson <erik.arvids...@gmail.com> wrote:

> Calling goog.array.map(win...

> On Fri, May 14, 2010 at 04:37, artemy tregubenko <m...@arty.name> wrote:
> > Hello,
>

> > I've jus...

[Nick Santos]

Shouldn't the function throw an exception if the array is undefined? I
think that "doing nothing" is just as unpredictable as the current
behavior.

But in general, i'm not sure that it makes sense to validate input
types of functions by default. It's enough to say in the docs that the
behavior of this function is undefined if the array parameter is not
array-like.

On Sun, May 16, 2010 at 6:26 AM, artemy tregubenko <m...@arty.name> wrote:
>
> This problem appears at runtime  and thus cannot be solved by
> compiler.

What?!? all problems can be solved by the compiler. ;)

there's currently a closure compiler plugin that will add runtime type
checks based on the jsdoc annotations. You could automatically produce
a closure library where all functions had run-time type-checking on
their inputs. Is that something you'd be interested in?

(I don't know if the plugin is ready for prime time yet; i'd have to
ask the people that wrote it)

Nick

[artemy tregubenko]

I see. Well, I had idea that libraries are for programmer's
convenience. Putting a check every time you use an iterator is
inconvenient, and I thought "I can place a check here and forget about
them in all other places". But if Closure is more for computer's
convenience, I won't insist.

[artemy tregubenko]

On May 17, 8:25 am, Nick Santos <nicksan...@google.com> wrote:
> Shouldn't the function throw an exception if the array is undefined? I
> think that "doing nothing" is just as unpredictable as the current
> behavior.

Well, anything is more predictable than current behavior. Exception is
good too. But I think that treating undefined the same way as empty
array is more convenient. It's like convenience of javascript's
negation operator: you can feed it with anything and it will treat
`false` the same way as 0, undefined, null, empty string or empty
array. Sure, sometimes you need exact checks for type, but you only
write them when you actually need them. "Make the simple things easy
and the complex things possible."

> But in general, i'm not sure that it makes sense to validate input
> types of functions by default. It's enough to say in the docs that the
> behavior of this function is undefined if the array parameter is not
> array-like.

I agree with you that it shouldn't validate input by default. But this
is the edge case when invalid input results in quite exotic behavior,
and I suggest to fix only that edge case. Docs are great, I highly
praise the Closure team for them! But neither docs nor tests are
silver bullet, they won't save us from all dangers.

> On Sun, May 16, 2010 at 6:26 AM, artemy tregubenko <m...@arty.name> wrote:
>
> > This problem appears at runtime  and thus cannot be solved by
> > compiler.
>
> What?!? all problems can be solved by the compiler. ;)
>
> there's currently a closure compiler plugin that will add runtime type
> checks based on the jsdoc annotations. You could automatically produce
> a closure library where all functions had run-time type-checking on
> their inputs.  Is that something you'd be interested in?
>
> (I don't know if the plugin is ready for prime time yet; i'd have to
> ask the people that wrote it)

Wow, I have to admit I was wrong about powers of compiler! : )

This plugin might be the solution, but for now it doesn't seem too
convenient either. First, it's still nice to have uncompressed code
while debugging. Second, it will increase the hassle of updating local
Closure code. Third, I'm not sure Closure's trunk works nice with it.
So I hesitate to ask for the plugin. Thank you for the idea though!

[Garry Boyer]

On Sun, May 16, 2010 at 6:26 AM, artemy tregubenko <m...@arty.name> wrote:

Thank you for your reply, but it looks like I've failed to explain my
problem.

The following example is quite close to the real code I have. I do not
call goog.array.map(window, fun). I do not call
goog.array.map(undefined, fun).  I have an object `foo`, containing
array as its property `bar`. My code iterates over this array:
goog.array.map(foo.bar, fun). Sometimes object doesn't have that
property `bar`, like object `baz` here. Since there's no array and
there're no elements of array, I expect code not to do anything.
However code calls `foo(window)`. This is the problem.

var foo = {bar: []}, baz = {};
var f = function(param){ console.log(param) };
goog.array.map(foo.bar, f); // does nothing
goog.array.map(baz.bar, f); // prints `[object Window]`

This second call is an error.  Erik is suggesting to add an assert (only in uncompiled mode) that raises an exception when this happens.

[artemy tregubenko]

On May 16, 8:20 pm, Garry Boyer <gbo...@google.com> wrote:
> On Sun, May 16, 2010 at 6:26 AM, artemy tregubenko <m...@arty.name> wrote:
> > Thank you for your reply, but it looks like I've failed to explain my
> > problem.
>
> > The following example is quite close to the real code I have. I do not
> > call goog.array.map(window, fun). I do not call
> > goog.array.map(undefined, fun).  I have an object `foo`, containing
> > array as its property `bar`. My code iterates over this array:
> > goog.array.map(foo.bar, fun). Sometimes object doesn't have that
> > property `bar`, like object `baz` here. Since there's no array and
> > there're no elements of array, I expect code not to do anything.
> > However code calls `foo(window)`. This is the problem.
>
> > var foo = {bar: []}, baz = {};
> > var f = function(param){ console.log(param) };
> > goog.array.map(foo.bar, f); // does nothing
> > goog.array.map(baz.bar, f); // prints `[object Window]`
>
> This second call is an error.  Erik is suggesting to add an assert (only in
> uncompiled mode) that raises an exception when this happens.

Thank you, I understand that. I'm trying to persuade Closure
developers that it should not be an error because that would be
convenient for programmer. Exception will do too, but exception
solution is not the best.

[Erik Arvidsson]

I'm adding asserts in debug mode for this.

However, where does it end? JS is a dynamic language and anyone can
change anything which might break anything. Protecting against all
possible errors will lead to insanity.

--
erik

[artemy tregubenko]

On Mon, 17 May 2010 23:03:52 +0400, Erik Arvidsson
<erik.ar...@gmail.com> wrote:

> I'm adding asserts in debug mode for this.

Yay, thanks! Hopefully this will make its way into public soon.

> However, where does it end? JS is a dynamic language and anyone can
> change anything which might break anything. Protecting against all
> possible errors will lead to insanity.

I understand your concerns. I do not ask to ignore i.e. missing element in
goog.dom.classes.add. But given case just asks for a check anyway because
of its erratic results.

So I suppose there's no chance to convince you make it do nothing when
first parameter is undefined?

[Nathan Naze]

Arty,

Erik's right - in general, we don't write type-checking code outside
of particular special cases. The JSDoc indicates what the input
should be -- if you give a function that expects an object of type
"Foo" an object of type "Bar" the behavior is (of course) undefined.

It's a dynamic language. We can't conceivably add type checking code
on every function and method -- it would balloon the size of the
library with needless boilerplate.

We use Closure Compiler to do our type checking. You should start
using the Closure Compiler to do this type verification for you. As
Nick mentioned, there's been some work to write runtime checks in
based on JSDoc (which would, essentially, be writing the boilerplate
for you at compile time).

Nathan

[artemy tregubenko]

Nathan,

Thank you for detailed explanation and advice. I have some ideas about
what library and code should do and I was wrong to expect same philosophy
in Closure. Now I have better understanding that Closure aims to have type
checking and compile-time errors. I adhere to more agile way of doing
what's possible with what I've got in parameters. This agility comes with
a price of less reliability and I should not force same tradeoff to be
made for Closure.