Move _fatal to dart:async

Move _fatal to dart:async

The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/bbf5b89473f38f4b1c674c83411fc02cb68e0abe

[Kevin Millikin (Google), 2017-02-09 13:41:33]

kmillikin@google.com changed reviewers:
+ asiva@google.com, hausner@google.com, lrn@google.com

[Kevin Millikin (Google), 2017-02-09 13:41:34]

Context for this change: the private function _fatal in dart:core was being
called from dart:async.  I don't think that actually worked, it probably just
threw a NoSuchMethod, but it doesn't seem easy to test from Dart code.

I moved it to dart:_internal and made it public so it could be called from
dart:async.  This exposed it to Dart code via mirrors, which doesn't seem
desirable.  Since dart:_internal could contain other public native functions
that violate the semantics of Dart, it doesn't seem safe to expose them via
mirrors anyway.

This is a breaking change if a user was relying on mirrors to get access to
members of dart:_internal.

[Lasse Reichstein Nielsen, 2017-02-09 15:30:02]

Break them. Break them all!

[siva, 2017-02-09 22:49:11]

asiva@google.com changed reviewers:
+ rmacnak@google.com

[siva, 2017-02-09 22:49:11]

+rmacnak

[rmacnak, 2017-02-09 23:08:50]

On 2017/02/09 22:49:11, siva wrote:
> +rmacnak

This is not correct. dart:_internal includes implementations of public
interfaces whose objects are accessible to the user, such as CodeUnits and
Symbol. Their public members are accessible non-reflectively, so they must
remain accessible reflectively.

_fatal should instead be removed and replaced with a regular throw. Compare uses
of "internal error" in dart:mirrors and dart:io.

[Lasse Reichstein Nielsen, 2017-02-10 06:53:57]

On 2017/02/09 23:08:50, rmacnak wrote:
> On 2017/02/09 22:49:11, siva wrote:
> > +rmacnak
> 
> This is not correct. dart:_internal includes implementations of public
> interfaces whose objects are accessible to the user, such as CodeUnits and
> Symbol. Their public members are accessible non-reflectively, so they must
> remain accessible reflectively.

Good point.

The problem here is that we have two different uses of dart:_internal:
- shared internal code 
- shared exported classes

The CodeUnits class is shared between libraries because it is created by
dart:core but recognized for optimization in dart:typed_data.
It's intended to be public and exported by dart:core, but we have a *sharing*
that we (read: I) solved by putting the class in CodeUnits.

Then we have methods like `makeListFixedLength` and
`makeFixedLengthListUnmodifiable` which should never leak to user code.

With mirrors, I expect that you can take `"x".codeUnits` and reflect that, get
is library, and find makeListFixedLength. That's bad.
We can't use being private as a marker because we need the name to be externally
visible.

I see two good solutions:
- Split the library into internal_private and internal_shared, hide everything
in _private and not the things in _shared.
- Use an annotation to expose the things that are intended as public or hide the
things that are not.

> 
> _fatal should instead be removed and replaced with a regular throw. Compare
uses
> of "internal error" in dart:mirrors and dart:io.

That solves this particular problem (which is good), but we should still look
into the problem.

[Kevin Millikin (Google), 2017-02-10 09:44:04]

On 2017/02/09 23:08:50, rmacnak wrote:
> _fatal should instead be removed and replaced with a regular throw. Compare
uses
> of "internal error" in dart:mirrors and dart:io.

I thought of that, but it didn't seem to be the original intent.  _fatal was
introduced exactly for this purpose (introduced to the VM but not used here:
https://codereview.chromium.org/953973003/, used here:
https://codereview.chromium.org/944893005/).  It just stopped working when the
async helper classes were moved out of dart:core and into dart:async and we
didn't notice it.  My intent was just to restore the intended behavior.

I'm happy throwing here if that's what we want.  We can also restore the
original behavior by moving private _fatal into dart:async.  We could even keep
public fatal in dart:_internal and just keep blacklisting it for the fuzzing
tests but that seems less good.

What do y'all think?

[Kevin Millikin (Google), 2017-02-10 11:59:42]

kmillikin@google.com changed reviewers:
+ kustermann@google.com, vegorov@google.com

[Kevin Millikin (Google), 2017-02-10 11:59:43]

I've restored the original (that is: FATAL, not throw NoSuchMethodError)
behavior.  PTAL.

[Kevin Millikin (Google), 2017-02-10 12:02:09]

Description was changed from

==========
Make all members of dart:_internal non-reflectable

Private members of the dart: libraries are non-reflectable.  This
change makes public members of the "private" library dart:_internal
non-reflectable as well.

The library dart:_internal may contain functions that violate the
normal rules of the Dart programming language and they should not be
exposed even via mirrors.

BUG=
==========

to

==========
The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.
==========

[Kevin Millikin (Google), 2017-02-10 12:02:41]

Description was changed from

==========
The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.
==========

to

==========
Move _fatal to dart:async

The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.
==========

[hausner, 2017-02-10 16:26:52]

I think restoring the originally intended behavior is better. I would not want
Dart code to be able to catch a thrown error and continue the program when we
discover an internal error.

[rmacnak, 2017-02-14 17:59:38]

lgtm

[Kevin Millikin (Google), 2017-02-20 10:52:59]

Description was changed from

==========
Move _fatal to dart:async

The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.
==========

to

==========
Move _fatal to dart:async

The function _fatal is only used in dart:async.  Change it from a public
function of dart:_internal, which can be invoked via mirrors, into a
private function of dart:async.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bbf5b89473f38f4b1c674c83411fc02cb68e0abe
==========

[Kevin Millikin (Google), 2017-02-20 10:53:01]

Committed patchset #2 (id:20001) manually as
bbf5b89473f38f4b1c674c83411fc02cb68e0abe (presubmit successful).