Give useful message for null derefs

Give useful message for null derefs

The exception implements NoSuchMethodError so that code that depends on
the previous exception type still works.

BUG=
R=johnmccutchan@google.com

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

[Cutch, 2016-10-07 23:24:11]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-10-07 23:24:11]

https://codereview.chromium.org/2407603002/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2407603002/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:130: * The [message] optionally describes the
circumstances under which [:null:] was dereferenced.
80 columns...

[Cutch, 2016-10-07 23:25:44]

LGTM after fixing column lengths

[Cutch, 2016-10-10 19:53:22]

Description was changed from

==========
Give useful message for null derefs

The exception implements NoSuchMethodError so that code that depends on
the previous exception type still works.

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

to

==========
Give useful message for null derefs

The exception implements NoSuchMethodError so that code that depends on
the previous exception type still works.

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

[Cutch, 2016-10-10 19:53:53]

Description was changed from

==========
Give useful message for null derefs

The exception implements NoSuchMethodError so that code that depends on
the previous exception type still works.

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

to

==========
Give useful message for null derefs

The exception implements NoSuchMethodError so that code that depends on
the previous exception type still works.

BUG=
R=johnmccutchan@google.com

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

[Cutch, 2016-10-10 19:53:54]

Committed patchset #4 (id:60001) manually as
588fed0368e5732c2cfe1929af7877d3018a2b2b (presubmit successful).

[sra1, 2016-10-10 21:19:32]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2016-10-10 21:19:33]

This CL can't work for dart2js because of the constraints of generating
efficient JavaScript.

It is probably best to change the VM Object.noSuchMethod implementation and
leave the complex and fragile JavaScript part alone.

As a user, the CL is both a step forward and a step backwards.
Null errors are a bit confusing to recognize, so I support them looking a bit
different, but I still need the arguments to methods and setters to be in error
message.

The js_library has a class called 'NullError' for these errors (when the
heuristics recognize the JavaScript TypeError). You should call your new Error
class NullError instead of 'NullDereferenceError'. There is no concept
'dereference' in the language spec.

The VM could do something that dart2js could never do - include a synopsis
expression as part of the message (like JavaScript does.).  That would aid
developer comprehension more that this change.

[sra1, 2016-10-10 21:19:37]

This CL can't work for dart2js because of the constraints of generating
efficient JavaScript.

It is probably best to change the VM Object.noSuchMethod implementation and
leave the complex and fragile JavaScript part alone.

As a user, the CL is both a step forward and a step backwards.
Null errors are a bit confusing to recognize, so I support them looking a bit
different, but I still need the arguments to methods and setters to be in error
message.

The js_library has a class called 'NullError' for these errors (when the
heuristics recognize the JavaScript TypeError). You should call your new Error
class NullError instead of 'NullDereferenceError'. There is no concept
'dereference' in the language spec.

The VM could do something that dart2js could never do - include a synopsis
expression as part of the message (like JavaScript does.).  That would aid
developer comprehension more that this change.

[rmacnak, 2016-10-10 21:29:38]

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

[rmacnak, 2016-10-10 21:29:39]

https://codereview.chromium.org/2407603002/diff/60001/runtime/lib/null_patch....
File runtime/lib/null_patch.dart (right):

https://codereview.chromium.org/2407603002/diff/60001/runtime/lib/null_patch....
runtime/lib/null_patch.dart:20: throw new NullDereferenceError('$name() was
called on a null value.');
DBC: "a null value" => "null"
There is only one.

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:125: class NullDereferenceError extends Error
implements NoSuchMethodError {
DBC: Don't make this type public.

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:136: return "Cannot dereference null.${ message != null
? ' $message' : ''}";
DBC: There is no such operation as "dereferencing" in Dart, only method
invocation. Also, `null.toString()`, `null.runtimeType`, etc do not cause any
exception. Consider something more along the lines of "unexpected invocation on
null".

[sra1, 2016-10-10 21:50:40]

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:125: class NullDereferenceError extends Error
implements NoSuchMethodError {
This class also throws away information. You can't inspect it to see the
arguments.

If you change the VM NoSuchMethodError formatting, then you can still inspect
the exception object in the debugger to see the arguments.

All the information needed to do a nicer job of presentation is already
available on NoSuchMethodError. There is already a lot of logic in there to help
users. Improve it.

You could change the print logic for a null receiver, for getters, or both. e.g.
from

NoSuchMethodError: method not found: 'isDeferredLoaderGetter'
Receiver: null
Arguments: []

to something like one of the following

NoSuchMethodError: null does not have a getter called 'isDeferredLoaderGetter'

NullError: getter not found: 'null.isDeferredLoaderGetter'

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:125: class NullDereferenceError extends Error
implements NoSuchMethodError {
On 2016/10/10 21:29:39, rmacnak wrote:
> DBC: Don't make this type public.

Yes, please remove from corelib.
We can't reliably support

    e is NullDereferenceError

in dart2js.

https://codereview.chromium.org/2407603002/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:136: return "Cannot dereference null.${ message != null
? ' $message' : ''}";
Still use the 'no such method' terminology.

This message adds confusion because you can 'dereference' null, e.g.

null.toString()   --> "null"
null.toString(1)  --> throws NoSuchMethodError

[Hixie, 2016-10-10 21:57:38]

Thanks for all the comments, I shall attempt this again with a different
approach!

My motivation here is that our customers are confused when they get a "no such
method" error and the receiver is "null". They expect to be told that the error
is that you're trying to do something with the receiver (namely, that it is
null), not that there's something wrong with the call itself.

To put it another way:

There's two classes of errors that give rise to NoSuchMethodError currently when
you have a call of the form foo.bar():

- "bar" is bad, e.g. it has the wrong arguments, or it's misspelled.
- "foo" is bad, e.g. it is the wrong type or is null.

To help developers, we should distinguish these two types visually, using
different language for each.

[Hixie, 2016-10-11 20:37:02]

Ok, second attempt is here: https://codereview.chromium.org/2405353002/