Uint8ClampedList no longer inherits from Uint8List

Uint8ClampedList no longer inherits from Uint8List

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27326

[sra1, 2013-08-26 12:08:25]

What is the motivation for this change?  It is likely to break some programs.

We made it this way because the browser typed arrays have this subclass
relationship.  In the browser console:

> Uint8ClampedArray.prototype.__proto__
Uint8Array {subarray: function, set: function, BYTES_PER_ELEMENT: 1}
>


Stuff breaks if the dart2js native class declarations don't accurately describe
the browser implementation inheritance.  The breakage depends on the order of
operations so is difficult to test correctly.  Your change as written will cause
some programs to incorrectly evaluate (x is Uint8ClampedList) to false, which is
how we found the problem.

https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
File tests/lib/typed_data/typed_data_hierarchy_test.dart (right):

https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
tests/lib/typed_data/typed_data_hierarchy_test.dart:41: Expect.isFalse(new
Uint8ClampedList(1) is Uint8List);
This appears to pass on dart2js because, among other reasons, it is
constant-folded.  I bet the generated code looks something like:

new $.Uint8ClampedArray(1);
$.Expect_isFalse(false, null);

I would guess a large percentage of our tests are broken due to optimization. 
We really need a function 'inscrutable' that is guaranteed to confuse the
optimizer:

Expect.isFalse(inscrutable(new Uint8ClampedList(1)) is Uint8List);

for now, this will work with dart2js:

var inscrutable = null;

main() {
  inscrutable = (x) => x;
  ...
}

[Cutch, 2013-08-26 12:12:06]

On 2013/08/26 12:08:25, sra1 wrote:
> What is the motivation for this change?  It is likely to break some programs.
> 
> We made it this way because the browser typed arrays have this subclass
> relationship.  In the browser console:
> 
> > Uint8ClampedArray.prototype.__proto__
> Uint8Array {subarray: function, set: function, BYTES_PER_ELEMENT: 1}
> >
> 
> 
> Stuff breaks if the dart2js native class declarations don't accurately
describe
> the browser implementation inheritance.  The breakage depends on the order of
> operations so is difficult to test correctly.  Your change as written will
cause
> some programs to incorrectly evaluate (x is Uint8ClampedList) to false, which
is
> how we found the problem.
> 

It turns out that Mozilla never properly implemented the hierarchy and the
official spec for ES6 alters the subclass relationship so that Uint8ClampedArray
is not a descendent of Uint8Array. This change was requested by Kenneth Russell
so that we stay in sync with the JS spec.

>
https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
> File tests/lib/typed_data/typed_data_hierarchy_test.dart (right):
> 
>
https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
> tests/lib/typed_data/typed_data_hierarchy_test.dart:41: Expect.isFalse(new
> Uint8ClampedList(1) is Uint8List);
> This appears to pass on dart2js because, among other reasons, it is
> constant-folded.  I bet the generated code looks something like:
> 
> new $.Uint8ClampedArray(1);
> $.Expect_isFalse(false, null);
> 
> I would guess a large percentage of our tests are broken due to optimization. 
> We really need a function 'inscrutable' that is guaranteed to confuse the
> optimizer:
> 
> Expect.isFalse(inscrutable(new Uint8ClampedList(1)) is Uint8List);
> 
> for now, this will work with dart2js:
> 
> var inscrutable = null;
> 
> main() {
>   inscrutable = (x) => x;
>   ...
> }

Thanks for catching this.

[siva, 2013-08-27 00:16:10]

I am fine with this API change, in fact this is how it was when I first
implemented typed-data. Later it was changed
in https://codereview.chromium.org//14316008 to make the
behaviour match typed array spec.

[sra1, 2013-08-27 02:02:25]

On 2013/08/26 12:12:06, Cutch wrote:
> On 2013/08/26 12:08:25, sra1 wrote:
> > What is the motivation for this change?  It is likely to break some
programs.
> > 
> > We made it this way because the browser typed arrays have this subclass
> > relationship.  In the browser console:
> > 
> > > Uint8ClampedArray.prototype.__proto__
> > Uint8Array {subarray: function, set: function, BYTES_PER_ELEMENT: 1}
> > >
> > 
> > 
> > Stuff breaks if the dart2js native class declarations don't accurately
> describe
> > the browser implementation inheritance.  The breakage depends on the order
of
> > operations so is difficult to test correctly.  Your change as written will
> cause
> > some programs to incorrectly evaluate (x is Uint8ClampedList) to false,
which
> is
> > how we found the problem.
> > 
> 
> It turns out that Mozilla never properly implemented the hierarchy and the
> official spec for ES6 alters the subclass relationship so that
Uint8ClampedArray
> is not a descendent of Uint8Array. This change was requested by Kenneth
Russell
> so that we stay in sync with the JS spec.

So some browsers have Uint8ClampedArray inheriting from Uint8Array and others do
not?
Whatever we do needs to work on both types of browser, regardless of what is
written in the spec.

This change makes Uint8List a leaf class in the class hierarchy. Leaf and
non-leaf classes cache their methods differently.  The leaf caching is much more
efficient but incorrect when the implementation in the browser is a non-leaf.

I think we can work around this by introducing a bogus leaf class for Uint8List.

class _Uint8ListSubclass extends Uint8List native "Bogus Uint8List subclass" {
  // Very few methods here since they will be determined to be reachable and
compiled into the code but unreachable in practice because this class will have
no instances.  I.e. any method is dead code that the compiler can't prove to be
dead.

  // Huge comment explaining this problem.
}


I don't like this solution because every time we have lied about the type
hierarchy as implemented by the browser it has come back to bite us.





> 
> >
>
https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
> > File tests/lib/typed_data/typed_data_hierarchy_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/23093027/diff/1/tests/lib/typed_data/typed_da...
> > tests/lib/typed_data/typed_data_hierarchy_test.dart:41: Expect.isFalse(new
> > Uint8ClampedList(1) is Uint8List);
> > This appears to pass on dart2js because, among other reasons, it is
> > constant-folded.  I bet the generated code looks something like:
> > 
> > new $.Uint8ClampedArray(1);
> > $.Expect_isFalse(false, null);
> > 
> > I would guess a large percentage of our tests are broken due to
optimization. 
> > We really need a function 'inscrutable' that is guaranteed to confuse the
> > optimizer:
> > 
> > Expect.isFalse(inscrutable(new Uint8ClampedList(1)) is Uint8List);
> > 
> > for now, this will work with dart2js:
> > 
> > var inscrutable = null;
> > 
> > main() {
> >   inscrutable = (x) => x;
> >   ...
> > }
> 
> Thanks for catching this.

[Cutch, 2013-08-28 14:43:17]

PTAL

[sra1, 2013-08-30 22:12:16]

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:622: _Uint8ListSubclass() {
Remove constructor.

Native classes can't have generative constructors.

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:624: inscrutable() {
remove this member

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:627: // See:
https://codereview.chromium.org/23093027/
Say something more.  It is ok to reference the bug, but just referencing the bug
is not enough.  At least indicate what the reader will learn by following the
reference.
 e.g.

On some browsers, Uint8ClampedArray is a subclass of Uint8Array, on others is
its a subclass of ArrayBufferView.
For dart2js to generate correct code it needs to know that there might be a
subclass of Uint8Array.  This bogus class indicates the possibility.


FWIW I'm only 80% sure that this hack will have the right behaviour.

[Cutch, 2013-09-08 15:43:10]

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:622: _Uint8ListSubclass() {
On 2013/08/30 22:12:16, sra1 wrote:
> Remove constructor.
> 
> Native classes can't have generative constructors.

Done.

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:624: inscrutable() {
On 2013/08/30 22:12:16, sra1 wrote:
> remove this member

Done.

https://chromiumcodereview.appspot.com/23093027/diff/9001/sdk/lib/typed_data/...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:627: // See:
https://codereview.chromium.org/23093027/
On 2013/08/30 22:12:16, sra1 wrote:
> Say something more.  It is ok to reference the bug, but just referencing the
bug
> is not enough.  At least indicate what the reader will learn by following the
> reference.
>  e.g.
> 
> On some browsers, Uint8ClampedArray is a subclass of Uint8Array, on others is
> its a subclass of ArrayBufferView.
> For dart2js to generate correct code it needs to know that there might be a
> subclass of Uint8Array.  This bogus class indicates the possibility.
> 
> 
> FWIW I'm only 80% sure that this hack will have the right behaviour.

Done.

[siva, 2013-09-09 15:51:40]

lgtm

[Cutch, 2013-09-10 09:45:12]

Committed patchset #5 manually as r27326 (presubmit successful).

[Cutch, 2013-09-10 09:59:59]

sra1: please advise how to properly structure the dart2js code:

../../sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:664:1: Error: Cannot
find constructor "".
class _Uint8ListSubclass extends Uint8List native "Bogus Uint8List subclass" {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Uncaught Error: Bad state: Failed to create mirror system.
Stack Trace:
#0      analyze.<anonymous closure>
(file:///mnt/data/b/build/slave/new_analyzer-linux-release/build/dart/sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234:7)
#1      _ThenFuture._zonedSendValue (dart:async/future_impl.dart:353:24)

[Cutch, 2013-09-10 10:06:35]

On 2013/09/10 09:59:59, Cutch wrote:
> sra1: please advise how to properly structure the dart2js code:
> 
> ../../sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:664:1: Error: Cannot
> find constructor "".
> class _Uint8ListSubclass extends Uint8List native "Bogus Uint8List subclass" {
>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Uncaught Error: Bad state: Failed to create mirror system.
> Stack Trace:
> #0      analyze.<anonymous closure>
>
(file:///mnt/data/b/build/slave/new_analyzer-linux-release/build/dart/sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234:7)
> #1      _ThenFuture._zonedSendValue (dart:async/future_impl.dart:353:24)

New CL: https://codereview.chromium.org/23464052/