New issue
Advanced search Search tips

Issue 704242 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Generated extension externs don't properly account for optional parameters in callbacks.

Project Member Reported by rdevlin....@chromium.org, Mar 22 2017

Issue description

An API might conditionally send a response parameter to a caller.
e.g., envision an API:
doStuff({includeResult: true})
which responds with a Result iff includeResult was present and true.

Currently, closure extern generation doesn't handle this case well,
and for a callback like:
callback FooCallback = void(optional FooResponse response);
/**
 * @param {function(!FooResponse):void} callback
 */
But this is incorrect, since it indicates that the response would
never be null.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9c093e54e11cb41c6f3402997a97be16568865f

commit f9c093e54e11cb41c6f3402997a97be16568865f
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Wed Mar 22 21:13:23 2017

[Closure Externs] Handle optional params sent to an API callback

An API might conditionally send a response parameter to a caller.
e.g., envision an API:
doStuff({includeResult: true})
which responds with a Result iff includeResult was present and true.

Currently, closure extern generation doesn't handle this case well,
and for a callback like:

callback FooCallback = void(optional FooResponse response);
static void doStuff(DoStuffParams params, FooCallback callback);

generates:

/**
 * @param {DoStuffParams} params
 * @param {function(!FooResponse):void} callback
 */

But this is incorrect, since it indicates that the response would
never be null.

Properly handle these optional responses, and update the test.

BUG= 704242 

Review-Url: https://codereview.chromium.org/2766363002
Cr-Commit-Position: refs/heads/master@{#458876}

[modify] https://crrev.com/f9c093e54e11cb41c6f3402997a97be16568865f/tools/json_schema_compiler/js_externs_generator_test.py
[modify] https://crrev.com/f9c093e54e11cb41c6f3402997a97be16568865f/tools/json_schema_compiler/js_util.py

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38727920b34836a7a31c36c831c59fdcb1ed4dda

commit 38727920b34836a7a31c36c831c59fdcb1ed4dda
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Thu Mar 23 00:42:18 2017

[Closure Externs] Add wrapping parens on optional params

crrev.com/f9c093e54e11cb41c6f3402997a97be16568865f fixed optional
params in callbacks, but forgot to wrap the type in params.
function(!Foo|undefined) -> function((!Foo|undefined))

BUG= 704242 

Review-Url: https://codereview.chromium.org/2768213002
Cr-Commit-Position: refs/heads/master@{#458958}

[modify] https://crrev.com/38727920b34836a7a31c36c831c59fdcb1ed4dda/tools/json_schema_compiler/js_externs_generator_test.py
[modify] https://crrev.com/38727920b34836a7a31c36c831c59fdcb1ed4dda/tools/json_schema_compiler/js_util.py

Sign in to add a comment